Clean listAdapterUpdater state if collectionView becomes nil during u…

…pdate

Summary:
If an `IGListAdapterUpdater` was unable to retrieve a collection view in `[IGListAdapterUpdater performBatchUpdatesWithCollectionViewBlock:]`, it would return early without cleaning state. This would sometimes cause future updates to crash, as this `IGListAdapterUpdater`'s state would now be out-of-sync.

This change also ensures that the `IGListAdapterUpdater`'s completion blocks run when `[IGListAdapterUpdater performBatchUpdatesWithCollectionViewBlock:]` and `[IGListAdapterUpdater performReloadDataWithCollectionViewBlock:]` return early due to having a nil `UICollectionView`.

Reviewed By: rnystrom

Differential Revision: D8056539

fbshipit-source-id: 1af7b675ec6805c2d8031f32d8a4c8e8929564e6

CHANGELOG.md

@@ -13,6 +13,9 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag
 
 - Experimental fix to get the `UICollectionView` for batch updating immediately before applying the update. [Ryan Nystrom](https://github.com/rnystrom) (tbd)
 
+
+- `[IGListAdapterUpdater performBatchUpdatesWithCollectionViewBlock:]` and `[IGListAdapterUpdater performReloadDataWithCollectionViewBlock:]` clean state and run completion blocks if their `UICollectionView` is nil. [Brandon Darin](https://github.com/jbd1030) (tbd)
+
 3.4.0
 -----
 

Source/IGListAdapterUpdater.m

@@ -44,20 +44,30 @@ - (BOOL)hasChanges {
 
 - (void)performReloadDataWithCollectionViewBlock:(IGListCollectionViewBlock)collectionViewBlock {
     IGAssertMainThread();
-
-    // bail early if the collection view has been deallocated in the time since the update was queued
-    UICollectionView *collectionView = collectionViewBlock();
-    if (collectionView == nil) {
-        return;
-    }
-
+
     id<IGListAdapterUpdaterDelegate> delegate = self.delegate;
     void (^reloadUpdates)(void) = self.reloadUpdates;
     IGListBatchUpdates *batchUpdates = self.batchUpdates;
     NSMutableArray *completionBlocks = [self.completionBlocks mutableCopy];
 
     [self cleanStateBeforeUpdates];
 
+    void (^executeCompletionBlocks)(BOOL) = ^(BOOL finished) {
+        for (IGListUpdatingCompletion block in completionBlocks) {
+            block(finished);
+        }
+
+        self.state = IGListBatchUpdateStateIdle;
+    };
+
+    // bail early if the collection view has been deallocated in the time since the update was queued
+    UICollectionView *collectionView = collectionViewBlock();
+    if (collectionView == nil) {
+        [self _cleanStateAfterUpdates];
+        executeCompletionBlocks(NO);
+        return;
+    }
+
     // item updates must not send mutations to the collection view while we are reloading
     self.state = IGListBatchUpdateStateExecutingBatchUpdateBlock;
 
@@ -86,23 +96,13 @@ - (void)performReloadDataWithCollectionViewBlock:(IGListCollectionViewBlock)coll
     [collectionView layoutIfNeeded];
     [delegate listAdapterUpdater:self didReloadDataWithCollectionView:collectionView];
 
-    for (IGListUpdatingCompletion block in completionBlocks) {
-        block(YES);
-    }
-
-    self.state = IGListBatchUpdateStateIdle;
+    executeCompletionBlocks(YES);
 }
 
 - (void)performBatchUpdatesWithCollectionViewBlock:(IGListCollectionViewBlock)collectionViewBlock {
     IGAssertMainThread();
     IGAssert(self.state == IGListBatchUpdateStateIdle, @"Should not call batch updates when state isn't idle");
 
-    // bail early if the collection view has been deallocated in the time since the update was queued
-    UICollectionView *collectionView = collectionViewBlock();
-    if (collectionView == nil) {
-        return;
-    }
-
     // create local variables so we can immediately clean our state but pass these items into the batch update block
     id<IGListAdapterUpdaterDelegate> delegate = self.delegate;
     NSArray *fromObjects = [self.fromObjects copy];
@@ -112,6 +112,26 @@ - (void)performBatchUpdatesWithCollectionViewBlock:(IGListCollectionViewBlock)co
     const BOOL animated = self.queuedUpdateIsAnimated;
     IGListBatchUpdates *batchUpdates = self.batchUpdates;
 
+    // clean up all state so that new updates can be coalesced while the current update is in flight
+    [self cleanStateBeforeUpdates];
+
+    void (^executeCompletionBlocks)(BOOL) = ^(BOOL finished) {
+        self.applyingUpdateData = nil;
+        self.state = IGListBatchUpdateStateIdle;
+
+        for (IGListUpdatingCompletion block in completionBlocks) {
+            block(finished);
+        }
+    };
+
+    // bail early if the collection view has been deallocated in the time since the update was queued
+    UICollectionView *collectionView = collectionViewBlock();
+    if (collectionView == nil) {
+        [self _cleanStateAfterUpdates];
+        executeCompletionBlocks(NO);
+        return;
+    }
+
     NSArray *toObjects = nil;
     if (toObjectsBlock != nil) {
         toObjects = objectsWithDuplicateIdentifiersRemoved(toObjectsBlock());
@@ -125,9 +145,6 @@ - (void)performBatchUpdatesWithCollectionViewBlock:(IGListCollectionViewBlock)co
     }
 #endif
 
-    // clean up all state so that new updates can be coalesced while the current update is in flight
-    [self cleanStateBeforeUpdates];
-
     void (^executeUpdateBlocks)(void) = ^{
         self.state = IGListBatchUpdateStateExecutingBatchUpdateBlock;
 
@@ -152,15 +169,6 @@ - (void)performBatchUpdatesWithCollectionViewBlock:(IGListCollectionViewBlock)co
         self.state = IGListBatchUpdateStateExecutedBatchUpdateBlock;
     };
 
-    void (^executeCompletionBlocks)(BOOL) = ^(BOOL finished) {
-        self.applyingUpdateData = nil;
-        self.state = IGListBatchUpdateStateIdle;
-
-        for (IGListUpdatingCompletion block in completionBlocks) {
-            block(finished);
-        }
-    };
-
     void (^reloadDataFallback)(void) = ^{
         executeUpdateBlocks();
         [self _cleanStateAfterUpdates];

Tests/IGListAdapterE2ETests.m

@@ -1881,4 +1881,96 @@ - (void)test_whenSwappingCollectionViewsAfterUpdate_thatUpdatePerformedOnTheCorr
     [self waitForExpectationsWithTimeout:30 handler:nil];
 }
 
+- (void)test_whenCollectionViewBecomesNilDuringPerformUpdates_thatStateCleanedCorrectly {
+    [self setupWithObjects:@[
+                             genTestObject(@1, @1)
+                             ]];
+    self.adapter.experiments |= IGListExperimentGetCollectionViewAtUpdate;
+
+    // perform update on listAdapter
+    XCTestExpectation *expectation1 = genExpectation;
+    [self.adapter performUpdatesAnimated:NO completion:^(BOOL finished) {
+        [expectation1 fulfill];
+    }];
+    [self waitForExpectationsWithTimeout:30 handler:nil];
+
+    // update the underlying contents before performing another update
+    self.dataSource.objects = @[
+                                genTestObject(@1, @1),
+                                genTestObject(@2, @1)
+                                ];
+
+    // perform update, but set the listAdapter's collectionView to nil during the update
+    XCTestExpectation *expectation2 = genExpectation;
+    [self.adapter performUpdatesAnimated:NO completion:^(BOOL finished) {
+        [expectation2 fulfill];
+    }];
+    self.adapter.collectionView = nil;
+    [self waitForExpectationsWithTimeout:30 handler:nil];
+
+    // add a new collectionView to the listAdapter
+    UICollectionView *collectionView2 = [[UICollectionView alloc] initWithFrame:self.window.frame collectionViewLayout:[UICollectionViewFlowLayout new]];
+    [self.window addSubview:collectionView2];
+    self.adapter.collectionView = collectionView2;
+
+    // update the underlying contents before performing update
+    self.dataSource.objects = @[
+                                genTestObject(@1, @1),
+                                genTestObject(@2, @1),
+                                genTestObject(@3, @1)
+                                ];
+
+    // perform update on listAdapter (now with a non-nil collectionView)
+    XCTestExpectation *expectation3 = genExpectation;
+    [self.adapter performUpdatesAnimated:NO completion:^(BOOL finished) {
+        [expectation3 fulfill];
+    }];
+    [self waitForExpectationsWithTimeout:30 handler:nil];
+}
+
+- (void)test_whenCollectionViewBecomesNilDuringReloadData_thatStateCleanedCorrectly {
+    [self setupWithObjects:@[
+                             genTestObject(@1, @1)
+                             ]];
+    self.adapter.experiments |= IGListExperimentGetCollectionViewAtUpdate;
+
+    // reload data on listAdapter
+    XCTestExpectation *expectation1 = genExpectation;
+    [self.adapter reloadDataWithCompletion:^(BOOL finished) {
+        [expectation1 fulfill];
+    }];
+    [self waitForExpectationsWithTimeout:30 handler:nil];
+
+    // update the underlying contents before reloading again
+    self.dataSource.objects = @[
+                                genTestObject(@1, @1),
+                                genTestObject(@2, @1)
+                                ];
+
+    // reload data, but set the listAdapter's collectionView to nil during the update
+    XCTestExpectation *expectation2 = genExpectation;
+    [self.adapter reloadDataWithCompletion:^(BOOL finished) {
+        [expectation2 fulfill];
+    }];
+    self.adapter.collectionView = nil;
+    [self waitForExpectationsWithTimeout:30 handler:nil];
+
+    // add a new collectionView to the listAdapter
+    UICollectionView *collectionView2 = [[UICollectionView alloc] initWithFrame:self.window.frame collectionViewLayout:[UICollectionViewFlowLayout new]];
+    [self.window addSubview:collectionView2];
+    self.adapter.collectionView = collectionView2;
+    self.dataSource.objects = @[
+                                genTestObject(@1, @1),
+                                genTestObject(@2, @1),
+                                genTestObject(@3, @1)
+                                ];
+
+    // reload data on listAdapter (now with a non-nil collectionView)
+    XCTestExpectation *expectation3 = genExpectation;
+    [self.adapter reloadDataWithCompletion:^(BOOL finished) {
+        [expectation3 fulfill];
+    }];
+    [self waitForExpectationsWithTimeout:30 handler:nil];
+}
+
 @end