[controls] fix memory leak with CarouselView & INotifyCollectionChanged#18267
Merged
jonathanpeppers merged 3 commits intodotnet:mainfrom Nov 7, 2023
Merged
Conversation
jonathanpeppers
commented
Oct 23, 2023
| return new ObservableItemsSource(new MarshalingObservableCollection(list), container, notifier); | ||
| case IEnumerable _ when itemsSource is INotifyCollectionChanged: | ||
| return new ObservableItemsSource(itemsSource as IEnumerable, container, notifier); | ||
| return new ObservableItemsSource(itemsSource, container, notifier); |
Member
Author
There was a problem hiding this comment.
This as was just extra, the itemsSource is already IEnumerable.
Comment on lines
+24
to
+25
| _collectionChanged = CollectionChanged; | ||
| _proxy.Subscribe((INotifyCollectionChanged)itemSource, _collectionChanged); |
Member
Author
There was a problem hiding this comment.
We store CollectionChanged in a field, so it will remain alive as long as the ObservableItemsSource.
Details here: https://github.com/dotnet/maui/wiki/Memory-Leaks#when-we-cant-use-weakeventmanager
Member
Author
|
The new test fails on iOS & Windows, looking into it. |
Member
Author
|
This one ended up being a big mess: different problems on each platform... If this is green, I'll rebase and explain in a better PR description/commit message. |
…anged` Context: dotnet#17726 In investigating dotnet#17726, I found a memory leak with `CarouselView`: 1. Have a long-lived `INotifyCollectionChanged` like `ObservableCollection`, that lives the life of the application. 2. Set `CarouselView.ItemsSource` to the collection. 3. `CarouselView` will live forever, even if the page is popped, etc. I further expanded upon `MemoryTests` to assert more things for each control, and was able to reproduce this issue. To fully solve this, I had to fix issues on each platform. ~~ Android ~~ `ObservableItemsSource` subscribes to the `INotifyCollectionChanged` event, making it live forever in this case. To fix this, I used the same technique in 58a42e5: `WeakNotifyCollectionChangedProxy`. `ObservableItemsSource` is used for `ItemsView`-like controls, so this may fix other leaks as well. ~~ Windows ~~ Windows has the same issue as Android, but for the following types: * `CarouselViewHandler` subscribes to `INotifyCollectionChanged` * `ObservableItemTemplateCollection` subscribes to `INotifyCollectionChanged` These could be fixed with `WeakNotifyCollectionChangedProxy`. Then I found another issue with `ItemTemplateContext` These three types are used for other `ItemsView`-like controls, so this may fix other leaks as well. ~~ iOS/Catalyst ~~ These platforms suffered from multiple circular references: * `CarouselViewController` -> `CarouselView` -> `CarouselViewHandler` -> `CarouselViewController` * `ItemsViewController` -> `CarouselView` -> `CarouselViewHandler` -> `ItemsViewController` (`CarouselViewController` subclasses this) * `CarouselViewLayout` -> `CarouselView` -> `CarouselViewHandler` -> `CarouselViewLayout` The analyzer added in 2e65626 did not yet catch these because I only added the analyzer so far for `Microsoft.Maui.dll` and these issues were in `Microsoft.Maui.Controls.dll`. In a future PR, we can proactively try to add the analyzer to all projects. ~~ Conclusion ~~ Unfortunately, this does not fully solve dotnet#17726, as there is at least one further leak on Android from my testing. But this is a good step forward.
fdb2bef to
b1d8176
Compare
CarouselView & INotifyCollectionChangedCarouselView & INotifyCollectionChanged
21 tasks
rmarinho
reviewed
Oct 31, 2023
src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs
Outdated
Show resolved
Hide resolved
rmarinho
approved these changes
Nov 7, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context: #17726
In investigating #17726, I found a memory leak with
CarouselView:Have a long-lived
INotifyCollectionChangedlikeObservableCollection, that lives the life of the application.Set
CarouselView.ItemsSourceto the collection.CarouselViewwill live forever, even if the page is popped, etc.I further expanded upon
MemoryTeststo assert more things for eachcontrol, and was able to reproduce this issue.
To fully solve this, I had to fix issues on each platform.
Android
ObservableItemsSourcesubscribes to theINotifyCollectionChangedevent, making it live forever in this case.
To fix this, I used the same technique in 58a42e5:
WeakNotifyCollectionChangedProxy.ObservableItemsSourceis used forItemsView-like controls, so this may fix other leaks as well.Windows
Windows has the same issue as Android, but for the following types:
CarouselViewHandlersubscribes toINotifyCollectionChangedObservableItemTemplateCollectionsubscribes toINotifyCollectionChangedThese could be fixed with
WeakNotifyCollectionChangedProxy.Then I found another issue with
ItemTemplateContext, it holds a strongreference to
BindableObject Container, theCarouselView. I foundthis kept the
CarouselViewalive indefinitely, but changing to aWeakReferencesolved this issue.These three types are used for other
ItemsView-like controls, so thismay fix other leaks as well.
iOS/Catalyst
These platforms suffered from multiple circular references:
CarouselViewController->CarouselView->CarouselViewHandler->CarouselViewControllerItemsViewController->CarouselView->CarouselViewHandler->ItemsViewController(CarouselViewControllersubclasses this)CarouselViewLayout->CarouselView->CarouselViewHandler->CarouselViewLayoutThe analyzer added in 2e65626 did not yet catch these because I only
added the analyzer so far for
Microsoft.Maui.dlland these issues werein
Microsoft.Maui.Controls.dll. In a future PR, we can proactively tryto add the analyzer to all projects.
Conclusion
Unfortunately, this does not fully solve #17726, as there is at least
one further leak on Android from my testing. But this is a good step
forward.