Revert "[ios] fix memory leak in CollectionView cells (#15831)"

[filipnavara]

Fixes #24304.

This reverts commit 05697a6 and updates the test.

The test was unfortunately testing the wrong thing - that manually created VerticalCell doesn't hold on to the handler and platform views. What we really want to test is that VerticalCell created through UICollectionView doesn't prevent the collection view from being collected.

[dotnet-policy-service]

Hey there @filipnavara! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[jonathanpeppers]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s), but failed to run 1 pipeline(s).

[jonathanpeppers]

I pushed a commit to restore the test.

[filipnavara]

I intentionally left it out because it’s guaranteed to fail. See the post at the top of the PR for rationale.

[filipnavara]

…but thanks for starting the CI, the big question is if the other tests fail, and it should give us the answer.

[jonathanpeppers]

So this used to be the circular reference (in 05697a6 ~June 2023):

• CollectionView -> handlers / etc. -> TemplatedCell -> LabelHandler -> Label -> CollectionView

But now we have 10e3fc4, so there is no strong reference from Label -> CollectionView.

If the old test is not valid, it seems like we should make sure some other test checks this.

[filipnavara]

If the old test is not valid, it seems like we should make sure some other test checks this.

Agreed. I wanted to have a full test run first to see if anything else fails. I'll have to think about bit on how to structure the test (or whether there is an existing test coverage).

[filipnavara]

I updated the test to ensure that CollectionView can get collected. The VerticalCell is still referenced, so it means there's no (strong) cycle.

[jonathanpeppers]

After these changes, when would the labels go away? Should we update this to check all the VerticalCell instances go away instead?

[filipnavara]

If the VerticalCell was not created synthetically and held in local variable then they should be gone with the collection view.

That would essentially mean rewriting the test to do what ClearingItemsSourceClearsBindingContext does and at the end checking that dropping reference to collection view also allows collecting the cells (and then the attached labels).

[filipnavara]

(The ClearingItemsSourceClearsBindingContext test was moved/renamed - it was originally added in https://github.com/dotnet/maui/pull/14619/files#diff-a1aed29ff55ec6fe5da9c27f12285c91406be414338738b0bcc3106ec4aa675f)

[filipnavara]

I think it's already tested by this:

maui/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs

Lines 106 to 169 in 639fcb0

	[Fact]
	public async Task ItemsSourceDoesNotLeak()
	{
	SetupBuilder();
	var weakReferences = new List<WeakReference>();
	{
	var labels = new List<Label>();
	IList logicalChildren = null;
	var collectionView = new CollectionView
	{
	Header = new Label { Text = "Header" },
	Footer = new Label { Text = "Footer" },
	ItemTemplate = new DataTemplate(() =>
	{
	var label = new Label();
	labels.Add(label);
	return label;
	}),
	};
	var navPage = new NavigationPage(new ContentPage { Title = "Page 1" });
	await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async handler =>
	{
	await navPage.PushAsync(new ContentPage { Content = collectionView });
	var data = new ObservableCollection<string>()
	{
	"Item 1",
	"Item 2",
	"Item 3"
	};
	weakReferences.Add(new(data));
	collectionView.ItemsSource = data;
	await Task.Delay(100);
	Assert.NotEmpty(labels);
	foreach (var label in labels)
	{
	weakReferences.Add(new(label));
	weakReferences.Add(new(label.Handler));
	weakReferences.Add(new(label.Handler.PlatformView));
	}
	// Get ItemsView._logicalChildren
	var flags = BindingFlags.NonPublic | BindingFlags.Instance;
	logicalChildren = typeof(Element).GetField("_internalChildren", flags).GetValue(collectionView) as IList;
	Assert.NotNull(logicalChildren);
	// Replace with cloned collection
	collectionView.ItemsSource = new ObservableCollection<string>(data);
	await Task.Delay(100);
	await navPage.PopAsync();
	});
	Assert.NotNull(logicalChildren);
	Assert.True(logicalChildren.Count <= 5, "_logicalChildren should not grow in size!");
	}
	await AssertionExtensions.WaitForGC([.. weakReferences]);
	}

[PureWeen]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[filipnavara]

The one failed test (HandlerDoesNotLeak) seems to be related to MauiWKWebView and not the changes in this PR. It failed on other unrelated CI runs:

[PureWeen]

I think this fix will somewhat regress what @jonathanpeppers did when the user navigates away from the page with the CV on it, because in those cases the UNBIND isn't going to get called

I tested with this sample here
https://github.com/Dani3654/CollectionViewMemoryLeaks/blob/main/CollectionViewMemoryLeaks/CollectionViewMemoryLeaks/Views/CollectionViewSamplePage.xaml

On the main branch the memory doesn't seem to really increase. Occasionally by 1 MB or so

But with this PR it steadily increases at a much quicker rate

I don't think this will be the case on net9 because in net9 we're going to disconnect from every handler when the user navigates away from the page

[filipnavara]

I'll look at it tomorrow. In our app we disconnect the handlers too. That said, the current status quo is that we get constant crashes which prevents us from releasing the update to our app (without building custom build of MAUI).

The assumption of the added test was that there's no longer a cycle preventing the CollectionView from being collected. I didn't test whether the handler itself, and in turn the platform view, is collected, but I assume that's the case.

It may just be a case of the behavior described in dotnet/runtime#104272 (comment) where it takes several GC cycles to collect everything.

[PureWeen]

I'll look at it tomorrow. In our app we disconnect the handlers too. That said, the current status quo is that we get constant crashes which prevents us from releasing the update to our app (without building custom build of MAUI).

The assumption of the added test was that there's no longer a cycle preventing the CollectionView from being collected. I didn't test whether the handler itself, and in turn the platform view, is collected, but I assume that's the case.

It may just be a case of the behavior described in dotnet/runtime#104272 (comment) where it takes several GC cycles to collect everything.

Yea, not sure how much it works around this but I added a long set of collects to try and force it.

Here's a branch that has that scenario added to our sandbox
2d04a9e

I added a big stretch of collects
2d04a9e#diff-04455adab0c330ee207405912af8f1271106c03f5cb2b8fb227fca781b5b5a02R13-R27

But perhaps it's still just the delayed collect you're referring to

[filipnavara]

Unfortunately I am still pretty sure that it's the delayed GC which needs a couple of cycles. I added a "GC" button to the original sample that simply calls GC.Collect().

This is the original sample running on MAUI 8.0.60:

The stairs in the Instruments graph refer to the GC collects.

Here's the same scenario with this PR:

Eventually the memory gets collected with enough GC cycles (~ 9 in this case; it's NativeAOT build so it may vary a bit from the regular MonoAOT build).

It's theoretically possible to keep the WeakReferences if we ensure that platform handler gets recreated when necessary, the measurements cells get abandoned if their platform handler was collected, and if we find a way to ensure that there exists a strong reference between the time when TemplatedCell.SetRenderer gets called and the reference to UICollectionViewCell gets passed to the native control which hold the strong reference from that point on (hopefully). It's incredibly difficult to reason about this and prove the correctness, unfortunately.

[filipnavara]

Closing this. We can do less invasive fix: #24304 (comment).

The less invasive fix will likely consist of:

• Adding GC.KeepAlive(renderer) here to prevent a small GC hole. (the hole cannot happen because view keeps a strong reference to its handler and we keep view in the local variable up until the UpdateSelectionColor(view); call)
• Updating ItemsViewHandler.DisconnectHandler implementation to update the UICollectionView properties in the same way as if VirtualView.ItemsSource = null was set, and thus disconnect it from the underlying list collection change events.