Repository navigation
[Windows] Shell: let flyout items release the shell on disconnect - #39591
FullDevStudio wants to merge 1 commit into
Conversation
Our app swaps between two shells, and on Windows every old shell stayed in memory with all its pages, about 90 MB per switch. A gcdump showed the path: WinUI keeps the ShellFlyoutItemView of each flyout entry alive through its CCW, and the view still holds _shell and _content (whose parent is the shell item). It only lets go of them when its DataContext changes, and that does not happen when the shell's handler disconnects. ShellView.Disconnect now also tells every ShellFlyoutItemView under it to release its content and the shell. The cleanup that used to sit in OnDataContextChanged is moved into ReleaseContent, which both paths use, so nothing changes for a DataContext change. I added a Windows device test that shows a shell with two flyout items, swaps the window's page, disconnects the shell handler and waits for the shell to be collected. Our workaround sets DataContext = null on each ShellFlyoutItemView after the swap; with it, twelve switches left one old shell instead of twelve.
GitHub Actions
Inactive
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 39591Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 39591" |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey there @@FullDevStudio! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
Hey there @FullDevStudio! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| { | ||
| ReleaseContent(); | ||
| Content = null; | ||
| _shell = null; |
There was a problem hiding this comment.
[critical] Logic and Correctness - Clearing _shell makes subsequent native measurement throw: MeasureOverride evaluates ShellView, whose getter dereferences _shell.Handler without checking _shell. Disconnecting a realized Shell while its native flyout is still attached, then allowing another layout pass, now produces a NullReferenceException instead of taking the existing null fallback. Clearing Content removes the template content, not the ShellFlyoutItemView from its NavigationViewItem, so it does not prevent this callback. Make the getter or measure callback tolerate the disconnected state. The added test removes the Shell before disconnecting and does not exercise measurement afterward.
|
|
||
| var shellReference = await CreateAndRemoveShellWithFlyoutItems(); | ||
|
|
||
| await AssertionExtensions.WaitForGC(shellReference); |
There was a problem hiding this comment.
[moderate] Regression Prevention and Test Coverage - This GC assertion runs after the native test window has already closed. CreateAndRemoveShellWithFlyoutItems awaits CreateHandlerAndAddToWindow, whose Windows SetupWindowForTests finally block disconnects the window handler and calls newWindow.Close() before returning. The reported leak occurs while swapping Shells inside a window that remains open; closing that window first removes the native lifetime condition this test needs to preserve. Keep the window open through the collection assertion and isolate the old Shell's strong locals in a separate helper, so the test checks release during a Shell swap rather than after window teardown.
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you check the ai's suggestions?
There was a problem hiding this comment.
🟡 Changes recommended
Clearing _shell leaves subsequent flyout measurement callbacks vulnerable to a null-reference exception.
1 open issue
What changed in this PR
This PR addresses Windows Shell retention by releasing flyout-item references when the Shell handler disconnects.
Changes:
- Reuses content cleanup for both data-context changes and disconnects.
- Disconnects descendant flyout-item views during Shell teardown.
- Adds a Windows device test that checks Shell collection after a page swap.
| File | Description |
|---|---|
| src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.Windows.cs | Adds a garbage-collection regression test. |
| src/Controls/src/Core/Handlers/Shell/Windows/ShellView.cs | Invokes flyout-item cleanup during disconnect. |
| src/Controls/src/Core/Handlers/Shell/Windows/ShellFlyoutItemView.cs | Extracts shared cleanup and clears retained references. |
Review effort: Balanced
| { | ||
| ReleaseContent(); | ||
| Content = null; | ||
| _shell = null; |
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
PR Test Gate
|
MauiBot
removed
the
s/agent-review-in-progress
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Our app swaps between two shells, and on Windows every old shell stayed in memory with all its pages, about 90 MB per switch. A gcdump showed the path: WinUI keeps the ShellFlyoutItemView of each flyout entry alive through its CCW, and the view still holds _shell and _content (whose parent is the shell item). It only lets go of them when its DataContext changes, and that does not happen when the shell's handler disconnects.
ShellView.Disconnect now also tells every ShellFlyoutItemView under it to release its content and the shell. The cleanup that used to sit in OnDataContextChanged is moved into ReleaseContent, which both paths use, so nothing changes for a DataContext change.
I added a Windows device test that shows a shell with two flyout items, swaps the window's page, disconnects the shell handler and waits for the shell to be collected. Our workaround sets DataContext = null on each ShellFlyoutItemView after the swap; with it, twelve switches left one old shell instead of twelve.
Description of Change
Issues Fixed
Fixes #