Skip to content

Commit

Permalink
keep pointer to self.collectionView.dataSource in IGListBatchUpdateTr…
Browse files Browse the repository at this point in the history
…ansaction

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
  • Loading branch information
Maxime Ollivier authored and facebook-github-bot committed Jan 17, 2024
1 parent b3a22ad commit e3c5cfb
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 2 deletions.
4 changes: 3 additions & 1 deletion Source/IGListDiffKit/IGListExperiments.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
};

/**
Expand Down
6 changes: 5 additions & 1 deletion Source/IGListKit/Internal/IGListBatchUpdateTransaction.m
Original file line number Diff line number Diff line change
Expand Up @@ -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<UICollectionViewDataSource> 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) {
Expand Down

0 comments on commit e3c5cfb

Please sign in to comment.