Skip to content

Commit fc3443f

Browse files
camrothmeta-codesync[bot]
authored andcommitted
Fix VoiceOver infinite recursion crash when delegate set to collectionView (Issue 1658)
Summary: #1658 This fix addresses a crash that occurs when VoiceOver is enabled and `scrollViewDelegate` or `collectionViewDelegate` is set to the adapter's own `UICollectionView`. ## The Problem When VoiceOver is enabled, accessibility queries trigger delegate method calls on the collection view. If the collection view is also set as its own scroll view delegate (via `IGListAdapter`'s proxy), these delegate calls get forwarded back to the collection view, creating infinite recursion: 1. VoiceOver queries accessibility on collectionView 2. `collectionView.delegate` (the proxy) forwards to `scrollViewTarget` 3. `scrollViewTarget` IS the collectionView → back to step 1 4. Stack overflow crash Example crash-triggering code: ``` adapter.collectionView = collectionView adapter.scrollViewDelegate = collectionView // Crash with VoiceOver! ``` ## The Fix Here I've added validation in three places to prevent this configuration: 1. `setScrollViewDelegate:` - Rejects if `delegate == collectionView` 2. `setCollectionViewDelegate:` - Rejects if `delegate == collectionView` 3. `setCollectionView:` - Clears delegates if they equal the new collectionView ## Why This Is Safe (No Negative Side Effects) **Setting UICollectionView as its own delegate provides no useful functionality:** - `UICollectionView` already handles scroll events internally (it's a `UIScrollView` subclass) - The `collectionView` doesn't implement custom `UIScrollViewDelegate` methods - This configuration ONLY results in crashes when VoiceOver is enabled **Behavior change is strictly beneficial:** - Before: Works without VoiceOver, crashes WITH VoiceOver - After: Silently rejects invalid config, works with VoiceOver **Developer feedback is preserved:** - `IGFailAssert` fires in `DEBUG` builds to alert developers to fix their code - Production gracefully handles the situation instead of crashing **No breaking changes for valid usage:** - Normal delegate patterns (separate delegate objects) are unaffected - Only the invalid self-referential pattern is blocked See: #1658 Reviewed By: dinhvh Differential Revision: D100641752 fbshipit-source-id: a1885c4ede6ca0555f3f94dd0c72b328a5af62bc
1 parent 01887c6 commit fc3443f

3 files changed

Lines changed: 88 additions & 0 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag
55
5.3.0 (Upcoming Release)
66
-----
77

8+
### Fixes
9+
10+
- An infinite recursion crash when VoiceOver is enabled and `scrollViewDelegate` or `collectionViewDelegate` is set to the adapter's own `UICollectionView`. [Cameron Roth](https://github.com/camroth) [(#1658)](https://github.com/Instagram/IGListKit/issues/1658)
11+
812
5.2.0
913
-----
1014

Source/IGListKit/IGListAdapter.m

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,20 @@ - (void)setCollectionView:(UICollectionView *)collectionView {
112112
_registeredSupplementaryViewIdentifiers = [NSMutableSet new];
113113
_registeredSupplementaryViewNibNames = [NSMutableSet new];
114114

115+
// Clear the scrollViewDelegate or collectionViewDelegate if they were pointing to the new collectionView,
116+
// as this would cause infinite recursion when VoiceOver is enabled.
117+
// See: https://github.com/Instagram/IGListKit/issues/1658
118+
if (_scrollViewDelegate == (id<UIScrollViewDelegate>)collectionView) {
119+
IGFailAssert(@"scrollViewDelegate was set to the UICollectionView which causes infinite recursion "
120+
@"when VoiceOver is enabled. Clearing the scrollViewDelegate.");
121+
_scrollViewDelegate = nil;
122+
}
123+
if (_collectionViewDelegate == (id<UICollectionViewDelegate>)collectionView) {
124+
IGFailAssert(@"collectionViewDelegate was set to the UICollectionView which causes infinite recursion "
125+
@"when VoiceOver is enabled. Clearing the collectionViewDelegate.");
126+
_collectionViewDelegate = nil;
127+
}
128+
115129
// We can't just swap out the collectionView, because we might have on-going or pending updates.
116130
// `_updater` can take care of that by wrapping the change in `performDataSourceChange`.
117131
[_updater performDataSourceChange:^{
@@ -169,6 +183,14 @@ - (void)setCollectionViewDelegate:(id<UICollectionViewDelegate>)collectionViewDe
169183
@"UICollectionViewDelegateFlowLayout conformance is automatically handled by IGListAdapter.");
170184

171185
if (_collectionViewDelegate != collectionViewDelegate) {
186+
// Setting the collectionViewDelegate to the collectionView itself will cause infinite recursion
187+
// when VoiceOver is enabled, as accessibility queries trigger delegate methods that get
188+
// forwarded back to the collectionView. See: https://github.com/Instagram/IGListKit/issues/1658
189+
if (collectionViewDelegate == (id<UICollectionViewDelegate>)_collectionView) {
190+
IGFailAssert(@"Setting collectionViewDelegate to the adapter's UICollectionView is not allowed. "
191+
@"This causes infinite recursion when VoiceOver is enabled.");
192+
return;
193+
}
172194
_collectionViewDelegate = collectionViewDelegate;
173195
[self _createProxyAndUpdateCollectionViewDelegate];
174196
}
@@ -178,6 +200,14 @@ - (void)setScrollViewDelegate:(id<UIScrollViewDelegate>)scrollViewDelegate {
178200
IGAssertMainThread();
179201

180202
if (_scrollViewDelegate != scrollViewDelegate) {
203+
// Setting the scrollViewDelegate to the collectionView itself will cause infinite recursion
204+
// when VoiceOver is enabled, as accessibility queries trigger delegate methods that get
205+
// forwarded back to the collectionView. See: https://github.com/Instagram/IGListKit/issues/1658
206+
if (scrollViewDelegate == (id<UIScrollViewDelegate>)_collectionView) {
207+
IGFailAssert(@"Setting scrollViewDelegate to the adapter's UICollectionView is not allowed. "
208+
@"This causes infinite recursion when VoiceOver is enabled.");
209+
return;
210+
}
181211
_scrollViewDelegate = scrollViewDelegate;
182212
[self _createProxyAndUpdateCollectionViewDelegate];
183213
}

Tests/IGListAdapterTests.m

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2382,4 +2382,58 @@ - (void)test_whenSettingSupplementaryView_thatViewForSupplementaryElementExists
23822382
XCTAssertNil([self.adapter viewForSupplementaryElementOfKind:UICollectionElementKindSectionHeader atIndex:1 sectionController:controller]);
23832383
}
23842384

2385+
#pragma mark - Recursive delegation prevention (Issue 1658)
2386+
2387+
- (void)test_whenSettingScrollViewDelegateToCollectionView_thatScrollViewDelegateIsNotSet {
2388+
// Issue 1658: Setting scrollViewDelegate to collectionView causes infinite recursion when VoiceOver is enabled
2389+
// The adapter should reject this and not update the scrollViewDelegate
2390+
self.adapter.scrollViewDelegate = (id<UIScrollViewDelegate>)self.collectionView;
2391+
XCTAssertNil(self.adapter.scrollViewDelegate);
2392+
}
2393+
2394+
- (void)test_whenSettingCollectionViewDelegateToCollectionView_thatCollectionViewDelegateIsNotSet {
2395+
// Issue 1658: Setting collectionViewDelegate to collectionView causes infinite recursion when VoiceOver is enabled
2396+
// The adapter should reject this and not update the collectionViewDelegate
2397+
self.adapter.collectionViewDelegate = (id<UICollectionViewDelegate>)self.collectionView;
2398+
XCTAssertNil(self.adapter.collectionViewDelegate);
2399+
}
2400+
2401+
- (void)test_whenSettingCollectionViewAfterScrollViewDelegateWasSet_thatScrollViewDelegateIsCleared {
2402+
// If scrollViewDelegate is set to a collectionView before that collectionView is assigned to the adapter,
2403+
// the adapter should detect this and clear the scrollViewDelegate to prevent recursion
2404+
UICollectionView *newCollectionView = [[UICollectionView alloc] initWithFrame:CGRectZero
2405+
collectionViewLayout:[UICollectionViewFlowLayout new]];
2406+
2407+
// First, create a new adapter with no collection view
2408+
IGListAdapter *newAdapter = [[IGListAdapter alloc] initWithUpdater:[IGListReloadDataUpdater new] viewController:nil];
2409+
2410+
// Set the scrollViewDelegate to the collection view we're about to set
2411+
// Note: This is done via the ivar since the setter would reject it if collectionView was already set
2412+
[newAdapter setValue:newCollectionView forKey:@"_scrollViewDelegate"];
2413+
2414+
// Now set the collection view - this should detect the issue and clear the scrollViewDelegate
2415+
newAdapter.collectionView = newCollectionView;
2416+
2417+
XCTAssertNil(newAdapter.scrollViewDelegate);
2418+
}
2419+
2420+
- (void)test_whenSettingCollectionViewAfterCollectionViewDelegateWasSet_thatCollectionViewDelegateIsCleared {
2421+
// If collectionViewDelegate is set to a collectionView before that collectionView is assigned to the adapter,
2422+
// the adapter should detect this and clear the collectionViewDelegate to prevent recursion
2423+
UICollectionView *newCollectionView = [[UICollectionView alloc] initWithFrame:CGRectZero
2424+
collectionViewLayout:[UICollectionViewFlowLayout new]];
2425+
2426+
// First, create a new adapter with no collection view
2427+
IGListAdapter *newAdapter = [[IGListAdapter alloc] initWithUpdater:[IGListReloadDataUpdater new] viewController:nil];
2428+
2429+
// Set the collectionViewDelegate to the collection view we're about to set
2430+
// Note: This is done via the ivar since the setter would reject it if collectionView was already set
2431+
[newAdapter setValue:newCollectionView forKey:@"_collectionViewDelegate"];
2432+
2433+
// Now set the collection view - this should detect the issue and clear the collectionViewDelegate
2434+
newAdapter.collectionView = newCollectionView;
2435+
2436+
XCTAssertNil(newAdapter.collectionViewDelegate);
2437+
}
2438+
23852439
@end

0 commit comments

Comments
 (0)