Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Mar 20, 2023

Conversation

jonathanpeppers
Copy link
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
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jonathanpeppers
Copy link
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.
@AlleSchonWeg
Copy link

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

@jonathanpeppers
Copy link
Member Author

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

@jonathanpeppers
Copy link
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
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
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

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 memory-leak 💦 Memory usage grows / objects live forever and removed legacy-area-perf Startup / Runtime performance labels Jul 12, 2023
@Hooterr
Copy link

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! memory-leak 💦 Memory usage grows / objects live forever
Projects
None yet
10 participants