From cbaa7a8dce971dd1da81de148585744ee5555e9d Mon Sep 17 00:00:00 2001 From: Maxime Ollivier Date: Wed, 16 Sep 2020 16:16:40 -0700 Subject: [PATCH] fix invalidateLayout crash Summary: Pretty sure in `-invalidateLayout`, we should calculate the `NSIndexPaths` after the delay. Otherwise, those `NSIndexPaths` might be incorrect after the update. Before this change, running `test_whenInvalidatingInsideBatchUpdate_andRemoveThatSectionController_thatCollectionViewDoesntCrash` crashes with: > Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'attempting to invalidate an item at an invalid indexPath: {length = 2, path = 1 - 0} globalIndex: 1 numItems: 1' Reviewed By: candance Differential Revision: D23742819 fbshipit-source-id: 39496d04025ff3554673313df3c7516c33c305d4 --- Source/IGListKit/IGListAdapter.m | 22 +++++++++------ Tests/IGListAdapterE2ETests.m | 33 +++++++++++++++++++++++ Tests/IGListExperimentalAdapterE2ETests.m | 33 +++++++++++++++++++++++ 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/Source/IGListKit/IGListAdapter.m b/Source/IGListKit/IGListAdapter.m index fe2f9d34..1ef2bda9 100644 --- a/Source/IGListKit/IGListAdapter.m +++ b/Source/IGListKit/IGListAdapter.m @@ -1271,6 +1271,17 @@ - (void)invalidateLayoutForSectionController:(IGListSectionController *)sectionController completion:(void (^)(BOOL finished))completion { + __weak __typeof__(self) weakSelf = self; + + // do not call -[UICollectionView performBatchUpdates:completion:] while already updating. defer it until completed. + [self _deferBlockBetweenBatchUpdates:^{ + // Note that we calculate the `NSIndexPaths` after the batch update, otherwise they're be wrong. + [weakSelf _invalidateLayoutForSectionController:sectionController completion:completion]; + }]; +} + +- (void)_invalidateLayoutForSectionController:(IGListSectionController *)sectionController + completion:(void (^)(BOOL finished))completion { const NSInteger section = [self sectionForSectionController:sectionController]; if (section == NSNotFound) { // The section controller is not in the map, which can happen if the associated object was deleted or after a full reload. @@ -1291,14 +1302,9 @@ UICollectionViewLayoutInvalidationContext *context = [[[layout.class invalidationContextClass] alloc] init]; [context invalidateItemsAtIndexPaths:indexPaths]; - __weak __typeof__(_collectionView) weakCollectionView = _collectionView; - - // do not call -[UICollectionView performBatchUpdates:completion:] while already updating. defer it until completed. - [self _deferBlockBetweenBatchUpdates:^{ - [weakCollectionView performBatchUpdates:^{ - [layout invalidateLayoutWithContext:context]; - } completion:completion]; - }]; + [_collectionView performBatchUpdates:^{ + [layout invalidateLayoutWithContext:context]; + } completion:completion]; } #pragma mark - IGListBatchContext diff --git a/Tests/IGListAdapterE2ETests.m b/Tests/IGListAdapterE2ETests.m index dec95759..4b8ac1bf 100644 --- a/Tests/IGListAdapterE2ETests.m +++ b/Tests/IGListAdapterE2ETests.m @@ -1597,6 +1597,39 @@ }]; } +- (void)test_whenInvalidatingInsideBatchUpdate_andRemoveThatSectionController_thatCollectionViewDoesntCrash { + IGTestObject *foo = genTestObject(@1, @"Foo"); + IGTestObject *bar = genTestObject(@0, @"Bar"); + self.dataSource.objects = @[foo, bar]; + + UICollectionView *collectionView = [[UICollectionView alloc] initWithFrame:self.window.frame collectionViewLayout:[UICollectionViewFlowLayout new]]; + [self.window addSubview:collectionView]; + IGListAdapterUpdater *updater = [IGListAdapterUpdater new]; + IGListAdapter *adapter = [[IGListAdapter alloc] initWithUpdater:updater viewController:nil]; + adapter.dataSource = self.dataSource; + adapter.collectionView = collectionView; + [collectionView layoutIfNeeded]; + + IGTestDelegateController *sectionToRemove = [adapter sectionControllerForObject:bar]; + + self.dataSource.objects = @[foo]; + + XCTestExpectation *expectation = genExpectation; + [adapter performUpdatesAnimated:YES completion:^(BOOL finished) { + XCTAssertTrue(finished); + [expectation fulfill]; + }]; + + XCTestExpectation *expectation2 = genExpectation; + [sectionToRemove.collectionContext invalidateLayoutForSectionController:sectionToRemove completion:^(BOOL finished) { + // That section-controller is about to be removed, so this should not finish. + XCTAssertFalse(finished); + [expectation2 fulfill]; + }]; + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + - (void)test_whenAddingMultipleUpdateListeners_withPerformUpdatesAnimated_thatEventsReceived { [self setupWithObjects:@[ genTestObject(@1, @1) diff --git a/Tests/IGListExperimentalAdapterE2ETests.m b/Tests/IGListExperimentalAdapterE2ETests.m index 1d227976..71472e8b 100644 --- a/Tests/IGListExperimentalAdapterE2ETests.m +++ b/Tests/IGListExperimentalAdapterE2ETests.m @@ -2005,6 +2005,39 @@ }]; } +- (void)test_whenInvalidatingInsideBatchUpdate_andRemoveThatSectionController_thatCollectionViewDoesntCrash { + IGTestObject *foo = genTestObject(@1, @"Foo"); + IGTestObject *bar = genTestObject(@0, @"Bar"); + self.dataSource.objects = @[foo, bar]; + + UICollectionView *collectionView = [[UICollectionView alloc] initWithFrame:self.window.frame collectionViewLayout:[UICollectionViewFlowLayout new]]; + [self.window addSubview:collectionView]; + IGListExperimentalAdapterUpdater *updater = [IGListExperimentalAdapterUpdater new]; + IGListAdapter *adapter = [[IGListAdapter alloc] initWithUpdater:updater viewController:nil]; + adapter.dataSource = self.dataSource; + adapter.collectionView = collectionView; + [collectionView layoutIfNeeded]; + + IGTestDelegateController *sectionToRemove = [adapter sectionControllerForObject:bar]; + + self.dataSource.objects = @[foo]; + + XCTestExpectation *expectation = genExpectation; + [adapter performUpdatesAnimated:YES completion:^(BOOL finished) { + XCTAssertTrue(finished); + [expectation fulfill]; + }]; + + XCTestExpectation *expectation2 = genExpectation; + [sectionToRemove.collectionContext invalidateLayoutForSectionController:sectionToRemove completion:^(BOOL finished) { + // That section-controller is about to be removed, so this should not finish. + XCTAssertFalse(finished); + [expectation2 fulfill]; + }]; + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + - (void)test_whenPerformingBatchSectionUpdate_thatTransactionObjectsGetsDeallocated { __weak IGListUpdateTransactionBuilder *transactionBuilder = nil; __block __weak IGListUpdateTransactionBuilder *lastTransactionBuilder = nil;