iOS PopModalAsync causes memory leaks

[dalux-era]

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

[AdamEssenmacher]

Possible duplicate or related:

#20094
#20119

[mattleibow]

/similarissues

[github-actions]

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:

• Page-level memory leak on modal navigation on iOS (#20094), similarity score: 0.84
• Page-level memory leak in NavigationPage on iOS (#20119), similarity score: 0.81

Closed similar issues:

• Page weak reference is still alive after pushing and popping pages modally or normally (#21143), similarity score: 0.78
• Memory Leak on Page Navigation while removing the page. (#21403), similarity score: 0.78
• Navigation.PopModalAsync not working on iOS (#3959), similarity score: 0.78

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

[PureWeen]

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

[brentpbc]

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

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(); 

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

[jonathanpeppers]

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

• [ios/catalyst] fix leak in NavigationPage #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]

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

[dalux-era]

@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.

[jonathanpeppers]

@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!

[brentpbc]

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

[dalux-era]

@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?

[jonathanpeppers]

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

[jonathanpeppers]

I think this will fix it:

• [ios/catalyst] fix more cycles in NavigationPage #23164

[brentpbc]

Thanks @jonathanpeppers !