Data race related to read/write on ReactMarker::logTaggedMarkerImpl (…

…#45557) Summary: When using `TSan` while running the Unit tests of `RNTester`, there are a few data races picked up. One is described [here](#45280), while this PR deals with a race related to concurrent read/write of `ReactMarker::logTaggedMarkerImpl`. Here is the `TSan` output: ``` WARNING: ThreadSanitizer: data race (pid=5236) Read of size 8 at 0x00011a602690 by thread T34: #0 std::__1::__function::__value_func<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::operator bool[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x18cd49c) #1 std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::operator bool[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x18cd2bc) #2 facebook::react::JSIExecutor::initializeRuntime() <null> (RNTesterUnitTests:arm64+0x1c96818) #3 facebook::react::NativeToJsBridge::initializeRuntime()::$_0::operator()(facebook::react::JSExecutor*) <null> (RNTesterUnitTests:arm64+0x1a7a074) #4 decltype(std::declval<facebook::react::NativeToJsBridge::initializeRuntime()::$_0&>()(std::declval<facebook::react::JSExecutor*>())) std::__1::__invoke[abi:ue170006]<facebook::react::NativeToJsBridge::initializeRuntime()::$_0&, facebook::react::JSExecutor*>(facebook::react::NativeToJsBridge::initializeRuntime()::$_0&, facebook::react::JSExecutor*&&) <null> (RNTesterUnitTests:arm64+0x1a79fbc) #5 void std::__1::__invoke_void_return_wrapper<void, true>::__call[abi:ue170006]<facebook::react::NativeToJsBridge::initializeRuntime()::$_0&, facebook::react::JSExecutor*>(facebook::react::NativeToJsBridge::initializeRuntime()::$_0&, facebook::react::JSExecutor*&&) <null> (RNTesterUnitTests:arm64+0x1a79e5c) #6 std::__1::__function::__alloc_func<facebook::react::NativeToJsBridge::initializeRuntime()::$_0, std::__1::allocator<facebook::react::NativeToJsBridge::initializeRuntime()::$_0>, void (facebook::react::JSExecutor*)>::operator()[abi:ue170006](facebook::react::JSExecutor*&&) <null> (RNTesterUnitTests:arm64+0x1a79d84) #7 std::__1::__function::__func<facebook::react::NativeToJsBridge::initializeRuntime()::$_0, std::__1::allocator<facebook::react::NativeToJsBridge::initializeRuntime()::$_0>, void (facebook::react::JSExecutor*)>::operator()(facebook::react::JSExecutor*&&) <null> (RNTesterUnitTests:arm64+0x1a75250) #8 std::__1::__function::__value_func<void (facebook::react::JSExecutor*)>::operator()[abi:ue170006](facebook::react::JSExecutor*&&) const <null> (RNTesterUnitTests:arm64+0x1abac9c) #9 std::__1::function<void (facebook::react::JSExecutor*)>::operator()(facebook::react::JSExecutor*) const <null> (RNTesterUnitTests:arm64+0x1aba9d0) #10 facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8::operator()() const <null> (RNTesterUnitTests:arm64+0x1aba8d4) #11 decltype(std::declval<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8&>()()) std::__1::__invoke[abi:ue170006]<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8&>(facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8&) <null> (RNTesterUnitTests:arm64+0x1aba6d4) #12 void std::__1::__invoke_void_return_wrapper<void, true>::__call[abi:ue170006]<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8&>(facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8&) <null> (RNTesterUnitTests:arm64+0x1aba4f8) #13 std::__1::__function::__alloc_func<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8, std::__1::allocator<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8>, void ()>::operator()[abi:ue170006]() <null> (RNTesterUnitTests:arm64+0x1aba45c) #14 std::__1::__function::__func<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8, std::__1::allocator<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8>, void ()>::operator()() <null> (RNTesterUnitTests:arm64+0x1ab4918) #15 std::__1::__function::__value_func<void ()>::operator()[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x3ce2e4) #16 std::__1::function<void ()>::operator()() const <null> (RNTesterUnitTests:arm64+0x3cdfd0) #17 facebook::react::tryAndReturnError(std::__1::function<void ()> const&) <null> (RNTesterUnitTests:arm64+0x4af18c) #18 facebook::react::RCTMessageThread::tryFunc(std::__1::function<void ()> const&) <null> (RNTesterUnitTests:arm64+0x51595c) #19 facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1::operator()() const <null> (RNTesterUnitTests:arm64+0x529df0) #20 decltype(std::declval<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1&>()()) std::__1::__invoke[abi:ue170006]<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1&>(facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1&) <null> (RNTesterUnitTests:arm64+0x529b54) #21 void std::__1::__invoke_void_return_wrapper<void, true>::__call[abi:ue170006]<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1&>(facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1&) <null> (RNTesterUnitTests:arm64+0x529978) #22 std::__1::__function::__alloc_func<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1, std::__1::allocator<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1>, void ()>::operator()[abi:ue170006]() <null> (RNTesterUnitTests:arm64+0x5298dc) #23 std::__1::__function::__func<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1, std::__1::allocator<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1>, void ()>::operator()() <null> (RNTesterUnitTests:arm64+0x524518) #24 std::__1::__function::__value_func<void ()>::operator()[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x3ce2e4) #25 std::__1::function<void ()>::operator()() const <null> (RNTesterUnitTests:arm64+0x3cdfd0) #26 invocation function for block in facebook::react::RCTMessageThread::runAsync(std::__1::function<void ()>) <null> (RNTesterUnitTests:arm64+0x515384) #27 __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__ <null> (CoreFoundation:arm64+0x8dc0c) #28 __NSThread__start__ <null> (Foundation:arm64+0x645c60) Previous write of size 8 at 0x00011a602690 by main thread: #0 std::__1::__function::__value_func<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::swap[abi:ue170006](std::__1::__function::__value_func<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>&) <null> (RNTesterUnitTests:arm64+0x43b078) #1 std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::swap(std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>&) <null> (RNTesterUnitTests:arm64+0x433100) #2 std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>& std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::operator=<registerPerformanceLoggerHooks(RCTPerformanceLogger*)::$_1, void>(registerPerformanceLoggerHooks(RCTPerformanceLogger*)::$_1&&) <null> (RNTesterUnitTests:arm64+0x432d50) #3 registerPerformanceLoggerHooks(RCTPerformanceLogger*) <null> (RNTesterUnitTests:arm64+0x4170fc) #4 -[RCTCxxBridge initWithParentBridge:] <null> (RNTesterUnitTests:arm64+0x416504) #5 -[RCTBridge setUp] <null> (RNTesterUnitTests:arm64+0x3bf6f4) #6 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc540) #7 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc124) #8 -[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority] <null> (RNTesterUnitTests:arm64+0x7de8) #9 __invoking___ <null> (CoreFoundation:arm64+0x13371c) Location is global 'facebook::react::ReactMarker::logTaggedMarkerImpl' at 0x00011a602678 (RNTesterUnitTests+0x438a690) Thread T34 (tid=11229216, running) created by main thread at: #0 pthread_create <null> (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x2bee4) #1 -[NSThread startAndReturnError:] <null> (Foundation:arm64+0x6458f0) #2 -[RCTBridge setUp] <null> (RNTesterUnitTests:arm64+0x3bf748) #3 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc540) #4 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc124) #5 -[RCTImageLoaderTests testImageLoaderUsesImageDecoderWithHighestPriority] <null> (RNTesterUnitTests:arm64+0xbe8c) #6 __invoking___ <null> (CoreFoundation:arm64+0x13371c) ``` The proposed solution is to wrap `logTaggedMarkerImpl` in a class that has a static getter and setter wherein a read/write lock is employed. It is my understanding that `logTaggedMarkerImpl` is read several times, but only assigned rarely, and thus it seems appropriate with a read/write lock. The getter and setter functions are also inlineable, such that one should not need to make an extra function call when obtaining the `logTaggedMarkerImpl` instance. In order to reproduce my findings and verify fix: * Clone this branch * Run setup code as described in README * Execute `git revert -n 6599883 83a2a3c` * Enable TSan for both `RNTester` and its test scheme. * Enable Runtime issue breakpoint for TSan * Run unit tests * Observe the `TSan` breakpoint is hit (possibly other places in the codebase as well) when accessing `logTaggedMarkerImpl`. Continue execution if other breakpoints are hit before this breakpoint. * Execute `git revert --abort` * Run the tests again and observe the `TSan` breakpoint does _not_ hit said code again. ## Changelog: [iOS][Fixed] Data race related to read/write on `ReactMarker::logTaggedMarkerImpl` Pull Request resolved: #45557 Test Plan: I believe there are existing tests that will cover the proposed changes. Reviewed By: cipolleschi Differential Revision: D60525080 Pulled By: dmytrorykun fbshipit-source-id: 78b0ce2a660af0e29909ff68c018698a9a1e29f8
facebook · Aug 12, 2024 · 7e41ea4 · 7e41ea4
1 parent 4faafb0
commit 7e41ea4
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 10 deletions.
diff --git a/packages/react-native/React/CxxBridge/RCTCxxBridge.mm b/packages/react-native/React/CxxBridge/RCTCxxBridge.mm
@@ -162,11 +162,13 @@ static void mapReactMarkerToPerformanceLogger(
 
 static void registerPerformanceLoggerHooks(RCTPerformanceLogger *performanceLogger)
 {
+  std::unique_lock lock(ReactMarker::logTaggedMarkerImplMutex);
   __weak RCTPerformanceLogger *weakPerformanceLogger = performanceLogger;
-  ReactMarker::logTaggedMarkerImpl = [weakPerformanceLogger](
-                                         const ReactMarker::ReactMarkerId markerId, const char *tag) {
+  ReactMarker::LogTaggedMarker newMarker = [weakPerformanceLogger](
+                                               const ReactMarker::ReactMarkerId markerId, const char *tag) {
     mapReactMarkerToPerformanceLogger(markerId, weakPerformanceLogger, tag);
   };
+  ReactMarker::logTaggedMarkerImpl = newMarker;
 }
 
 @interface RCTCxxBridge () <RCTModuleDataCallInvokerProvider>

diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/JReactMarker.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/JReactMarker.cpp
@@ -16,6 +16,7 @@ namespace facebook::react {
 void JReactMarker::setLogPerfMarkerIfNeeded() {
   static std::once_flag flag{};
   std::call_once(flag, []() {
+    std::unique_lock lock(ReactMarker::logTaggedMarkerImplMutex);
     ReactMarker::logTaggedMarkerImpl = JReactMarker::logPerfMarker;
     ReactMarker::logTaggedMarkerBridgelessImpl =
         JReactMarker::logPerfMarkerBridgeless;

diff --git a/packages/react-native/ReactCommon/cxxreact/ReactMarker.cpp b/packages/react-native/ReactCommon/cxxreact/ReactMarker.cpp
@@ -16,8 +16,9 @@ namespace ReactMarker {
 #pragma clang diagnostic ignored "-Wglobal-constructors"
 #endif
 
-LogTaggedMarker logTaggedMarkerImpl = nullptr;
 LogTaggedMarker logTaggedMarkerBridgelessImpl = nullptr;
+LogTaggedMarker logTaggedMarkerImpl = nullptr;
+std::shared_mutex logTaggedMarkerImplMutex;
 
 #if __clang__
 #pragma clang diagnostic pop
@@ -28,7 +29,14 @@ void logMarker(const ReactMarkerId markerId) {
 }
 
 void logTaggedMarker(const ReactMarkerId markerId, const char* tag) {
-  logTaggedMarkerImpl(markerId, tag);
+  LogTaggedMarker marker = nullptr;
+  {
+    std::shared_lock lock(logTaggedMarkerImplMutex);
+    marker = logTaggedMarkerImpl;
+  }
+  if (marker != nullptr) {
+    marker(markerId, tag);
+  }
 }
 
 void logMarkerBridgeless(const ReactMarkerId markerId) {

diff --git a/packages/react-native/ReactCommon/cxxreact/ReactMarker.h b/packages/react-native/ReactCommon/cxxreact/ReactMarker.h
@@ -8,6 +8,7 @@
 #pragma once
 
 #include <cmath>
+#include <shared_mutex>
 
 #ifdef __APPLE__
 #include <functional>
@@ -51,7 +52,10 @@ typedef void (*LogTaggedMarkerBridgeless)(const ReactMarkerId, const char* tag);
 #define RN_EXPORT __attribute__((visibility("default")))
 #endif
 
-extern RN_EXPORT LogTaggedMarker logTaggedMarkerImpl; // Bridge only
+extern RN_EXPORT std::shared_mutex logTaggedMarkerImplMutex;
+/// - important: To ensure this gets read and written to in a thread safe
+/// manner, make use of `logTaggedMarkerImplMutex`.
+extern RN_EXPORT LogTaggedMarker logTaggedMarkerImpl;
 extern RN_EXPORT LogTaggedMarker logTaggedMarkerBridgelessImpl;
 
 extern RN_EXPORT void logMarker(const ReactMarkerId markerId); // Bridge only

diff --git a/packages/react-native/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp b/packages/react-native/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp
@@ -139,8 +139,11 @@ void JSIExecutor::initializeRuntime() {
   if (runtimeInstaller_) {
     runtimeInstaller_(*runtime_);
   }
-
-  bool hasLogger(ReactMarker::logTaggedMarkerImpl);
+  bool hasLogger = false;
+  {
+    std::shared_lock lock(ReactMarker::logTaggedMarkerImplMutex);
+    hasLogger = ReactMarker::logTaggedMarkerImpl != nullptr;
+  }
   if (hasLogger) {
     ReactMarker::logMarker(ReactMarker::CREATE_REACT_CONTEXT_STOP);
   }
@@ -150,8 +153,11 @@ void JSIExecutor::loadBundle(
     std::unique_ptr<const JSBigString> script,
     std::string sourceURL) {
   SystraceSection s("JSIExecutor::loadBundle");
-
-  bool hasLogger(ReactMarker::logTaggedMarkerImpl);
+  bool hasLogger = false;
+  {
+    std::shared_lock lock(ReactMarker::logTaggedMarkerImplMutex);
+    hasLogger = ReactMarker::logTaggedMarkerImpl != nullptr;
+  }
   std::string scriptName = simpleBasename(sourceURL);
   if (hasLogger) {
     ReactMarker::logTaggedMarker(

diff --git a/packages/react-native/ReactCommon/jsiexecutor/jsireact/JSINativeModules.cpp b/packages/react-native/ReactCommon/jsiexecutor/jsireact/JSINativeModules.cpp
@@ -67,7 +67,11 @@ void JSINativeModules::reset() {
 std::optional<Object> JSINativeModules::createModule(
     Runtime& rt,
     const std::string& name) {
-  bool hasLogger(ReactMarker::logTaggedMarkerImpl);
+  bool hasLogger = false;
+  {
+    std::shared_lock lock(ReactMarker::logTaggedMarkerImplMutex);
+    hasLogger = ReactMarker::logTaggedMarkerImpl != nullptr;
+  }
   if (hasLogger) {
     ReactMarker::logTaggedMarker(
         ReactMarker::NATIVE_MODULE_SETUP_START, name.c_str());