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

Shell Page Memory Leak #22645

Open
ESO-ST opened this issue May 24, 2024 · 36 comments
Open

Shell Page Memory Leak #22645

ESO-ST opened this issue May 24, 2024 · 36 comments
Assignees
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout memory-leak 💦 Memory usage grows / objects live forever (sub: perf) partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with platform/iOS 🍎 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Milestone

Comments

@ESO-ST
Copy link

ESO-ST commented May 24, 2024

Description

Shell pages don't get garbage collected and end up causing a memory leak.
In the provided sample project, we have two types of pages, one that's simply a ContentPage which we will refer to as non-shell, and one that is a Shell, and all we do is switch between them using App.Current.MainPage = new MainPage(); for the non-shell page and App.Current.MainPage = new AppShell(); for the shell one.

We have a destructor for all classes involved, the shell, the shell's ContentPage and the non-shell page. We simply output to the console when the destructors get called.

When we are on the shell page, the destructor of the non-shell page gets called properly, and hence it is garbage collected, however when we are on the non-shell page, the destructor of the shell page (or its shell) never gets called, and so leaks memory.
We can keep switching back and forth between the shell and non-shell page and we will see the memory leaking and the shell destructors never being called.

Steps to Reproduce

  1. tap on the button "Go To Non-Shell Page" to navigate away from the shell page.
  2. Now we are on the non-shell page, press the button "GC Collect and output memory used" this will force GC.Collect() and output the memory used to the console.
  3. Notice how our shell and our shell page destructors never get called; nothing outputted to the console.
  4. Now press on the button "Go To Shell Page" to go the shell page again; this will be a new instance of the shell and its page.
  5. Now that we are on the shell page, press "GC Collect and output memory used"
  6. Notice how our non-shell page's destructor is called properly.

Link to public reproduction project repository

https://github.com/ESO-ST/WebViewCrashMidLoad/tree/MemoryLeak

Version with bug

8.0.40 SR5

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

iOS

Affected platform versions

No response

Did you find any workaround?

No response

Relevant log output

No response

@ESO-ST ESO-ST added the t/bug Something isn't working label May 24, 2024
Copy link
Contributor

Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!

Open similar issues:

Closed similar issues:

Note: You can give me feedback by thumbs upping or thumbs downing this comment.

@PureWeen PureWeen added the partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with label May 24, 2024
@ninachen03 ninachen03 added s/verified Verified / Reproducible Issue ready for Engineering Triage s/triaged Issue has been reviewed labels May 27, 2024
@ninachen03
Copy link

Verified this issue with Visual Studio 17.11.0 Preview 1.0 (8.0.40 & 8.0.14). Can repro it.
image

@mattleibow mattleibow added area-controls-shell Shell Navigation, Routes, Tabs, Flyout memory-leak 💦 Memory usage grows / objects live forever (sub: perf) t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) platform/iOS 🍎 t/bug Something isn't working partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with s/triaged Issue has been reviewed and removed t/bug Something isn't working partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with s/triaged Issue has been reviewed memory-leak 💦 Memory usage grows / objects live forever (sub: perf) labels May 27, 2024
@PureWeen PureWeen removed s/verified Verified / Reproducible Issue ready for Engineering Triage s/triaged Issue has been reviewed labels Jun 1, 2024
@jsuarezruiz jsuarezruiz added this to the .NET 8 + Servicing milestone Jun 4, 2024
@Redth
Copy link
Member

Redth commented Jun 4, 2024

@rolfbjarne could you take a look at this one?

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Jun 4, 2024

I think there is a general issue if you do:

// AppShell lives forever
App.Current.MainPage = new AppShell();
App.Current.MainPage = new ContentPage();

I think AppShell has some circular references that keep it from going away on iOS or MacCatalyst, while something like a ContentPage works fine.

But since most apps have only one shell, those would be fine:

Given, there is a real issue here, just not all apps would run into it.

@ESO-ST as a workaround, could you create a single AppShell and reuse it?

@ESO-ST
Copy link
Author

ESO-ST commented Jun 5, 2024

that would be problematic as we would have to redesign many aspects of the application, but more importantly it will not solve our memory leak problem that eventually leads to an out of memory exception, our app has a lot of pages in the Shell, and if those pages never get garbage collected, the memory will simply keep growing, slowing the app down and eventually causing an out of memory exception.

But since most apps have only one shell, those would be fine

Won't the Shell pages still not be garbage collected in those cases too? so any page created, along with its children will not be garbage collected even when navigating away from it, and will keep occupying memory.

@jonathanpeppers
Copy link
Member

If you only have one AppShell instance in the entire app, any sub pages should be able to go away just fine.

If you need to change Application.MainPage, you could consider storing the AppShell in a variable to be reused later if needed.

@ESO-ST
Copy link
Author

ESO-ST commented Jun 5, 2024

I don't see that behavior in the repro project even when only using a single Shell instance. The finalizers of the sub pages and their elements (buttons, images, etc.) are never called.

Could you please take a quick look. I updated the project to add more subpages.

@ESO-ST
Copy link
Author

ESO-ST commented Jun 5, 2024

When should we expect the shell subpages and their content to be garbage collected?

@jonathanpeppers
Copy link
Member

@ESO-ST if you are just navigating to a page and then navigate back (so it is popped), then those pages should go away. If you have at least one GC.Collect() in your app (just for debugging purposes), the page should go away pretty quickly.

I have not had a chance to try your sample yet; I'm finishing up a leak related to CollectionView.

@jonathanpeppers
Copy link
Member

For pages displayed as a tab, it appears the behavior is they will stay around. Xamarin.Forms had some logic that was a little different: it would keep 3 pages around on Android in a TabbedPage with a setting to adjust the number.

This is related to:

One thing you could try is overriding OnNavigatedFrom() and call Handler?.DisconnectHandler() on each page (or try Content?.Handler?.DisconnectHandler()). This would at least allow much of the native side to go away and free up memory. The MAUI views use some memory, but not as much. Returning to a tab would hopefully recreate the native side.

Let us know if that works!

@ESO-ST
Copy link
Author

ESO-ST commented Jun 6, 2024

I tried that, when we return back to those tabs they are blank now

protected override void OnNavigatedFrom(NavigatedFromEventArgs args)
        {
            base.OnNavigatedFrom(args);
            Handler?.DisconnectHandler();
        }

image

Just to be on the same page here, this approach of re-using the same Shell instance won't work for us, the app was not designed to work that way, we would need to reset all pages and return them to their original state if we follow the single Shell approach (the pages have stateful controls/data on them), and that's something the app was not designed to do, it would be a big refactoring.

The app was designed to throw the Shell away when it's done with it, and simply make a new one when it needs it, basically start from scratch, the pages need to be clean and have no states/data in them.

App.Current.MainPage = new AppShell(); // we need a shell, make a new one.
App.Current.MainPage = new ContentPage(); // we no longer need a shell.

and so users would freely navigate between areas in the app where a shell is needed, to areas where a shell is not needed.

@PureWeen PureWeen self-assigned this Jun 6, 2024
@PureWeen
Copy link
Member

PureWeen commented Jun 6, 2024

Can you try this?

This worked for me and the finalizer is called

    protected override void OnNavigatedFrom(NavigatedFromEventArgs args)
    {
        base.OnNavigatedFrom(args);
		if (this.Parent is ShellContent shellContent)
		{
			FieldInfo fieldInfo = shellContent.GetType().GetField("_contentCache", BindingFlags.NonPublic | BindingFlags.Instance);
			// Set the value of the private field
			fieldInfo.SetValue(shellContent, null);
			shellContent.RemoveLogicalChild(this);
			shellContent.ContentTemplate = new DataTemplate(() => new MainPage());
		}
    }

@ESO-ST
Copy link
Author

ESO-ST commented Jun 7, 2024

Can you try this?

This worked for me and the finalizer is called

    protected override void OnNavigatedFrom(NavigatedFromEventArgs args)
    {
        base.OnNavigatedFrom(args);
		if (this.Parent is ShellContent shellContent)
		{
			FieldInfo fieldInfo = shellContent.GetType().GetField("_contentCache", BindingFlags.NonPublic | BindingFlags.Instance);
			// Set the value of the private field
			fieldInfo.SetValue(shellContent, null);
			shellContent.RemoveLogicalChild(this);
			shellContent.ContentTemplate = new DataTemplate(() => new MainPage());
		}
    }

You tried it in the repro project? I tried it in the sample repro project, it didn't work, the finalizers are still not being called.

While I appreciate the effort for the suggested workaround (re-use same Shell instance), we can't really apply that approach in our app, it would require a major re-work, we've tried going that route upon your suggestion and after a while it became abundantly clear it would be a major refactoring. We really must create a new Shell.

I would be happy to show you the app and walk you through the hows and whys if you'd like.

A workaround that we could use would be one where we would manually clean the Shell in some way once we're done with it.

@PureWeen
Copy link
Member

PureWeen commented Jun 7, 2024

Can you try this?
This worked for me and the finalizer is called

    protected override void OnNavigatedFrom(NavigatedFromEventArgs args)
    {
        base.OnNavigatedFrom(args);
		if (this.Parent is ShellContent shellContent)
		{
			FieldInfo fieldInfo = shellContent.GetType().GetField("_contentCache", BindingFlags.NonPublic | BindingFlags.Instance);
			// Set the value of the private field
			fieldInfo.SetValue(shellContent, null);
			shellContent.RemoveLogicalChild(this);
			shellContent.ContentTemplate = new DataTemplate(() => new MainPage());
		}
    }

You tried it in the repro project? I tried it in the sample repro project, it didn't work, the finalizers are still not being called.

While I appreciate the effort for the suggested workaround (re-use same Shell instance), we can't really apply that approach in our app, it would require a major re-work, we've tried going that route upon your suggestion and after a while it became abundantly clear it would be a major refactoring. We really must create a new Shell.

I would be happy to show you the app and walk you through the hows and whys if you'd like.

A workaround that we could use would be one where we would manually clean the Shell in some way once we're done with it.

Rolling back a bit to the initial dance of swapping out the mainpage for a new shell.
What might work for that scenario is to clear out the shell before swapping in the new mainpage.
Shell itself will probably still stick around (you could try disconnecting the handlers) but at least the whole stack of pages will finalize

I tried this on your sample

public ShellPage()
        {
            InitializeComponent();
            i++;
            Console.WriteLine($"ShellPage Created: {i}");
        }

        static int i = 0;
        ~ShellPage()
        {
            i--;
            Console.WriteLine($"ShellPage destroyed: {i}");
        }

        private async void Button_Clicked(object sender, EventArgs e)
        {
            var shell = Shell.Current;
            shell.Items.Clear();
            shell.Items.Add(new ShellContent() { Content = new ContentPage() });
            await Task.Yield();
            App.Current.MainPage = new MainPage();
            App.Current.MainPage.Loaded += OnLoaded;

            async void OnLoaded(object sender, EventArgs e)
            {
                shell = null;
                App.Current.MainPage.Loaded -= OnLoaded;

            }
        }

When I do that all the pages finalized

image

If you're wanting pages on the shell to finalize while you navigate you could apply a similar strategy.

For example, if you one bottom tab1, remove and re-add everything on tab2.

though maybe being able to cleanup your old shell is enough for now?

@ESO-ST
Copy link
Author

ESO-ST commented Jun 11, 2024

I verified the workaround works well on the sample project, I attempted it in our app but unfortunately it doesn't work there. I'm investigating on my end in case there's something still holding a reference to the pages preventing them from being garbage collected.

In our app we have a Shell TitleView, so I tested out adding a Shell TitleView to the repro sample
Microsoft.Maui.Controls.Shell.SetTitleView(this, someTitleView);
this effectively stopped the pages from being garbage collected, so then I simply set this TitleView to null when we execute our workaround and that fixed it.

I did the same in our app but still something else is preventing the pages from being garbage collected.
Can renderers/handlers/effects/behaviors/styles (possibly some controls or ui elements) be holding an internal reference to the page preventing it from being garbage collected? any ideas?

@samhouts
Copy link
Member

Did you try the scenario with nightly builds to see if the fix for TitleView (#21968) fixed that bit?

@ESO-ST
Copy link
Author

ESO-ST commented Jun 13, 2024

Did you try the scenario with nightly builds to see if the fix for TitleView (#21968) fixed that bit?

I got past the TitleView blocker by simply nulling the TitleView when exiting the shell
Microsoft.Maui.Controls.Shell.SetTitleView(this, null);

I also tried the latest nightly build as of today in our prod app, but i still can't get PureWeen's workaround to work there. I am unable to memory profile the iOS app using dotnet gcdump, it seems that's not currently supported. I tried DotMemory from Jetbrains but that's also not supported.

@PureWeen Please let me know if dotnet gcdump supports iOS, that could speed things up if im able to memory profile and see what is still holding a reference to the pages.

That's why I was asking if there could be other aspects of Shell (or MAUI) that are keeping a reference to its pages, preventing them from being garbage collected, just like the TitleView was.

@jonathanpeppers
Copy link
Member

@ESO-ST dotnet-gcdump should work fine in combination with dotnet-dsrouter. If your app runs on MacCatalyst, you can also skip using dotnet-dsrouter.

The iOS docs are here:

But the Android doc might explain a bit better conceptually:

@ESO-ST
Copy link
Author

ESO-ST commented Jun 18, 2024

@ESO-ST dotnet-gcdump should work fine in combination with dotnet-dsrouter. If your app runs on MacCatalyst, you can also skip using dotnet-dsrouter.

The iOS docs are here:

But the Android doc might explain a bit better conceptually:

I've already tried using dotnet-dsrouter, it does work but only with dotnet-trace, not with dotnet-gcdump. The docs linked also don't mention dotnet-gcdump, only dotnet-trace. Is there documentation specific to dotnet-gcdump and dsrouter? I didn't find any.
Unfortunately our app does not run on MacCatalyst, only iOS.

@jonathanpeppers
Copy link
Member

The dotnet-gcdump command should be identical to dotnet-trace, and should work. One thing to check is if all the tools have the same version number.

Do you get an error message at all?

@ESO-ST
Copy link
Author

ESO-ST commented Jun 18, 2024

I just tried it again, dot-net trace works fine, but dotnet gcdump gives me "--diagnostic-port is only supporting connect mode."

image

@jonathanpeppers
Copy link
Member

Ok, I've been using -p {PID of dsrouter} instead of --diagnostic-port. The Android instructions describe this workflow a bit.

@jonathanpeppers
Copy link
Member

Here are my steps to use dotnet-gcdump, they are not intuitive. I would expect folks to have trouble!

First, I build a Release build of the app with -p:_BundlerDebug=true:

dotnet build YourApp.csproj -f net8.0-ios -c Release -p:_BundlerDebug=true

This includes the optional Mono diagnostic component in the app. You could also try a Debug build, but if you are measuring performance some Debug-mode things might get in the way.

Next, we'll need to use mlaunch to launch the simulator and set an environment variable, $DOTNET_DiagnosticPorts.

Find mlaunch:

find /usr/local/share/dotnet/packs | grep -E 'mlaunch$'
/usr/local/share/dotnet/packs/Microsoft.iOS.Sdk/17.2.8053/tools/lib/mlaunch/mlaunch.app/Contents/MacOS/mlaunch

I picked the newest one.

Then set an alias for it, I put this in my ~/.zprofile as well:

alias mlaunch="/usr/local/share/dotnet/packs/Microsoft.iOS.Sdk/17.2.8053/tools/lib/mlaunch/mlaunch.app/Contents/MacOS/mlaunch"

Now you can just run mlaunch later.

Find a simulator to use:

xcrun simctl list devices
== Devices ==
-- iOS 17.5 --
    iPhone 15 (472C1E4D-B981-4C75-9638-111E9CB15E5D) (Shutdown)

I just picked iPhone 15, we'll need the GUID here in the mlaunch command later.

Run dotnet-dsrouter:

dotnet-dsrouter ios-sim
How to connect current dotnet-dsrouter pid=26122 with ios simulator and diagnostics tooling.
Start an application on ios simulator with ONE of the following environment variables set:
[Default Tracing]
DOTNET_DiagnosticPorts=127.0.0.1:9000,nosuspend,listen
[Startup Tracing]
DOTNET_DiagnosticPorts=127.0.0.1:9000,suspend,listen
Run diagnotic tool connecting application on ios simulator through dotnet-dsrouter pid=26122:
dotnet-trace collect -p 26122
See https://learn.microsoft.com/en-us/dotnet/core/diagnostics/dotnet-dsrouter for additional details and examples.

Take note of the DOTNET_DiagnosticPorts=127.0.0.1:9000,suspend,listen and the PID.

Run mlaunch using the udid of the simulator:

mlaunch --launchsim bin/Release/net8.0-ios/iossimulator-arm64/YourApp.app --device :v2:udid=472C1E4D-B981-4C75-9638-111E9CB15E5D --wait-for-exit --stdout=$(tty) --stderr=$(tty) --argument --connection-mode --argument none '--setenv:DOTNET_DiagnosticPorts=127.0.0.1:9000,nosuspend,listen'

I used nosuspend, so the app wouldn't pause on launch. suspend is only useful if you are measuring startup.

Once the app is running, you can do either one of:

dotnet-trace collect -p 26122 --format speedscope
dotnet-gcdump collect -p 26122

The *.gcdump files can only be opened on Windows in Visual Studio or PerfView, so I normally drop those on a shared OneDrive.

@ESO-ST
Copy link
Author

ESO-ST commented Jun 18, 2024

thanks @jonathanpeppers I was able to get dotnet-gcdump working using
image

@BenBtg
Copy link
Contributor

BenBtg commented Jun 19, 2024

@ESO-ST please can you send me the gcdump so I can pass it on?

@ESO-ST
Copy link
Author

ESO-ST commented Jun 19, 2024

I got the workaround to function in our app, I had to modify it a bit.
The only caveat here is the page the shell is currently on -Shell.CurrentPage-, along with its UI elements, does not get garbage collected, it's ignored, I'm still investigating that part.

 public void LoadPageAndCleanShell(Page page)
 {
         Shell shell = Shell.Current;
         if (shell != null)
         {
             foreach (ShellItem item in shell.Items)
             {
                 foreach (ShellSection section in item.Items)
                 {
                     section.Items.Clear();
                 }
                 item.Items.Clear();
             }
             shell.Items.Clear();
             shell.Items.Add(new ShellContent() { Content = new ContentPage() });
         }
         App.Current.MainPage = page;
         App.Current.MainPage.Loaded += OnLoaded;

         void OnLoaded(object sender, EventArgs e)
         {
             shell = null;
             App.Current.MainPage.Loaded -= OnLoaded;
         }
 }

Thanks for your continued help, getting dotnet-gcdump working was invaluable here :)

@ESO-ST
Copy link
Author

ESO-ST commented Jun 20, 2024

I'm running into this issue now #22951 (comment)
If I downgrade to v8.0.40 the carousel issue goes away, but the memory leak issue comes back (even with the workaround above, the pages get garbage collected, but their UI elements do not get garbage collected)

@PureWeen
Copy link
Member

@ESO-ST I don't think @22951 is related it's just the same type of exception.

Do you have a repro? Or the XAML for your CarV? Does this happen if you load directly to that CarouselView and have done zero main page swaps?

I'm wondering if a left over CarouselView is bound to a reused collection

@ESO-ST
Copy link
Author

ESO-ST commented Jun 25, 2024

@ESO-ST I don't think @22951 is related it's just the same type of exception.

Do you have a repro? Or the XAML for your CarV? Does this happen if you load directly to that CarouselView and have done zero main page swaps?

I'm wondering if a left over CarouselView is bound to a reused collection

I'm trying to setup a repro project for this, mimicking what our app does for carousel. It is unrelated to the work around we did here, I can reproduce it in our app consistently as soon as i upgrade MAUI to v8.0.60+
It happens even without any page changing, even when we load directly into a page that has the CarouselView.

@BenBtg
Copy link
Contributor

BenBtg commented Jun 26, 2024

@ESO-ST as the Carousel crash issue is unrelated to the memory leak and workaround. Please can you raise an issue specific to the crash once you have a repro?

@ESO-ST
Copy link
Author

ESO-ST commented Jun 26, 2024

@ESO-ST as the Carousel crash issue is unrelated to the memory leak and workaround. Please can you raise an issue specific to the crash once you have a repro?

here's the new ticket for this, with a repro project

@tele-bird
Copy link

tele-bird commented Sep 4, 2024

It is not advised to switch MainPage during runtime in Maui, so I wonder why all the activity on this issue.
Is this article still accurate? We designed our app to comply...
https://visualstudiomagazine.com/Articles/2024/02/02/maui-dos-and-donts.aspx

@Sztub
Copy link

Sztub commented Sep 24, 2024

In my case memory leak was caused by adding a bindable property into Shell.TitleView Content like this on a child page:

    <Shell.TitleView>
        <title:AwesomeTitleView Title="{StaticResource Page_Title}"
                                TapCommand="{Binding BindingContext.NavigateToSecondPageCommand ,Source={x:Reference DetailsTemplate} }" />
    </Shell.TitleView>

TapCommand binds to command defined in VM. That causes a circular reference between Page, VM and Shell. It works fina on Android but leaks on iOS. Tested on version 8.0.70

@jonathanpeppers
Copy link
Member

@Sztub what is NavigateToSecondPageCommand? Is it a custom command or Command<T>?

There is this issue that probably applies to various controls using a custom ICommand:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout memory-leak 💦 Memory usage grows / objects live forever (sub: perf) partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with platform/iOS 🍎 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
None yet
Development

No branches or pull requests