Skip to content

Commit

Permalink
[controls] fix memory leaks in Page & navigation
Browse files Browse the repository at this point in the history
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!");
    }

After a lot of investigating, I found even a "basic" `Page` test leaks:

    [Fact("Basic Page Does Not Leak")]
    public async Task BasicPageDoesNotLeak()
    {
        var pageReference = new WeakReference(new ContentPage());
        await CreateHandlerAndAddToWindow<PageHandler>((Page)pageReference.Target, _ => Task.CompletedTask);
        await Task.Yield();
        GC.Collect();
        GC.WaitForPendingFinalizers();
        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
* My "basic page" test leaks on all platforms.

Comparing various GC heap dumps in Visual Studio, I was able to find
the sources of these issues. I'll try to list them.

* Tests
    * `CreateHandlerAndAddToWindow` should unset `Window.Page`
    * `ControlsHandlerTestBase.Windows.cs` needed some `null` checks
* Controls.Core
    * `ModalNavigationManager` needs a `Disconnect()` method to unset
      `_previousPage` and call `NavigationModel.Clear()`
    * `Window.Destroy()` then needs to call
      `ModalNavigationManager.Disconnect()`
    * `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`.
    * `ShellContentFragment.Destroy()` should unset `_page`
    * `NavigationViewFragment.OnDestroy()` should unset `_currentView`
      and `_fragmentContainerView`
    * `ScopedFragment.OnDestroy()` should unset `DetailView`
* iOS/Catalyst
    * `ContentViewHandler.DisconnectHandler` should unset
      `platformView.CrossPlatformMeasure` and
      `platformView.CrossPlatformArrange`.

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.

WIP:

* I still need to test on iOS/Catalyst & retest user sample
* I need to document how to get `*.gcdump` files w/ Mono runtime
  • Loading branch information
jonathanpeppers committed Mar 14, 2023
1 parent 74fce38 commit 8e29705
Show file tree
Hide file tree
Showing 14 changed files with 104 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ void Destroy()
_viewhandler = null;
_shellContent = null;
_shellPageContainer = null;
_page = null;
}

protected override void Dispose(bool disposing)
Expand Down
1 change: 1 addition & 0 deletions src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,7 @@ void OnPageChanged(Page? oldPage, Page? newPage)
{
if (oldPage != null)
{
_menuBarTracker.Target = null;
InternalChildren.Remove(oldPage);
oldPage.HandlerChanged -= OnPageHandlerChanged;
oldPage.HandlerChanging -= OnPageHandlerChanging;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ Task SetupWindowForTests<THandler>(IWindow window, Func<Task> runTests, IMauiCon
}
finally
{
window.Handler.DisconnectHandler();
window.Handler?.DisconnectHandler();
await Task.Delay(10);
newWindow?.Close();
appStub.Handler.DisconnectHandler();
appStub.Handler?.DisconnectHandler();
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,5 +287,28 @@ TabbedPage CreateTabbedPage(string title) => new TabbedPage()
};
});
}

[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!");
}
}
}
36 changes: 35 additions & 1 deletion src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,7 @@ public async Task NavigatedFiresAfterSwitchingFlyoutItems()
});
}

#if !IOS
#if !IOS && !MACCATALYST
[Fact]
public async Task ChangingToNewMauiContextDoesntCrash()
{
Expand Down Expand Up @@ -919,6 +919,40 @@ public async Task TitleViewMeasures()
}
#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<ShellHandler>(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();
});

// Two GCs required currently
for (int i = 0; i < 2; i++)
{
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
}

Assert.NotNull(pageReference);
Assert.False(pageReference.IsAlive, "Page should not be alive!");
}

protected Task<Shell> CreateShellAsync(Action<Shell> action) =>
InvokeOnMainThreadAsync(() =>
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
10 changes: 10 additions & 0 deletions src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,15 @@ public static void MapContent(IContentViewHandler handler, IContentView page)
{
UpdateContent(handler);
}

protected override void DisconnectHandler(ContentView platformView)
{
platformView.CrossPlatformMeasure = null;
platformView.CrossPlatformArrange = null;
// TODO: this specific call fixes leaks in iOS/Catalyst
// But it doesn't feel quite right
platformView.RemoveFromSuperview();
base.DisconnectHandler(platformView);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 11 additions & 3 deletions src/Core/src/Platform/Android/Navigation/ScopedFragment.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Android.OS;
using System;
using Android.OS;
using Android.Views;
using AndroidX.Fragment.App;

Expand All @@ -8,7 +9,7 @@ class ScopedFragment : Fragment
{
readonly IMauiContext _mauiContext;

public IView DetailView { get; private set; }
public IView? DetailView { get; private set; }

public ScopedFragment(IView detailView, IMauiContext mauiContext)
{
Expand All @@ -19,7 +20,14 @@ public ScopedFragment(IView detailView, IMauiContext mauiContext)
public override View OnCreateView(LayoutInflater inflater, ViewGroup? container, Bundle? savedInstanceState)
{
var pageMauiContext = _mauiContext.MakeScoped(layoutInflater: inflater, fragmentManager: ChildFragmentManager);
return DetailView.ToPlatform(pageMauiContext);
var detailView = DetailView ?? throw new InvalidOperationException($"DetailView is null. OnDestroy must have been called!");
return detailView.ToPlatform(pageMauiContext);
}

public override void OnDestroy()
{
DetailView = null;
base.OnDestroy();
}
}
}
1 change: 1 addition & 0 deletions src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,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
Expand Down
1 change: 1 addition & 0 deletions src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Microsoft.Maui.Handlers.SwipeItemButton.SwipeItemButton() -> void
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
Expand Down
1 change: 1 addition & 0 deletions src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,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
Expand Down

1 comment on commit 8e29705

@danardelean
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward to be merged and released

Please sign in to comment.