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.

Sign up for GitHub

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

Jump to bottom

Page-level memory leak in NavigationPage on iOS #20119

Closed
AdamEssenmacher opened this issue Jan 23, 2024 · 3 comments · Fixed by #22810
Closed

Page-level memory leak in NavigationPage on iOS #20119

AdamEssenmacher opened this issue Jan 23, 2024 · 3 comments · Fixed by #22810
Assignees
@jonathanpeppers
Labels
area-navigation NavigationPage delighter-sc fixed-in-8.0.60 fixed-in-9.0.0-preview.6.24327.7 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 🍎 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Milestone
.NET 8 SR6

Comments

@AdamEssenmacher
Copy link

AdamEssenmacher commented Jan 23, 2024 •
edited by PureWeen
Loading

Description

NavigationPages are on iOS are not garbage collected after being removed.

Page-level leaks are serious, as they prevent child elements (in this case, whole child pages) from being collected as well.

Steps to Reproduce

  1. Set Application.Current.MainPage to a NavigationPage
  2. Swap Application.Current.MainPage with a different page
  3. Force GC runs
  4. Observe navigation page (and its children) never collected

Link to public reproduction project repository

https://github.com/AdamEssenmacher/iOSNavigationPageLeak.Maui

Version with bug

8.0.3

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

iOS 17.2

Did you find any workaround?

Calling DisconnectHandler() on the NavigationPage's Handler before removing it from use will prevent the navigation page and its children from leaking.

Relevant log output

No response
@AdamEssenmacher AdamEssenmacher added the t/bug Something isn't working label Jan 23, 2024
@samhouts samhouts added the area-navigation NavigationPage label Jan 25, 2024
@AdamEssenmacher
Copy link
Author

Would the area/perf label be appropriate here as well? This issue causes entire pages (and possibly nav stacks) not to be GC'ed, which causes noticeable performance degradation once the app starts needing swap space, and ultimately leading to forced app termination.

@PureWeen PureWeen added the legacy-area-perf Startup / Runtime performance label Jan 26, 2024
@PureWeen PureWeen added this to the Backlog milestone Jan 26, 2024
@ghost
Copy link

ghost commented Jan 26, 2024

We've added this issue to our backlog, and we will work to address it as time and resources allow. If you have any additional information or questions about this issue, please leave a comment. For additional info about issue management, please read our Triage Process.

@AdamEssenmacher AdamEssenmacher mentioned this issue Apr 12, 2024
Closed
@PureWeen PureWeen added the memory-leak 💦 Memory usage grows / objects live forever (sub: perf) label Apr 25, 2024
@PureWeen PureWeen modified the milestones: Backlog, .NET 8 SR6 Apr 25, 2024
@kevinxufei kevinxufei added the s/triaged Issue has been reviewed label Apr 29, 2024
@kevinxufei
Copy link

Can repro this issue at iOS platform on the latest 17.10 Preview 6(8.0.3/8.0.21).

@kevinxufei kevinxufei added the s/verified Verified / Reproducible Issue ready for Engineering Triage label May 7, 2024
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
@samhouts samhouts 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 30, 2024
@jonathanpeppers jonathanpeppers self-assigned this Jun 3, 2024
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Jun 3, 2024
@jonathanpeppers
[ios/catalyst] fix leak in NavigationPage
b2dfc11 
Fixes: dotnet#20119

PR dotnet#13833 fixed a leak in child pages of a `NavigationPage`, but it
appears there are several cycles that would prevent the `NavigationPage`
itself from going away.

So, if you did something like this:

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

I could reproduce the same problem in a new device test.

The cycles (and solutions) are:

1. `NavigationPage` -> `NavigationRenderer` -> `ParentingViewController` -> `NavigationPage`

    * Solution: make `_child` a `WeakReference`

2. `NavigationPage` -> `MenuItemTracker` -> `NavigationPage`

    * Solution: make `_target` a `WeakReference`, and `_additionalTargets` a `List<WeakReference>`

3. `NavigationPage` -> `MenuItemTracker.CollectionChanged` -> `NavigationPage`

    * Solution: subscribe/unsubscribe in `WillMoveToParentViewController`

After these changes the sample app works, and the new device test passes.
@jonathanpeppers jonathanpeppers mentioned this issue Jun 3, 2024
Merged
@github-actions github-actions bot mentioned this issue Jun 4, 2024
Closed
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Jun 4, 2024
@jonathanpeppers
[ios/catalyst] fix leak in NavigationPage
f457d03 
Fixes: dotnet#20119

PR dotnet#13833 fixed a leak in child pages of a `NavigationPage`, but it
appears there are several cycles that would prevent the `NavigationPage`
itself from going away.

So, if you did something like this:

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

I could reproduce the same problem in a new device test.

The cycles (and solutions) are:

1. `NavigationPage` -> `NavigationRenderer` -> `ParentingViewController` -> `NavigationPage`

    * Solution: make `_child` a `WeakReference`

2. `NavigationPage` -> `MenuItemTracker` -> `NavigationPage`

    * Solution: make `_target` a `WeakReference`, and `_additionalTargets` a `List<WeakReference>`

3. `NavigationPage` -> `MenuItemTracker.CollectionChanged` -> `NavigationPage`

    * Solution: subscribe/unsubscribe in `WillMoveToParentViewController`

After these changes the sample app works, and the new device test passes.
PureWeen pushed a commit that referenced this issue Jun 5, 2024
@jonathanpeppers
[ios/catalyst] fix leak in NavigationPage (#22810)
c8ce71e 
* [ios/catalyst] fix leak in NavigationPage

Fixes: #20119

PR #13833 fixed a leak in child pages of a `NavigationPage`, but it
appears there are several cycles that would prevent the `NavigationPage`
itself from going away.

So, if you did something like this:

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

I could reproduce the same problem in a new device test.

The cycles (and solutions) are:

1. `NavigationPage` -> `NavigationRenderer` -> `ParentingViewController` -> `NavigationPage`

    * Solution: make `_child` a `WeakReference`

2. `NavigationPage` -> `MenuItemTracker` -> `NavigationPage`

    * Solution: make `_target` a `WeakReference`, and `_additionalTargets` a `List<WeakReference>`

3. `NavigationPage` -> `MenuItemTracker.CollectionChanged` -> `NavigationPage`

    * Solution: subscribe/unsubscribe in `WillMoveToParentViewController`

After these changes the sample app works, and the new device test passes.

* Skip test on Windows for now
@github-project-automation github-project-automation bot moved this from Todo to Done in MAUI SDK Ongoing Jun 5, 2024
@github-actions github-actions bot mentioned this issue Jun 13, 2024
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jonathanpeppers jonathanpeppers

Labels
area-navigation NavigationPage delighter-sc fixed-in-8.0.60 fixed-in-9.0.0-preview.6.24327.7 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 🍎 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
MAUI SDK Ongoing
Status: Done
Milestone
.NET 8 SR6
Development

Successfully merging a pull request may close this issue.

[ios/catalyst] fix leak in NavigationPage jonathanpeppers/maui
6 participants
@Eilon @jonathanpeppers @PureWeen @samhouts @AdamEssenmacher @kevinxufei