Skip to content

Commit

Permalink
[android] fix memory leak in TabbedPage (#21218)
Browse files Browse the repository at this point in the history
* [android] fix memory leak in `TabbedPage`

Fixes: #17959
Context: https://github.com/VoznichkaSviatoslav1/TabbedPageMemoryLeak

In the sample app, I added some `GC.Collect()` calls to force the GC
to run, but the following log message never prints:

    public partial class MyTabbedPage : TabbedPage
    {
        public MyTabbedPage()
        {
            InitializeComponent();

            Console.WriteLine("Constructor was called");
        }

        // Doesn't print!!!
        ~MyTabbedPage() => Console.WriteLine("Destructor was called");

I took a memory snapshot with `dotnet-gcdump` and found a tree like:

    SampleApp.MyTabbedPage ->
        Microsoft.Maui.Controls.Platform.MultiPageFragmentStateAdapter<Microsoft.Maui.Controls.Page> ->
            Action<Microsoft.Maui.Controls.Platform.AdapterItemKey>

I didn't find an explanation of what was holding
`MultiPageFragmentStateAdapter`. I tried eliminating the `Action<T>`,
but that didn't solve the underlying issue; it just made the object
tree shorter:

    SampleApp.MyTabbedPage ->
        Microsoft.Maui.Controls.Platform.MultiPageFragmentStateAdapter<Microsoft.Maui.Controls.Page>

I could also reproduce the problem in a new device test.

Reviewing the code, I found we were doing:

    _viewPager.Adapter = new MultiPageFragmentStateAdapter<Page>(tabbedPage, FragmentManager, _context) { CountOverride = tabbedPage.Children.Count };

But then in the cleanup code, we never set `Adapter` to `null`:

    _viewPager.Adapter = null;

After this change, the memory is released as expected. I am not
exactly sure *why* this leaked, but the change does seem appropriate
and fixes the problem.

Now the new device test passes & the sample app prints:

    03-14 15:06:32.394  6055  6055 I DOTNET  : Constructor was called
    03-14 15:06:37.008  6055  6089 I DOTNET  : Destructor was called

* [windows] skip test

I spent 2.5 hours on this, and did not figure it out! Comment for now as original issue is Android.

* Somehow duplicate [Fact]
  • Loading branch information
jonathanpeppers authored Mar 17, 2024
1 parent 1b423ab commit 2317119
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ internal void SetElement(TabbedPage tabbedPage)
Element.Disappearing -= OnTabbedPageDisappearing;
RemoveTabs();
_viewPager.LayoutChange -= OnLayoutChanged;
_viewPager.Adapter = null;
}

Element = tabbedPage;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,37 @@ IEnumerator IEnumerable.GetEnumerator()
}
}

[Fact(DisplayName = "Does Not Leak"
#if WINDOWS
, Skip = "FIXME: fails on Windows"
#endif
)]
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 TabbedPage
{
Children =
{
new ContentPage
{
Content = new VerticalStackLayout { new Label() },
}
}
};
pageReference = new WeakReference(page);
await navPage.Navigation.PushAsync(page);
await navPage.Navigation.PopAsync();
});

await AssertionExtensions.WaitForGC(pageReference);
}


TabbedPage CreateBasicTabbedPage(bool bottomTabs = false, bool isSmoothScrollEnabled = true, IEnumerable<Page> pages = null)
{
Expand Down

0 comments on commit 2317119

Please sign in to comment.