From f639cdd2871aa9273b1d97f1ce02dfd645d043dd Mon Sep 17 00:00:00 2001 From: Ryan Nystrom Date: Tue, 28 Feb 2017 07:15:35 -0800 Subject: [PATCH] Prevent stale adapter:collectionView corruptions Summary: This becomes an issue pretty easily in embedded `IGListAdapter` environments (lists in lists). IGListKit creates a single `IGListSectionController` instance per object, but cells are reused within `IGListCollectionView`. If you assign `adapter.collectionView = cell.collectionView` (like we recommend in our [example](https://github.com/Instagram/IGListKit/blob/master/Examples/Examples-iOS/IGListKitExamples/SectionControllers/HorizontalSectionController.swift#L40)), then you can have many adapters with a weak ref to the **same collection view**. Obviously when updates happen across all embedded lists, adapters could potentially update the same collection view and apply a corrupted batch update. I proposed using `` and using associated objects, but since `OBJC_ASSOCIATION_ASSIGN` is not zerod-on-release reference (its just `unsafe_unretained`) I'd have to make a weak wrapped object, which is overkill. Instead I think a global weak:weak map is fine. We d Closes https://github.com/Instagram/IGListKit/pull/517 Differential Revision: D4623300 Pulled By: rnystrom fbshipit-source-id: 53d2dd158923c431e793b0c8e28997e9bbf55b8b --- CHANGELOG.md | 2 + Source/IGListAdapter.m | 10 ++++- Tests/IGListAdapterE2ETests.m | 71 +++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6ec2f34..2cc961ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,8 @@ This release closes the [3.0.0 milestone](https://github.com/Instagram/IGListKit - Fix bug where scroll position would be incorrect in call to `-[IGListAdapter scrollToObject:supplementaryKinds:scrollDirection:scrollPosition:animated:` with scrollDirection/scrollPosition of UICollectionViewScrollDirectionVertical/UICollectionViewScrollPositionCenteredVertically or UICollectionViewScrollDirectionHorizontal/UICollectionViewScrollPositionCenteredHorizontally and with a collection view with nonzero contentInset. [David Yamnitsky](https://github.com/nitsky) [(5cc0fcd)](https://github.com/Instagram/IGListKit/commit/5cc0fcd1d77d6296f57ce1c298301b9881cb4d4a) +- Fix a crash when reusing collection views between embedded `IGListAdapter`s. [Ryan Nystrom](https://github.com/rnystrom) [(#517)](https://github.com/Instagram/IGListKit/pull/517) + 2.1.0 ----- diff --git a/Source/IGListAdapter.m b/Source/IGListAdapter.m index 49509416..0410ce79 100644 --- a/Source/IGListAdapter.m +++ b/Source/IGListAdapter.m @@ -64,11 +64,19 @@ - (void)setCollectionView:(IGListCollectionView *)collectionView { IGAssertMainThread(); - IGParameterAssert([collectionView isKindOfClass:[IGListCollectionView class]]); // if collection view has been used by a different list adapter, treat it as if we were using a new collection view // this happens when embedding a IGListCollectionView inside a UICollectionViewCell that is reused if (_collectionView != collectionView || _collectionView.dataSource != self) { + // if the collection view was being used with another IGListAdapter (e.g. cell reuse) + // destroy the previous association so the old adapter doesn't update the wrong collection view + static NSMapTable *globalCollectionViewAdapterMap = nil; + if (globalCollectionViewAdapterMap == nil) { + globalCollectionViewAdapterMap = [NSMapTable weakToWeakObjectsMapTable]; + } + [[globalCollectionViewAdapterMap objectForKey:collectionView] setCollectionView:nil]; + [globalCollectionViewAdapterMap setObject:self forKey:collectionView]; + // dump old registered section controllers in the case that we are changing collection views or setting for // the first time _registeredCellClasses = [NSMutableSet new]; diff --git a/Tests/IGListAdapterE2ETests.m b/Tests/IGListAdapterE2ETests.m index d1dc5160..d9e3c545 100644 --- a/Tests/IGListAdapterE2ETests.m +++ b/Tests/IGListAdapterE2ETests.m @@ -1365,4 +1365,75 @@ [self waitForExpectationsWithTimeout:15 handler:nil]; } +- (void)test_whenAdaptersSwapCollectionViews_thatOldAdapterDoesntUpdateOldCollectionView { + IGListAdapter *adapter1 = [[IGListAdapter alloc] initWithUpdater:[IGListAdapterUpdater new] viewController:nil workingRangeSize:0]; + IGTestDelegateDataSource *dataSource1 = [IGTestDelegateDataSource new]; + dataSource1.objects = @[genTestObject(@1, @2)]; + adapter1.dataSource = dataSource1; + adapter1.collectionView = self.collectionView; + + [self.collectionView layoutIfNeeded]; + XCTAssertEqual([self.collectionView numberOfSections], 1); + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 2); + + IGListAdapter *adapter2 = [[IGListAdapter alloc] initWithUpdater:[IGListAdapterUpdater new] viewController:nil workingRangeSize:0]; + IGTestDelegateDataSource *dataSource2 = [IGTestDelegateDataSource new]; + dataSource2.objects = @[genTestObject(@1, @1), genTestObject(@2, @1)]; + adapter2.dataSource = dataSource2; + adapter2.collectionView = self.collectionView; + + XCTAssertEqual([self.collectionView numberOfSections], 2); + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 1); + XCTAssertEqual([self.collectionView numberOfItemsInSection:1], 1); + + dataSource1.objects = @[genTestObject(@1, @2), genTestObject(@2, @2), genTestObject(@3, @2), genTestObject(@4, @2)]; + XCTestExpectation *expectation = genExpectation; + + [adapter1 performUpdatesAnimated:YES completion:^(BOOL finished) { + XCTAssertEqual([self.collectionView numberOfSections], 2); + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 1); + XCTAssertEqual([self.collectionView numberOfItemsInSection:1], 1); + [expectation fulfill]; + }]; + + [self waitForExpectationsWithTimeout:15 handler:nil]; +} + +- (void)test_whenAdaptersSwapCollectionViews_ { + IGListAdapter *adapter1 = [[IGListAdapter alloc] initWithUpdater:[IGListAdapterUpdater new] viewController:nil workingRangeSize:0]; + IGTestDelegateDataSource *dataSource1 = [IGTestDelegateDataSource new]; + dataSource1.objects = @[genTestObject(@1, @2)]; + adapter1.dataSource = dataSource1; + adapter1.collectionView = self.collectionView; + + [self.collectionView layoutIfNeeded]; + XCTAssertEqual([self.collectionView numberOfSections], 1); + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 2); + + IGListAdapter *adapter2 = [[IGListAdapter alloc] initWithUpdater:[IGListAdapterUpdater new] viewController:nil workingRangeSize:0]; + IGTestDelegateDataSource *dataSource2 = [IGTestDelegateDataSource new]; + dataSource2.objects = @[genTestObject(@1, @1), genTestObject(@2, @1)]; + adapter2.dataSource = dataSource2; + adapter2.collectionView = self.collectionView; + + [self.collectionView layoutIfNeeded]; + XCTAssertEqual([self.collectionView numberOfSections], 2); + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 1); + XCTAssertEqual([self.collectionView numberOfItemsInSection:1], 1); + + dataSource2.objects = @[genTestObject(@1, @2), genTestObject(@2, @1), genTestObject(@3, @1), genTestObject(@4, @1)]; + XCTestExpectation *expectation = genExpectation; + + [adapter2 performUpdatesAnimated:YES completion:^(BOOL finished) { + XCTAssertEqual([self.collectionView numberOfSections], 4); + XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 2); + XCTAssertEqual([self.collectionView numberOfItemsInSection:1], 1); + XCTAssertEqual([self.collectionView numberOfItemsInSection:1], 1); + XCTAssertEqual([self.collectionView numberOfItemsInSection:1], 1); + [expectation fulfill]; + }]; + + [self waitForExpectationsWithTimeout:15 handler:nil]; +} + @end