Repository navigation
fix(ios): make the unmountChildComponentView assert message bounds-safe - #57865
Closed
dongdongbh wants to merge 1 commit into
Closed
dongdongbh wants to merge 1 commit into
dongdongbh wants to merge 1 commit into
Conversation
|
Hi @dongdongbh! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
meta-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
the
Shared with Meta
|
@fabriziocucci has imported this pull request. If you are a Meta employee, you can view this in D115424016. |
|
@fabriziocucci merged this pull request in 1291d7c. |
xesrevinu
pushed a commit
to opraying/react-native-macos
that referenced
this pull request
Aug 30, 2026
…fe (react#57865) Summary: `-[RCTViewComponentView unmountChildComponentView:index:]` bounds-checks the `RCTAssert` *condition*, but the failure message it formats afterwards calls `-objectAtIndex:` on the same out-of-range `index`: ```objc RCTAssert( (self.currentContainerView.subviews.count > index) && // guarded [self.currentContainerView.subviews objectAtIndex:index] == childComponentView, @"... tag at index: %@)", ..., @([[self.currentContainerView.subviews objectAtIndex:index] tag])); // not guarded ``` So the exact situation the assert exists to report — a shadow-tree/native child-index mismatch — raises `NSRangeException` *while being reported*, instead of being reported. That is normally invisible, because a source Release build strips `RCTAssert` and the mismatch is harmless: `index` is used only inside the asserts, and the real work, `[childComponentView removeFromSuperview]`, never needed it. But the prebuilt `React.framework` published to Maven Central is built with assertions enabled (react#57454), so this is live in App Store builds — we hit it on RN 0.81.5 via Expo SDK 54, symbolicated against the published `reactnative-core-dSYM-release` artifact: ``` NSRangeException: *** -[__NSArrayM objectAtIndex:]: index 13 beyond bounds [0 .. 8] -[RCTViewComponentView unmountChildComponentView:index:] (RCTViewComponentView.mm:163) RCTPerformMountInstructions(...) (RCTMountingManager.mm:97) -[RCTMountingManager performTransaction:] (RCTMountingManager.mm:258) -[RCTMountingManager initiateTransaction:] (RCTMountingManager.mm:247) ``` This is the second of the two fixes suggested in react#57454 and stands on its own for any build with assertions enabled. The change also reads `self.currentContainerView` once instead of four times. That getter is not a plain accessor — it creates or tears down `_containerView` and reparents subviews — so re-invoking it inside an assert's arguments is worth avoiding regardless. The hoisted locals and the assert sit inside `#ifndef NS_BLOCK_ASSERTIONS`, matching the existing guard at `RCTViewComponentView.mm:335`. Without it the locals are unused once assertions are compiled out, which is a `-Werror,-Wunused-variable` build failure, and the hoist would otherwise make that side-effecting getter run on every unmount in a Release build where it previously did not run at all. With the guard it is read once per unmount in Debug and not at all in Release. ## Changelog: [IOS] [FIXED] - Report a `RCTViewComponentView` child-index mismatch instead of raising `NSRangeException` while formatting the assert message Pull Request resolved: react#57865 Test Plan: I don't have a macOS build environment, so I have not compiled this — flagging that plainly. What backs the change: - Five App Store crash reports (RN 0.81.5, iOS 26.5.2 / 26.6, three device models) all symbolicate to the message argument on this line, never to the guarded condition. - `strings` on `react-native-artifacts-0.81.5-reactnative-core-release.tar.gz` shows both `Attempt to unmount…` format strings present, confirming the call sites are compiled into the release artifact — same check react#57454 reports for 0.85.3 and 0.86.0, so the artifact defect reaches at least as far back as 0.81. - Behaviour is unchanged whenever the assert passes; when it fails, the message now prints `out of bounds` in place of a tag that cannot be read. Happy to rework this if you would rather the assert drop the `tag at index` field entirely, or if fixing the artifact build flags is considered sufficient on its own. ## Added on import: build, tests, and answers to the open questions Compiled and tested, which the author could not do. ``` buck2 test fbsource//xplat/js/react-native-github:MountingTestsApple → Pass 19. Fail 0. ``` Adds `React/Tests/Mounting/RCTViewComponentViewUnmountTests.mm`, picked up by the existing `MountingTestsApple` glob. Restoring the pre-fix `RCTViewComponentView.mm` and re-running fails exactly the out-of-bounds case: ``` ✗ RCTViewComponentViewUnmountTests/testUnmountWithOutOfBoundsIndexReportsRatherThanRaisingRangeException Tests finished: Pass 18. Fail 1. ``` The second case, `testUnmountWithInBoundsMismatchStillReportsTagAtIndex`, passes against both old and new code on purpose: it pins that the in-bounds mismatch path still reports the real tag rather than the new `out of bounds` placeholder. ### Why the test is shaped the way it is Two non-obvious constraints, both of which broke a more natural first attempt: 1. `RCT_NSASSERT` is defined as `RCT_DEBUG`, so in a debug build a failing `RCTAssert` calls the custom handler **and then** raises through `NSAssertionHandler`. Every failing assert throws, fixed or not, so `XCTAssertNoThrow` cannot be the assertion. The discriminator is that pre-fix the message arguments raise `NSRangeException` at the call site *before* `_RCTAssertFormat` runs, so the handler never fires at all. The test asserts the handler ran and that whatever escaped was not `NSRangeException`. 2. `RCTPerformBlockWithAssertFunction` calls `block()` between pushing and popping its handler with no `try`/`finally`, so an exception escaping the block leaks the handler into every later test in the process. The raise is therefore caught inside the block. ### Other checks - `index` is `NSInteger` (`RCTComponentViewProtocol.h:64`), so the added `index >= 0` is meaningful rather than tautological. The previous `count > index` relied on a negative index promoting to a large `NSUInteger` and failing the unsigned comparison, which was correct by accident. - The `NSNumber *` / `NSString *` ternary in the message compiles without warning. - `arc lint -e extra` on the new test reports only a `NULLSAFECLANG` infrastructure failure that self-identifies as "not a code issue". The 55 pre-existing CLANGTIDY warnings in `RCTViewComponentView.mm` are untouched and are not attributed to this diff. ### NOTE: on the author's two questions **Is fixing the artifact build flags sufficient on its own?** No. react#57454 is the right root-cause fix and should still happen, but this change is worth having independently: the assert is broken *as an assert*. In any build where assertions are compiled in, including local Debug, the assert that exists to report an index mismatch raises while reporting it, so the diagnostic is unavailable exactly when it is needed. Fixing the artifact flags would hide that from production without repairing it. **Should the `tag at index` field be dropped instead?** Recommend keeping it. It is the field that tells you which view actually occupies the slot, which is the useful part of the diagnostic, and the second test above locks in that it still appears when the index is in bounds. ### Release-build guard (added after the first sanity-check failure) The first import version failed the sanity check with: ``` error: unused variable 'isIndexInBounds' [-Werror,-Wunused-variable] ``` `NS_BLOCK_ASSERTIONS` makes `RCTAssert` expand to `do {} while(false)`, so with assertions compiled out the hoisted locals have no remaining use. The hoisted lines and the assert are now wrapped in `#ifndef NS_BLOCK_ASSERTIONS`, which is the idiom this same file already uses at `RCTViewComponentView.mm:335` for a local pulled out of an assert. That also restores a property the hoist had quietly removed. Before this diff the four `self.currentContainerView` reads sat inside the macro arguments and disappeared entirely in a Release build. Hoisting them out made the getter run on every unmount in Release, and that getter is not a plain accessor: it can allocate `_containerView`, reparent every subview into it and move `clipsToBounds` and `layer.mask`, or in the other branch tear the container down and nil it. With the guard the getter is read once per unmount in Debug and not at all in Release, which is better than both the original and the unguarded version. Verified locally by defining `NS_BLOCK_ASSERTIONS` at the top of the file to simulate a Release build, against `RCTFabricComponentViewsBaseApple`, which is the target that owns this file: | State | Result | |---|---| | Unguarded, assertions off | `error: unused variable 'isIndexInBounds'`, BUILD FAILED | | Guarded, assertions off | exit 0 | | Guarded, assertions on (debug) | Pass 19. Fail 0. | Reviewed By: christophpurrer Differential Revision: D115424016 Pulled By: fabriziocucci fbshipit-source-id: b825f145c1867a230c0c6cd12a41950a83468ed2 (cherry picked from commit 1291d7c)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
-[RCTViewComponentView unmountChildComponentView:index:]bounds-checks theRCTAssertcondition, but the failure message it formats afterwards calls-objectAtIndex:on the same out-of-rangeindex:So the exact situation the assert exists to report — a shadow-tree/native child-index mismatch — raises
NSRangeExceptionwhile being reported, instead of being reported.That is normally invisible, because a source Release build strips
RCTAssertand the mismatch is harmless:indexis used only inside the asserts, and the real work,[childComponentView removeFromSuperview], never needed it. But the prebuiltReact.frameworkpublished to Maven Central is built with assertions enabled (#57454), so this is live in App Store builds — we hit it on RN 0.81.5 via Expo SDK 54, symbolicated against the publishedreactnative-core-dSYM-releaseartifact:This is the second of the two fixes suggested in #57454 and stands on its own for any build with assertions enabled.
The change also reads
self.currentContainerViewonce instead of four times. That getter is not a plain accessor — it creates or tears down_containerViewand reparents subviews — so re-invoking it inside an assert's arguments is worth avoiding regardless.Changelog:
[IOS] [FIXED] - Report a
RCTViewComponentViewchild-index mismatch instead of raisingNSRangeExceptionwhile formatting the assert messageTest Plan
I don't have a macOS build environment, so I have not compiled this — flagging that plainly. What backs the change:
stringsonreact-native-artifacts-0.81.5-reactnative-core-release.tar.gzshows bothAttempt to unmount…format strings present, confirming the call sites are compiled into the release artifact — same check [iOS] Prebuilt React-Core Release artifacts are built with NSAssertions enabled — RCTAssert failures become fatal NSRangeException crashes in production #57454 reports for 0.85.3 and 0.86.0, so the artifact defect reaches at least as far back as 0.81.out of boundsin place of a tag that cannot be read.Happy to rework this if you would rather the assert drop the
tag at indexfield entirely, or if fixing the artifact build flags is considered sufficient on its own.