From e3c5cfb59eb5efb49feb9ed89d1aec29fed70e4a Mon Sep 17 00:00:00 2001 From: Maxime Ollivier Date: Wed, 17 Jan 2024 14:06:13 -0800 Subject: [PATCH] keep pointer to self.collectionView.dataSource in IGListBatchUpdateTransaction Summary: ## Issue We're seeing crashes like this one: ``` Invalid batch updates detected: the number of sections and/or items returned by the data source before and after performing the batch updates are inconsistent with the updates. Data source before updates = { 6 sections with item counts: [1, 0, 6, 8, 5, 7] } Data source after updates = { 1 section with item counts: [0] } Updates = [ ] ``` I'm thinking the `IGListAdapter` gets deallocated during an update. 1. `1 section with item counts: [0]` is the default when the `self.collectionView.dataSource` is `nil`. I don't really see a way we'd return a single section with zero cells. 2. We're not seeing any of the additional crash information set by the `IGListAdapterUpdaterDelegate`, which would happen if `IGListAdapter` gets deallocated (it holds the strong pointer to the delegate). ## Fix Lets keep a strong pointer within the scope of the function to make sure it sticks around. Theoretically, there could be a situation where the section-controller holds a strong pointer to the list-adapter in an update block, which gets released after it runs. This seems pretty unlikely, so lets test this fix to see if that's it. Reviewed By: candance Differential Revision: D52747014 fbshipit-source-id: 545aaa3deb90af85a011e716ac870659da42106f --- Source/IGListDiffKit/IGListExperiments.h | 4 +++- Source/IGListKit/Internal/IGListBatchUpdateTransaction.m | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Source/IGListDiffKit/IGListExperiments.h b/Source/IGListDiffKit/IGListExperiments.h index 826c29419..1c660a88c 100644 --- a/Source/IGListDiffKit/IGListExperiments.h +++ b/Source/IGListDiffKit/IGListExperiments.h @@ -19,7 +19,9 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) { /// Test invalidating layout when cell reloads/updates in IGListBindingSectionController. IGListExperimentInvalidateLayoutForUpdates = 1 << 2, /// Throw NSInternalInconsistencyException during an update - IGListExperimentThrowOnInconsistencyException = 1 << 3 + IGListExperimentThrowOnInconsistencyException = 1 << 3, + /// Test keeping a strong pointer to the collectionView.dataSource during a batch update to avoid a crash + IGListExperimentKeepPointerToCollectionViewDataSource = 1 << 4 }; /** diff --git a/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m b/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m index d497998d6..14fd9da0d 100644 --- a/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m +++ b/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m @@ -136,7 +136,11 @@ - (void)_didDiff:(IGListIndexSetResult *)diffResult onBackground:(BOOL)onBackgro [self.delegate listAdapterUpdater:self.updater didDiffWithResults:diffResult onBackgroundThread:onBackground]; @try { - if (self.collectionView.dataSource == nil) { + // Experiment with keeping a pointer to the self.collectionView.dataSource, because we're seeing a crash where it could be missing. + const BOOL keepDataSource = IGListExperimentEnabled(self.config.experiments, IGListExperimentKeepPointerToCollectionViewDataSource); + id const collectionViewDataSource = keepDataSource ? self.collectionView.dataSource : nil; + + if (self.collectionView.dataSource == nil || (keepDataSource && collectionViewDataSource == nil)) { // If the data source is nil, we should not call any collection view update. [self _bail]; } else if (diffResult.changeCount > 100 && self.config.allowsReloadingOnTooManyUpdates) {