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
This commit is contained in:
Ryan Nystrom 2017-09-20 09:40:59 -07:00 committed by Facebook Github Bot
parent 9ddc64f4ce
commit d9a89c9b00
3 changed files with 92 additions and 8 deletions

View file

@ -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
-----

View file

@ -18,6 +18,8 @@
@implementation IGListAdapter {
NSMapTable<UICollectionReusableView *, IGListSectionController *> *_viewSectionControllerMap;
// An array of blocks to execute once batch updates are finished
NSMutableArray<void (^)()> *_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

View file

@ -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