Skip to content

Commit ecd5c35

Browse files
javachefacebook-github-bot
authored andcommitted
Fix TSAN data races in InspectorFlags and InspectorPackagerConnection
Summary: Fix two ThreadSanitizer data races caught by the messenger_demo integration test under dev-tsan mode. **InspectorFlags**: `loadFlagsAndAssertUnchanged()` reads and writes `cachedValues_` without synchronization. The main thread calls it via `getIsProfilingBuild()` while the JS thread calls it via `getNetworkInspectionEnabled()`, racing on the shared `optional<Values>`. Add a `std::mutex` to protect `cachedValues_` access. The lock is taken after computing `newValues` from feature flags, so flag reads stay outside the critical section. **InspectorPackagerConnection**: `Impl::connect()` is called from the main thread but accesses `webSocket_` which is also read by `didFailWithError()` on the inspector thread. Dispatch the body of `connect()` to the inspector thread via `scheduleCallback()`, matching the pattern `reconnect()` already uses. Changelog: [Internal] Differential Revision: D101809616
1 parent 5f69a91 commit ecd5c35

3 files changed

Lines changed: 23 additions & 9 deletions

File tree

packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,9 @@ bool InspectorFlags::getPerfIssuesEnabled() const {
5050
}
5151

5252
void InspectorFlags::dangerouslyResetFlags() {
53-
*this = InspectorFlags{};
53+
cachedValues_.reset();
54+
inconsistentFlagsStateLogged_ = false;
55+
fuseboxDisabledForTest_ = false;
5456
}
5557

5658
void InspectorFlags::dangerouslyDisableFuseboxForTest() {
@@ -84,6 +86,8 @@ const InspectorFlags::Values& InspectorFlags::loadFlagsAndAssertUnchanged()
8486
.perfIssuesEnabled = ReactNativeFeatureFlags::perfIssuesEnabled(),
8587
};
8688

89+
// Protect against concurrent calls
90+
std::lock_guard<std::mutex> lock(mutex_);
8791
if (cachedValues_.has_value() && !inconsistentFlagsStateLogged_) {
8892
if (cachedValues_ != newValues) {
8993
LOG(ERROR)
@@ -93,7 +97,6 @@ const InspectorFlags::Values& InspectorFlags::loadFlagsAndAssertUnchanged()
9397
inconsistentFlagsStateLogged_ = true;
9498
}
9599
}
96-
97100
cachedValues_ = newValues;
98101

99102
return cachedValues_.value();

packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
#pragma once
99

10+
#include <mutex>
1011
#include <optional>
1112

1213
namespace facebook::react::jsinspector_modern {
@@ -82,9 +83,10 @@ class InspectorFlags {
8283

8384
InspectorFlags() = default;
8485
InspectorFlags(const InspectorFlags &) = delete;
85-
InspectorFlags &operator=(const InspectorFlags &) = default;
86+
InspectorFlags &operator=(const InspectorFlags &) = delete;
8687
~InspectorFlags() = default;
8788

89+
mutable std::mutex mutex_;
8890
mutable std::optional<Values> cachedValues_;
8991
mutable bool inconsistentFlagsStateLogged_{false};
9092
bool fuseboxDisabledForTest_{false};

packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -328,12 +328,21 @@ bool InspectorPackagerConnection::Impl::isConnected() const {
328328
}
329329

330330
void InspectorPackagerConnection::Impl::connect() {
331-
if (closed_) {
332-
LOG(ERROR)
333-
<< "Illegal state: Can't connect after having previously been closed.";
334-
return;
335-
}
336-
webSocket_ = delegate_->connectWebSocket(url_, weak_from_this());
331+
delegate_->scheduleCallback(
332+
[weakSelf = weak_from_this()]() mutable {
333+
auto strongSelf = weakSelf.lock();
334+
if (!strongSelf) {
335+
return;
336+
}
337+
if (strongSelf->closed_) {
338+
LOG(ERROR)
339+
<< "Illegal state: Can't connect after having previously been closed.";
340+
return;
341+
}
342+
strongSelf->webSocket_ = strongSelf->delegate_->connectWebSocket(
343+
strongSelf->url_, std::move(weakSelf));
344+
},
345+
0ms);
337346
}
338347

339348
void InspectorPackagerConnection::Impl::reconnect() {

0 commit comments

Comments
 (0)