From f457d03cdc1639c4d1cad4d375335b89d894cc76 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Mon, 3 Jun 2024 16:43:00 -0500 Subject: [PATCH 1/2] [ios/catalyst] fix leak in NavigationPage Fixes: https://github.com/dotnet/maui/issues/20119 PR #13833 fixed a leak in child pages of a `NavigationPage`, but it appears there are several cycles that would prevent the `NavigationPage` itself from going away. So, if you did something like this: // The original NavigationPage & children leak Application.Current.MainPage = new NavigationPage(new Page1()); Application.Current.MainPage = new Page2(); I could reproduce the same problem in a new device test. The cycles (and solutions) are: 1. `NavigationPage` -> `NavigationRenderer` -> `ParentingViewController` -> `NavigationPage` * Solution: make `_child` a `WeakReference` 2. `NavigationPage` -> `MenuItemTracker` -> `NavigationPage` * Solution: make `_target` a `WeakReference`, and `_additionalTargets` a `List` 3. `NavigationPage` -> `MenuItemTracker.CollectionChanged` -> `NavigationPage` * Solution: subscribe/unsubscribe in `WillMoveToParentViewController` After these changes the sample app works, and the new device test passes. --- .../NavigationPage/iOS/NavigationRenderer.cs | 55 +++++++++++++------ src/Controls/src/Core/Menu/MenuItemTracker.cs | 39 ++++++++++--- .../NavigationPage/NavigationPageTests.cs | 26 ++++++++- 3 files changed, 93 insertions(+), 27 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 4d44d032c580..7c6a684c1492 100644 --- a/src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs +++ b/src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs @@ -674,11 +674,11 @@ void RemovePage(Page page) void RemoveViewControllers(bool animated) { var controller = TopViewController as ParentingViewController; - if (controller == null || controller.Child == null || controller.Child.Handler == null) + if (controller?.Child is not Page child || child.Handler == null) return; // Gesture in progress, lets not be proactive and just wait for it to finish - var task = GetAppearedOrDisappearedTask(controller.Child); + var task = GetAppearedOrDisappearedTask(child); task.ContinueWith(t => { @@ -1113,7 +1113,7 @@ class ParentingViewController : UIViewController { readonly WeakReference _navigation; - Page _child; + WeakReference _child; bool _disposed; ToolbarTracker _tracker = new ToolbarTracker(); @@ -1128,19 +1128,25 @@ public ParentingViewController(NavigationRenderer navigation) public Page Child { - get { return _child; } + get => _child?.GetTargetOrDefault(); set { - if (_child == value) + var child = Child; + if (child == value) return; - if (_child != null) - _child.PropertyChanged -= HandleChildPropertyChanged; - - _child = value; + if (child is not null) + child.PropertyChanged -= HandleChildPropertyChanged; - if (_child != null) - _child.PropertyChanged += HandleChildPropertyChanged; + if (value is not null) + { + _child = new(value); + value.PropertyChanged += HandleChildPropertyChanged; + } + else + { + _child = null; + } UpdateHasBackButton(); UpdateLargeTitles(); @@ -1228,7 +1234,6 @@ public override void ViewDidLoad() _tracker.Target = Child; _tracker.AdditionalTargets = Child.GetParentPages(); - _tracker.CollectionChanged += TrackerOnCollectionChanged; UpdateToolbarItems(); } @@ -1247,6 +1252,20 @@ public override void ViewWillAppear(bool animated) base.ViewWillAppear(animated); } + public override void WillMoveToParentViewController(UIViewController parent) + { + base.WillMoveToParentViewController(parent); + + if (parent is null) + { + _tracker.CollectionChanged -= TrackerOnCollectionChanged; + } + else + { + _tracker.CollectionChanged += TrackerOnCollectionChanged; + } + } + protected override void Dispose(bool disposing) { if (_disposed) @@ -1258,10 +1277,10 @@ protected override void Dispose(bool disposing) if (disposing) { - if (Child != null) + if (Child is Page child) { - Child.SendDisappearing(); - Child.PropertyChanged -= HandleChildPropertyChanged; + child.SendDisappearing(); + child.PropertyChanged -= HandleChildPropertyChanged; Child = null; } @@ -1517,17 +1536,17 @@ void TrackerOnCollectionChanged(object sender, EventArgs eventArgs) void UpdateHasBackButton() { - if (Child == null || NavigationItem.HidesBackButton == !NavigationPage.GetHasBackButton(Child)) + if (Child is not Page child || NavigationItem.HidesBackButton == !NavigationPage.GetHasBackButton(child)) return; - NavigationItem.HidesBackButton = !NavigationPage.GetHasBackButton(Child); + NavigationItem.HidesBackButton = !NavigationPage.GetHasBackButton(child); NavigationRenderer n; if (!_navigation.TryGetTarget(out n)) return; if (!(OperatingSystem.IsIOSVersionAtLeast(11) || OperatingSystem.IsMacCatalystVersionAtLeast(11)) || n._parentFlyoutPage != null) - UpdateTitleArea(Child); + UpdateTitleArea(child); } void UpdateNavigationBarVisibility(bool animated) diff --git a/src/Controls/src/Core/Menu/MenuItemTracker.cs b/src/Controls/src/Core/Menu/MenuItemTracker.cs index a6c614d086f3..e25510688dab 100644 --- a/src/Controls/src/Core/Menu/MenuItemTracker.cs +++ b/src/Controls/src/Core/Menu/MenuItemTracker.cs @@ -12,7 +12,8 @@ internal abstract class MenuItemTracker where TMenuItem : BaseMenuItem { int _flyoutDetails; - Page _target; + WeakReference _target; + List> _additionalTargets = new(); public MenuItemTracker() { } @@ -21,7 +22,28 @@ public MenuItemTracker() protected abstract IComparer CreateComparer(); - public IEnumerable AdditionalTargets { get; set; } + public IEnumerable AdditionalTargets + { + get + { + foreach (var target in _additionalTargets) + { + if (target.TryGetTarget(out var page)) + yield return page; + } + } + set + { + _additionalTargets.Clear(); + if (value is not null) + { + foreach (var page in value) + { + _additionalTargets.Add(new(page)); + } + } + } + } public bool HaveFlyoutPage { @@ -32,17 +54,18 @@ public bool HaveFlyoutPage public Page Target { - get { return _target; } + get => _target is not null && _target.TryGetTarget(out var target) ? target : null; set { - if (_target == value) + var target = Target; + if (target == value) return; - UntrackTarget(_target); - _target = value; + UntrackTarget(target); + _target = value is null ? null : new(value); - if (_target != null) - TrackTarget(_target); + if (value != null) + TrackTarget(value); EmitCollectionChanged(); } } diff --git a/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs b/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs index ab675465a492..55fbdc59f7db 100644 --- a/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs @@ -303,9 +303,33 @@ await CreateHandlerAndAddToWindow(new Window(navPage), async }); } - [Fact(DisplayName = "NavigationPage Does Not Leak")] + [Fact(DisplayName = "Does Not Leak")] public async Task DoesNotLeak() { + SetupBuilder(); + WeakReference pageReference = null; + WeakReference handlerReference = null; + + { + var navPage = new NavigationPage(new ContentPage()); + var window = new Window(navPage); + + await CreateHandlerAndAddToWindow(window, (handler) => + { + pageReference = new WeakReference(navPage); + handlerReference = new WeakReference(handler); + + // Just replace the page with a new one + window.Page = new ContentPage(); + }); + } + + await AssertionExtensions.WaitForGC(pageReference, handlerReference); + } + + [Fact(DisplayName = "Child Pages Do Not Leak")] + public async Task ChildPagesDoNotLeak() + { #if ANDROID if (!OperatingSystem.IsAndroidVersionAtLeast(30)) From ae26bece39d3ef5ab3f29f4af9ba727727fdd134 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Tue, 4 Jun 2024 13:15:37 -0500 Subject: [PATCH 2/2] Skip test on Windows for now --- .../Elements/NavigationPage/NavigationPageTests.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs b/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs index 55fbdc59f7db..ec1bfac0cd6e 100644 --- a/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs @@ -303,7 +303,11 @@ await CreateHandlerAndAddToWindow(new Window(navPage), async }); } - [Fact(DisplayName = "Does Not Leak")] + [Fact(DisplayName = "Does Not Leak" +#if WINDOWS + , Skip = "Failing" +#endif + )] public async Task DoesNotLeak() { SetupBuilder();