Skip to content

Commit 923e7dd

Browse files
OrfeasZmeta-codesync[bot]
OrfeasZ
authored and
meta-codesync[bot]
committed
Add guards around nativeProps usage to prevent race conditions (#52646)
Summary: Some third-party libraries, like react-native-reanimated, can clone nodes in a different thread while react-native is calling `setNativeProps_DEPRECATED`. This results in a race condition, where a stale pointer to `nativeProps_DEPRECATED` can be accessed, resulting in a crash. This usually manifests as a `EXC_BAD_ACCESS` crash on iOS. On Android it seems more rare. We've added a lock around accesses to nativeProps_DEPRECATED, but alternative options of fixing this can be considered too. For more information see software-mansion/react-native-reanimated#7666 ## Changelog: [INTERNAL] [FIXED] - Fixed crashes caused by race conditions when third-party libraries clone the shadow dom from a different thread Pull Request resolved: #52646 Test Plan: Due to this being a race condition that only manifests in rare circumstances, it's very difficult to create a reliable reproduction case. The issue mentioned above contains ThreadSanitizer logs that demonstrate this issue. TSan no longer complains with this patch applied, and we've not seen any additional issues from it after deploying it in production over the past week. Added unit test covering the `nativeProps_DEPRECATED` merge logic in `UIManager::cloneNode` and `ShadowNode::clone`: ``` buck2 test //xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/uimanager:tests -- --regex FabricUIManagerTest ``` Reviewed By: zeyap Differential Revision: D110169424 Pulled By: javache fbshipit-source-id: 6139253dcc2c33348a0c1a3bd01e695d15aa83bc
1 parent d1c5ee0 commit 923e7dd

13 files changed

Lines changed: 240 additions & 75 deletions

‎packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -137,27 +137,35 @@ std::shared_ptr<ShadowNode> ShadowNode::clone(
137137
const ShadowNodeFragment& fragment) const {
138138
const auto& family = *family_;
139139
const auto& componentDescriptor = family.componentDescriptor_;
140-
if (family.nativeProps_DEPRECATED != nullptr) {
141-
auto propsParserContext = PropsParserContext{family_->getSurfaceId(), {}};
142-
if (fragment.props == ShadowNodeFragment::propsPlaceholder()) {
143-
// Clone existing `props_` with `family.nativeProps_DEPRECATED` to apply
144-
// previously set props via `setNativeProps` API.
145-
auto props = componentDescriptor.cloneProps(
146-
propsParserContext, props_, RawProps(*family.nativeProps_DEPRECATED));
147-
auto clonedNode = componentDescriptor.cloneShadowNode(
148-
*this,
149-
{.props = props,
150-
.children = fragment.children,
151-
.state = fragment.state});
152-
return clonedNode;
153-
} else {
154-
// TODO: We might need to merge fragment.props with
155-
// `family.nativeProps_DEPRECATED`.
156-
return componentDescriptor.cloneShadowNode(*this, fragment);
140+
141+
std::optional<RawProps> propsOverride;
142+
{
143+
std::lock_guard<std::mutex> lock(family.nativePropsMutex);
144+
if (family.nativeProps_DEPRECATED != nullptr) {
145+
if (fragment.props == ShadowNodeFragment::propsPlaceholder()) {
146+
propsOverride.emplace(*family.nativeProps_DEPRECATED);
147+
} else {
148+
// TODO: We might need to merge fragment.props with
149+
// `family.nativeProps_DEPRECATED`.
150+
}
157151
}
158-
} else {
159-
return componentDescriptor.cloneShadowNode(*this, fragment);
160152
}
153+
154+
if (propsOverride) {
155+
// Clone existing `props_` with `family.nativeProps_DEPRECATED` to
156+
// apply previously set props via `setNativeProps` API. The parsed
157+
// result escapes the lock so the rest of the clone work runs
158+
// unblocked.
159+
auto propsParserContext = PropsParserContext{family_->getSurfaceId(), {}};
160+
return componentDescriptor.cloneShadowNode(
161+
*this,
162+
{.props = componentDescriptor.cloneProps(
163+
propsParserContext, props_, std::move(*propsOverride)),
164+
.children = fragment.children,
165+
.state = fragment.state});
166+
}
167+
168+
return componentDescriptor.cloneShadowNode(*this, fragment);
161169
}
162170

163171
std::shared_ptr<const ContextContainer> ShadowNode::getContextContainer()

‎packages/react-native/ReactCommon/react/renderer/core/ShadowNodeFamily.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#pragma once
99

1010
#include <memory>
11+
#include <mutex>
1112
#include <shared_mutex>
1213

1314
#include <react/renderer/core/EventEmitter.h>
@@ -114,6 +115,7 @@ class ShadowNodeFamily final : public jsi::NativeState {
114115
* architecture and will be removed in the future.
115116
*/
116117
mutable std::unique_ptr<folly::dynamic> nativeProps_DEPRECATED;
118+
mutable std::mutex nativePropsMutex;
117119

118120
/**
119121
* @return tag for the ShadowNodeFamily.

‎packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.cpp‎

Lines changed: 58 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -118,49 +118,51 @@ std::shared_ptr<ShadowNode> UIManager::cloneNode(
118118

119119
auto& componentDescriptor = shadowNode.getComponentDescriptor();
120120
auto& family = shadowNode.getFamily();
121-
auto props = ShadowNodeFragment::propsPlaceholder();
122121

122+
auto props = ShadowNodeFragment::propsPlaceholder();
123123
if (!rawProps.isEmpty()) {
124-
if (family.nativeProps_DEPRECATED != nullptr) {
125-
// 1. update the nativeProps_DEPRECATED props.
126-
//
127-
// In this step, we want the most recent value for the props
128-
// managed by setNativeProps.
129-
// Values in `rawProps` patch (take precedence over)
130-
// `nativeProps_DEPRECATED`. For example, if both
131-
// `nativeProps_DEPRECATED` and `rawProps` contain key 'A'.
132-
// Value from `rawProps` overrides what was previously in
133-
// `nativeProps_DEPRECATED`. Notice that the `nativeProps_DEPRECATED`
134-
// patch will not get more props from `rawProps`: if the key is not
135-
// present in `nativeProps_DEPRECATED`, it will not be added.
136-
//
137-
// The result of this operation is the new `nativeProps_DEPRECATED`.
138-
family.nativeProps_DEPRECATED =
139-
std::make_unique<folly::dynamic>(mergeDynamicProps(
140-
*family.nativeProps_DEPRECATED, // source
141-
(folly::dynamic)rawProps, // patch
142-
NullValueStrategy::Ignore));
143-
144-
// 2. Compute the final set of props.
145-
//
146-
// This step takes the new props handled by `setNativeProps` and
147-
// merges them in the `rawProps` managed by React.
148-
// The new props handled by `nativeProps` now takes precedence
149-
// on the props handled by React, as we want to make sure that
150-
// all the props are applied to the component.
151-
// We use these finalProps as source of truth for the component.
152-
auto finalProps = mergeDynamicProps(
153-
(folly::dynamic)rawProps, // source
154-
*family.nativeProps_DEPRECATED, // patch
155-
NullValueStrategy::Override);
156-
157-
// 3. Clone the props by using finalProps.
158-
props = componentDescriptor.cloneProps(
159-
propsParserContext, shadowNode.getProps(), RawProps(finalProps));
160-
} else {
161-
props = componentDescriptor.cloneProps(
162-
propsParserContext, shadowNode.getProps(), std::move(rawProps));
124+
std::optional<folly::dynamic> finalProps;
125+
{
126+
std::lock_guard<std::mutex> lock(family.nativePropsMutex);
127+
if (family.nativeProps_DEPRECATED != nullptr) {
128+
// 1. update the nativeProps_DEPRECATED props.
129+
//
130+
// In this step, we want the most recent value for the props
131+
// managed by setNativeProps.
132+
// Values in `rawProps` patch (take precedence over)
133+
// `nativeProps_DEPRECATED`. For example, if both
134+
// `nativeProps_DEPRECATED` and `rawProps` contain key 'A'.
135+
// Value from `rawProps` overrides what was previously in
136+
// `nativeProps_DEPRECATED`. Notice that the `nativeProps_DEPRECATED`
137+
// patch will not get more props from `rawProps`: if the key is not
138+
// present in `nativeProps_DEPRECATED`, it will not be added.
139+
//
140+
// The result of this operation is the new `nativeProps_DEPRECATED`.
141+
family.nativeProps_DEPRECATED =
142+
std::make_unique<folly::dynamic>(mergeDynamicProps(
143+
*family.nativeProps_DEPRECATED, // source
144+
(folly::dynamic)rawProps, // patch
145+
NullValueStrategy::Ignore));
146+
147+
// 2. Compute the final set of props.
148+
//
149+
// This step takes the new props handled by `setNativeProps` and
150+
// merges them in the `rawProps` managed by React.
151+
// The new props handled by `nativeProps` now takes precedence
152+
// on the props handled by React, as we want to make sure that
153+
// all the props are applied to the component.
154+
// We use these finalProps as source of truth for the component.
155+
finalProps = mergeDynamicProps(
156+
(folly::dynamic)rawProps, // source
157+
*family.nativeProps_DEPRECATED, // patch
158+
NullValueStrategy::Override);
159+
}
163160
}
161+
162+
props = componentDescriptor.cloneProps(
163+
propsParserContext,
164+
shadowNode.getProps(),
165+
finalProps ? RawProps(std::move(*finalProps)) : std::move(rawProps));
164166
}
165167

166168
auto clonedShadowNode = componentDescriptor.cloneShadowNode(
@@ -448,19 +450,22 @@ void UIManager::setNativeProps_DEPRECATED(
448450
const std::shared_ptr<const ShadowNode>& shadowNode,
449451
RawProps rawProps) const {
450452
auto& family = shadowNode->getFamily();
451-
if (family.nativeProps_DEPRECATED) {
452-
// Values in `rawProps` patch (take precedence over)
453-
// `nativeProps_DEPRECATED`. For example, if both `nativeProps_DEPRECATED`
454-
// and `rawProps` contain key 'A'. Value from `rawProps` overrides what
455-
// was previously in `nativeProps_DEPRECATED`.
456-
family.nativeProps_DEPRECATED =
457-
std::make_unique<folly::dynamic>(mergeDynamicProps(
458-
*family.nativeProps_DEPRECATED,
459-
(folly::dynamic)rawProps,
460-
NullValueStrategy::Override));
461-
} else {
462-
family.nativeProps_DEPRECATED =
463-
std::make_unique<folly::dynamic>((folly::dynamic)rawProps);
453+
{
454+
std::lock_guard<std::mutex> lock(family.nativePropsMutex);
455+
if (family.nativeProps_DEPRECATED) {
456+
// Values in `rawProps` patch (take precedence over)
457+
// `nativeProps_DEPRECATED`. For example, if both
458+
// `nativeProps_DEPRECATED` and `rawProps` contain key 'A'. Value from
459+
// `rawProps` overrides what was previously in `nativeProps_DEPRECATED`.
460+
family.nativeProps_DEPRECATED =
461+
std::make_unique<folly::dynamic>(mergeDynamicProps(
462+
*family.nativeProps_DEPRECATED,
463+
(folly::dynamic)rawProps,
464+
NullValueStrategy::Override));
465+
} else {
466+
family.nativeProps_DEPRECATED =
467+
std::make_unique<folly::dynamic>((folly::dynamic)rawProps);
468+
}
464469
}
465470

466471
shadowTreeRegistry_.visit(

‎packages/react-native/ReactCommon/react/renderer/uimanager/tests/FabricUIManagerTest.cpp‎

Lines changed: 144 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,151 @@
88
#include <memory>
99

1010
#include <gtest/gtest.h>
11+
#include <react/renderer/components/view/ViewShadowNode.h>
12+
#include <react/renderer/core/ShadowNodeFragment.h>
13+
#include <react/renderer/element/Element.h>
14+
#include <react/renderer/element/testUtils.h>
1115
#include <react/renderer/uimanager/UIManager.h>
1216

13-
using namespace facebook::react;
17+
namespace facebook::react {
1418

15-
TEST(UIManagerTest, testSomething) {
16-
// TODO
19+
class FabricUIManagerTest : public ::testing::Test {
20+
public:
21+
FabricUIManagerTest() {
22+
contextContainer_ = std::make_shared<ContextContainer>();
23+
24+
ComponentDescriptorProviderRegistry componentDescriptorProviderRegistry{};
25+
auto componentDescriptorRegistry =
26+
componentDescriptorProviderRegistry.createComponentDescriptorRegistry(
27+
ComponentDescriptorParameters{
28+
.eventDispatcher = EventDispatcher::Shared{},
29+
.contextContainer = contextContainer_,
30+
.flavor = nullptr});
31+
32+
componentDescriptorProviderRegistry.add(
33+
concreteComponentDescriptorProvider<RootComponentDescriptor>());
34+
componentDescriptorProviderRegistry.add(
35+
concreteComponentDescriptorProvider<ViewComponentDescriptor>());
36+
37+
builder_ = std::make_unique<ComponentBuilder>(componentDescriptorRegistry);
38+
39+
RuntimeExecutor runtimeExecutor =
40+
[](std::function<void(
41+
facebook::jsi::Runtime & runtime)>&& /*callback*/) {};
42+
uiManager_ =
43+
std::make_unique<UIManager>(runtimeExecutor, contextContainer_);
44+
uiManager_->setComponentDescriptorRegistry(componentDescriptorRegistry);
45+
46+
buildAndCommitTree();
47+
}
48+
49+
void TearDown() override {
50+
uiManager_->stopSurface(surfaceId_);
51+
}
52+
53+
protected:
54+
std::shared_ptr<RootShadowNode> buildTree() {
55+
std::shared_ptr<RootShadowNode> rootNode;
56+
57+
// clang-format off
58+
auto element =
59+
Element<RootShadowNode>()
60+
.tag(1)
61+
.surfaceId(surfaceId_)
62+
.reference(rootNode)
63+
.props([] {
64+
auto sharedProps = std::make_shared<RootProps>();
65+
sharedProps->layoutConstraints = LayoutConstraints{
66+
.minimumSize = {.width = 0, .height = 0},
67+
.maximumSize = {.width = 500, .height = 500}};
68+
return sharedProps;
69+
})
70+
.children({
71+
Element<ViewShadowNode>()
72+
.tag(viewTag_)
73+
.surfaceId(surfaceId_)
74+
.props([] {
75+
auto sharedProps = std::make_shared<ViewShadowNodeProps>();
76+
sharedProps->nativeId = "initial";
77+
sharedProps->opacity = 1.0;
78+
return sharedProps;
79+
})
80+
});
81+
// clang-format on
82+
83+
builder_->build(element);
84+
return rootNode;
85+
}
86+
87+
void buildAndCommitTree() {
88+
auto rootNode = buildTree();
89+
90+
auto shadowTree = std::make_unique<ShadowTree>(
91+
surfaceId_,
92+
LayoutConstraints{},
93+
LayoutContext{},
94+
*uiManager_,
95+
*contextContainer_);
96+
97+
shadowTreePtr_ = shadowTree.get();
98+
99+
shadowTree->commit(
100+
[&rootNode](const RootShadowNode& /*oldRootShadowNode*/) {
101+
return std::static_pointer_cast<RootShadowNode>(rootNode);
102+
},
103+
{true});
104+
105+
uiManager_->startSurface(
106+
std::move(shadowTree),
107+
"test",
108+
folly::dynamic::object,
109+
DisplayMode::Visible);
110+
}
111+
112+
std::shared_ptr<const ShadowNode> currentViewNode() const {
113+
auto root = shadowTreePtr_->getCurrentRevision().rootShadowNode;
114+
return root->getChildren().front();
115+
}
116+
117+
const ViewProps& viewPropsInTree() const {
118+
return static_cast<const ViewProps&>(*currentViewNode()->getProps());
119+
}
120+
121+
SurfaceId surfaceId_{0};
122+
Tag viewTag_{42};
123+
std::shared_ptr<ContextContainer> contextContainer_;
124+
std::unique_ptr<ComponentBuilder> builder_;
125+
std::unique_ptr<UIManager> uiManager_;
126+
ShadowTree* shadowTreePtr_{nullptr};
127+
};
128+
129+
// Demonstrates the merge between props set by `setNativeProps_DEPRECATED`
130+
// (the legacy native-side prop override path) and props supplied by a
131+
// regular React re-render via `cloneNode`. The native override must
132+
// survive a React render that does not touch the overridden key, and
133+
// other React-supplied keys must still apply.
134+
TEST_F(FabricUIManagerTest, SetNativePropsMergesWithReactRender) {
135+
ASSERT_EQ(viewPropsInTree().nativeId, "initial");
136+
ASSERT_FLOAT_EQ(viewPropsInTree().opacity, 1.0);
137+
138+
// Native override on `opacity` only. The committed tree picks up the new
139+
// opacity and leaves the unrelated `nativeId` untouched.
140+
uiManager_->setNativeProps_DEPRECATED(
141+
currentViewNode(), RawProps(folly::dynamic::object("opacity", 0.5)));
142+
EXPECT_EQ(viewPropsInTree().nativeId, "initial");
143+
EXPECT_FLOAT_EQ(viewPropsInTree().opacity, 0.5);
144+
145+
// Simulate a React re-render that updates `nativeID` only. The native
146+
// `opacity` override must survive (it is not present in rawProps and
147+
// therefore not refreshed away by React), and the new `nativeID` must
148+
// apply on top.
149+
auto rerendered = uiManager_->cloneNode(
150+
*currentViewNode(),
151+
/*children=*/nullptr,
152+
RawProps(folly::dynamic::object("nativeID", "from-react")));
153+
auto& merged = static_cast<const ViewProps&>(*rerendered->getProps());
154+
EXPECT_EQ(merged.nativeId, "from-react");
155+
EXPECT_FLOAT_EQ(merged.opacity, 0.5);
17156
}
157+
158+
} // namespace facebook::react

‎scripts/cxx-api/api-snapshots/ReactAndroidDebugCxx.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4660,6 +4660,7 @@ class facebook::react::ShadowNodeFamily : public facebook::jsi::NativeState {
46604660
public facebook::react::SharedEventEmitter getEventEmitter() const;
46614661
public facebook::react::SurfaceId getSurfaceId() const;
46624662
public facebook::react::Tag getTag() const;
4663+
public mutable std::mutex nativePropsMutex;
46634664
public std::shared_ptr<const facebook::react::State> getMostRecentState() const;
46644665
public using AncestorList = std::vector<std::pair<std::reference_wrapper<const facebook::react::ShadowNode>, int>>;
46654666
public using Shared = std::shared_ptr<facebook::react::ShadowNodeFamily>;

‎scripts/cxx-api/api-snapshots/ReactAndroidNewarchCxx.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4474,6 +4474,7 @@ class facebook::react::ShadowNodeFamily : public facebook::jsi::NativeState {
44744474
public facebook::react::SharedEventEmitter getEventEmitter() const;
44754475
public facebook::react::SurfaceId getSurfaceId() const;
44764476
public facebook::react::Tag getTag() const;
4477+
public mutable std::mutex nativePropsMutex;
44774478
public std::shared_ptr<const facebook::react::State> getMostRecentState() const;
44784479
public using AncestorList = std::vector<std::pair<std::reference_wrapper<const facebook::react::ShadowNode>, int>>;
44794480
public using Shared = std::shared_ptr<facebook::react::ShadowNodeFamily>;

‎scripts/cxx-api/api-snapshots/ReactAndroidReleaseCxx.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4651,6 +4651,7 @@ class facebook::react::ShadowNodeFamily : public facebook::jsi::NativeState {
46514651
public facebook::react::SharedEventEmitter getEventEmitter() const;
46524652
public facebook::react::SurfaceId getSurfaceId() const;
46534653
public facebook::react::Tag getTag() const;
4654+
public mutable std::mutex nativePropsMutex;
46544655
public std::shared_ptr<const facebook::react::State> getMostRecentState() const;
46554656
public using AncestorList = std::vector<std::pair<std::reference_wrapper<const facebook::react::ShadowNode>, int>>;
46564657
public using Shared = std::shared_ptr<facebook::react::ShadowNodeFamily>;

‎scripts/cxx-api/api-snapshots/ReactAppleDebugCxx.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6861,6 +6861,7 @@ class facebook::react::ShadowNodeFamily : public facebook::jsi::NativeState {
68616861
public facebook::react::SharedEventEmitter getEventEmitter() const;
68626862
public facebook::react::SurfaceId getSurfaceId() const;
68636863
public facebook::react::Tag getTag() const;
6864+
public mutable std::mutex nativePropsMutex;
68646865
public std::shared_ptr<const facebook::react::State> getMostRecentState() const;
68656866
public using AncestorList = std::vector<std::pair<std::reference_wrapper<const facebook::react::ShadowNode>, int>>;
68666867
public using Shared = std::shared_ptr<facebook::react::ShadowNodeFamily>;

‎scripts/cxx-api/api-snapshots/ReactAppleNewarchCxx.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6703,6 +6703,7 @@ class facebook::react::ShadowNodeFamily : public facebook::jsi::NativeState {
67036703
public facebook::react::SharedEventEmitter getEventEmitter() const;
67046704
public facebook::react::SurfaceId getSurfaceId() const;
67056705
public facebook::react::Tag getTag() const;
6706+
public mutable std::mutex nativePropsMutex;
67066707
public std::shared_ptr<const facebook::react::State> getMostRecentState() const;
67076708
public using AncestorList = std::vector<std::pair<std::reference_wrapper<const facebook::react::ShadowNode>, int>>;
67086709
public using Shared = std::shared_ptr<facebook::react::ShadowNodeFamily>;

0 commit comments

Comments
 (0)