Skip to content

Commit

Permalink
[controls] fix memory leak with CarouselView & `INotifyCollectionCh…
Browse files Browse the repository at this point in the history
…anged` (#18267)

Context: #17726

In investigating #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 #17726, as there is at least
one further leak on Android from my testing. But this is a good step
forward.
  • Loading branch information
jonathanpeppers authored Nov 7, 2023
1 parent 25249e8 commit 6f073d6
Show file tree
Hide file tree
Showing 10 changed files with 153 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public static IItemsViewSource Create(IEnumerable itemsSource, BindableObject co
case IList list when itemsSource is INotifyCollectionChanged:
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);
case IList list:
return new ListSource(list);
case IEnumerable<object> generic:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,19 @@ internal class ObservableItemsSource : IItemsViewSource, IObservableItemsViewSou
readonly IEnumerable _itemsSource;
readonly BindableObject _container;
readonly ICollectionChangedNotifier _notifier;
readonly WeakNotifyCollectionChangedProxy _proxy = new();
readonly NotifyCollectionChangedEventHandler _collectionChanged;
bool _disposed;

~ObservableItemsSource() => _proxy.Unsubscribe();

public ObservableItemsSource(IEnumerable itemSource, BindableObject container, ICollectionChangedNotifier notifier)
{
_itemsSource = itemSource as IList ?? itemSource as IEnumerable;
_itemsSource = itemSource;
_container = container;
_notifier = notifier;

((INotifyCollectionChanged)itemSource).CollectionChanged += CollectionChanged;
_collectionChanged = CollectionChanged;
_proxy.Subscribe((INotifyCollectionChanged)itemSource, _collectionChanged);
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ public partial class CarouselViewHandler : ItemsViewHandler<CarouselView>
WScrollBarVisibility? _horizontalScrollBarVisibilityWithoutLoop;
WScrollBarVisibility? _verticalScrollBarVisibilityWithoutLoop;
Size _currentSize;
NotifyCollectionChangedEventHandler _collectionChanged;
readonly WeakNotifyCollectionChangedProxy _proxy = new();

~CarouselViewHandler() => _proxy.Unsubscribe();

protected override IItemsLayout Layout { get; }

Expand All @@ -47,9 +51,7 @@ protected override void DisconnectHandler(ListViewBase platformView)
if (ListViewBase != null)
{
ListViewBase.SizeChanged -= OnListViewSizeChanged;

if (CollectionViewSource?.Source is ObservableItemTemplateCollection observableItemsSource)
observableItemsSource.CollectionChanged -= OnCollectionItemsSourceChanged;
_proxy.Unsubscribe();
}

if (_scrollViewer != null)
Expand Down Expand Up @@ -120,7 +122,10 @@ protected override CollectionViewSource CreateCollectionViewSource()
GetItemHeight(), GetItemWidth(), GetItemSpacing(), MauiContext);

if (collectionViewSource is ObservableItemTemplateCollection observableItemsSource)
observableItemsSource.CollectionChanged += OnCollectionItemsSourceChanged;
{
_collectionChanged ??= OnCollectionItemsSourceChanged;
_proxy.Subscribe(observableItemsSource, _collectionChanged);
}

return new CollectionViewSource
{
Expand Down
Loading

0 comments on commit 6f073d6

Please sign in to comment.