Skip to content

Commit 1291d7c

Browse files
dongdongbhmeta-codesync[bot]
authored andcommitted
fix(ios): make the unmountChildComponentView assert message bounds-safe (#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 (#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 #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: #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 #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. #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
1 parent e0aa7a8 commit 1291d7c

2 files changed

Lines changed: 93 additions & 4 deletions

File tree

packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -255,15 +255,18 @@ - (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompo
255255
childComponentView,
256256
@(index),
257257
@([childComponentView.superview tag]));
258+
#ifndef NS_BLOCK_ASSERTIONS
259+
NSArray<UIView *> *containerSubviews = self.currentContainerView.subviews;
260+
BOOL isIndexInBounds = index >= 0 && (NSUInteger)index < containerSubviews.count;
258261
RCTAssert(
259-
(self.currentContainerView.subviews.count > index) &&
260-
[self.currentContainerView.subviews objectAtIndex:index] == childComponentView,
262+
isIndexInBounds && [containerSubviews objectAtIndex:index] == childComponentView,
261263
@"Attempt to unmount a view which has a different index. (parent: %@, child: %@, index: %@, actual index: %@, tag at index: %@)",
262264
self,
263265
childComponentView,
264266
@(index),
265-
@([self.currentContainerView.subviews indexOfObject:childComponentView]),
266-
@([[self.currentContainerView.subviews objectAtIndex:index] tag]));
267+
@([containerSubviews indexOfObject:childComponentView]),
268+
isIndexInBounds ? @([[containerSubviews objectAtIndex:index] tag]) : @"out of bounds");
269+
#endif
267270
}
268271

269272
[childComponentView removeFromSuperview];
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
#import <React/RCTAssert.h>
9+
#import <React/RCTViewComponentView.h>
10+
#import <XCTest/XCTest.h>
11+
12+
@interface RCTViewComponentViewUnmountTests : XCTestCase
13+
@end
14+
15+
@implementation RCTViewComponentViewUnmountTests
16+
17+
// RCTPerformBlockWithAssertFunction pops its handler after calling the block,
18+
// without @finally, so an exception escaping the block would leave the handler
19+
// installed for every later test in this process. RCT_NSASSERT is on in debug,
20+
// which means a failing assert always raises once it has been reported, so the
21+
// raise has to be caught inside the block rather than around the call.
22+
static NSString *RCTUnmountAndReportAssert(
23+
RCTViewComponentView *parent,
24+
UIView<RCTComponentViewProtocol> *child,
25+
NSInteger index,
26+
NSString **outMessage)
27+
{
28+
__block NSString *thrownName = nil;
29+
__block NSString *message = nil;
30+
31+
RCTPerformBlockWithAssertFunction(
32+
^{
33+
@try {
34+
[parent unmountChildComponentView:child index:index];
35+
} @catch (NSException *exception) {
36+
thrownName = exception.name;
37+
}
38+
},
39+
^(NSString *condition, NSString *fileName, NSNumber *lineNumber, NSString *function, NSString *assertMessage) {
40+
message = assertMessage;
41+
});
42+
43+
*outMessage = message;
44+
return thrownName;
45+
}
46+
47+
// The index is read only inside RCTAssert here, so a mismatch is harmless once
48+
// assertions are compiled out. While they are compiled in, the arguments that
49+
// build the failure message are evaluated at the call site, and an out-of-range
50+
// index used to reach objectAtIndex: there. The assert that exists to report the
51+
// mismatch raised NSRangeException instead of reporting it.
52+
- (void)testUnmountWithOutOfBoundsIndexReportsRatherThanRaisingRangeException
53+
{
54+
RCTViewComponentView *parent = [[RCTViewComponentView alloc] initWithFrame:CGRectZero];
55+
RCTViewComponentView *child = [[RCTViewComponentView alloc] initWithFrame:CGRectZero];
56+
[parent mountChildComponentView:child index:0];
57+
58+
NSString *message = nil;
59+
// Only one child is mounted, so index 1 is past the end.
60+
NSString *thrownName = RCTUnmountAndReportAssert(parent, child, 1, &message);
61+
62+
XCTAssertNotNil(message, @"the assert must report the mismatch");
63+
XCTAssertTrue([message containsString:@"different index"]);
64+
XCTAssertNotEqualObjects(thrownName, NSRangeException, @"reporting must not raise out of bounds");
65+
}
66+
67+
// The in-bounds mismatch path must keep reporting the tag at that index rather
68+
// than the out-of-range placeholder.
69+
- (void)testUnmountWithInBoundsMismatchStillReportsTagAtIndex
70+
{
71+
RCTViewComponentView *parent = [[RCTViewComponentView alloc] initWithFrame:CGRectZero];
72+
RCTViewComponentView *first = [[RCTViewComponentView alloc] initWithFrame:CGRectZero];
73+
RCTViewComponentView *second = [[RCTViewComponentView alloc] initWithFrame:CGRectZero];
74+
[parent mountChildComponentView:first index:0];
75+
[parent mountChildComponentView:second index:1];
76+
77+
NSString *message = nil;
78+
// `second` is really at index 1, so index 0 is in bounds but wrong.
79+
NSString *thrownName = RCTUnmountAndReportAssert(parent, second, 0, &message);
80+
81+
XCTAssertNotNil(message);
82+
XCTAssertFalse([message containsString:@"out of bounds"]);
83+
XCTAssertNotEqualObjects(thrownName, NSRangeException);
84+
}
85+
86+
@end

0 commit comments

Comments
 (0)