Skip to content

[controls] fix memory leaks in Page & navigation#13833

Merged
PureWeen merged 3 commits intodotnet:mainfrom
jonathanpeppers:wip-page-leaks
Mar 20, 2023
Merged

[controls] fix memory leaks in Page & navigation#13833
PureWeen merged 3 commits intodotnet:mainfrom
jonathanpeppers:wip-page-leaks

Conversation

@jonathanpeppers
Copy link
Copy Markdown
Member

@jonathanpeppers jonathanpeppers commented Mar 10, 2023

Fixes #13520
Fixes #12930

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!");
}

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 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.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Mar 10, 2023
Context: dotnet/maui#13833

Writing down how to do this for future self and others!
@jsuarezruiz
Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 2 pipeline(s).

@jonathanpeppers
Copy link
Copy Markdown
Member Author

My new tests don't pass on iOS yet, so going to see about getting *.gcdump files for iOS similar to:

dotnet/android#7875

@Eilon Eilon added the legacy-area-perf Startup / Runtime performance label Mar 14, 2023
@jonathanpeppers jonathanpeppers force-pushed the wip-page-leaks branch 3 times, most recently from 362b833 to 8e29705 Compare March 14, 2023 22:07
@jonathanpeppers jonathanpeppers force-pushed the wip-page-leaks branch 2 times, most recently from f8fe477 to 510ab5e Compare March 15, 2023 21:51
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!");
    }

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.
@jonathanpeppers jonathanpeppers marked this pull request as ready for review March 16, 2023 13:39
@AlleSchonWeg
Copy link
Copy Markdown
Contributor

Hi @jonathanpeppers ,
is it possible/necessary to port your "leak fixes" to Xamarin Forms?

@jonathanpeppers
Copy link
Copy Markdown
Member Author

@AlleSchonWeg they might not even apply to Xamarin.Forms I have no idea. The codebase is quite different.

@jonathanpeppers
Copy link
Copy Markdown
Member Author

Ok, it looks like there is one iOS test failure:

RemovingAllPagesDoesntCrash, System.TimeoutException : Arg_TimeoutException

{
if (parent == null)
{
var view = ViewIfLoaded ?? currentPlatformView;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we use this to call Disconnect also ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This fixed caused a different problem, so I'm trying 9a36b51 now.

@PureWeen PureWeen merged commit 7d0af63 into dotnet:main Mar 20, 2023
jonpryor pushed a commit to dotnet/android that referenced this pull request Mar 21, 2023
Context: dotnet/maui#13833

Writing down how to do this for future self and others!

Co-authored-by: Filip Navara <filip.navara@gmail.com>
@kojini
Copy link
Copy Markdown

kojini commented May 8, 2023

I don't see whether this fix will be backported to .NET 7.
Will it be backported to .NET 7? Do you have ETA on it?
(FYI, I'll be posting the same questions regarding some of memory leak issues/PRs as I'm tracking down when the bug fixes might be released)

@jonathanpeppers jonathanpeppers added perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) and removed legacy-area-perf Startup / Runtime performance labels Jul 12, 2023
@Hooterr
Copy link
Copy Markdown

Hooterr commented Aug 31, 2023

That looks like something that should've been backported to .NET 7.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Every MAUI Page generates a Memory Leak on iOS and macCatalyst Performance degradation when navigating to a page and going back several times

10 participants