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

[filipnavara]

Fixes #24304.

This reverts commit 05697a6b1fc8a068ad537cbf455d2407e22e7e72 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[bot]]

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[bot]]

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

[PureWeen]

/azp run

[azure-pipelines[bot]]

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:

[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 https://github.com/dotnet/runtime/issues/104272#issuecomment-2239290778 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 https://github.com/dotnet/maui/commit/2d04a9ed070e02d8d92cd65e77ef3f6dbf0dd592

I added a big stretch of collects https://github.com/dotnet/maui/commit/2d04a9ed070e02d8d92cd65e77ef3f6dbf0dd592#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: https://github.com/dotnet/maui/issues/24304#issuecomment-2331723517.

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.