Fix some crash from IGListKit that nil dataSource might result in crash

Summary:
This happens when we set collectionView.dataSource to be nil before calling performUpdates:, the crash is easily reproduced by a simple unit test.

To make the infra more robust and we don't crash the app, let's add an early return before applying a collectionView update.

Reviewed By: rnystrom, calimarkus

Differential Revision: D12896196

fbshipit-source-id: ab024d0e7e9282d50c3be297e1e67cfccaff8242
This commit is contained in:
Zhisheng Huang 2018-11-06 11:35:33 -08:00 committed by Facebook Github Bot
parent 1bba448a5b
commit 6cdd112790
4 changed files with 70 additions and 2 deletions

View file

@ -22,6 +22,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag
### Fixes
- Fixed crash when the data source is nil before calling `-[IGListAdapterUpdater performUpdateWithCollectionViewBlock:fromObjects:toObjectsBlock:animated:objectTransitionBlock:completion:]`. [Zhisheng Huang](https://github.com/lorixx) (tbd)
- 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)

View file

@ -236,7 +236,10 @@ willPerformBatchUpdatesWithCollectionView:collectionView
fromObjects:fromObjects
toObjects:toObjects
listIndexSetResult:result];
if (result.changeCount > 100 && IGListExperimentEnabled(experiments, IGListExperimentReloadDataFallback)) {
if (collectionView.dataSource == nil) {
// If the data source is nil, we should not call any collection view update.
batchUpdatesCompletionBlock(NO);
} else if (result.changeCount > 100 && IGListExperimentEnabled(experiments, IGListExperimentReloadDataFallback)) {
reloadDataFallback();
} else if (animated) {
[collectionView performBatchUpdates:^{

View file

@ -11,7 +11,7 @@
@implementation UICollectionView (IGListBatchUpdateData)
- (void)ig_applyBatchUpdateData:(IGListBatchUpdateData *)updateData {
- (void)ig_applyBatchUpdateData:(IGListBatchUpdateData *)updateData {
[self deleteItemsAtIndexPaths:updateData.deleteIndexPaths];
[self insertItemsAtIndexPaths:updateData.insertIndexPaths];
[self reloadItemsAtIndexPaths:updateData.updateIndexPaths];

View file

@ -97,6 +97,17 @@
XCTAssertEqual([self.collectionView numberOfSections], 0);
}
- (void)test_whenReloadingDataWithNilDataSourceBefore_thatCollectionViewNotCrash {
self.dataSource.sections = @[[IGSectionObject sectionWithObjects:@[@1]], [IGSectionObject sectionWithObjects:@[@2]]];
[self.updater performReloadDataWithCollectionViewBlock:[self collectionViewBlock]];
XCTAssertEqual([self.collectionView numberOfSections], 2);
self.collectionView.dataSource = nil;
self.dataSource.sections = @[];
[self.updater performReloadDataWithCollectionViewBlock:[self collectionViewBlock]];
XCTAssertEqual([self.collectionView numberOfSections], 1); // Setting collectionView's dataSource to nil would yield a single section by default.
}
- (void)test_whenInsertingSection_thatCollectionViewUpdates {
NSArray *from = @[
[IGSectionObject sectionWithObjects:@[]]
@ -531,6 +542,28 @@
[mockDelegate verify];
}
- (void)test_whenCollectionViewNotInWindow_andBackgroundReloadFlag_isDefaultYES_andDataSourceWasSetToNilBefore_thatCollectionViewNotCrash {
[self.collectionView removeFromSuperview];
id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)];
self.updater.delegate = mockDelegate;
[[mockDelegate reject] listAdapterUpdater:self.updater willPerformBatchUpdatesWithCollectionView:self.collectionView fromObjects:@[] toObjects:@[] listIndexSetResult:OCMOCK_ANY];
[[mockDelegate reject] listAdapterUpdater:self.updater didPerformBatchUpdates:OCMOCK_ANY collectionView:self.collectionView];
XCTestExpectation *expectation = genExpectation;
IGListToObjectBlock to = ^NSArray *{
return @[
[IGSectionObject sectionWithObjects:@[]]
];
};
self.collectionView.dataSource = nil;
[self.updater performUpdateWithCollectionViewBlock:[self collectionViewBlock] fromObjects:self.dataSource.sections toObjectsBlock:to animated:NO objectTransitionBlock:self.updateBlock completion:^(BOOL finished) {
[expectation fulfill];
}];
waitExpectation;
[mockDelegate verify];
}
- (void)test_whenReloadBatchedWithUpdate_thatCompletionBlockStillExecuted {
IGSectionObject *object = [IGSectionObject sectionWithObjects:@[@0, @1, @2]];
self.dataSource.sections = @[object];
@ -688,6 +721,36 @@
[mockDelegate verify];
}
- (void)test_whenPerformUpdates_dataSourceWasSetToNil_shouldNotCrash {
NSArray<IGSectionObject *> *from = @[[IGSectionObject sectionWithObjects:@[@1] identifier:@"id1"]];
NSArray<IGSectionObject *> *to = @[[IGSectionObject sectionWithObjects:@[@2] identifier:@"id1"],
[IGSectionObject sectionWithObjects:@[@22] identifier:@"id2"]];
self.dataSource.sections = from;
id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)];
self.updater.delegate = mockDelegate;
[mockDelegate setExpectationOrderMatters:YES];
[[mockDelegate expect] listAdapterUpdater:self.updater willPerformBatchUpdatesWithCollectionView:self.collectionView fromObjects:from toObjects:to listIndexSetResult:[OCMArg checkWithBlock:^BOOL(IGListIndexSetResult *result) {
if (result.deletes.count != 0 || result.moves.count != 0) {
return NO;
}
// Make sure we note that index 1 is updated (id1 from @[@1] -> @[@2]), and "id2" was inserted at index 1
return result.updates.firstIndex == 0 && result.inserts.firstIndex == 1;
}]];
[[mockDelegate expect] listAdapterUpdater:self.updater didPerformBatchUpdates:OCMOCK_ANY collectionView:self.collectionView];
XCTestExpectation *expectation = genExpectation;
// Manually set the data source to be nil.
self->_collectionView.dataSource = nil;
[self.updater performUpdateWithCollectionViewBlock:[self collectionViewBlock] fromObjects:from toObjectsBlock:genToBlock animated:NO objectTransitionBlock:^(NSArray * _Nonnull toObjects) {
} completion:^(BOOL finished) {
[expectation fulfill];
}];
waitExpectation;
[mockDelegate verify];
}
# pragma mark - preferItemReloadsFroSectionReloads
- (void)test_whenReloadIsCalledWithSameItemCount_andPreferItemReload_updateIndexPathsHappen {