[iOS] Fixed memory leak with PopToRootAsync when using TitleView #28547
Conversation
|
Hey there @Vignesh-SF3580! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
dotnet-policy-service
bot
added
community ✨
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a memory leak issue by ensuring that the TitleView is correctly disposed when navigating back to the root page on iOS. Key changes include:
- Adding disposal logic in NavigationRenderer.cs for popped view controllers.
- Introducing new test cases in both TestCases.Shared.Tests and TestCases.HostApp to validate that the TitleView is disposed.
- Updating navigation behavior to prevent memory leaks during PopToRootAsync.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue28201.cs | Adds a test case to check page disposal behavior |
| src/Controls/tests/TestCases.HostApp/Issues/Issue28201.xaml.cs | Updates the UI test page for validating TitleView disposal |
| src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs | Enhances PopToRootAsync to dispose popped controllers properly |
Files not reviewed (1)
- src/Controls/tests/TestCases.HostApp/Issues/Issue28201.xaml: Language not supported
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue28201.cs
Outdated
Show resolved
Hide resolved
|
The title is bit confusing. Does the leak has something to do with |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| @@ -310,11 +310,26 @@ protected virtual async Task<bool> OnPopToRoot(Page page, bool animated) | |||
|
|
|||
| var task = GetAppearedOrDisappearedTask(page); | |||
|
|
|||
| PopToRootViewController(animated); | |||
| var poppedControllers = PopToRootViewController(animated); | |||
There was a problem hiding this comment.
A this point, this method should be responsible for managing the cleanup of the ViewController that is currently at the top of the view controller stack. It ensures proper disposal of resources if the user action leads to a view being removed from the visual stack. I think inside this method, we should ensure we dispose the TitleView if exists.
Something like:
NavigationItem.TitleView.Dispose();
There was a problem hiding this comment.
@jsuarezruiz In PopToRootViewController, the removal and disposal of controllers are restricted by the _ignorePopCall variable. Removing _ignorePopCall resolves the issue, as the removed controllers are disposed of in the RemoveViewControllers method. However, I am unsure if removing _ignorePopCall could break any existing behaviors, as the same approach is followed in Xamarin.
Therefore, I applied the fix by calling Disconnect, referring to the approach used in PopViewAsync. Could you please review and share your thoughts?
Yes, the destructor is not invoked when using PopToRootAsync with TitleView. However, it is invoked properly when TitleView is not used. |
@jsuarezruiz The issue is only reproduced when using TitleView through a XAML file and does not occur when using it through C# code. Therefore, I added a UI test to verify this issue. |
jfversluis
added
area-controls-titleview
Issue Detail
When navigating using PushAsync and PopToRootAsync, the destructor was not invoked when using PopToRootAsync.
Root Cause
The TitleView was not properly disposed when using PopToRootAsync.
Description of Change
Resolved the issue by disposing the TitleView using Disconnect, similar to PopAsync.
Reference: https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs#L345
Issues Fixed
Fixes #28201
Screenshots
28201BeforeFix.mov
28201AfterFix.mov