mirror of
https://github.com/facebook/react-native.git
synced 2025-11-01 09:14:26 +00:00
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<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::operator bool[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x18cd49c) https://github.com/facebook/react-native/issues/1 std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::operator bool[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x18cd2bc) https://github.com/facebook/react-native/issues/2 facebook::react::JSIExecutor::initializeRuntime() <null> (RNTesterUnitTests:arm64+0x1c96818) https://github.com/facebook/react-native/issues/3 facebook::react::NativeToJsBridge::initializeRuntime()::$_0::operator()(facebook::react::JSExecutor*) <null> (RNTesterUnitTests:arm64+0x1a7a074) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/8 std::__1::__function::__value_func<void (facebook::react::JSExecutor*)>::operator()[abi:ue170006](facebook::react::JSExecutor*&&) const <null> (RNTesterUnitTests:arm64+0x1abac9c) https://github.com/facebook/react-native/issues/9 std::__1::function<void (facebook::react::JSExecutor*)>::operator()(facebook::react::JSExecutor*) const <null> (RNTesterUnitTests:arm64+0x1aba9d0) https://github.com/facebook/react-native/issues/10 facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8::operator()() const <null> (RNTesterUnitTests:arm64+0x1aba8d4) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/15 std::__1::__function::__value_func<void ()>::operator()[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x3ce2e4) https://github.com/facebook/react-native/issues/16 std::__1::function<void ()>::operator()() const <null> (RNTesterUnitTests:arm64+0x3cdfd0) https://github.com/facebook/react-native/issues/17 facebook::react::tryAndReturnError(std::__1::function<void ()> const&) <null> (RNTesterUnitTests:arm64+0x4af18c) https://github.com/facebook/react-native/issues/18 facebook::react::RCTMessageThread::tryFunc(std::__1::function<void ()> const&) <null> (RNTesterUnitTests:arm64+0x51595c) https://github.com/facebook/react-native/issues/19 facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1::operator()() const <null> (RNTesterUnitTests:arm64+0x529df0) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/24 std::__1::__function::__value_func<void ()>::operator()[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x3ce2e4) https://github.com/facebook/react-native/issues/25 std::__1::function<void ()>::operator()() const <null> (RNTesterUnitTests:arm64+0x3cdfd0) https://github.com/facebook/react-native/issues/26 invocation function for block in facebook::react::RCTMessageThread::runAsync(std::__1::function<void ()>) <null> (RNTesterUnitTests:arm64+0x515384) https://github.com/facebook/react-native/issues/27 __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__ <null> (CoreFoundation:arm64+0x8dc0c) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/3 registerPerformanceLoggerHooks(RCTPerformanceLogger*) <null> (RNTesterUnitTests:arm64+0x4170fc) https://github.com/facebook/react-native/issues/4 -[RCTCxxBridge initWithParentBridge:] <null> (RNTesterUnitTests:arm64+0x416504) https://github.com/facebook/react-native/issues/5 -[RCTBridge setUp] <null> (RNTesterUnitTests:arm64+0x3bf6f4) https://github.com/facebook/react-native/issues/6 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc540) https://github.com/facebook/react-native/issues/7 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc124) https://github.com/facebook/react-native/issues/8 -[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority] <null> (RNTesterUnitTests:arm64+0x7de8) https://github.com/facebook/react-native/issues/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) https://github.com/facebook/react-native/issues/1 -[NSThread startAndReturnError:] <null> (Foundation:arm64+0x6458f0) https://github.com/facebook/react-native/issues/2 -[RCTBridge setUp] <null> (RNTesterUnitTests:arm64+0x3bf748) https://github.com/facebook/react-native/issues/3 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc540) https://github.com/facebook/react-native/issues/4 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc124) https://github.com/facebook/react-native/issues/5 -[RCTImageLoaderTests testImageLoaderUsesImageDecoderWithHighestPriority] <null> (RNTesterUnitTests:arm64+0xbe8c) https://github.com/facebook/react-native/issues/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 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
This commit is contained in:
Håkon Knutzen
committed by
Facebook GitHub Bot
Facebook GitHub Bot
parent
4faafb0aa4
commit
7e41ea4c9d
@@ -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>
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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());
|
||||
|
||||
Reference in New Issue
Block a user