prevent crash when inserting and deleting the same NSIndexPath multip…

…le times

Summary:
Issue: The `NSIndexPath` level updates are really tricky because they don't go through the diffing process. It's up to the caller to make sure that the inserts/deletes/moves all add up to the final number of items. And it's not uncommon that 2 different updates will try to change the same `NSIndexPath` at the same time, which can cause headaches and crashes. For example, if we insert/delete the same `NSIndexPath` twice in the same update pass, we end up with 2 inserts and 1 delete, because we remove duplicated deletes. This causes an `NSInternalInconsistencyException` since things don't add up at the end.

Fix: Lets also remove duplicate `NSIndexPath` inserts to balance things out. In the future, it would be nice to get rid of this API entirely and have everything go through the diffing.

Reviewed By: lorixx

Differential Revision: D17423776

fbshipit-source-id: 62fdf5bd521a915e984e8de180c74858f7ae7ef4

CHANGELOG.md

@@ -57,6 +57,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag
 
 - Added `IGListExperimentAvoidLayoutOnScrollToObject` to avoid creating off-screen cells when calling `[IGListAdapter scrollToObject ...]`. [Maxime Ollivier](https://github.com/maxolls) (tbd)
 
+- Added `IGListExperimentFixIndexPathImbalance` to test fixing a crash when inserting and deleting the same NSIndexPath multiple times. [Maxime Ollivier](https://github.com/maxolls) (tbd)
+
 3.4.0
 -----
 

Source/Common/IGListBatchUpdateData.h

@@ -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 fixIndexPathImbalance When enabled, we remove duplicate NSIndexPath inserts to avoid insert/delete imbalance and a crash.
 
  @return A new batch update object.
  */
@@ -76,7 +77,19 @@ 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
+ fixIndexPathImbalance:(BOOL)fixIndexPathImbalance NS_DESIGNATED_INITIALIZER;
+
+/**
+ Convenience initializer with fixIndexPathImbalance disabled.
+ */
+- (instancetype)initWithInsertSections:(NSIndexSet *)insertSections
+ deleteSections:(NSIndexSet *)deleteSections
+ moveSections:(NSSet<IGListMoveIndex *> *)moveSections
+ insertIndexPaths:(NSArray<NSIndexPath *> *)insertIndexPaths
+ deleteIndexPaths:(NSArray<NSIndexPath *> *)deleteIndexPaths
+ updateIndexPaths:(NSArray<NSIndexPath *> *)updateIndexPaths
+ moveIndexPaths:(NSArray<IGListMoveIndexPath *> *)moveIndexPaths;
 
 /**
  :nodoc:

Source/Common/IGListBatchUpdateData.mm

@@ -43,6 +43,23 @@ + (void)_cleanIndexPathsWithMap:(const std::unordered_map<NSInteger, IGListMoveI
  }
 }
 
+- (instancetype)initWithInsertSections:(NSIndexSet *)insertSections
+ deleteSections:(NSIndexSet *)deleteSections
+ moveSections:(NSSet<IGListMoveIndex *> *)moveSections
+ insertIndexPaths:(NSArray<NSIndexPath *> *)insertIndexPaths
+ deleteIndexPaths:(NSArray<NSIndexPath *> *)deleteIndexPaths
+ updateIndexPaths:(NSArray<NSIndexPath *> *)updateIndexPaths
+ moveIndexPaths:(NSArray<IGListMoveIndexPath *> *)moveIndexPaths {
+ return [self initWithInsertSections:insertSections
+ deleteSections:deleteSections
+ moveSections:moveSections
+ insertIndexPaths:insertIndexPaths
+ deleteIndexPaths:deleteIndexPaths
+ updateIndexPaths:updateIndexPaths
+ moveIndexPaths:moveIndexPaths
+ fixIndexPathImbalance:NO];
+}
+
 /**
  Converts all section moves that are also reloaded, or have index path inserts, deletes, or reloads into a section
  delete + insert in order to avoid UICollectionView heap corruptions, exceptions, and animation/snapshot bugs.
@@ -53,7 +70,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
+ fixIndexPathImbalance:(BOOL)fixIndexPathImbalance {
  IGParameterAssert(insertSections != nil);
  IGParameterAssert(deleteSections != nil);
  IGParameterAssert(moveSections != nil);
@@ -88,12 +106,20 @@ - (instancetype)initWithInsertSections:(nonnull NSIndexSet *)insertSections
  }
  }
 
- NSMutableArray<NSIndexPath *> *mInsertIndexPaths = [insertIndexPaths mutableCopy];
-
  // 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;
+ if (fixIndexPathImbalance) {
+ // Since we remove duplicate deletes (see above) we also need to remove inserts to keep the same insert/delete
+ // balance. For example, if we reload (insert & delete) the same NSIndexPath twice, we would otherwise end up
+ // with 2 inserts and 1 delete.
+ mInsertIndexPaths = [[[NSSet setWithArray:insertIndexPaths] allObjects] mutableCopy];
+ } else {
+ 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];
 

Source/Common/IGListExperiments.h

@@ -22,6 +22,8 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) {
  IGListExperimentReloadDataFallback = 1 << 3,
  /// Test removing the layout pass when calling scrollToObject to avoid creating off-screen cells.
  IGListExperimentAvoidLayoutOnScrollToObject = 1 << 4,
+ /// Test fixing a crash when inserting and deleting the same NSIndexPath multiple times.
+ IGListExperimentFixIndexPathImbalance = 1 << 5,
  /// Test deferring object creation until just before diffing.
  IGListExperimentDeferredToObjectCreation = 1 << 6,
  /// Test getting collection view at update time.

Source/IGListAdapterUpdater.m

@@ -369,13 +369,15 @@ - (IGListBatchUpdateData *)_flushCollectionView:(UICollectionView *)collectionVi
  [itemDeletes addObjectsFromArray:[reloadDeletePaths allObjects]];
  [itemInserts addObjectsFromArray:[reloadInsertPaths allObjects]];
 
+ const BOOL fixIndexPathImbalance = IGListExperimentEnabled(self.experiments, IGListExperimentFixIndexPathImbalance);
  IGListBatchUpdateData *updateData = [[IGListBatchUpdateData alloc] initWithInsertSections:inserts
  deleteSections:deletes
  moveSections:moves
  insertIndexPaths:itemInserts
  deleteIndexPaths:itemDeletes
  updateIndexPaths:itemUpdates
- moveIndexPaths:itemMoves];
+ moveIndexPaths:itemMoves
+ fixIndexPathImbalance:fixIndexPathImbalance];
  [collectionView ig_applyBatchUpdateData:updateData];
  return updateData;
 }

Tests/IGListAdapterUpdaterTests.m

@@ -751,6 +751,42 @@ - (void)test_whenPerformUpdates_dataSourceWasSetToNil_shouldNotCrash {
  [mockDelegate verify];
 }
 
+- (void)test_whenPerformIndexPathUpdates_insertDeleteTheSameIndexPathMultipleTimes_shouldNotCrash {
+ // Set up the fix
+ self.updater.experiments |= IGListExperimentFixIndexPathImbalance;
+
+ // Set up data
+ NSArray<IGSectionObject *> *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id"]];
+ self.dataSource.sections = from;
+
+ // Mock delegate to confirm update did work
+ id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)];
+ self.updater.delegate = mockDelegate;
+ [mockDelegate setExpectationOrderMatters:YES];
+ [[mockDelegate expect] listAdapterUpdater:self.updater didPerformBatchUpdates:OCMOCK_ANY collectionView:self.collectionView];
+
+ // Expectation to wait for performUpdate to finish
+ XCTestExpectation *expectation = genExpectation;
+
+ NSArray<NSIndexPath *> *indexPaths = @[[NSIndexPath indexPathForItem:0 inSection:0]];
+ [self.updater performUpdateWithCollectionViewBlock:[self collectionViewBlock]
+ animated:NO
+ itemUpdates:^{
+ [self.updater insertItemsIntoCollectionView:self.collectionView indexPaths:indexPaths];
+ [self.updater deleteItemsFromCollectionView:self.collectionView indexPaths:indexPaths];
+
+ [self.updater insertItemsIntoCollectionView:self.collectionView indexPaths:indexPaths];
+ [self.updater deleteItemsFromCollectionView:self.collectionView indexPaths:indexPaths];
+ }
+ completion:^(BOOL finished) {
+ [expectation fulfill];
+ }];
+
+ waitExpectation;
+
+ [mockDelegate verify];
+}
+
 # pragma mark - preferItemReloadsFroSectionReloads
 
 - (void)test_whenReloadIsCalledWithSameItemCount_andPreferItemReload_updateIndexPathsHappen {

Tests/IGListBatchUpdateDataTests.m

@@ -138,4 +138,29 @@ - (void)test_whenMovingSections_withMoveFromConflictWithDelete_thatResultDropsTh
  XCTAssertEqualObjects(result.moveSections.anyObject, newMove(0, 2));
 }
 
+- (void)test_whenDeletingSameIndexPathMultipleTimes_thatResultDropsTheDuplicates {
+ IGListBatchUpdateData *result = [[IGListBatchUpdateData alloc] initWithInsertSections:indexSet(@[])
+ deleteSections:indexSet(@[])
+ moveSections:[NSSet new]
+ insertIndexPaths:@[]
+ deleteIndexPaths:@[newPath(2, 0), newPath(2, 0)]
+ updateIndexPaths:@[]
+ moveIndexPaths:@[]];
+
+ XCTAssertEqualObjects(result.deleteIndexPaths, @[newPath(2, 0)]);
+}
+
+- (void)test_whenInsertingSameIndexPathMultipleTimes_thatResultDropsTheDuplicates {
+ IGListBatchUpdateData *result = [[IGListBatchUpdateData alloc] initWithInsertSections:indexSet(@[])
+ deleteSections:indexSet(@[])
+ moveSections:[NSSet new]
+ insertIndexPaths:@[newPath(2, 0), newPath(2, 0)]
+ deleteIndexPaths:@[]
+ updateIndexPaths:@[]
+ moveIndexPaths:@[]
+ fixIndexPathImbalance:YES];
+
+ XCTAssertEqualObjects(result.insertIndexPaths, @[newPath(2, 0)]);
+}
+
 @end