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

Memory leak while repeatedly navigating between pages #10578

Closed
Vroomer opened this issue Oct 9, 2022 · 2 comments · Fixed by #14329
Closed

Memory leak while repeatedly navigating between pages #10578

Vroomer opened this issue Oct 9, 2022 · 2 comments · Fixed by #14329
Assignees
Labels
area-navigation NavigationPage fixed-in-8.0.0-preview.4.8333 Look for this fix in 8.0.0-preview.4.8333! platform/windows 🪟 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)

Comments

@Vroomer
Copy link

Vroomer commented Oct 9, 2022

Description

Recurring navigation to a page and back causes memory leak. It can lead to extreme degradation in performance in a case of an app that uses TabbedPage with complex Views that contain a lot of controls.

In the app I'm developing I have a CollectionView of items which can be opened by user to show detailed information. The user has to be able to do such action hundreds of times during a session. However there seems to be something lingering in the memory which can't be collected by GC and isn't properly disposed.

I made a simple example consisting of two pages. The app is made so that you can easily push and pop a page. The pushed page consists only of a simple CollectionView.

  • Tested on MAUI 7.0.0-rc.1.6683.
  • The issue happens in Windows. I don't have the opportunity to test other platforms right now.
  • The problem occurs with both Shell and standard page navigation.
  • When there is no control on the pushed page, there is no leak.
  • It doesn't seem to apply only to CollectionView.
  • Instances of pages are kept in memory even after popping them. Triggering GC successfully collects them except for the last instance - not sure that is ideal.

Steps to Reproduce

  1. Load the repository and run the app.
  2. Repeatedly push and pop the page using the first button while observing memory usage in ResMon/memory profiler.
  3. Click on the "Force GC" button to trigger GC - it will free some memory, but the memory leak should be apparent. You will also see text output from WeakReferences to pushed pages and their collections.

Link to public reproduction project repository

https://github.com/Vroomer/MAUI-navigation-memory-leak.git

Version with bug

Unknown/Other (please specify)

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

Windows SDK 10.0.22000

Did you find any workaround?

No response

Relevant log output

No response

@Vroomer Vroomer added the t/bug Something isn't working label Oct 9, 2022
@jsuarezruiz jsuarezruiz added area-navigation NavigationPage legacy-area-perf Startup / Runtime performance platform/windows 🪟 labels Oct 10, 2022
@rmarinho rmarinho added this to the Backlog milestone Oct 10, 2022
@ghost
Copy link

ghost commented Oct 10, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Mar 30, 2023

At first thought, I thought this could be fixed by: #13833

But when I test the above sample, it leaks on Windows unless I make this change (<CollectionView x:Name="collectionView"):

private async void Button_Clicked(object sender, EventArgs e)
{
    await Navigation.PopAsync();
    // Fixes the leak?
    collectionView.Handler?.DisconnectHandler();
}

More investigation needed on this one, there must be another issue to solve.

@jonathanpeppers jonathanpeppers self-assigned this Mar 30, 2023
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Mar 31, 2023
Fixes: dotnet#10578
Context: https://github.com/Vroomer/MAUI-navigation-memory-leak.git

After testing the above sample, I found that adding a `CollectionView`
to a `Page`, makes it and the entire page live forever.

This is WIP, as it is still failing on iOS/Catalyst. This fixes Android
& Windows so far.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Apr 10, 2023
Fixes: dotnet#10578
Context: https://github.com/Vroomer/MAUI-navigation-memory-leak.git

After testing the above sample, I found that adding a `CollectionView`
to a `Page`, makes it and the entire page live forever.

This is WIP, as it is still failing on iOS/Catalyst. This fixes Android
& Windows so far.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Apr 11, 2023
Fixes: dotnet#10578
Context: https://github.com/Vroomer/MAUI-navigation-memory-leak.git

After testing the above sample, I found that adding a `CollectionView`
to a `Page`, makes it and the entire page live forever.

Android & Windows:

* `MauiRecyclerView` and `StructuredItemsViewHandler` respectively
  subscribed to `ItemsLayout.PropertyChanged`. This kept the
  `CollectionView` alive -> all the way up to the `Page`.

* Switched to using `WeakNotifyPropertyChangedProxy` solved the issue
  for these two platforms.

iOS:

* Generally had a small "nest" of circular references. Initially, I saw
  `CollectionView`, `UICollectionView`, and various helper classes that
  would live forever.

* `ItemsViewController : UICollectionViewController` ->
  * `ObservableItemsSource` ->
    * `UICollectionViewController` and `UICollectionView`

* `UICollectionView` ->
  * `ItemsViewLayout : UICollectionViewFlowLayout` ->
    * `Func<UICollectionViewCell>` ->
      * `ItemsViewController : UICollectionViewController` ->
        * `UICollectionView`

I switched to using `WeakReference<T>` to break the circular references.
This required several null checks, where `null` references were not
possible before.

After these changes my tests pass, yay!
jonathanpeppers added a commit that referenced this issue Apr 13, 2023
Fixes: #10578
Context: https://github.com/Vroomer/MAUI-navigation-memory-leak.git

After testing the above sample, I found that adding a `CollectionView`
to a `Page`, makes it and the entire page live forever.

Android & Windows:

* `MauiRecyclerView` and `StructuredItemsViewHandler` respectively
  subscribed to `ItemsLayout.PropertyChanged`. This kept the
  `CollectionView` alive -> all the way up to the `Page`.

* Switched to using `WeakNotifyPropertyChangedProxy` solved the issue
  for these two platforms.

iOS:

* Generally had a small "nest" of circular references. Initially, I saw
  `CollectionView`, `UICollectionView`, and various helper classes that
  would live forever.

* `ItemsViewController : UICollectionViewController` ->
  * `ObservableItemsSource` ->
    * `UICollectionViewController` and `UICollectionView`

* `UICollectionView` ->
  * `ItemsViewLayout : UICollectionViewFlowLayout` ->
    * `Func<UICollectionViewCell>` ->
      * `ItemsViewController : UICollectionViewController` ->
        * `UICollectionView`

I switched to using `WeakReference<T>` to break the circular references.
This required several null checks, where `null` references were not
possible before.

After these changes my tests pass, yay!
@ghost ghost locked as resolved and limited conversation to collaborators May 14, 2023
@samhouts samhouts modified the milestones: Backlog, .NET 8 May 24, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.4.8333 Look for this fix in 8.0.0-preview.4.8333! label Jun 8, 2023
@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-navigation NavigationPage fixed-in-8.0.0-preview.4.8333 Look for this fix in 8.0.0-preview.4.8333! platform/windows 🪟 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

Successfully merging a pull request may close this issue.

6 participants