Skip to content

Commit

Permalink
[ios/catalyst] fix leak in NavigationPage
Browse files Browse the repository at this point in the history
Fixes: #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<WeakReference>`

3. `NavigationPage` -> `MenuItemTracker.CollectionChanged` -> `NavigationPage`

    * Solution: subscribe/unsubscribe in `WillMoveToParentViewController`

After these changes the sample app works, and the new device test passes.
  • Loading branch information
jonathanpeppers committed Jun 3, 2024
1 parent 4a1c76c commit b2dfc11
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
{
Expand Down Expand Up @@ -1113,7 +1113,7 @@ class ParentingViewController : UIViewController
{
readonly WeakReference<NavigationRenderer> _navigation;

Page _child;
WeakReference<Page> _child;
bool _disposed;
ToolbarTracker _tracker = new ToolbarTracker();

Expand All @@ -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();
Expand Down Expand Up @@ -1228,7 +1234,6 @@ public override void ViewDidLoad()

_tracker.Target = Child;
_tracker.AdditionalTargets = Child.GetParentPages();
_tracker.CollectionChanged += TrackerOnCollectionChanged;

UpdateToolbarItems();
}
Expand All @@ -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)
Expand All @@ -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;
}

Expand Down Expand Up @@ -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)
Expand Down
39 changes: 31 additions & 8 deletions src/Controls/src/Core/Menu/MenuItemTracker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ internal abstract class MenuItemTracker<TMenuItem>
where TMenuItem : BaseMenuItem
{
int _flyoutDetails;
Page _target;
WeakReference<Page> _target;
List<WeakReference<Page>> _additionalTargets = new();
public MenuItemTracker()
{
}
Expand All @@ -21,7 +22,28 @@ public MenuItemTracker()

protected abstract IComparer<TMenuItem> CreateComparer();

public IEnumerable<Page> AdditionalTargets { get; set; }
public IEnumerable<Page> 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
{
Expand All @@ -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();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,33 @@ await CreateHandlerAndAddToWindow<WindowHandlerStub>(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<WindowHandlerStub>(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))
Expand Down

0 comments on commit b2dfc11

Please sign in to comment.