Skip to content

Commit

Permalink
[core] fix memory leak with CarouselView & INotifyCollectionChanged
Browse files Browse the repository at this point in the history
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 solve the problem, I used the same technique in 58a42e5. We can use
`WeakNotifyCollectionChangedProxy` in the `ObservableItemsSource` class
to prevent `CarouselView` from leaking. `ObservableItemsSource` is used
for other controls, so this may fix other leaks as well.

Unfortunately, this does not fully solve dotnet#17726, as there is at least
one further leak on Android from my testing.
  • Loading branch information
jonathanpeppers committed Oct 23, 2023
1 parent 0049595 commit c3c996b
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 4 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
15 changes: 15 additions & 0 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using System;
using System.Collections.ObjectModel;
using System.Threading.Tasks;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Controls.Handlers.Compatibility;
using Microsoft.Maui.Controls.Handlers.Items;
using Microsoft.Maui.Handlers;
using Microsoft.Maui.Hosting;
using Xunit;
Expand All @@ -18,6 +20,7 @@ void SetupBuilder()
builder.ConfigureMauiHandlers(handlers =>
{
handlers.AddHandler<Border, BorderHandler>();
handlers.AddHandler<CarouselView, CarouselViewHandler>();
handlers.AddHandler<CheckBox, CheckBoxHandler>();
handlers.AddHandler<DatePicker, DatePickerHandler>();
handlers.AddHandler<Entry, EntryHandler>();
Expand All @@ -38,6 +41,7 @@ void SetupBuilder()

[Theory("Handler Does Not Leak")]
[InlineData(typeof(Border))]
[InlineData(typeof(CarouselView))]
[InlineData(typeof(ContentView))]
[InlineData(typeof(CheckBox))]
[InlineData(typeof(DatePicker))]
Expand Down Expand Up @@ -65,11 +69,22 @@ public async Task HandlerDoesNotLeak(Type type)
WeakReference platformViewReference = null;
WeakReference handlerReference = null;

var observable = new ObservableCollection<int> { 1, 2, 3 };

await InvokeOnMainThreadAsync(() =>
{
var layout = new Grid();
var view = (View)Activator.CreateInstance(type);
layout.Add(view);
if (view is ContentView content)
{
content.Content = new Label();
}
else if (view is ItemsView items)
{
items.ItemTemplate = new DataTemplate(() => new Label());
items.ItemsSource = observable;
}
var handler = CreateHandler<LayoutHandler>(layout);
viewReference = new WeakReference(view);
handlerReference = new WeakReference(view.Handler);
Expand Down

0 comments on commit c3c996b

Please sign in to comment.