maui icon indicating copy to clipboard operation
maui copied to clipboard

iOS PopModalAsync causes memory leaks

Open dalux-era opened this issue 1 year ago • 1 comments

Description

Whne you push a modal page that is wrapped in a NavigationPage Navigation.PushModalAsync(new NavigationPage(new ModularPage())); on iOS we can see that the Page is never released from the memory. We are using Perfiew for debugging.

Here is a GCDump where we open and close a modal page multiple times 20240326_110245_0.zip

I am adding a sample which has both solutions outcommented. Seems counterintuitive that we need to call DisconnectHandler on existing pages and controls when popping a page from the Navigation stack.

NavigationSample.zip

Steps to Reproduce

  1. Open sample app
  2. Start profiling
  3. Press the "Click me" button
  4. Press "Close" button
  5. Repeat steps 3 and 4
  6. MAke a memory GCdump

Link to public reproduction project repository

No response

Version with bug

8.0.6 SR1

Is this a regression from previous behavior?

Yes, this used to work in Xamarin.Forms

Last version that worked well

Unknown/Other

Affected platforms

iOS

Affected platform versions

No response

Did you find any workaround?

Seems that this can only be fixed by running disconnectHanler on each element on the Page including the page

page.Handler?.DisconnectHandler();

MyLayout.Handler?.DisconnectHandler();

Handler?.DisconnectHandler();

or creating a custom handler for the NavigationPage and running

ViewController.Dispose();
var viewController = ViewController as PageViewController;
viewController.CurrentView = null;

Relevant log output

No response

dalux-era avatar Mar 26 '24 12:03 dalux-era

Possible duplicate or related:

https://github.com/dotnet/maui/issues/20094 https://github.com/dotnet/maui/issues/20119

AdamEssenmacher avatar Apr 12 '24 20:04 AdamEssenmacher

/similarissues

mattleibow avatar May 20 '24 15:05 mattleibow

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.

github-actions[bot] avatar May 20 '24 15:05 github-actions[bot]

Can you test with the latest nightly build? https://github.com/dotnet/maui/wiki/Nightly-Builds

PureWeen avatar May 31 '24 16:05 PureWeen

I just tried with 8.0.60-ci.net8.24303.1 which seems to be the latest on https://dev.azure.com/xamarin/public/_artifacts/feed/maui-nightly

image

But creating a modal page with a NavigationPage still stops the page from being collected by the garbage collector on a real iOS device. A plain modal with no NavigationPage does get cleaned up after GC Collect.

GC.Collect();
GC.WaitForPendingFinalizers(); 

image

See attached test app. To test tap one of the page buttons, then navigate back and tap the Collect GC button. The isAlive should return to false and the text should change from red to black if it has been successfully cleaned up by the garbage collector. The destructors on the BasePage and BaseViewModel classes should also emit a Debug.Writeline() to the console when they are cleaned up.

MemoryTestApp.zip

brentpbc avatar Jun 04 '24 07:06 brentpbc

NavigationPage has circular references, which would be unrelated to modals:

  • https://github.com/dotnet/maui/pull/22810

You can just do this to see it:

// The original NavigationPage & children leak
Application.Current.MainPage = new NavigationPage(new Page1());
Application.Current.MainPage = new Page2();

jonathanpeppers avatar Jun 04 '24 13:06 jonathanpeppers

Does the problem still occur with the 8.0.60 service release? The issue with NavigationPage was resolved there.

jonathanpeppers avatar Jun 18 '24 14:06 jonathanpeppers

@jonathanpeppers I have tried the latest 8.0.60 version , however I can see that the memory is still not released. There are a couple problems I can see. First and foremost, NavigationRenderer has a hard reference to the Current Element public VisualElement Element { get => _viewHandlerWrapper.Element ?? _element; } This means that the page has a circular reference as the NavigationPage.Handler has a reference to the Renderer. So what we do currently is

public class CustomNavigationHandler : NavigationRenderer
{
	protected override void Dispose(bool disposing)
	{
		base.Dispose(disposing);
		SetElement(null);
	}
}

Secondly, there is a more common behavior that we can see. Currently, Handlers disposure is not connected to anything. As Microsoft documentation says, the developers "should" decide when to disconnect the handler of each Element. However, because each VisualElement has a handler with a hard reference, the page will not be release until page.Handler?.DisconnectHandler() is called.

My question is, is it still true that this is the way the framework is indented? This would mean that each MAUI dev would need to in one way or another extend the Navigation service and release elements manually. We have tried previously to hook this into Unloaded as specified in docs, but Unloaded is being called when we navigate away from a page, either in its Navigation stack or in a TabbedPage, making Unloaded unreliable event for page disposure.

dalux-era avatar Jun 19 '24 07:06 dalux-era

@era-maui does the problem go away if you use the stock NavigationPage as-is?

After #22810, NavigationPage is able to go away in the test and attached sample app. In your example, what type lives forever?

As for the architectural questions, I'm on the .NET for Android team, so I don't know why some of the decisions were made. I'm just trying to help fix some of these issues. Thanks!

jonathanpeppers avatar Jun 19 '24 17:06 jonathanpeppers

Hi @jonathanpeppers the Memory leak still exists in 8.0.60 on iOS, see attached repo. Tap "Modal with Navigation Page" > Tap Done > Tap/spam Collect GC, the page is never collected.

MemoryTestApp 8.0.60.zip

Screenshot 2024-06-20 at 9 38 59 am

brentpbc avatar Jun 19 '24 23:06 brentpbc

@jonathanpeppers Understandable. I tested with a regular NavigationPage and the results were the same. I looked at the test from the commit you referenced. Are we sure that CreateHandlerAndAddToWindow is equivalent to await Navigation.PushModalAsyncFixed(new NavigationPage(page)); ? Does it engage the NavigationRenderer the same way?

dalux-era avatar Jun 20 '24 07:06 dalux-era

I'm testing your latest sample today, thanks for sharing. It's possible there is something we missed.

jonathanpeppers avatar Jun 20 '24 14:06 jonathanpeppers

I think this will fix it:

  • https://github.com/dotnet/maui/pull/23164

jonathanpeppers avatar Jun 20 '24 15:06 jonathanpeppers

Thanks @jonathanpeppers !

brentpbc avatar Jun 23 '24 23:06 brentpbc