Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

ItemsViewController null checks #15638

Merged
merged 1 commit into from Jan 9, 2023
Merged

ItemsViewController null checks #15638

merged 1 commit into from Jan 9, 2023

Conversation

reid-kirkpatrick
Copy link
Contributor

Description of Change

In the ItemsViewController multiple methods (i.e. UICollectionViewController.GetCell() which calls ItemsViewController.UpdateTemplatedCell() and UICollectionViewController.GetItemsCount() and UICollectionViewController.NumberOfSections() which both call ItemsViewController.CheckForEmptySource()) can be called by iOS after ItemsViewController.Dispose() has been called and _measurementCells has been set to null. There are already null checks for _measurementCells in certain places, but not all. This should cover the rest of the cases where it can be null.

Issues Resolved

API Changes

None

Platforms Affected

  • iOS

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

I was able to reproduce the crash in the attached sample from #15637. I then created a local nuget package from the 5.0.0 branch and confirmed the cause. After making these changes I created another nuget package, updated the repro sample and confirmed the crash no longer happened.

PR Checklist

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

@jtorvald
Copy link
Contributor

jtorvald commented Jan 5, 2023

@reid-kirkpatrick I just created a patch and saw that you already fixed it last week. Thanks!

@jfversluis can this one please be merged and released on short notice?
It's a very annoying issue with the CollectionView and makes it almost unusable.

@jfversluis
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jfversluis
Copy link
Member

Thanks for this @reid-kirkpatrick ! Appreciate you looking into this and opening a PR!

Don't think this is a shocking change so I don't see any issues merging this. Releasing on short notice I can't promise, but it will be part of the next service release when merged.

@jtorvald
Copy link
Contributor

jtorvald commented Jan 9, 2023

Thanks @jfversluis and @reid-kirkpatrick !

@jfversluis
Copy link
Member

Confirmed here that this build works.

Thanks again @reid-kirkpatrick !

@jfversluis jfversluis merged commit 33dd32a into xamarin:5.0.0 Jan 9, 2023
@reid-kirkpatrick reid-kirkpatrick deleted the measurementCells-crash branch January 9, 2023 15:39
@beeradmoore
Copy link
Contributor

@jfversluis , do we have any ETA of Xamarin.Forms 5.0 SR 14 where this fix will be included?

@reid-kirkpatrick
Copy link
Contributor Author

@jfversluis any update here? We just released a new version of our app that added some uses of controls affected by this and our crash rate is a little troubling.

@jfversluis
Copy link
Member

Working on a new SR, should be soon. Hoping next week, but no solid promises at this point.

@ardentra
Copy link

@jfversluis 🙏 thank you!

@oridotaoyebode
Copy link

@jfversluis any news on the SR for this week? Pretty please.

@jtorvald
Copy link
Contributor

jtorvald commented Mar 9, 2023

@oridotaoyebode it has been released yesterday

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Go backwards crash the app while CollectionView scrolling
6 participants