Skip to content

Commit 0b941bc

Browse files
OrfeasZgabrieldonadel
OrfeasZ
authored and
gabrieldonadel
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 [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 cdfba52 commit 0b941bc

10 files changed

Lines changed: 237 additions & 75 deletions

File tree

‎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(
@@ -439,19 +441,22 @@ void UIManager::setNativeProps_DEPRECATED(
439441
const std::shared_ptr<const ShadowNode>& shadowNode,
440442
RawProps rawProps) const {
441443
auto& family = shadowNode->getFamily();
442-
if (family.nativeProps_DEPRECATED) {
443-
// Values in `rawProps` patch (take precedence over)
444-
// `nativeProps_DEPRECATED`. For example, if both `nativeProps_DEPRECATED`
445-
// and `rawProps` contain key 'A'. Value from `rawProps` overrides what
446-
// was previously in `nativeProps_DEPRECATED`.
447-
family.nativeProps_DEPRECATED =
448-
std::make_unique<folly::dynamic>(mergeDynamicProps(
449-
*family.nativeProps_DEPRECATED,
450-
(folly::dynamic)rawProps,
451-
NullValueStrategy::Override));
452-
} else {
453-
family.nativeProps_DEPRECATED =
454-
std::make_unique<folly::dynamic>((folly::dynamic)rawProps);
444+
{
445+
std::lock_guard<std::mutex> lock(family.nativePropsMutex);
446+
if (family.nativeProps_DEPRECATED) {
447+
// Values in `rawProps` patch (take precedence over)
448+
// `nativeProps_DEPRECATED`. For example, if both
449+
// `nativeProps_DEPRECATED` and `rawProps` contain key 'A'. Value from
450+
// `rawProps` overrides what was previously in `nativeProps_DEPRECATED`.
451+
family.nativeProps_DEPRECATED =
452+
std::make_unique<folly::dynamic>(mergeDynamicProps(
453+
*family.nativeProps_DEPRECATED,
454+
(folly::dynamic)rawProps,
455+
NullValueStrategy::Override));
456+
} else {
457+
family.nativeProps_DEPRECATED =
458+
std::make_unique<folly::dynamic>((folly::dynamic)rawProps);
459+
}
455460
}
456461

457462
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
@@ -4635,6 +4635,7 @@ class facebook::react::ShadowNodeFamily : public facebook::jsi::NativeState {
46354635
public facebook::react::SharedEventEmitter getEventEmitter() const;
46364636
public facebook::react::SurfaceId getSurfaceId() const;
46374637
public facebook::react::Tag getTag() const;
4638+
public mutable std::mutex nativePropsMutex;
46384639
public std::shared_ptr<const facebook::react::State> getMostRecentState() const;
46394640
public using AncestorList = std::vector<std::pair<std::reference_wrapper<const facebook::react::ShadowNode>, int>>;
46404641
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
@@ -4626,6 +4626,7 @@ class facebook::react::ShadowNodeFamily : public facebook::jsi::NativeState {
46264626
public facebook::react::SharedEventEmitter getEventEmitter() const;
46274627
public facebook::react::SurfaceId getSurfaceId() const;
46284628
public facebook::react::Tag getTag() const;
4629+
public mutable std::mutex nativePropsMutex;
46294630
public std::shared_ptr<const facebook::react::State> getMostRecentState() const;
46304631
public using AncestorList = std::vector<std::pair<std::reference_wrapper<const facebook::react::ShadowNode>, int>>;
46314632
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
@@ -7219,6 +7219,7 @@ class facebook::react::ShadowNodeFamily : public facebook::jsi::NativeState {
72197219
public facebook::react::SharedEventEmitter getEventEmitter() const;
72207220
public facebook::react::SurfaceId getSurfaceId() const;
72217221
public facebook::react::Tag getTag() const;
7222+
public mutable std::mutex nativePropsMutex;
72227223
public std::shared_ptr<const facebook::react::State> getMostRecentState() const;
72237224
public using AncestorList = std::vector<std::pair<std::reference_wrapper<const facebook::react::ShadowNode>, int>>;
72247225
public using Shared = std::shared_ptr<facebook::react::ShadowNodeFamily>;

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7210,6 +7210,7 @@ class facebook::react::ShadowNodeFamily : public facebook::jsi::NativeState {
72107210
public facebook::react::SharedEventEmitter getEventEmitter() const;
72117211
public facebook::react::SurfaceId getSurfaceId() const;
72127212
public facebook::react::Tag getTag() const;
7213+
public mutable std::mutex nativePropsMutex;
72137214
public std::shared_ptr<const facebook::react::State> getMostRecentState() const;
72147215
public using AncestorList = std::vector<std::pair<std::reference_wrapper<const facebook::react::ShadowNode>, int>>;
72157216
public using Shared = std::shared_ptr<facebook::react::ShadowNodeFamily>;

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3195,6 +3195,7 @@ class facebook::react::ShadowNodeFamily : public facebook::jsi::NativeState {
31953195
public facebook::react::SharedEventEmitter getEventEmitter() const;
31963196
public facebook::react::SurfaceId getSurfaceId() const;
31973197
public facebook::react::Tag getTag() const;
3198+
public mutable std::mutex nativePropsMutex;
31983199
public std::shared_ptr<const facebook::react::State> getMostRecentState() const;
31993200
public using AncestorList = std::vector<std::pair<std::reference_wrapper<const facebook::react::ShadowNode>, int>>;
32003201
public using Shared = std::shared_ptr<facebook::react::ShadowNodeFamily>;

0 commit comments

Comments
 (0)