Skip to content

Conversation

@jonathanpeppers
Copy link
Member

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

Fixes: dotnet#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
@jonathanpeppers jonathanpeppers added platform/android perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) labels Mar 14, 2024
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner March 14, 2024 20:57
PureWeen
PureWeen previously approved these changes Mar 14, 2024
@jonathanpeppers
Copy link
Member Author

So I fixed Android, but the new test discovered it leaks on Windows?

image

@jsuarezruiz
Copy link
Contributor

Weird, I have triggered the UITests again.
image

@jonathanpeppers
Copy link
Member Author

No Windows is actually leaking when you use TabbedPage:

image

I haven't figured it out yet.

I spent 2.5 hours on this, and did not figure it out! Comment for now as original issue is Android.
@jonathanpeppers
Copy link
Member Author

I disabled the test on Windows for now, since the original issue is Android. I spent 2 hours commenting out code, and can't seem to figure it out.

I don't understand what the [Dependent Handle] entries in the above screenshot are on Windows. Is there a doc somewhere on internals of WindowsAppSDK?

@jonathanpeppers
Copy link
Member Author

WindowsAppSDK looks like it makes extensive use of DependentHandle:

Probably worth me learning about it, might be useful for other things.

@PureWeen PureWeen enabled auto-merge (squash) March 16, 2024 23:14
@PureWeen PureWeen merged commit 2317119 into dotnet:main Mar 17, 2024
@jonathanpeppers jonathanpeppers deleted the TabbedPageLeak branch March 18, 2024 13:28
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[regression/8.0.0] Navigating back from a TabbedPage does not invoked a destructor

4 participants