From 510ab5e25de494237c18f7c74016c08f83a784a6 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Tue, 7 Mar 2023 14:24:24 -0600 Subject: [PATCH] [controls] fix memory leaks in `Page` & navigation Fixes: https://github.com/dotnet/maui/issues/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(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. --- .../NavigationPage/iOS/NavigationRenderer.cs | 10 ++++ .../Core/HandlerImpl/Window/Window.Impl.cs | 1 + .../ControlsHandlerTestBase.Windows.cs | 4 +- .../NavigationPage/NavigationPageTests.cs | 43 ++++++++++++++ .../DeviceTests/Elements/Shell/ShellTests.cs | 58 ++++++++++++++++++- .../ContentView/ContentViewHandler.Android.cs | 2 + .../ContentView/ContentViewHandler.Windows.cs | 7 +++ .../ContentView/ContentViewHandler.iOS.cs | 7 +++ .../Navigation/NavigationViewFragment.cs | 8 +++ .../Platform/iOS/ContainerViewController.cs | 11 ++++ .../net-android/PublicAPI.Unshipped.txt | 1 + .../PublicAPI/net-ios/PublicAPI.Unshipped.txt | 2 + .../net-maccatalyst/PublicAPI.Unshipped.txt | 2 + .../net-windows/PublicAPI.Unshipped.txt | 1 + 14 files changed, 154 insertions(+), 3 deletions(-) diff --git a/src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs index 789d88e6d0a6..af393e69dea0 100644 --- a/src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs +++ b/src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs @@ -1156,6 +1156,16 @@ public override void ViewWillAppear(bool animated) base.ViewWillAppear(animated); } + public override void WillMoveToParentViewController(UIViewController parent) + { + base.WillMoveToParentViewController(parent); + + foreach (var child in ChildViewControllers) + { + child.WillMoveToParentViewController(parent); + } + } + protected override void Dispose(bool disposing) { if (_disposed) diff --git a/src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs b/src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs index 7444d8a533aa..7aa7f43dd426 100644 --- a/src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs +++ b/src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs @@ -598,6 +598,7 @@ void OnPageChanged(Page? oldPage, Page? newPage) { if (oldPage != null) { + _menuBarTracker.Target = null; InternalChildren.Remove(oldPage); oldPage.HandlerChanged -= OnPageHandlerChanged; oldPage.HandlerChanging -= OnPageHandlerChanging; diff --git a/src/Controls/tests/DeviceTests/ControlsHandlerTestBase.Windows.cs b/src/Controls/tests/DeviceTests/ControlsHandlerTestBase.Windows.cs index 87ab469ce799..8b1b54e28763 100644 --- a/src/Controls/tests/DeviceTests/ControlsHandlerTestBase.Windows.cs +++ b/src/Controls/tests/DeviceTests/ControlsHandlerTestBase.Windows.cs @@ -40,10 +40,10 @@ Task SetupWindowForTests(IWindow window, Func runTests, IMauiCon } finally { - window.Handler.DisconnectHandler(); + window.Handler?.DisconnectHandler(); await Task.Delay(10); newWindow?.Close(); - appStub.Handler.DisconnectHandler(); + appStub.Handler?.DisconnectHandler(); } }); } diff --git a/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs b/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs index a161651674f6..77cc6e9eb48a 100644 --- a/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs @@ -33,6 +33,7 @@ void SetupBuilder() handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); + handlers.AddHandler(); }); }); } @@ -287,5 +288,47 @@ await CreateHandlerAndAddToWindow(new Window(navPage), async }; }); } + + [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(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!"); + } + + [Fact(DisplayName = "Can Reuse Pages")] + public async Task CanReusePages() + { + SetupBuilder(); + var navPage = new NavigationPage(new ContentPage { Title = "Page 1" }); + var reusedPage = new ContentPage + { + Content = new Label() + }; + + await CreateHandlerAndAddToWindow(new Window(navPage), async (handler) => + { + await navPage.Navigation.PushAsync(reusedPage); + await navPage.Navigation.PopAsync(); + await navPage.Navigation.PushAsync(reusedPage); + await OnLoadedAsync(reusedPage.Content); + }); + } } } \ No newline at end of file diff --git a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs index 319c0698c329..4f9c662d01ed 100644 --- a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs @@ -660,7 +660,7 @@ await CreateHandlerAndAddToWindow(shell, async (handler) => }); } -#if !IOS +#if !IOS && !MACCATALYST [Fact] public async Task ChangingToNewMauiContextDoesntCrash() { @@ -917,6 +917,62 @@ await CreateHandlerAndAddToWindow(new Window(navPage), async } #endif + [Fact(DisplayName = "Pages Do Not Leak")] + public async Task PagesDoNotLeak() + { + SetupBuilder(); + var shell = await CreateShellAsync(shell => + { + shell.CurrentItem = new ContentPage() { Title = "Page 1" }; + }); + + WeakReference pageReference = null; + + await CreateHandlerAndAddToWindow(shell, async (handler) => + { + await OnLoadedAsync(shell.CurrentPage); + + var page = new ContentPage { Title = "Page 2" }; + pageReference = new WeakReference(page); + + await shell.Navigation.PushAsync(page); + await shell.Navigation.PopAsync(); + }); + + // 3 GCs were required in Android API 23, 2 worked otherwise + for (int i = 0; i < 3; i++) + { + await Task.Yield(); + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + + Assert.NotNull(pageReference); + Assert.False(pageReference.IsAlive, "Page should not be alive!"); + } + + [Fact(DisplayName = "Can Reuse Pages")] + public async Task CanReusePages() + { + SetupBuilder(); + var rootPage = new ContentPage(); + var shell = await CreateShellAsync(shell => + { + shell.CurrentItem = rootPage; + }); + var reusedPage = new ContentPage + { + Content = new Label { Text = "HEY" } + }; + await CreateHandlerAndAddToWindow(shell, async (handler) => + { + await shell.Navigation.PushAsync(reusedPage); + await shell.Navigation.PopAsync(); + await shell.Navigation.PushAsync(reusedPage); + await OnLoadedAsync(reusedPage.Content); + }); + } + protected Task CreateShellAsync(Action action) => InvokeOnMainThreadAsync(() => { diff --git a/src/Core/src/Handlers/ContentView/ContentViewHandler.Android.cs b/src/Core/src/Handlers/ContentView/ContentViewHandler.Android.cs index 61d5c550319e..45f89dba74ca 100644 --- a/src/Core/src/Handlers/ContentView/ContentViewHandler.Android.cs +++ b/src/Core/src/Handlers/ContentView/ContentViewHandler.Android.cs @@ -52,6 +52,8 @@ public static void MapContent(IContentViewHandler handler, IContentView page) protected override void DisconnectHandler(ContentViewGroup platformView) { // If we're being disconnected from the xplat element, then we should no longer be managing its children + platformView.CrossPlatformMeasure = null; + platformView.CrossPlatformArrange = null; platformView.RemoveAllViews(); base.DisconnectHandler(platformView); } diff --git a/src/Core/src/Handlers/ContentView/ContentViewHandler.Windows.cs b/src/Core/src/Handlers/ContentView/ContentViewHandler.Windows.cs index b56b6cfc28ed..b1303431bafc 100644 --- a/src/Core/src/Handlers/ContentView/ContentViewHandler.Windows.cs +++ b/src/Core/src/Handlers/ContentView/ContentViewHandler.Windows.cs @@ -49,5 +49,12 @@ public static void MapContent(IContentViewHandler handler, IContentView page) { UpdateContent(handler); } + + protected override void DisconnectHandler(ContentPanel platformView) + { + platformView.CrossPlatformMeasure = null; + platformView.CrossPlatformArrange = null; + base.DisconnectHandler(platformView); + } } } diff --git a/src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs b/src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs index 9bebb97dec91..3dc040405c85 100644 --- a/src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs +++ b/src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs @@ -53,5 +53,12 @@ public static void MapContent(IContentViewHandler handler, IContentView page) { UpdateContent(handler); } + + protected override void DisconnectHandler(ContentView platformView) + { + platformView.CrossPlatformMeasure = null; + platformView.CrossPlatformArrange = null; + base.DisconnectHandler(platformView); + } } } diff --git a/src/Core/src/Platform/Android/Navigation/NavigationViewFragment.cs b/src/Core/src/Platform/Android/Navigation/NavigationViewFragment.cs index f8df45ab8851..97a25c1e7954 100644 --- a/src/Core/src/Platform/Android/Navigation/NavigationViewFragment.cs +++ b/src/Core/src/Platform/Android/Navigation/NavigationViewFragment.cs @@ -78,6 +78,14 @@ public override void OnResume() base.OnResume(); } + public override void OnDestroy() + { + _currentView = null; + _fragmentContainerView = null; + + base.OnDestroy(); + } + public override Animation OnCreateAnimation(int transit, bool enter, int nextAnim) { int id = 0; diff --git a/src/Core/src/Platform/iOS/ContainerViewController.cs b/src/Core/src/Platform/iOS/ContainerViewController.cs index d7fc21feec7f..f3679393e734 100644 --- a/src/Core/src/Platform/iOS/ContainerViewController.cs +++ b/src/Core/src/Platform/iOS/ContainerViewController.cs @@ -90,5 +90,16 @@ public override void ViewDidLayoutSubviews() } public void Reload() => SetView(CurrentView, true); + + public override void WillMoveToParentViewController(UIViewController? parent) + { + if (parent == null) + { + var view = ViewIfLoaded ?? currentPlatformView; + view?.ClearSubviews(); + } + + base.WillMoveToParentViewController(parent); + } } } \ No newline at end of file diff --git a/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt b/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt index f5ff35068b91..8ef5ce620062 100644 --- a/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt +++ b/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt @@ -10,6 +10,7 @@ override Microsoft.Maui.Handlers.EditorHandler.PlatformArrange(Microsoft.Maui.Gr override Microsoft.Maui.Handlers.RadioButtonHandler.PlatformArrange(Microsoft.Maui.Graphics.Rect frame) -> void override Microsoft.Maui.Layouts.FlexBasis.Equals(object? obj) -> bool override Microsoft.Maui.Layouts.FlexBasis.GetHashCode() -> int +override Microsoft.Maui.Platform.NavigationViewFragment.OnDestroy() -> void override Microsoft.Maui.SizeRequest.Equals(object? obj) -> bool override Microsoft.Maui.SizeRequest.GetHashCode() -> int static Microsoft.Maui.FontSize.operator !=(Microsoft.Maui.FontSize left, Microsoft.Maui.FontSize right) -> bool diff --git a/src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt b/src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt index af7defd76aa7..e039e4d234b2 100644 --- a/src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt +++ b/src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt @@ -7,6 +7,7 @@ Microsoft.Maui.Hosting.MauiApp.DisposeAsync() -> System.Threading.Tasks.ValueTas Microsoft.Maui.Layouts.FlexBasis.Equals(Microsoft.Maui.Layouts.FlexBasis other) -> bool Microsoft.Maui.LifecycleEvents.iOSLifecycle.PerformFetch Microsoft.Maui.SizeRequest.Equals(Microsoft.Maui.SizeRequest other) -> bool +override Microsoft.Maui.Handlers.ContentViewHandler.DisconnectHandler(Microsoft.Maui.Platform.ContentView! platformView) -> void override Microsoft.Maui.Handlers.SwipeItemButton.Frame.get -> CoreGraphics.CGRect override Microsoft.Maui.Handlers.SwipeItemButton.Frame.set -> void override Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.ConnectHandler(UIKit.UIButton! platformView) -> void @@ -14,6 +15,7 @@ override Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.DisconnectHandler(UIKi override Microsoft.Maui.Handlers.SwitchHandler.NeedsContainer.get -> bool override Microsoft.Maui.Layouts.FlexBasis.Equals(object? obj) -> bool override Microsoft.Maui.Layouts.FlexBasis.GetHashCode() -> int +override Microsoft.Maui.Platform.ContainerViewController.WillMoveToParentViewController(UIKit.UIViewController? parent) -> void override Microsoft.Maui.Platform.MauiTextView.Font.get -> UIKit.UIFont? override Microsoft.Maui.Platform.MauiTextView.Font.set -> void override Microsoft.Maui.SizeRequest.Equals(object? obj) -> bool diff --git a/src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt b/src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt index caa978707724..01abff310d95 100644 --- a/src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt +++ b/src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt @@ -6,6 +6,7 @@ Microsoft.Maui.IApplication.UserAppTheme.get -> Microsoft.Maui.ApplicationModel. Microsoft.Maui.Hosting.MauiApp.DisposeAsync() -> System.Threading.Tasks.ValueTask Microsoft.Maui.Layouts.FlexBasis.Equals(Microsoft.Maui.Layouts.FlexBasis other) -> bool Microsoft.Maui.SizeRequest.Equals(Microsoft.Maui.SizeRequest other) -> bool +override Microsoft.Maui.Handlers.ContentViewHandler.DisconnectHandler(Microsoft.Maui.Platform.ContentView! platformView) -> void override Microsoft.Maui.Handlers.SwipeItemButton.Frame.get -> CoreGraphics.CGRect override Microsoft.Maui.Handlers.SwipeItemButton.Frame.set -> void override Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.ConnectHandler(UIKit.UIButton! platformView) -> void @@ -13,6 +14,7 @@ override Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.DisconnectHandler(UIKi override Microsoft.Maui.Handlers.SwitchHandler.NeedsContainer.get -> bool override Microsoft.Maui.Layouts.FlexBasis.Equals(object? obj) -> bool override Microsoft.Maui.Layouts.FlexBasis.GetHashCode() -> int +override Microsoft.Maui.Platform.ContainerViewController.WillMoveToParentViewController(UIKit.UIViewController? parent) -> void override Microsoft.Maui.Platform.MauiTextView.Font.get -> UIKit.UIFont? override Microsoft.Maui.Platform.MauiTextView.Font.set -> void override Microsoft.Maui.SizeRequest.Equals(object? obj) -> bool diff --git a/src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt b/src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt index 4653108a9e81..325d7513e50a 100644 --- a/src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt +++ b/src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt @@ -4,6 +4,7 @@ Microsoft.Maui.Hosting.MauiApp.DisposeAsync() -> System.Threading.Tasks.ValueTas Microsoft.Maui.Layouts.FlexBasis.Equals(Microsoft.Maui.Layouts.FlexBasis other) -> bool Microsoft.Maui.Platform.MauiWebView.MauiWebView(Microsoft.Maui.Handlers.WebViewHandler! handler) -> void Microsoft.Maui.SizeRequest.Equals(Microsoft.Maui.SizeRequest other) -> bool +override Microsoft.Maui.Handlers.ContentViewHandler.DisconnectHandler(Microsoft.Maui.Platform.ContentPanel! platformView) -> void override Microsoft.Maui.Layouts.FlexBasis.Equals(object? obj) -> bool override Microsoft.Maui.Layouts.FlexBasis.GetHashCode() -> int override Microsoft.Maui.SizeRequest.Equals(object? obj) -> bool