From 7e41ea4c9de943a5c73ca9821bf552d39aa49dcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20Knutzen?= <2263015+hakonk@users.noreply.github.com> Date: Mon, 12 Aug 2024 05:06:05 -0700 Subject: [PATCH] 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](https://github.com/facebook/react-native/issues/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::operator bool[abi:ue170006]() const (RNTesterUnitTests:arm64+0x18cd49c) https://github.com/facebook/react-native/issues/1 std::__1::function::operator bool[abi:ue170006]() const (RNTesterUnitTests:arm64+0x18cd2bc) https://github.com/facebook/react-native/issues/2 facebook::react::JSIExecutor::initializeRuntime() (RNTesterUnitTests:arm64+0x1c96818) https://github.com/facebook/react-native/issues/3 facebook::react::NativeToJsBridge::initializeRuntime()::$_0::operator()(facebook::react::JSExecutor*) (RNTesterUnitTests:arm64+0x1a7a074) https://github.com/facebook/react-native/issues/4 decltype(std::declval()(std::declval())) std::__1::__invoke[abi:ue170006](facebook::react::NativeToJsBridge::initializeRuntime()::$_0&, facebook::react::JSExecutor*&&) (RNTesterUnitTests:arm64+0x1a79fbc) https://github.com/facebook/react-native/issues/5 void std::__1::__invoke_void_return_wrapper::__call[abi:ue170006](facebook::react::NativeToJsBridge::initializeRuntime()::$_0&, facebook::react::JSExecutor*&&) (RNTesterUnitTests:arm64+0x1a79e5c) https://github.com/facebook/react-native/issues/6 std::__1::__function::__alloc_func, void (facebook::react::JSExecutor*)>::operator()[abi:ue170006](facebook::react::JSExecutor*&&) (RNTesterUnitTests:arm64+0x1a79d84) https://github.com/facebook/react-native/issues/7 std::__1::__function::__func, void (facebook::react::JSExecutor*)>::operator()(facebook::react::JSExecutor*&&) (RNTesterUnitTests:arm64+0x1a75250) https://github.com/facebook/react-native/issues/8 std::__1::__function::__value_func::operator()[abi:ue170006](facebook::react::JSExecutor*&&) const (RNTesterUnitTests:arm64+0x1abac9c) https://github.com/facebook/react-native/issues/9 std::__1::function::operator()(facebook::react::JSExecutor*) const (RNTesterUnitTests:arm64+0x1aba9d0) https://github.com/facebook/react-native/issues/10 facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function&&)::$_8::operator()() const (RNTesterUnitTests:arm64+0x1aba8d4) https://github.com/facebook/react-native/issues/11 decltype(std::declval&&)::$_8&>()()) std::__1::__invoke[abi:ue170006]&&)::$_8&>(facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function&&)::$_8&) (RNTesterUnitTests:arm64+0x1aba6d4) https://github.com/facebook/react-native/issues/12 void std::__1::__invoke_void_return_wrapper::__call[abi:ue170006]&&)::$_8&>(facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function&&)::$_8&) (RNTesterUnitTests:arm64+0x1aba4f8) https://github.com/facebook/react-native/issues/13 std::__1::__function::__alloc_func&&)::$_8, std::__1::allocator&&)::$_8>, void ()>::operator()[abi:ue170006]() (RNTesterUnitTests:arm64+0x1aba45c) https://github.com/facebook/react-native/issues/14 std::__1::__function::__func&&)::$_8, std::__1::allocator&&)::$_8>, void ()>::operator()() (RNTesterUnitTests:arm64+0x1ab4918) https://github.com/facebook/react-native/issues/15 std::__1::__function::__value_func::operator()[abi:ue170006]() const (RNTesterUnitTests:arm64+0x3ce2e4) https://github.com/facebook/react-native/issues/16 std::__1::function::operator()() const (RNTesterUnitTests:arm64+0x3cdfd0) https://github.com/facebook/react-native/issues/17 facebook::react::tryAndReturnError(std::__1::function const&) (RNTesterUnitTests:arm64+0x4af18c) https://github.com/facebook/react-native/issues/18 facebook::react::RCTMessageThread::tryFunc(std::__1::function const&) (RNTesterUnitTests:arm64+0x51595c) https://github.com/facebook/react-native/issues/19 facebook::react::RCTMessageThread::runOnQueue(std::__1::function&&)::$_1::operator()() const (RNTesterUnitTests:arm64+0x529df0) https://github.com/facebook/react-native/issues/20 decltype(std::declval&&)::$_1&>()()) std::__1::__invoke[abi:ue170006]&&)::$_1&>(facebook::react::RCTMessageThread::runOnQueue(std::__1::function&&)::$_1&) (RNTesterUnitTests:arm64+0x529b54) https://github.com/facebook/react-native/issues/21 void std::__1::__invoke_void_return_wrapper::__call[abi:ue170006]&&)::$_1&>(facebook::react::RCTMessageThread::runOnQueue(std::__1::function&&)::$_1&) (RNTesterUnitTests:arm64+0x529978) https://github.com/facebook/react-native/issues/22 std::__1::__function::__alloc_func&&)::$_1, std::__1::allocator&&)::$_1>, void ()>::operator()[abi:ue170006]() (RNTesterUnitTests:arm64+0x5298dc) https://github.com/facebook/react-native/issues/23 std::__1::__function::__func&&)::$_1, std::__1::allocator&&)::$_1>, void ()>::operator()() (RNTesterUnitTests:arm64+0x524518) https://github.com/facebook/react-native/issues/24 std::__1::__function::__value_func::operator()[abi:ue170006]() const (RNTesterUnitTests:arm64+0x3ce2e4) https://github.com/facebook/react-native/issues/25 std::__1::function::operator()() const (RNTesterUnitTests:arm64+0x3cdfd0) https://github.com/facebook/react-native/issues/26 invocation function for block in facebook::react::RCTMessageThread::runAsync(std::__1::function) (RNTesterUnitTests:arm64+0x515384) https://github.com/facebook/react-native/issues/27 __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__ (CoreFoundation:arm64+0x8dc0c) https://github.com/facebook/react-native/issues/28 __NSThread__start__ (Foundation:arm64+0x645c60) Previous write of size 8 at 0x00011a602690 by main thread: #0 std::__1::__function::__value_func::swap[abi:ue170006](std::__1::__function::__value_func&) (RNTesterUnitTests:arm64+0x43b078) https://github.com/facebook/react-native/issues/1 std::__1::function::swap(std::__1::function&) (RNTesterUnitTests:arm64+0x433100) https://github.com/facebook/react-native/issues/2 std::__1::function& std::__1::function::operator=(registerPerformanceLoggerHooks(RCTPerformanceLogger*)::$_1&&) (RNTesterUnitTests:arm64+0x432d50) https://github.com/facebook/react-native/issues/3 registerPerformanceLoggerHooks(RCTPerformanceLogger*) (RNTesterUnitTests:arm64+0x4170fc) https://github.com/facebook/react-native/issues/4 -[RCTCxxBridge initWithParentBridge:] (RNTesterUnitTests:arm64+0x416504) https://github.com/facebook/react-native/issues/5 -[RCTBridge setUp] (RNTesterUnitTests:arm64+0x3bf6f4) https://github.com/facebook/react-native/issues/6 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] (RNTesterUnitTests:arm64+0x3bc540) https://github.com/facebook/react-native/issues/7 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] (RNTesterUnitTests:arm64+0x3bc124) https://github.com/facebook/react-native/issues/8 -[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority] (RNTesterUnitTests:arm64+0x7de8) https://github.com/facebook/react-native/issues/9 __invoking___ (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 (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x2bee4) https://github.com/facebook/react-native/issues/1 -[NSThread startAndReturnError:] (Foundation:arm64+0x6458f0) https://github.com/facebook/react-native/issues/2 -[RCTBridge setUp] (RNTesterUnitTests:arm64+0x3bf748) https://github.com/facebook/react-native/issues/3 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] (RNTesterUnitTests:arm64+0x3bc540) https://github.com/facebook/react-native/issues/4 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] (RNTesterUnitTests:arm64+0x3bc124) https://github.com/facebook/react-native/issues/5 -[RCTImageLoaderTests testImageLoaderUsesImageDecoderWithHighestPriority] (RNTesterUnitTests:arm64+0xbe8c) https://github.com/facebook/react-native/issues/6 __invoking___ (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 65998835c2198b9d626160a6883744801fa056a9 83a2a3c9b4e5ea588a6cc3a9281ad385a388b84a` * 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: https://github.com/facebook/react-native/pull/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 --- .../react-native/React/CxxBridge/RCTCxxBridge.mm | 6 ++++-- .../src/main/jni/react/jni/JReactMarker.cpp | 1 + .../ReactCommon/cxxreact/ReactMarker.cpp | 12 ++++++++++-- .../ReactCommon/cxxreact/ReactMarker.h | 6 +++++- .../jsiexecutor/jsireact/JSIExecutor.cpp | 14 ++++++++++---- .../jsiexecutor/jsireact/JSINativeModules.cpp | 6 +++++- 6 files changed, 35 insertions(+), 10 deletions(-) diff --git a/packages/react-native/React/CxxBridge/RCTCxxBridge.mm b/packages/react-native/React/CxxBridge/RCTCxxBridge.mm index dc19bd2eb142b5..aeece2d3be09d7 100644 --- 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 () 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 index 131237c6d49bff..16d523719d051f 100644 --- 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 index 86c3a03c9df5bf..9180effba85a3b 100644 --- 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 index e50746780d4867..b74748a655bf2c 100644 --- a/packages/react-native/ReactCommon/cxxreact/ReactMarker.h +++ b/packages/react-native/ReactCommon/cxxreact/ReactMarker.h @@ -8,6 +8,7 @@ #pragma once #include +#include #ifdef __APPLE__ #include @@ -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 index 51fabddc8f0f03..368f36485550b7 100644 --- 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 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 index 56503c683e4a39..f61f46f2cfe121 100644 --- 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 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());