Skip to content

Commit

Permalink
Fixes crash when accessing a cell within working range updates
Browse files Browse the repository at this point in the history
Summary:
I was able to build a unit test that reproduces the issue. We can avoid the crash by simply returning `nil` when accessing a cell while working range events are being vended.

There is definitely something weird going on here though. When debugging `cellForItemAtIndexPath:` I found:

```
(lldb) po indexPath
<NSIndexPath: 0xc000000000000516> {length = 2, path = 5 - 0}
(lldb) po [[self collectionView] numberOfSections]
11
```

So in theory we should be fine, right? But when I continue I get

```
*** Assertion failure in -[UICollectionViewData numberOfItemsBeforeSection:], /BuildRoot/Library/Caches/com.apple.xbs/Sources/UIKit_Sim/UIKit-3600.5.2/UICollectionViewData.m:611
*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'request for number of items before section 5 when there are only 3 sections in the collection view'
```

There _were_ 3 sections in the UICV before the update, but the data source and structure powering
Closes #216

Differential Revision: D4204625

Pulled By: rnystrom

fbshipit-source-id: 455ed199dfc115077e4294e2843016a50e179015
  • Loading branch information
Ryan Nystrom authored and Facebook Github Bot committed Nov 18, 2016
1 parent cb8e0ab commit 74affe0
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 6 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ This release closes the [2.0.0 milestone](https://github.com/Instagram/IGListKit
- Added new `-[IGListAdapter visibleObjects]` API. [Ryan Nystrom](https://github.com/rnystrom) [(386ae07)](https://github.com/Instagram/IGListKit/commit/386ae0786445c06e1eabf074a4181614332f155f)
- Added new `-[IGListAdapter objectForSectionController:]` API. [Ayush Saraswat](https://github.com/saraswatayu) [(#204)](https://github.com/Instagram/IGListKit/pull/204)

### Fixes

- Prevent `UICollectionView` bug when accessing a cell during working range updates. [Ryan Nystrom](https://github.com/rnystrom) [(#216)](https://github.com/Instagram/IGListKit/pull/216)

1.0.0
-----

Expand Down
8 changes: 6 additions & 2 deletions Source/IGListAdapter.m
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
@implementation IGListAdapter {
NSMapTable<UICollectionViewCell *, IGListSectionController<IGListSectionType> *> *_cellSectionControllerMap;
BOOL _isDequeuingCell;
BOOL _isSendingWorkingRangeDisplayUpdates;
}

- (void)dealloc {
Expand Down Expand Up @@ -613,7 +614,10 @@ - (void)collectionView:(UICollectionView *)collectionView willDisplayCell:(UICol

id object = [self.sectionMap objectForSection:indexPath.section];
[self.displayHandler willDisplayCell:cell forListAdapter:self sectionController:sectionController object:object indexPath:indexPath];

_isSendingWorkingRangeDisplayUpdates = YES;
[self.workingRangeHandler willDisplayItemAtIndexPath:indexPath forListAdapter:self];
_isSendingWorkingRangeDisplayUpdates = NO;
}

- (void)collectionView:(UICollectionView *)collectionView didEndDisplayingCell:(UICollectionViewCell *)cell forItemAtIndexPath:(NSIndexPath *)indexPath {
Expand Down Expand Up @@ -691,8 +695,8 @@ - (__kindof UICollectionViewCell *)cellForItemAtIndex:(NSInteger)index
IGAssertMainThread();
IGParameterAssert(sectionController != nil);

// if this is accessed while a cell is being dequeued, just return nil
if (_isDequeuingCell) {
// if this is accessed while a cell is being dequeued or displaying working range elements, just return nil
if (_isDequeuingCell || _isSendingWorkingRangeDisplayUpdates) {
return nil;
}

Expand Down
33 changes: 33 additions & 0 deletions Tests/IGListAdapterE2ETests.m
Original file line number Diff line number Diff line change
Expand Up @@ -1022,4 +1022,37 @@ - (void)test_whenBatchUpdating_withDuplicateIdentifiers_thatHaveDifferentValues_
[self waitForExpectationsWithTimeout:15 handler:nil];
}

- (void)test_whenPerformingUpdates_withWorkingRange_thatAccessingCellDoesntCrash {
[self setupWithObjects:@[
genTestObject(@1, @1),
genTestObject(@2, @1),
genTestObject(@3, @1),
]];

// section controller try to access a cell in -listAdapter:sectionControllerWillEnterWorkingRange:
// add items beyond the 100x100 frame so they access unavailable cells
self.dataSource.objects = @[
genTestObject(@1, @1),
genTestObject(@2, @1),
genTestObject(@3, @1),
genTestObject(@4, @1),
genTestObject(@5, @1),
genTestObject(@6, @1),
genTestObject(@7, @1),
genTestObject(@8, @1),
genTestObject(@9, @1),
genTestObject(@10, @1),
genTestObject(@11, @1),
];
XCTestExpectation *expectation = genExpectation;

// this will call -collectionView:performBatchUpdates:, trigger collectionView:willDisplayCell:forItemAtIndexPath:,
// which kicks off the working range logic
[self.adapter performUpdatesAnimated:YES completion:^(BOOL finished) {
[expectation fulfill];
}];

[self waitForExpectationsWithTimeout:15 handler:nil];
}

@end
6 changes: 3 additions & 3 deletions Tests/IGListWorkingRangeHandlerTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

Expand Down Expand Up @@ -48,7 +48,7 @@ - (UIView *)emptyViewForListAdapter:(IGListAdapter *)listAdapter {
}

- (IGListSectionController<IGListSectionType> *)listAdapter:(IGListAdapter *)listAdapter
sectionControllerForObject:(id)object {
sectionControllerForObject:(id)object {
return [_map objectForKey:object];
}

Expand Down Expand Up @@ -211,7 +211,7 @@ - (void)test_whenDisplayingItemAtPath_withWorkingRangeSizeOne_thenEndDisplayingT
// Act: Hide the first item, and watch for the second item to leave the working range.
[[mockWorkingRangeDelegate expect] listAdapter:adapter sectionControllerDidExitWorkingRange:controller2];
[adapter.workingRangeHandler didEndDisplayingItemAtIndexPath:[NSIndexPath indexPathForRow:0 inSection:0] forListAdapter:adapter];

[mockWorkingRangeDelegate verifyWithDelay:5];
}

Expand Down
2 changes: 1 addition & 1 deletion Tests/Objects/IGTestDelegateController.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

@class IGTestObject;

@interface IGTestDelegateController : IGListSectionController <IGListSectionType, IGListDisplayDelegate>
@interface IGTestDelegateController : IGListSectionController <IGListSectionType, IGListDisplayDelegate, IGListWorkingRangeDelegate>

@property (nonatomic, strong, readonly) IGTestObject *item;

Expand Down
9 changes: 9 additions & 0 deletions Tests/Objects/IGTestDelegateController.m
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ - (instancetype)init {
if (self = [super init]) {
_willDisplayCellIndexes = [NSCountedSet new];
_didEndDisplayCellIndexes = [NSCountedSet new];
self.workingRangeDelegate = self;
}
return self;
}
Expand Down Expand Up @@ -81,4 +82,12 @@ - (void)listAdapter:(IGListAdapter *)listAdapter didEndDisplayingSectionControll

- (void)listAdapter:(IGListAdapter *)listAdapter didScrollSectionController:(IGListSectionController <IGListSectionType> *)sectionController {}

#pragma mark - IGListWorkingRangeDelegate

- (void)listAdapter:(IGListAdapter *)listAdapter sectionControllerWillEnterWorkingRange:(IGListSectionController<IGListSectionType> *)sectionController {
__unused UICollectionViewCell *cell = [self.collectionContext cellForItemAtIndex:0 sectionController:self];
}

- (void)listAdapter:(IGListAdapter *)listAdapter sectionControllerDidExitWorkingRange:(IGListSectionController<IGListSectionType> *)sectionController {}

@end

0 comments on commit 74affe0

Please sign in to comment.