[controls] fix memory leaks in Page & navigation#13833
Conversation
Context: dotnet/maui#13833 Writing down how to do this for future self and others!
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
My new tests don't pass on iOS yet, so going to see about getting |
362b833 to
8e29705
Compare
f8fe477 to
510ab5e
Compare
Fixes: dotnet#13520 Context: https://github.com/danardelean/MauiTestFinalizer In the above sample, you can see `Page` objects leaking while using Shell navigation. I was able to reproduce this in a test with both Shell & `NavigationPage` such as: [Fact(DisplayName = "NavigationPage Does Not Leak")] public async Task DoesNotLeak() { SetupBuilder(); WeakReference pageReference = null; var navPage = new NavigationPage(new ContentPage { Title = "Page 1" }); await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async (handler) => { var page = new ContentPage { Title = "Page 2" }; pageReference = new WeakReference(page); await navPage.Navigation.PushAsync(page); await navPage.Navigation.PopAsync(); }); await Task.Yield(); GC.Collect(); GC.WaitForPendingFinalizers(); Assert.NotNull(pageReference); Assert.False(pageReference.IsAlive, "Page should not be alive!"); } To summarize: * Customer's sample only leaks on iOS/Catalyst * My Shell/NavigationPage tests leak on Android/iOS/Catalyst Comparing various GC heap dumps in Visual Studio, I was able to find the sources of these issues. I'll try to list them. * Controls.Core * `Window.OnPageChanged()` should unset `_menuBarTracker.Target` * Windows * `ContentViewHandler.DisconnectHandler` should unset `platformView.CrossPlatformMeasure` and `platformView.CrossPlatformArrange`. * Android * `ContentViewHandler.DisconnectHandler` should unset `platformView.CrossPlatformMeasure` and `platformView.CrossPlatformArrange`. * `NavigationViewFragment.OnDestroy()` should unset `_currentView` and `_fragmentContainerView` * iOS/Catalyst * `ContentViewHandler.DisconnectHandler` should unset `platformView.CrossPlatformMeasure` and `platformView.CrossPlatformArrange`. * `ContainerViewController`0 should call `ClearSubviews()` on its view in `WillMoveToParentViewController()` * `ParentingViewController` should forward `WillMoveToParentViewController()` to its child view controllers After these changes, my tests pass! They literally would not pass without fixing all of the above... Each issue I found while reviewing GC dumps. Other test changes: * `ControlsHandlerTestBase.Windows.cs` needed some `null` checks * `ShellTests` needed an `&& !MACCATALYST` to build for MacCatalyst * Added tests proving we can reuse pages still.
510ab5e to
3aee8b3
Compare
|
Hi @jonathanpeppers , |
|
@AlleSchonWeg they might not even apply to Xamarin.Forms I have no idea. The codebase is quite different. |
|
Ok, it looks like there is one iOS test failure: |
| { | ||
| if (parent == null) | ||
| { | ||
| var view = ViewIfLoaded ?? currentPlatformView; |
There was a problem hiding this comment.
Should we use this to call Disconnect also ?
There was a problem hiding this comment.
This fixed caused a different problem, so I'm trying 9a36b51 now.
Context: dotnet/maui#13833 Writing down how to do this for future self and others! Co-authored-by: Filip Navara <filip.navara@gmail.com>
|
I don't see whether this fix will be backported to .NET 7. |
|
That looks like something that should've been backported to .NET 7. |
Fixes #13520
Fixes #12930
Context: https://github.com/danardelean/MauiTestFinalizer
In the above sample, you can see
Pageobjects leaking while usingShell navigation. I was able to reproduce this in a test with both
Shell &
NavigationPagesuch as:To summarize:
Customer's sample only leaks on iOS/Catalyst
My Shell/NavigationPage tests leak on Android/iOS/Catalyst
Comparing various GC heap dumps in Visual Studio, I was able to find
the sources of these issues. I'll try to list them.
Window.OnPageChanged()should unset_menuBarTracker.TargetContentViewHandler.DisconnectHandlershould unsetplatformView.CrossPlatformMeasureandplatformView.CrossPlatformArrange.ContentViewHandler.DisconnectHandlershould unsetplatformView.CrossPlatformMeasureandplatformView.CrossPlatformArrange.NavigationViewFragment.OnDestroy()should unset_currentViewand
_fragmentContainerViewContentViewHandler.DisconnectHandlershould unsetplatformView.CrossPlatformMeasureandplatformView.CrossPlatformArrange.ContainerViewControllershould callClearSubviews()on itsview in
WillMoveToParentViewController()ParentingViewControllershould forwardWillMoveToParentViewController()to its child view controllers
After these changes, my tests pass! They literally would not pass
without fixing all of the above... Each issue I found while reviewing
GC dumps.
Other test changes:
ControlsHandlerTestBase.Windows.csneeded somenullchecksShellTestsneeded an&& !MACCATALYSTto build for MacCatalyst