From d9a89c9b00aa1a9537a24d9affb6919f83065f65 Mon Sep 17 00:00:00 2001 From: Ryan Nystrom Date: Wed, 20 Sep 2017 09:40:59 -0700 Subject: [PATCH] Defer invalidatation until all batch updates are finished Summary: Added a mechanism to `IGListAdapter` to defer (_queue or execute_) blocks around asking the updater to do batch updates. I discovered a crash where the `_updateCompletionHandler` (private ivar) of `UICollectionView` was not being niled when two batch updates collide. There is an assert in `-[UICollectionView dealloc]` that fires if this block != nil. Took some reverse eng to track down what is happening. Sadly I cannot reproduce the issue in a unit test _or_ sample app. However I can 100% repro this in my GitHawk app. Applying this patch fixes the problem. I added a unit test that got me _close_ to the state of the crash. Consider this new test future-proofing (also covers the enter/exit/defer stuff). Also dumping the `UICollectionView` internals confirms that the bad state (`_updateCompletionHandler != nil`) is no longer true. Issue fixed: #929 Depends on #930 - [x] All tests pass. Demo project builds and runs. - [x] I added tests, an experiment, or detailed why my change isn't tested. - [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. Closes https://github.com/Instagram/IGListKit/pull/931 Differential Revision: D5863761 Pulled By: rnystrom fbshipit-source-id: 496aa939e3a5e83472b26bfb5a4cdc872f58cc0e --- CHANGELOG.md | 8 +++--- Source/IGListAdapter.m | 43 ++++++++++++++++++++++++++---- Tests/IGListAdapterE2ETests.m | 49 +++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ec3a808..08a9d48d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,13 +5,15 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag 3.2.0 (upcoming release) ----- +### Enhancements + +- Added `-[IGListSectionController didHighlightItemAtIndex:]` and `-[IGListSectionController didUnhighlightItemAtIndex:]` APIs to support `UICollectionView` cell highlighting. [Kevin Delannoy](https://github.com/delannoyk) [(#933)](https://github.com/Instagram/IGListKit/pull/933) + ### Fixes - Weakly reference the `UICollectionView` in coalescence so that it can be released if the rest of system is destroyed. [Ryan Nystrom](https://github.com/rnystrom) [(#tbd)](https://github.com/Instagram/IGListKit/pull/tbd) -### Enhancements - -- Added `-[IGListSectionController didHighlightItemAtIndex:]` and `-[IGListSectionController didUnhighlightItemAtIndex:]` APIs to support `UICollectionView` cell highlighting. [Kevin Delannoy](https://github.com/delannoyk) [(#933)](https://github.com/Instagram/IGListKit/pull/933) +- Avoid crash when invalidating the layout while inside `-[UICollectionView performBatchUpdates:completion:]. [Ryan Nystrom](https://github.com/rnystrom) [(#tbd)](https://github.com/Instagram/IGListKit/pull/tbd) 3.1.1 ----- diff --git a/Source/IGListAdapter.m b/Source/IGListAdapter.m index 090a9fc9..a3410002 100644 --- a/Source/IGListAdapter.m +++ b/Source/IGListAdapter.m @@ -18,6 +18,8 @@ @implementation IGListAdapter { NSMapTable *_viewSectionControllerMap; + // An array of blocks to execute once batch updates are finished + NSMutableArray *_queuedCompletionBlocks; } - (void)dealloc { @@ -317,6 +319,8 @@ NSArray *fromObjects = self.sectionMap.objects; NSArray *newObjects = [dataSource objectsForListAdapter:self]; + [self enterBatchUpdates]; + __weak __typeof__(self) weakSelf = self; [self.updater performUpdateWithCollectionView:collectionView fromObjects:fromObjects @@ -335,6 +339,8 @@ if (completion) { completion(finished); } + + [weakSelf exitBatchUpdates]; }]; } @@ -702,6 +708,26 @@ [_viewSectionControllerMap removeObjectForKey:view]; } +- (void)deferBlockBetweenBatchUpdates:(void (^)())block { + IGAssertMainThread(); + if (_queuedCompletionBlocks == nil) { + block(); + } else { + [_queuedCompletionBlocks addObject:block]; + } +} + +- (void)enterBatchUpdates { + _queuedCompletionBlocks = [NSMutableArray new]; +} + +- (void)exitBatchUpdates { + for (void (^block)() in _queuedCompletionBlocks) { + block(); + } + _queuedCompletionBlocks = nil; +} + #pragma mark - UIScrollViewDelegate - (void)scrollViewDidScroll:(UIScrollView *)scrollView { @@ -964,6 +990,8 @@ IGParameterAssert(updates != nil); UICollectionView *collectionView = self.collectionView; IGAssert(collectionView != nil, @"Performing batch updates without a collection view."); + + [self enterBatchUpdates]; __weak __typeof__(self) weakSelf = self; [self.updater performUpdateWithCollectionView:collectionView animated:animated itemUpdates:^{ @@ -976,6 +1004,8 @@ if (completion) { completion(finished); } + + [weakSelf exitBatchUpdates]; }]; } @@ -1004,11 +1034,14 @@ UICollectionViewLayoutInvalidationContext *context = [[[layout.class invalidationContextClass] alloc] init]; [context invalidateItemsAtIndexPaths:indexPaths]; - void (^block)() = ^{ - [layout invalidateLayoutWithContext:context]; - }; - - [_collectionView performBatchUpdates:block completion:completion]; + __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]; + }]; } #pragma mark - IGListBatchContext diff --git a/Tests/IGListAdapterE2ETests.m b/Tests/IGListAdapterE2ETests.m index 48067c15..00762f1c 100644 --- a/Tests/IGListAdapterE2ETests.m +++ b/Tests/IGListAdapterE2ETests.m @@ -1552,4 +1552,53 @@ [self waitForExpectationsWithTimeout:30 handler:nil]; } +- (void)test_whenInvalidatingInsideBatchUpdate_withSystemReleased_thatSystemNil_andCollectionViewDoesntCrashOnDealloc { + __weak id weakAdapter = nil; + __block BOOL executedItemUpdate = NO; + XCTestExpectation *expectation = genExpectation; + + @autoreleasepool { + self.dataSource.objects = @[ + genTestObject(@1, @"Bar"), + genTestObject(@0, @"Foo") + ]; + + 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 *section = [adapter sectionControllerForObject:self.dataSource.objects.firstObject]; + + __weak typeof(section) weakSection = section; + section.itemUpdateBlock = ^{ + executedItemUpdate = YES; + [weakSection.collectionContext invalidateLayoutForSectionController:weakSection completion:nil]; + }; + + self.dataSource.objects = @[ + genTestObject(@1, @"Bar"), + genTestObject(@0, @"Foo") + ]; + + [adapter performUpdatesAnimated:YES completion:^(BOOL finished) { + XCTAssertNotNil(collectionView); + XCTAssertNotNil(adapter); + [collectionView removeFromSuperview]; + [expectation fulfill]; + }]; + + weakAdapter = adapter; + XCTAssertNotNil(weakAdapter); + } + + [self waitForExpectationsWithTimeout:30 handler:^(NSError * _Nullable error) { + XCTAssertTrue(executedItemUpdate); + XCTAssertNil(weakAdapter); + }]; +} + @end