Skip to content

Commit

Permalink
[windows] fix memory leak when CollectionView.ItemsSource changes
Browse files Browse the repository at this point in the history
Fixes: dotnet#13393
Context: https://github.com/ivan-todorov-progress/maui-collection-view-memory-leak-bug

Debugging the above sample, I found that `ItemsView._logicalChildren`
grew indefinitely in size if you replace a `CollectionView`'s
`ItemsSource` over and over.

I could fix this by creating a new `internal`
`ItemsView.ClearLogicalChildren()` method and call it from the Windows
`ItemsViewHandler`.

I could reproduce this issue in a Device Test running on Windows.

It appears the problem does not occur on Android or iOS due to the
recycling behavior of the underlying platforms.

For example, Android calls `OnViewRecycled()` which calls
`RemoveLogicalChild(view)`:

* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/Adapters/ItemsViewAdapter.cs#L52-L57
* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/TemplatedItemViewHolder.cs#L37-L45

On Windows, we merely have a `ItemContentControl` and there is no
callback for when things are recycled. We *do* get a callback for when
the `FormsDataContext` value changes, so an option is to clear the
list in this case.

After this change, my test passes -- but it was already passing on iOS
and Android.
  • Loading branch information
jonathanpeppers committed Feb 24, 2023
1 parent 33240ea commit c7837db
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ protected virtual void CleanUpCollectionViewSource()
CollectionViewSource.Source = null;
CollectionViewSource = null;
}
VirtualView?.ClearLogicalChildren();

if (VirtualView?.ItemsSource == null)
{
Expand Down
9 changes: 9 additions & 0 deletions src/Controls/src/Core/Items/ItemsView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,15 @@ public void RemoveLogicalChild(Element element)
VisualDiagnostics.OnChildRemoved(this, element, oldLogicalIndex);
}

internal void ClearLogicalChildren()
{
// Reverse for-loop, so children can be removed while iterating
for (int i = _logicalChildren.Count - 1; i >= 0; i--)
{
RemoveLogicalChild(_logicalChildren[i]);
}
}

internal override IReadOnlyList<Element> LogicalChildrenInternal => _logicalChildren.AsReadOnly();

internal static readonly BindableProperty InternalItemsLayoutProperty =
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
using Microsoft.Maui.Controls;
using System;
using System.Collections;
using System.Collections.ObjectModel;
using System.Reflection;
using System.Threading.Tasks;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Controls.Handlers.Items;
using Microsoft.Maui.DeviceTests.Stubs;
using Microsoft.Maui.Handlers;
Expand Down Expand Up @@ -28,5 +33,49 @@ void SetupBuilder()
});
});
}

[Fact]
public async Task ItemsSourceDoesNotLeak()
{
SetupBuilder();

IList logicalChildren = null;
WeakReference weakReference = null;
var collectionView = new CollectionView
{
ItemTemplate = new DataTemplate(() => new Label())
};

await CreateHandlerAndAddToWindow<CollectionViewHandler>(collectionView, async handler =>
{
var data = new ObservableCollection<string>()
{
"Item 1",
"Item 2",
"Item 3"
};
weakReference = new WeakReference(data);
collectionView.ItemsSource = data;
await Task.Delay(100);
// Get ItemsView._logicalChildren
var flags = BindingFlags.NonPublic | BindingFlags.Instance;
logicalChildren = typeof(ItemsView).GetField("_logicalChildren", flags).GetValue(collectionView) as IList;
Assert.NotNull(logicalChildren);
// Replace with cloned collection
collectionView.ItemsSource = new ObservableCollection<string>(data);
await Task.Delay(100);
});

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

Assert.NotNull(weakReference);
Assert.False(weakReference.IsAlive, "ObservableCollection should not be alive!");
Assert.NotNull(logicalChildren);
Assert.Equal(3, logicalChildren.Count);
}
}
}

0 comments on commit c7837db

Please sign in to comment.