Skip to content

Commit

Permalink
fix crash when reloading the same cell index multiple times
Browse files Browse the repository at this point in the history 
Summary:
## Context
Merging multiple batch updates of the same section is inherently unsafe, because each block can mutate the underlaying list and lead to insert/deletes that operate on different before/after lists. The long term fix is to get rid of item level batch updates (insert/delete calls) and use diffing like for sections, but that's a much larger project.

Right now, we have a crash spiking because we're reloading the same cell multiple times. Each time `IGListDiff(...)` returns a pair of insert/delete for the same index (lets say 0). Once these updates are merge together, they look like this:
```
Item inserts = 0, 0
Item deletes = 0, 0
```
We dedupe deletes in `IGListBatchUpdateData`, but don't do anything for inserts, so it looks liket this:

```
Item inserts = 0, 0
Item deletes = 0
```

So we create an insert/delete unbalance and can crash. Tbh, not sure how this hasn't been a bigger issue in the past.

D21822502 tried to remove all duplicate inserts, but that caused other types of crashes, because multiple inserts are allowed.

## This diff

* Lets keep the net insert/delete count the same by keeping track of the number of deletes we remove and apply the same to inserts.
* Add some tests
* Add an experiment flag to kill this if needed to be safe. IG only: Note that I'm making the default true because the crash is happening pretty at start-up.

## Follow up

Over time, the batch update clean up logic was slowly added to multiple place (IGListBatchUpdateData, IGListAdapterUpdaterHelpers, IGListIndexPathResult), which is making it hard to follow. A first step would be to move it  in 1 or 2 places and avoid redundant logic (like getting unique deletes). However, the real fix is getting rid of batch updates all together.

Differential Revision: D61719884

fbshipit-source-id: eda7c264c8239a6a106dbe0256fe777b38fae335
  • Loading branch information
@facebook-github-bot
Maxime Ollivier authored and facebook-github-bot committed Aug 23, 2024
1 parent bcd8b07 commit 0c25779
Show file tree
Hide file tree
Showing 10 changed files with 176 additions and 29 deletions.
4 changes: 3 additions & 1 deletion Source/IGListDiffKit/IGListBatchUpdateData.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ NS_SWIFT_NAME(ListBatchUpdateData)
@param deleteIndexPaths Item index paths to delete.
@param updateIndexPaths Item index paths to update.
@param moveIndexPaths Item index paths to move.
@param enableNetItemCountFix Fix item count balance issue when removing duplicate deletes
@return A new batch update object.
*/
Expand All @@ -76,7 +77,8 @@ NS_SWIFT_NAME(ListBatchUpdateData)
insertIndexPaths:(NSArray<NSIndexPath *> *)insertIndexPaths
deleteIndexPaths:(NSArray<NSIndexPath *> *)deleteIndexPaths
updateIndexPaths:(NSArray<NSIndexPath *> *)updateIndexPaths
moveIndexPaths:(NSArray<IGListMoveIndexPath *> *)moveIndexPaths NS_DESIGNATED_INITIALIZER;
moveIndexPaths:(NSArray<IGListMoveIndexPath *> *)moveIndexPaths
enableNetItemCountFix:(BOOL)enableNetItemCountFix NS_DESIGNATED_INITIALIZER;

/**
:nodoc:
Expand Down
38 changes: 34 additions & 4 deletions Source/IGListDiffKit/IGListBatchUpdateData.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ - (instancetype)initWithInsertSections:(nonnull NSIndexSet *)insertSections
insertIndexPaths:(nonnull NSArray<NSIndexPath *> *)insertIndexPaths
deleteIndexPaths:(nonnull NSArray<NSIndexPath *> *)deleteIndexPaths
updateIndexPaths:(nonnull NSArray<NSIndexPath *> *)updateIndexPaths
moveIndexPaths:(nonnull NSArray<IGListMoveIndexPath *> *)moveIndexPaths {
moveIndexPaths:(nonnull NSArray<IGListMoveIndexPath *> *)moveIndexPaths
enableNetItemCountFix:(BOOL)enableNetItemCountFix {
IGParameterAssert(insertSections != nil);
IGParameterAssert(deleteSections != nil);
IGParameterAssert(moveSections != nil);
Expand Down Expand Up @@ -95,11 +96,40 @@ - (instancetype)initWithInsertSections:(nonnull NSIndexSet *)insertSections
toMap[to] = move;
}
}

NSMutableArray<NSIndexPath *> *mDeleteIndexPaths;
NSMutableArray<NSIndexPath *> *mInsertIndexPaths;

// avoid a flaky UICollectionView bug when deleting from the same index path twice
// Avoid a flaky UICollectionView bug when deleting from the same index path twice
// exposes a possible data source inconsistency issue
NSMutableArray<NSIndexPath *> *mDeleteIndexPaths = [[[NSSet setWithArray:deleteIndexPaths] allObjects] mutableCopy];
NSMutableArray<NSIndexPath *> *mInsertIndexPaths = [insertIndexPaths mutableCopy];
if (enableNetItemCountFix) {
NSMutableDictionary<NSIndexPath *, NSNumber *> *const deleteCounts = [NSMutableDictionary new];

// If we need to remove a duplicate delete, we also need to remove an insert to balance the count.
// Lets build the delete counts for each index, which we can use to skip corresponding inserts.
for (NSIndexPath *deleteIndexPath in deleteIndexPaths) {
const NSInteger deleteCount = deleteCounts[deleteIndexPath].integerValue;
deleteCounts[deleteIndexPath] = @(deleteCount + 1);
}

// Skip inserts that have an associated skipped delete
NSMutableArray<NSIndexPath *> *const trimmedInsertIndexPath = [NSMutableArray new];
for (NSIndexPath *insertIndexPath in insertIndexPaths) {
const NSInteger deleteCount = deleteCounts[insertIndexPath].integerValue;
if (deleteCount > 1) {
// Skip!
deleteCounts[insertIndexPath] = @(deleteCount - 1);
} else {
[trimmedInsertIndexPath addObject:insertIndexPath];
}
}

mDeleteIndexPaths = [[deleteCounts allKeys] mutableCopy];
mInsertIndexPaths = trimmedInsertIndexPath;
} else {
mDeleteIndexPaths = [[[NSSet setWithArray:deleteIndexPaths] allObjects] mutableCopy];
mInsertIndexPaths = [insertIndexPaths mutableCopy];
}

// avoids a bug where a cell is animated twice and one of the snapshot cells is never removed from the hierarchy
[IGListBatchUpdateData _cleanIndexPathsWithMap:fromMap moves:mMoveSections indexPaths:mDeleteIndexPaths deletes:mDeleteSections inserts:mInsertSections];
Expand Down
2 changes: 2 additions & 0 deletions Source/IGListDiffKit/IGListExperiments.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) {
IGListExperimentThrowOnInconsistencyException = 1 << 3,
/// Remove the early exit so multiple updates can't happen at once
IGListExperimentRemoveDataSourceChangeEarlyExit = 1 << 4,
/// Fix item count balance issue when removing duplicate deletes
IGListExperimentEnableNetItemCountFix = 1 << 5,
};

/**
Expand Down
3 changes: 2 additions & 1 deletion Source/IGListKit/Internal/IGListAdapterUpdaterHelpers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ IGListBatchUpdateData *IGListApplyUpdatesToCollectionView(UICollectionView *coll
NSMutableArray<IGListMoveIndexPath *> *itemMoves,
NSArray<id<IGListDiffable>> *fromObjects,
BOOL sectionMovesAsDeletesInserts,
BOOL preferItemReloadsForSectionReloads);
BOOL preferItemReloadsForSectionReloads,
BOOL enableNetItemCountFix);

NSIndexSet *IGListSectionIndexFromIndexPaths(NSArray<NSIndexPath *> *indexPaths);

Expand Down
6 changes: 4 additions & 2 deletions Source/IGListKit/Internal/IGListAdapterUpdaterHelpers.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ void IGListConvertReloadToDeleteInsert(NSMutableIndexSet *reloads,
NSMutableArray<IGListMoveIndexPath *> *itemMoves,
NSArray<id<IGListDiffable>> *fromObjects,
BOOL sectionMovesAsDeletesInserts,
BOOL preferItemReloadsForSectionReloads) {
BOOL preferItemReloadsForSectionReloads,
BOOL enableNetItemCountFix) {
NSSet *moves = [[NSSet alloc] initWithArray:diffResult.moves];

// combine section reloads from the diff and manual reloads via reloadItems:
Expand Down Expand Up @@ -125,7 +126,8 @@ void IGListConvertReloadToDeleteInsert(NSMutableIndexSet *reloads,
insertIndexPaths:itemInserts
deleteIndexPaths:itemDeletes
updateIndexPaths:itemUpdates
moveIndexPaths:itemMoves];
moveIndexPaths:itemMoves
enableNetItemCountFix:enableNetItemCountFix];
[collectionView ig_applyBatchUpdateData:updateData];
return updateData;
}
Expand Down
7 changes: 5 additions & 2 deletions Source/IGListKit/Internal/IGListBatchUpdateTransaction.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ - (void)_applyDataUpdates {
}

- (void)_applyCollectioViewUpdates:(IGListIndexSetResult *)diffResult {
const BOOL enableNetItemCountFix = IGListExperimentEnabled(self.config.experiments, IGListExperimentEnableNetItemCountFix);
if (self.config.singleItemSectionUpdates) {
[self.collectionView deleteSections:diffResult.deletes];
[self.collectionView insertSections:diffResult.inserts];
Expand All @@ -256,7 +257,8 @@ - (void)_applyCollectioViewUpdates:(IGListIndexSetResult *)diffResult {
insertIndexPaths:@[]
deleteIndexPaths:@[]
updateIndexPaths:@[]
moveIndexPaths:@[]];
moveIndexPaths:@[]
enableNetItemCountFix:enableNetItemCountFix];
} else {
self.actualCollectionViewUpdates = IGListApplyUpdatesToCollectionView(self.collectionView,
diffResult,
Expand All @@ -267,7 +269,8 @@ - (void)_applyCollectioViewUpdates:(IGListIndexSetResult *)diffResult {
self.inUpdateItemCollector.itemMoves,
self.sectionData.fromObjects ?: @[],
self.config.sectionMovesAsDeletesInserts,
self.config.preferItemReloadsForSectionReloads);
self.config.preferItemReloadsForSectionReloads,
enableNetItemCountFix);
}
}

Expand Down
21 changes: 14 additions & 7 deletions Tests/IGListAdapterUpdaterTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,8 @@ - (void)test_whenReloadIsCalledWithSameItemCount_deleteInsertSectionHappen {
insertIndexPaths:@[]
deleteIndexPaths:@[]
updateIndexPaths:@[]
moveIndexPaths:@[]];
moveIndexPaths:@[]
enableNetItemCountFix:NO];
NSArray<IGSectionObject *> *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id"]];
NSArray<IGSectionObject *> *to = @[[IGSectionObject sectionWithObjects:@[@2] identifier:@"id"]];
self.dataSource.sections = from;
Expand Down Expand Up @@ -996,7 +997,8 @@ - (void)test_whenReloadIsCalledWithSameItemCount_andPreferItemReload_updateIndex
insertIndexPaths:@[]
deleteIndexPaths:@[]
updateIndexPaths:@[[NSIndexPath indexPathForItem:0 inSection:0]]
moveIndexPaths:@[]];
moveIndexPaths:@[]
enableNetItemCountFix:NO];
NSArray<IGSectionObject *> *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id"]];
// Update the items
NSArray<IGSectionObject *> *to = @[[IGSectionObject sectionWithObjects:@[@2] identifier:@"id"]];
Expand Down Expand Up @@ -1031,7 +1033,8 @@ - (void)test_whenReloadIsCalledWithDifferentItemCount_andPreferItemReload_delete
insertIndexPaths:@[]
deleteIndexPaths:@[]
updateIndexPaths:@[]
moveIndexPaths:@[]];
moveIndexPaths:@[]
enableNetItemCountFix:NO];
NSArray<IGSectionObject *> *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id"]];
// more items in the section
NSArray<IGSectionObject *> *to = @[[IGSectionObject sectionWithObjects:@[@1, @2] identifier:@"id"]];
Expand Down Expand Up @@ -1067,7 +1070,8 @@ - (void)test_whenReloadIsCalledWithSectionMoveAndUpdate_andPreferItemReload_dele
insertIndexPaths:@[]
deleteIndexPaths:@[]
updateIndexPaths:@[]
moveIndexPaths:@[]];
moveIndexPaths:@[]
enableNetItemCountFix:NO];
NSArray<IGSectionObject *> *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id1"],
[IGSectionObject sectionWithObjects:@[@2] identifier:@"id2"]];
// move section, and also update the item for "id2"
Expand Down Expand Up @@ -1109,7 +1113,8 @@ - (void)test_whenReloadIsCalledWithSectionMoveAndUpdate_withDifferentSectionLeng
insertIndexPaths:@[]
deleteIndexPaths:@[]
updateIndexPaths:@[]
moveIndexPaths:@[]];
moveIndexPaths:@[]
enableNetItemCountFix:NO];
NSArray<IGSectionObject *> *from = @[[IGSectionObject sectionWithObjects:@[@1, @2, @3] identifier:@"id1"],
[IGSectionObject sectionWithObjects:@[@2] identifier:@"id2"]];
// move section, and also update the item for "id2"
Expand Down Expand Up @@ -1152,7 +1157,8 @@ - (void)test_whenReloadIsCalledWithSectionMoveAndUpdate_withThreeSections_delete
insertIndexPaths:@[]
deleteIndexPaths:@[]
updateIndexPaths:@[]
moveIndexPaths:@[]];
moveIndexPaths:@[]
enableNetItemCountFix:NO];
NSArray<IGSectionObject *> *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id1"],
[IGSectionObject sectionWithObjects:@[@2] identifier:@"id2"],
[IGSectionObject sectionWithObjects:@[@3] identifier:@"id3"]];
Expand Down Expand Up @@ -1195,7 +1201,8 @@ - (void)test_whenReloadIsCalledWithSectionInsertAndUpdate_andPreferItemReload_no
insertIndexPaths:@[]
deleteIndexPaths:@[]
updateIndexPaths:@[]
moveIndexPaths:@[]];
moveIndexPaths:@[]
enableNetItemCountFix:NO];
NSArray<IGSectionObject *> *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id1"]];
NSArray<IGSectionObject *> *to = @[[IGSectionObject sectionWithObjects:@[@2] identifier:@"id1"],
[IGSectionObject sectionWithObjects:@[@22] identifier:@"id2"]];
Expand Down
Loading

0 comments on commit 0c25779

Please sign in to comment.