[iOS] Clear BindingContext when cell is queued for reuse

[filipnavara]

trafficstars

Description of Change

This avoid holding references to objects that were already removed. Previously UICollectionView would cache the cells for reuse and hold the references to bound objects for undefined period of time.

[github-actions[bot]]

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

[jfversluis]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[filipnavara]

error RS0016: Symbol 'PrepareForReuse' is not part of the declared public API... I suppose I need to add it to some list? (the API needs to be public since it's OS framework API).

[MartyIX]

The repo uses https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md. The APIs are listed in files like https://github.com/dotnet/maui/blob/main/src/Core/src/PublicAPI/net-ios/PublicAPI.Shipped.txt.

It seems that Visual Studio might help you with a lightbulb suggestion to modify text files for you.

No guarrantees. Just a passerby.

[jfversluis]

Aah yeah, I should make a doc for this at some point I guess... What @MartyIX says is pretty much it! It should give you red squiggles, let IntelliSense fix it.

Let me know if you can't figure it out!

[filipnavara]

Let me know if you can't figure it out!

I did figure it out but apparently there's something broken on GitHub since it doesn't show the new commit in the PR (despite being visible on the branch if I click through).

[rmarinho]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[filipnavara]

You need the public api changes also for net7.0-maccatalyst

Done.

[rmarinho]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[jonathanpeppers]

I think we need a device test for CollectionView on iOS:

1. Set ItemsSource to something
2. Find the underlying UICollectionViewCell and/or other MAUI views, save them in a WeakReference
3. Clear ItemsSource
4. Run the GC
5. Assert WeakReference.IsAlive is false

Maybe someone on the MAUI team can help write this test? I don't know how to do step 2.

Here is an example with NavigationPage:

https://github.com/dotnet/maui/blob/2b823f8503748f115028e671b4fff9048628c462/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs#L295-L296

[filipnavara]

There should be a way to observe it like this:

• Create collection view with some items
• Make sure it's presented, there are cell views in LogicalChildren and they have BindingContext assigned
• Clear the items source for the collection view
• Check the LogicalChildren again and make sure that each of them has BindingContext == null.

That doesn't necessarily ensure that all references to the items are gone but it's observable that this specific one is cleared.

(Unfortunately I left my laptop at office so I cannot look into writing it now...)

[jonathanpeppers]

@filipnavara I found this test:

https://github.com/dotnet/maui/blob/2b823f8503748f115028e671b4fff9048628c462/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs#L37-L38

It asserts that after a given CollectionView's ItemsSource is cleared, the ObservableCollection can be GC'd.

Does it prove there might not be a problem here? Or do we need to adjust this test? It might not reproduce the problem exactly.

[jonathanpeppers]

This passes on Windows with dotnet/maui/main, but I wonder if it fails on iOS/Catalyst:

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

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

	await CreateHandlerAndAddToWindow<CollectionViewHandler>(collectionView, async handler =>
	{
		var data = new ObservableCollection<MyRecord>()
		{
			new MyRecord("Item 1"),
			new MyRecord("Item 2"),
			new MyRecord("Item 3"),
		};
		foreach (var item in data)
			items.Add(new WeakReference(item));
		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);

		// Clear collection
		collectionView.ItemsSource = null;
		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.True(logicalChildren.Count <= 3, "_logicalChildren should not grow in size!");

	foreach (var item in items)
	{
		Assert.False(weakReference.IsAlive, "MyRecord should not be alive!");
	}
}

record MyRecord(string Name);

[filipnavara]

This passes on Windows with dotnet/maui/main, but I wonder if it fails on iOS/Catalyst:

Good find. I'll have to figure out how to debug the iOS tests in my environment. Few observations by just reading it though:

• Setting ItemsSource = null may not behave the same as removing/clearing items from an observable source.
• MonoVM doesn't have precise GC which makes me wonder how reliable the test would be in the first place when running on iOS.
• The weak references are observed after CreateHandlerAndAddToWindow was run. I am not familiar with the test structure. Does that guarantee the window still exist with the collection view and its handler after the callback was run? (I assume it does but double checking.)

[filipnavara]

I managed to run the test. I am not entirely sure what is going on yet, but the logicalChildren collection has exactly one label, even when it should have three... perhaps it's just not big enough to actually show the items, so the cells are not even created.

[filipnavara]

I modified the test to actually match my comment above only to discover that it doesn't actually quite solve the issue described in the PR's first comment. I had to apply few explicit width/height requests to get everything to display but that was relatively easy.

Now I have to take a step back to explain why I pursued this in the first place and why it helps my case but doesn't quite fix the leak.

We use a custom collection that is backed by a database and in-memory cache and we often bind an item source with 10k-100k items into the collection view. There are some quirks about how this is handled. One of them is that some objects could be reloaded from database and no longer satisfy reference equality (to be resolved by #14613) while still satisfying object.Equals equality. Other quirk is that we may issue a Replace notification for several items where the e.OldItems == e.NewItems. Essentially, we don't implement PropertyChanged on each item because it would be prohibitively expensive and instead issue those Replace notifications on the collection. The problem with this is that it doesn't play well with the MAUI data binding. When we do that, the old cells are queued for reuse, but they are also immediately reused for the same items. In some cases, the exactly matching cells (in the same order) are used, in other cases the cells get shuffled and get different binding. In the case where the order is matched we end up setting view.BindingContext to identical value that was already there, and that results in the "replace" action being ignored (since to the template's view it looks like no change) or, worse case, hitting some rebinding bugs (unrelated and not properly understood yet).

We had the PrepareForReuse fix locally for a long time (even in XF, before attempting to migrate to MAUI). The reason it works for that particular use case is that it guaranteed that in the case of cell reuse it's always reset to BindingContext = null before setting BindingContext again. Unfortunately, I mistakenly assumed that PrepareForUse gets called when the cell goes out of use (*) but in reality, it's called as part of the dequeue operation when reusing the cell...

(There are probably cases where PrepareForReuse may be called at different time but they are not so easy to trigger.)

(*) The independent UICollectionView reimplementation from back in the day does exactly that, and calls PrepareForReuse during the enqueuing of reusable cells, not their dequeuing. Apple implementation is a black box, so it may work one way or the other, but I couldn't get it to trigger reliably.

---

I found a partial explanation of what is happening under the covers (https://openradar.appspot.com/39604024):

With prefetching enabled UICollectionView uses a cache for pages that are loaded, but not yet displayed on screen. This is a dictionary <NSIndexPath : _UICollectionViewPrefetchItem>, stored under the _prefetchCacheItems instance variable.

_UICollectionViewPrefetchItem stores the layout attributes and reusable view (UICollectionViewCell in our case) for a specific index path. Views that are put into _prefetchCacheItems also get their visibility set to "hidden". This is however not via the default hidden property, bit via dedicated methods _isHiddenForReuse and _setHiddenForReuse: defined on UIView. hidden reflects the value of _isHiddenForReuse, but does not change it. When prefetched cells are determined to become visible, they are removed from the cache and _isHiddenForReuse is set to NO so they show up.

If the collection view layout gets invalidated (which happens for us when we show or hide the navigation bar), the prefetch cache gets cleared. During this, new layout attributes and potentially new prefetched cells might get queried from the layout / data source. Those cells might not match the previous prefetched cells (e.g., only cells in the last scrolled direction are preloaded again).

Here's where the problem appears to happen. During cache eviction, the removed cells do not get their _isHiddenForReuse value updated. It remains the same. Some of those cells then get re-added to new _UICollectionViewPrefetchItem objects, which ensures their _isHiddenForReuse is updated when the cell comes on screen. But some cells don't get "prefetched" again. There is no _UICollectionViewPrefetchItem created for them. Their _isHiddenForReuse value remains set and they remain at the same position they were before invalidating the layout. UICollectionView appears to be perfectly ok with thinking there's nothing wrong with that cell and just keeps it in position as is. It even sends out "will appear" delegate calls for it. But since the cell is hidden, we can't see anything.

I verified that manually ensuring _isHiddenForReuse is not set for every visible cell during layout passes fixes the issues. Prefetching appears to work fine after that.

There's also a vague note in the official documentation:

In iOS 15 and later, the collection view retains a prepared cell in the following situations:

• Cells that the collection view prefetches and retains in its cache of prepared cells, but that aren’t visible because the collection view hasn’t displayed them yet.

• Cells that the collection view finishes displaying and continues to retain in its cache of prepared cells because they remain near the visible region and might scroll back into view.

• The cell that contains the first responder.

• The cell that has focus.

I will check what happens on older iOS versions and/or if prefetching is disabled.

[filipnavara]

I found a way to accomplish the correct behaviour with CellDisplayingEnded/WillDisplayCell on ItemsViewDelegator. Not sure about the proper approach and performance implications but I tried several different options and the results were promising.

The options I examined:

1. Clear the BindingContext in CellDisplayingEnded and repopulate it in WillDisplayCell. That's probably the easiest one to accomplish. It may also have some unforeseen performance implications.
2. Clear the BindingContext in CellDisplayingEnded only when the cell is not in the items source. This is safer option in terms of performance but slightly harder to reason about.

It also seems that the test fails on Android too. (Fixed: Added a Task.Delay that pushes the checks after the next layout pass.)

[rmarinho]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[github-actions[bot]]

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

[rmarinho]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[samhouts]

I'm wondering if this would have a positive impact on these issues: #16125 #15204 #14664 #14654 #7237

[filipnavara]

#16125

Possibly yes. It looks like the same underlying issue.

#15204

Unlikely.

#14664

That sounds like multiple different issues. This could possibly be one of them too.

#14654

I believe that's unrelated and already addressed by @jonathanpeppers.

#7237

Looks unrelated.

[filipnavara]

I am currently on vacation, happy to make the changes once I get back in a week.

[jfversluis]

/azp run