fix crash when calling reloadObjects during an update

Summary:
Follow up to D47263265

## What's happening
We're calling `listAdapter.reloadObjects(...)` in the middle of the IGListKit's internal data update, so it's using the new section index path, instead of the old one. So we're going from a list of 2, insert at 0, and try to reload index 1, then we crash (it'll try to update index 2, which doesn't exist in the old array)

## Fix
Just like for `[IGListAdapter performBatchAnimated:updates:completion:]`, lets use the old index. To get this working, lets ask the updater if we're in an update, rather than `IGListAdapter` try to keep track of it.

Sorry the diff is a bit long, but in case this feels reasonable to pick, I might

Differential Revision: D47281472

fbshipit-source-id: 193153cfc15c87084b67f58c50e921db459d6800

CHANGELOG.md

@@ -94,6 +94,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag
 
 ### Fixes
 
+- Testing crash fix when calling `-[IGListAdapter reloadObjects ...]` during an update [Maxime Ollivier](https://github.com/maxolls) (tbd)
+
 - Repaired Swift Package Manager support. [Petro Rovenskyy](https://github.com/3a4oT/) [(#1487)](https://github.com/Instagram/IGListKit/pull/1487)
 
 - `IGListCollectionViewLayout` should get the section/index counts via `UICollectionView` to stay in sync, instead of the `dataSource` [Maxime Ollivier](https://github.com/maxolls) [(677ce77)](https://github.com/Instagram/IGListKit/commit/677ce77ecad11850f61436681ee1d04a5e67e96a)

Source/IGListDiffKit/IGListExperiments.h

@@ -21,7 +21,9 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) {
     /// Test skipping performBatchUpdate if we don't have any updates.
     IGListExperimentSkipPerformUpdateIfPossible = 1 << 3,
     /// Test skipping creating {view : section controller} map, which has inconsistency issue.
-    IGListExperimentSkipViewSectionControllerMap = 1 << 4
+    IGListExperimentSkipViewSectionControllerMap = 1 << 4,
+    /// Use the correct section index when calling -[IGListAdapter reloadObjects ...] within the update block.
+    IGListExperimentFixCrashOnReloadObjects = 1 << 5
 };
 
 /**

Source/IGListKit/IGListAdapter.m

@@ -28,6 +28,11 @@
     CGFloat max;
 } OffsetRange;
 
+@interface IGListAdapter ()
+// Temporary param to key the old behavior during testing
+@property (nonatomic, assign) BOOL legacyIsInDataUpdateBlock;
+@end
+
 @implementation IGListAdapter {
     NSMapTable<UICollectionReusableView *, IGListSectionController *> *_viewSectionControllerMap;
     // An array of blocks to execute once batch updates are finished
@@ -422,6 +427,13 @@ - (void)performUpdatesAnimated:(BOOL)animated completion:(IGListUpdaterCompletio
             return;
         }
 
+        if (IGListExperimentEnabled(strongSelf.experiments, IGListExperimentFixCrashOnReloadObjects)) {
+            // All the backgroundView changes during the update will be disabled, since we don't know how
+            // many cells we'll really have until it's all done. As a follow up, we should probably have
+            // a centralized place to do this, and change the background before the animation starts.
+            [strongSelf _updateBackgroundViewShouldHide:![strongSelf _itemCountIsZero]];
+        }
+
         // release the previous items
         strongSelf.previousSectionMap = nil;
         [strongSelf _notifyDidUpdate:IGListAdapterUpdateTypePerformUpdates animated:animated];
@@ -765,7 +777,7 @@ - (void)_updateWithData:(IGListTransitionData *)data {
 }
 
 - (void)_updateBackgroundViewShouldHide:(BOOL)shouldHide {
-    if (self.isInUpdateBlock) {
+    if ([self isInDataUpdateBlock]) {
         return; // will be called again when update block completes
     }
 
@@ -797,7 +809,7 @@ - (BOOL)_itemCountIsZero {
 - (IGListSectionMap *)_sectionMapUsingPreviousIfInUpdateBlock:(BOOL)usePreviousMapIfInUpdateBlock {
     // if we are inside an update block, we may have to use the /previous/ item map for some operations
     IGListSectionMap *previousSectionMap = self.previousSectionMap;
-    if (usePreviousMapIfInUpdateBlock && self.isInUpdateBlock && previousSectionMap != nil) {
+    if (usePreviousMapIfInUpdateBlock && [self isInDataUpdateBlock] && previousSectionMap != nil) {
         return previousSectionMap;
     } else {
         return self.sectionMap;
@@ -902,6 +914,15 @@ - (void)_exitBatchUpdates {
     }
 }
 
+- (BOOL)isInDataUpdateBlock {
+    if (IGListExperimentEnabled(_experiments, IGListExperimentFixCrashOnReloadObjects)) {
+        // The updater has a better picture of when we're updating the data, including both section and item level changes.
+        return self.updater.isInDataUpdateBlock;
+    } else {
+        return self.legacyIsInDataUpdateBlock;
+    }
+}
+
 #pragma mark - UIScrollViewDelegate
 
 - (void)scrollViewDidScroll:(UIScrollView *)scrollView {
@@ -1268,10 +1289,10 @@ - (void)performBatchAnimated:(BOOL)animated updates:(void (^)(id<IGListBatchCont
     [self _enterBatchUpdates];
     __weak __typeof__(self) weakSelf = self;
     [self.updater performUpdateWithCollectionViewBlock:[self _collectionViewBlock] animated:animated itemUpdates:^{
-        weakSelf.isInUpdateBlock = YES;
+        weakSelf.legacyIsInDataUpdateBlock = YES;
         // the adapter acts as the batch context with its API stripped to just the IGListBatchContext protocol
         updates(weakSelf);
-        weakSelf.isInUpdateBlock = NO;
+        weakSelf.legacyIsInDataUpdateBlock = NO;
     } completion: ^(BOOL finished) {
         [weakSelf _updateBackgroundViewShouldHide:![weakSelf _itemCountIsZero]];
         [weakSelf _notifyDidUpdate:IGListAdapterUpdateTypeItemUpdates animated:animated];

Source/IGListKit/IGListAdapterUpdater.m

@@ -121,6 +121,10 @@ - (void)update {
     [transaction begin];
 }
 
+- (BOOL)isInDataUpdateBlock {
+    return self.transaction.state == IGListBatchUpdateStateExecutingBatchUpdateBlock;
+}
+
 #pragma mark - IGListUpdatingDelegate
 
 static BOOL IGListIsEqual(const void *a, const void *b, NSUInteger (*size)(const void *item)) {
@@ -173,7 +177,7 @@ - (void)performUpdateWithCollectionViewBlock:(IGListCollectionViewBlock)collecti
 
     // if already inside the execution of the update block, immediately unload the itemUpdates block.
     // the completion blocks are executed later in the lifecycle, so that still needs to be added to the batch
-    if (self.transaction.state == IGListBatchUpdateStateExecutingBatchUpdateBlock) {
+    if ([self isInDataUpdateBlock]) {
         if (completion != nil) {
             [self.transaction addCompletionBlock:completion];
         }
@@ -238,7 +242,7 @@ - (void)insertItemsIntoCollectionView:(UICollectionView *)collectionView indexPa
     IGAssertMainThread();
     IGParameterAssert(collectionView != nil);
     IGParameterAssert(indexPaths != nil);
-    if (self.transaction.state == IGListBatchUpdateStateExecutingBatchUpdateBlock) {
+    if ([self isInDataUpdateBlock]) {
         [self.transaction insertItemsAtIndexPaths:indexPaths];
     } else {
         [self.delegate listAdapterUpdater:self willInsertIndexPaths:indexPaths collectionView:collectionView];
@@ -250,7 +254,7 @@ - (void)deleteItemsFromCollectionView:(UICollectionView *)collectionView indexPa
     IGAssertMainThread();
     IGParameterAssert(collectionView != nil);
     IGParameterAssert(indexPaths != nil);
-    if (self.transaction.state == IGListBatchUpdateStateExecutingBatchUpdateBlock) {
+    if ([self isInDataUpdateBlock]) {
         [self.transaction deleteItemsAtIndexPaths:indexPaths];
     } else {
         [self.delegate listAdapterUpdater:self willDeleteIndexPaths:indexPaths collectionView:collectionView];
@@ -261,7 +265,7 @@ - (void)deleteItemsFromCollectionView:(UICollectionView *)collectionView indexPa
 - (void)moveItemInCollectionView:(UICollectionView *)collectionView
                    fromIndexPath:(NSIndexPath *)fromIndexPath
                      toIndexPath:(NSIndexPath *)toIndexPath {
-    if (self.transaction.state == IGListBatchUpdateStateExecutingBatchUpdateBlock) {
+    if ([self isInDataUpdateBlock]) {
         [self.transaction moveItemFromIndexPath:fromIndexPath toIndexPath:toIndexPath];
     } else {
         [self.delegate listAdapterUpdater:self willMoveFromIndexPath:fromIndexPath toIndexPath:toIndexPath collectionView:collectionView];
@@ -272,7 +276,7 @@ - (void)moveItemInCollectionView:(UICollectionView *)collectionView
 - (void)reloadItemInCollectionView:(UICollectionView *)collectionView
                      fromIndexPath:(NSIndexPath *)fromIndexPath
                        toIndexPath:(NSIndexPath *)toIndexPath {
-    if (self.transaction.state == IGListBatchUpdateStateExecutingBatchUpdateBlock) {
+    if ([self isInDataUpdateBlock]) {
         [self.transaction reloadItemFromIndexPath:fromIndexPath toIndexPath:toIndexPath];
     } else {
         [self.delegate listAdapterUpdater:self willReloadIndexPaths:@[fromIndexPath] collectionView:collectionView];
@@ -284,7 +288,7 @@ - (void)reloadCollectionView:(UICollectionView *)collectionView sections:(NSInde
     IGAssertMainThread();
     IGParameterAssert(collectionView != nil);
     IGParameterAssert(sections != nil);
-    if (self.transaction.state == IGListBatchUpdateStateExecutingBatchUpdateBlock) {
+    if ([self isInDataUpdateBlock]) {
         [self.transaction reloadSections:sections];
     } else {
         [self.delegate listAdapterUpdater:self willReloadSections:sections collectionView:collectionView];

Source/IGListKit/IGListReloadDataUpdater.m

@@ -7,7 +7,9 @@
 
 #import "IGListReloadDataUpdater.h"
 
-@implementation IGListReloadDataUpdater
+@implementation IGListReloadDataUpdater {
+    BOOL _isInDataUpdateBlock;
+}
 
 #pragma mark - IGListUpdatingDelegate
 
@@ -22,7 +24,9 @@ - (void)performUpdateWithCollectionViewBlock:(IGListCollectionViewBlock)collecti
                                   completion:(nullable IGListUpdatingCompletion)completion {
     IGListTransitionData *sectionData = sectionDataBlock ? sectionDataBlock() : nil;
     if (sectionData != nil && applySectionDataBlock != nil) {
+        _isInDataUpdateBlock = YES;
         applySectionDataBlock(sectionData);
+        _isInDataUpdateBlock = NO;
     }
     [self _synchronousReloadDataWithCollectionView:collectionViewBlock()];
     if (completion) {
@@ -84,4 +88,8 @@ - (void)_synchronousReloadDataWithCollectionView:(UICollectionView *)collectionV
     [collectionView layoutIfNeeded];
 }
 
+- (BOOL)isInDataUpdateBlock {
+    return _isInDataUpdateBlock;
+}
+
 @end

Source/IGListKit/IGListUpdatingDelegate.h

@@ -196,6 +196,12 @@ NS_SWIFT_NAME(ListUpdatingDelegate)
  */
 - (void)reloadCollectionView:(UICollectionView *)collectionView sections:(NSIndexSet *)sections;
 
+/**
+ True if the updater is currently updating the source of truth, like executing applySectionDataBlock and
+ itemUpdates just before updating the UICollectionView.
+ */
+- (BOOL)isInDataUpdateBlock;
+
 @end
 
 NS_ASSUME_NONNULL_END

Source/IGListKit/Internal/IGListAdapter+DebugDescription.m

@@ -28,7 +28,7 @@ - (NSString *)debugDescription {
     [debug addObject:[NSString stringWithFormat:@"Data source: %@", self.dataSource]];
     [debug addObject:[NSString stringWithFormat:@"Collection view delegate: %@", self.collectionViewDelegate]];
     [debug addObject:[NSString stringWithFormat:@"Scroll view delegate: %@", self.scrollViewDelegate]];
-    [debug addObject:[NSString stringWithFormat:@"Is in update block: %@", IGListDebugBOOL(self.isInUpdateBlock)]];
+    [debug addObject:[NSString stringWithFormat:@"Is in update block: %@", IGListDebugBOOL(self.isInDataUpdateBlock)]];
     [debug addObject:[NSString stringWithFormat:@"View controller: %@", self.viewController]];
     if (@available(iOS 10.0, tvOS 10, *)) {
         [debug addObject:[NSString stringWithFormat:@"Is prefetching enabled: %@", IGListDebugBOOL(self.collectionView.isPrefetchingEnabled)]];

Source/IGListKit/Internal/IGListAdapterInternal.h

@@ -59,8 +59,11 @@ IGListBatchContext
 
  Note that the previous section controller map is destroyed as soon as a transition is finished so there is no dangling
  objects or section controllers.
+ 
+ During this period, we're updating IGListKit's internal data, but not the UICollectionView just yet. This is a dangerous time, since the internal
+ data might only be partially updated.
  */
-@property (nonatomic, assign) BOOL isInUpdateBlock;
+@property (nonatomic, assign, readonly) BOOL isInDataUpdateBlock;
 @property (nonatomic, strong, nullable) IGListSectionMap *previousSectionMap;
 
 /**

Tests/IGListAdapterE2ETests.m

@@ -667,6 +667,41 @@ - (void)test_whenReloadingItems_withPerformUpdates_thatReloadIsApplied {
     [self waitForExpectationsWithTimeout:30 handler:nil];
 }
 
+- (void)test_whenReloadingItems_inItemUpdateBlock_thatDoesntCrash {
+    self.adapter.experiments |= IGListExperimentFixCrashOnReloadObjects;
+    // Without this fix, this test crashes with: Terminating app due to uncaught exception 'NSRangeException', reason: '*** -[__NSArrayI objectAtIndexedSubscript:]: index 2 beyond bounds [0 .. 1]'
+
+    [self setupWithObjects:@[
+        genTestObject(@1, @1),
+        genTestObject(@2, @2),
+    ]];
+
+    IGTestObject *object = self.dataSource.objects[1];
+    IGTestDelegateController *sectionController = [self.adapter sectionControllerForObject:object];
+    sectionController.itemUpdateBlock = ^{
+        // We shouldn't trigger updates within -didUpdateToObject, but in case we do,
+        // we should at least try to resolve it correctly.
+        [self.adapter reloadObjects:@[object]];
+    };
+
+    // Move object from index 1 -> 2, so the -reloadObjects will need to use the previous index (1)
+    self.dataSource.objects = @[
+        genTestObject(@1, @1),
+        genTestObject(@3, @3),
+        genTestObject(@2, @2), // Create a new object to trigger -didUpdateToObject
+    ];
+
+    XCTestExpectation *expectation = genExpectation;
+    [self.adapter performUpdatesAnimated:YES completion:^(BOOL finished2) {
+        // Most importantly, we don't want to crash, but lets also check the order is correct
+        XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 1);
+        XCTAssertEqual([self.collectionView numberOfItemsInSection:1], 3);
+        XCTAssertEqual([self.collectionView numberOfItemsInSection:2], 2);
+        [expectation fulfill];
+    }];
+    [self waitForExpectationsWithTimeout:30 handler:nil];
+}
+
 - (void)test_whenSectionControllerMutates_whenThereIsNoWindow_thatCollectionViewCountsAreUpdated {
     // remove the collection view from self.window so that we use reloadData
     [self.collectionView removeFromSuperview];

Tests/IGListAdapterTests.m

@@ -1304,7 +1304,7 @@ - (void)test_whenReloading_withSectionControllerNotFound_thatNoUpdatesHappen {
     self.dataSource.objects = @[@0, @1, @2];
     [self.adapter reloadDataWithCompletion:nil];
 
-    id mockDelegate = [OCMockObject mockForProtocol:@protocol(IGListUpdatingDelegate)];
+    id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListUpdatingDelegate)];
     [[mockDelegate reject] reloadCollectionView:[OCMArg any] sections:[OCMArg any]];
     self.adapter.updater = mockDelegate;