Repository navigation
fix(iOS, Stack v4): iterate over a copy of reactSubviews when updating header shadow state - #4851
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
kmichalikk
left a comment
There was a problem hiding this comment.
Hi @ilp0
Looks good, thanks for the PR. Please rebase the PR and simplify the test a bit (at least remove the search controller, it threw me off for a moment).
…g header shadow state
Use ScreenStackItem with the header subviews in an explicit order instead of native-stack. The title after the right item is what makes the mutation visible to fast enumeration, so the search bar is no longer needed.
00194b1 to
cbc4b52
Compare
|
Hi @kmichalikk, thanks for the review. I rebased onto main and simplified the test The test now uses ScreenStackItem directly with the title after the right item, and there’s no search bar anymore. I used the search bar as an example, because this is how I caught the bug in the first place :) On main in FabricExample it crashed right after pressing “Start” in 5 out of 5 runs without the fix, and didn’t crash with it. |
kmichalikk
left a comment
There was a problem hiding this comment.
Verified on physical device with iOS 26.6.1, too
didn't trigger on 27 for me
still, worth fixing for 26
Description
-[RNSScreenStackHeaderConfig updateHeaderStateInShadowTreeInContextOfNavigationBar:]loops overself.reactSubviewsand updates the shadow state of each header subview. Since #3933 (4.19.0) that update is synchronous, so it can commit and mount right away, and the mount includes any React commit that's still pending. If that commit adds or removes a header subview, the array changes while we're still looping over it, and iOS throwsNSGenericException: Collection was mutated while being enumerated.Fast enumeration only detects the change when it moves on to the next element, so it only crashes when another header subview comes after the one being updated. In practice the item that commits is a left or right item that was just mounted. With native-stack, the only subview mounted after the right items is the search bar, so there it takes custom header items that change often plus
headerSearchBarOptions. WithScreenStackItemthe order of the subviews is up to the app, and any subview after the item is enough.Closes #4811.
Changes
reactSubviewsinupdateHeaderStateInShadowTreeInContextOfNavigationBar:.Test4811, which reproduces the crash.I only changed this one loop on purpose. The other
reactSubviewsloops in the file (hitTest:withEvent:,hasSubviewOfType:andloadBackButtonImageInViewController:) only read values and don't call anything that can commit, so they can't hit this. Happy to change them too if you'd rather be defensive everywhere.Copying the array is cheap, since a header only has a handful of subviews.
Test plan
Test4811: press "Start". The screen commits as fast as possible. The left and right header items are mounted and unmounted at different rates, and the title is placed after the right item.Tested in FabricExample on
main(iOS 26.5 simulator):I also checked the native-stack case with a search bar on
main: without the fix it crashed in 5 out of 5 runs within 6 seconds, with the fix there were no crashes in 3 runs of 60 seconds.Checklist