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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[windows] fix memory leak in ListView #16762

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Aug 15, 2023

Context: https://github.com/Tamilarasan-Paranthaman/Maui-Itemssource-Switching-Issue
Fixes #16450

In a customer sample, swapping a ListView.ItemsSource back and forth, such as:

listView.ItemsSource = employeeInfoRepository.EmployeesInfo1;
listView.ItemsSource = employeeInfoRepository.EmployeesInfo2;

Appears to create new Cell's for each call -- and each new cell lives forever. I could reproduce this issue in a device test.

Viewing a memory snapshot, each Cell via Cell.Parent subscribes to events on their parent, the ListView:

This makes the ListView hold onto each Cell causing them to live forever. However, the leak would go away if you navigated away from the current Page, removed the ListView from the screen, etc.

Possible workarounds were:

  1. Use two ListView instead, and show/hide them.

  2. Use a CollectionView instead.

The fix I came up with appears to be reasonable. In CellControl.OnUnloaded:

void OnUnloaded(object sender, RoutedEventArgs e)
{
    // ...
    // 馃殌 unsubscribe from propertychanged
    Cell.PropertyChanged -= _propertyChangedHandler;
    // Allows the Cell to unsubscribe from Parent.PropertyChanged
    Cell.Parent = null;
}

This event is fired when WinUI has removed the CellControl from the visual tree. We can safely unset the Cell's Parent, which unsubscribes from the problematic events.

If the Cell is later re-added to the visual tree, it's DataContext will be updated. In CellControl's SetCell(object newContext) the Cell.Parent is reset to the ListView.

I added two test cases for this scenario: one for the leak, and one to prove that re-setting ItemsSource still works.

Context: https://github.com/Tamilarasan-Paranthaman/Maui-Itemssource-Switching-Issue
Fixes: dotnet#16450

In a customer sample, swapping a `ListView.ItemsSource` back and forth,
such as:

    listView.ItemsSource = employeeInfoRepository.EmployeesInfo1;
    listView.ItemsSource = employeeInfoRepository.EmployeesInfo2;

Appears to create new `Cell`'s for each call -- and each new cell lives
forever. I could reproduce this issue in a device test.

Viewing a memory snapshot, each `Cell` via `Cell.Parent` subscribes to
events on their parent, the `ListView`:

* https://github.com/dotnet/maui/blob/9fcccd57b5a3d664e4788b3c7fc3edc10010beaf/src/Controls/src/Core/Cells/Cell.cs#L197-L201
* https://github.com/dotnet/maui/blob/9fcccd57b5a3d664e4788b3c7fc3edc10010beaf/src/Controls/src/Core/Element/Element.cs#L322

This makes the `ListView` hold onto each `Cell` causing them to live
forever. However, the leak would go away if you navigated away from the
current `Page`, removed the `ListView` from the screen, etc.

Possible workarounds were:

1. Use two `ListView` instead, and show/hide them.

2. Use a `CollectionView` instead.

The fix I came up with appears to be reasonable. In
`CellControl.OnUnloaded`:

    void OnUnloaded(object sender, RoutedEventArgs e)
    {
        // ...
        // 馃殌 unsubscribe from propertychanged
        Cell.PropertyChanged -= _propertyChangedHandler;
        // Allows the Cell to unsubscribe from Parent.PropertyChanged
        Cell.Parent = null;
    }

This event is fired when WinUI has removed the `CellControl` from the
visual tree. We can safely *unset* the `Cell`'s `Parent`, which
unsubscribes from the problematic events.

If the `Cell` is later re-added to the visual tree, it's `DataContext`
will be updated. In `CellControl`'s `SetCell(object newContext)` the
`Cell.Parent` is reset to the `ListView`.

I added two test cases for this scenario: one for the leak, and one to
prove that re-setting `ItemsSource` still works.
@jonathanpeppers
Copy link
Member Author

I haven't checked the other platforms yet... I wonder if they are going to leak as well.

@Eilon Eilon added the area-controls-listview ListView and TableView label Aug 15, 2023
@samhouts samhouts added this to the .NET 8 GA milestone Aug 15, 2023
@samhouts samhouts added the memory-leak 馃挦 Memory usage grows / objects live forever label Aug 15, 2023
@jonathanpeppers
Copy link
Member Author

The CellsDoNotLeak test fails on both iOS & Android. Let me investigate more, to see if I should try to fix all 3 platforms at once.

It could be the way cells recycle on those platforms, the test might always fail.

@jonathanpeppers jonathanpeppers marked this pull request as ready for review August 17, 2023 14:29
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner August 17, 2023 14:29
@rmarinho rmarinho merged commit 040f2f1 into dotnet:main Aug 17, 2023
39 checks passed
@jonathanpeppers jonathanpeppers deleted the ListViewOhNo branch August 17, 2023 19:06
rmarinho pushed a commit that referenced this pull request Aug 19, 2023
* [windows] fix memory leak in ListView

Context: https://github.com/Tamilarasan-Paranthaman/Maui-Itemssource-Switching-Issue
Fixes: #16450

In a customer sample, swapping a `ListView.ItemsSource` back and forth,
such as:

    listView.ItemsSource = employeeInfoRepository.EmployeesInfo1;
    listView.ItemsSource = employeeInfoRepository.EmployeesInfo2;

Appears to create new `Cell`'s for each call -- and each new cell lives
forever. I could reproduce this issue in a device test.

Viewing a memory snapshot, each `Cell` via `Cell.Parent` subscribes to
events on their parent, the `ListView`:

* https://github.com/dotnet/maui/blob/9fcccd57b5a3d664e4788b3c7fc3edc10010beaf/src/Controls/src/Core/Cells/Cell.cs#L197-L201
* https://github.com/dotnet/maui/blob/9fcccd57b5a3d664e4788b3c7fc3edc10010beaf/src/Controls/src/Core/Element/Element.cs#L322

This makes the `ListView` hold onto each `Cell` causing them to live
forever. However, the leak would go away if you navigated away from the
current `Page`, removed the `ListView` from the screen, etc.

Possible workarounds were:

1. Use two `ListView` instead, and show/hide them.

2. Use a `CollectionView` instead.

The fix I came up with appears to be reasonable. In
`CellControl.OnUnloaded`:

    void OnUnloaded(object sender, RoutedEventArgs e)
    {
        // ...
        // 馃殌 unsubscribe from propertychanged
        Cell.PropertyChanged -= _propertyChangedHandler;
        // Allows the Cell to unsubscribe from Parent.PropertyChanged
        Cell.Parent = null;
    }

This event is fired when WinUI has removed the `CellControl` from the
visual tree. We can safely *unset* the `Cell`'s `Parent`, which
unsubscribes from the problematic events.

If the `Cell` is later re-added to the visual tree, it's `DataContext`
will be updated. In `CellControl`'s `SetCell(object newContext)` the
`Cell.Parent` is reset to the `ListView`.

I added two test cases for this scenario: one for the leak, and one to
prove that re-setting `ItemsSource` still works.

* Update ListViewTests.cs
@deviprasad987
Copy link

@jonathanpeppers @rmarinho @Eilon @samhouts @radical

In which VS version this memory leak issue will be fixed?

@Eilon
Copy link
Member

Eilon commented Sep 14, 2023

@deviprasad987 the .NET 8 RC1 release from this week should have this fix in it. More info on the release here: https://devblogs.microsoft.com/dotnet/announcing-dotnet-8-rc1/

Redth added a commit that referenced this pull request Nov 1, 2023
Adds a couple other tests that have since been added in net8.0, but also port over cleanly to net7.0
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-listview ListView and TableView memory-leak 馃挦 Memory usage grows / objects live forever platform/windows 馃獰
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory Leak issue in ListView
5 participants