fix memory leak (and some crashes) in ItemsViewController (iOS)

[tmijieux]

Description of Change

When a CollectionView is removed from a layout or removed from view hierarchy (My use case is the following: when the user rotate the screen the layout changes. So, in my code, I recreate a new layout with a new CollectionView), the BindingContext is set to null, hence the ItemsSourceProperty is set to null and the ItemsSource property in the Xamarin.Forms.Platform.iOS.ItemsViewController will be updated (through ItemsViewRenderer.OnElementPropertyChanged).

The problem is, if the IItemsViewSource is an Xamarin.Forms.Platform.iOS.ObservableItemsSource , the dispose method on the old ItemsSource value is not called and that dispose method is supposed to unsubscribe from the event delegate.
In the end, that object is still subscribed on the ObservableCollection in my ViewModel that has not reached the end of its lifetime(I continue using the same viewmodel), hence the ObservableItemSource is leaked because still referenced by the CollectionChanged event delegate, but the ItemsViewController which is supposed to own the reference to that ObservableItemsSource has no reference anymore.

The change in this PR is:

• Disposing pre-existing ObservableItemsSource(if any) when setting ItemsSource property to a new value (in ItemsViewController)

Issues Resolved

I did not yet do a complete check of what issues could be affected by this change.
One of the crash i encountered myself was exactly the same as in #13910 (same stacktrace) (when on INotifyCollectionChanged collection call the CollectionChanged event with the Reset event).

The reason this crash was happening is (i think) because the ObservableItemsSource, or the referenced views (if any) are removed from view hierarchy and they are not in a valid state anymore.

I will work on creating an issue with a minimal reproduction for my problem as soon as this PR is submitted.
EDIT: -> #14113

API Changes

None

Platforms Affected

• iOS

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

PR Checklist

• Targets the correct branch
• Tests are passing (or failures are unrelated)

[jtorvald]

Same issue with multiple other leaky items was also fixed in #14076

[rachelkang]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[rmarinho]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[rmarinho]

Is it possible to add a test case so we can make sure it fixes the issue ?

[tmijieux]

@rmarinho do you mean a unit test ? I'm currently trying to add one. I figured it would land in the Xamarin.Forms.Platform.iOS.UnitTest namespace,... I'm not yet sure how to run the tests that lies in this namespaces(currently updating stuff on my mac server to see if it is possible to run on device, but i'm not sure this is the way to go...)

If just a simple repro project is needed a already linked one with the associated issue:

#14113
https://github.com/tmijieux/XFBugItemsViewController/

[net-foundation-cla]

All CLA requirements met.

[rmarinho]

I was saying a a sample page on the Issues gallery, and you can add a UITest

[tmijieux]

i added two test,

• the first one DoesNotLeakdoes not seem to work yet, (it pass both before and after the fix)
• the second one DoesNotCrash seem to work as expected (fail before and pass after) .

I was about to look into my first test, but about at the same time i figured i would rebase on the lastest commit of xamarin/Xamarin.Forms:5.0.0 to see if things changed in between, but now i cannot deploy the ControlGallery to my iphone anymore (there is a build error that i can get rid of for now that have to do with a path length issue seemingly 😡 ) i'm not even sure that my deploy issue is related to the rebase or not, but i cant recall on what commit i was based before...

[rmarinho]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tmijieux]

Updated the tests, both tests now fail without the fix and pass with the fix.

I was saying a a sample page on the Issues gallery, and you can add a UITest

I'm not sure what you expect exactly, can you give a little more guidance please(is there a guideline I should read) ?

I put my tests in the unit tests because i just mimicked what was done in other unit tests to reproduce my bug, and the code in UITests seemed harder to grasp for me at first. But my feeling about the tests and the bugs is that they are not exactly trivial, so i understand if you want me to take them out of the "unit" tests.

[jsuarezruiz]

The added test are passing.

[rmarinho]

I think a best fix is calling

ItemsSource?.Dispose();

On the UpdateItemsSource() method