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 362b833
Show file tree
Hide file tree
Showing 20 changed files with 152 additions and 16 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
2 changes: 2 additions & 0 deletions src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ void IWindow.Destroying()

Application?.RemoveWindow(this);
Handler?.DisconnectHandler();
ModalNavigationManager?.Disconnect();
}

void IWindow.Resumed()
Expand Down Expand Up @@ -596,6 +597,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 @@ -60,5 +60,11 @@ internal void SettingNewPage()
partial void OnPageAttachedHandler();

public void PageAttachedHandler() => OnPageAttachedHandler();

public void Disconnect()
{
_navModel?.Clear();
_previousPage = null;
}
}
}
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
6 changes: 2 additions & 4 deletions src/Controls/tests/DeviceTests/ControlsHandlerTestBase.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,8 @@ Task SetupWindowForTests<THandler>(IWindow window, Func<Task> runTests, IMauiCon
else
window.Handler?.DisconnectHandler();
var vc =
(window.Content?.Handler as IPlatformViewHandler)?
.ViewController;
var platformHandler = windowHandler as IPlatformViewHandler;
var vc = platformHandler?.ViewController;
vc?.RemoveFromParentViewController();
vc?.View?.RemoveFromSuperview();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ public async Task PushModalUsingTransparencies(Page rootPage, Page modalPage)
var currentView = currentPage.Handler.PlatformView as UIView;
Assert.NotNull(currentView);
Assert.NotNull(currentView.Window);
await currentPage.Navigation.PopModalAsync();
});
}
}
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
16 changes: 10 additions & 6 deletions src/Controls/tests/DeviceTests/Stubs/WindowHandlerStub.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ async void ReplaceCurrentView(IView view, UIWindow platformView, Action finished
return;
}

var vc = (view.Handler as IPlatformViewHandler).ViewController;
var handler = view.Handler as IPlatformViewHandler;
var vc = handler?.ViewController;
var virtualView = VirtualView;

if (view is IFlyoutView)
Expand Down Expand Up @@ -113,18 +114,21 @@ async void ReplaceCurrentView(IView view, UIWindow platformView, Action finished
// So, we're just simulating that cleanup here ourselves.
var presentedVC =
platformView?.RootViewController?.PresentedViewController ??
vc.PresentedViewController;
vc?.PresentedViewController;

if (presentedVC is Microsoft.Maui.Controls.Platform.ModalWrapper mw)
{
await mw.PresentingViewController.DismissViewControllerAsync(false);
}

vc.RemoveFromParentViewController();
if (vc != null)
{
vc.RemoveFromParentViewController();

view
.ToPlatform()
.RemoveFromSuperview();
view
.ToPlatform()
.RemoveFromSuperview();
}

finishedClosing.Invoke();

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);
}
}
}
8 changes: 8 additions & 0 deletions src/Core/src/Handlers/View/ViewHandlerOfT.cs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,14 @@ protected virtual void ConnectHandler(TPlatformView platformView)

protected virtual void DisconnectHandler(TPlatformView platformView)
{
#if IOS || MACCATALYST
var vc = ViewController;
if (vc is not null)
{
vc.Dispose();
ViewController = null;
}
#endif
}

private protected override PlatformView OnCreatePlatformView()
Expand Down
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();
}
}
}
17 changes: 17 additions & 0 deletions src/Core/src/Platform/iOS/ContainerViewController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -90,5 +90,22 @@ public override void ViewDidLayoutSubviews()
}

public void Reload() => SetView(CurrentView, true);

protected override void Dispose(bool disposing)
{
if (disposing)
{
if (_view is ContentView c)
{
c.CrossPlatformArrange = null;
c.CrossPlatformMeasure = null;
}
_view = null;
_pendingLoadedView = null;
currentPlatformView = null;
}

base.Dispose(disposing);
}
}
}
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
2 changes: 2 additions & 0 deletions src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ 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
override Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.DisconnectHandler(UIKit.UIButton! platformView) -> void
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.Dispose(bool disposing) -> 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@ 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
override Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.DisconnectHandler(UIKit.UIButton! platformView) -> void
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.Dispose(bool disposing) -> 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
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

0 comments on commit 362b833

Please sign in to comment.