maui icon indicating copy to clipboard operation
maui copied to clipboard

[Android] Ensure disconnected ItemsViewHandler doesn't hold onto the items source

Open filipnavara opened this issue 1 year ago • 15 comments

Description of Change

Any control derived from ItemsView (CarouselView, CollectionView) never disconnected from the ItemsSource.CollectionChanged callback when DisconnectHandler() is called. If the lifetime of the item source outlives the controls it can result in the CollectionChanged callback keeping part of the UI tree alive in a half-zombie state. Any change then calls the callback and it may cause crashes with NullReferenceException as seen in #24304.

Issues Fixed

Fixes #24304

filipnavara avatar Sep 05 '24 15:09 filipnavara

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.

/azp run

PureWeen avatar Sep 05 '24 15:09 PureWeen

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Sep 05 '24 15:09 azure-pipelines[bot]

/azp run

PureWeen avatar Sep 05 '24 19:09 PureWeen

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Sep 05 '24 19:09 azure-pipelines[bot]

/azp run

PureWeen avatar Sep 06 '24 17:09 PureWeen

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Sep 06 '24 17:09 azure-pipelines[bot]

Looks like Unit tests are failing

Apparently MarshalingObservableCollection tests depend on the collection changing without ever registering for the CollectionChanged event. That makes it tricky to implement the behavior on Android correctly since there's no explicit disposal of the MarshalingObservableCollection that is internally created by the ItemsSourceFactory.

filipnavara avatar Sep 07 '24 14:09 filipnavara

Looks like Unit tests are failing

Apparently MarshalingObservableCollection tests depend on the collection changing without ever registering for the CollectionChanged event. That makes it tricky to implement the behavior on Android correctly since there's no explicit disposal of the MarshalingObservableCollection that is internally created by the ItemsSourceFactory.

Hmmm, maybe for now we just restore MarshalingObservableCollection so it subscribes in the ctor and make sure that calling disconnecthandler on Android will correctly unsubscribe. I realize that's not currently ideal but that would at least let us merge this PR and fix the crasher on iOS

PureWeen avatar Sep 10 '24 01:09 PureWeen

Hmmm, maybe for now we just restore MarshalingObservableCollection so it subscribes in the ctor and make sure that calling disconnecthandler on Android will correctly unsubscribe.

Would it be okay to add implementation of IDisposable interface to MarshalingObservableCollection?

filipnavara avatar Sep 10 '24 06:09 filipnavara

/azp run

rmarinho avatar Sep 10 '24 10:09 rmarinho

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Sep 10 '24 10:09 azure-pipelines[bot]

The test failure looks relevant. I don't see why it happens though, and I will need to reproduce it locally.

filipnavara avatar Sep 10 '24 18:09 filipnavara

Sure. I am away until next week but I will revive the PR when I get back home.

filipnavara avatar Oct 10 '24 16:10 filipnavara

/rebase

jsuarezruiz avatar Oct 23 '24 09:10 jsuarezruiz

/azp run

rmarinho avatar Nov 27 '24 13:11 rmarinho

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Nov 27 '24 13:11 azure-pipelines[bot]

/azp run

rmarinho avatar Jan 07 '25 22:01 rmarinho

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Jan 07 '25 22:01 azure-pipelines[bot]

@filipnavara I screwed up this rebase.. I confused the branch on your side, do you mind rebase and reopen this one with your local copy.

Thanks

rmarinho avatar Jan 07 '25 22:01 rmarinho

/azp run

rmarinho avatar Jan 14 '25 19:01 rmarinho

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Jan 14 '25 19:01 azure-pipelines[bot]