From a8cf12a9327a1b77db159258355927d2571478bc Mon Sep 17 00:00:00 2001 From: Marc Horowitz Date: Thu, 1 Sep 2016 19:55:29 -0700 Subject: [PATCH] Fix some unsafe thread behavior Reviewed By: javache Differential Revision: D3789293 fbshipit-source-id: 80118c7f8faf487fe35d4d83a91f023219f6bf80 --- .../RCTNetwork.xcodeproj/project.pbxproj | 12 ++-- .../{RCTNetworking.m => RCTNetworking.mm} | 35 ++++++----- .../{RCTModuleData.m => RCTModuleData.mm} | 63 +++++++++---------- React/React.xcodeproj/project.pbxproj | 8 +-- 4 files changed, 62 insertions(+), 56 deletions(-) rename Libraries/Network/{RCTNetworking.m => RCTNetworking.mm} (95%) rename React/Base/{RCTModuleData.m => RCTModuleData.mm} (87%) diff --git a/Libraries/Network/RCTNetwork.xcodeproj/project.pbxproj b/Libraries/Network/RCTNetwork.xcodeproj/project.pbxproj index 83951342b6539c..7a7a856d94fc0d 100644 --- a/Libraries/Network/RCTNetwork.xcodeproj/project.pbxproj +++ b/Libraries/Network/RCTNetwork.xcodeproj/project.pbxproj @@ -7,12 +7,12 @@ objects = { /* Begin PBXBuildFile section */ - 134E969A1BCEB7F800AFFDA1 /* RCTDataRequestHandler.m in Sources */ = {isa = PBXBuildFile; fileRef = 134E96991BCEB7F800AFFDA1 /* RCTDataRequestHandler.m */; settings = {ASSET_TAGS = (); }; }; + 134E969A1BCEB7F800AFFDA1 /* RCTDataRequestHandler.m in Sources */ = {isa = PBXBuildFile; fileRef = 134E96991BCEB7F800AFFDA1 /* RCTDataRequestHandler.m */; }; 1372B7371AB03E7B00659ED6 /* RCTNetInfo.m in Sources */ = {isa = PBXBuildFile; fileRef = 1372B7361AB03E7B00659ED6 /* RCTNetInfo.m */; }; 13D6D66A1B5FCF8200883BE9 /* RCTNetworkTask.m in Sources */ = {isa = PBXBuildFile; fileRef = 13D6D6691B5FCF8200883BE9 /* RCTNetworkTask.m */; }; - 13EF800E1BCBE015003F47DD /* RCTFileRequestHandler.m in Sources */ = {isa = PBXBuildFile; fileRef = 13EF800D1BCBE015003F47DD /* RCTFileRequestHandler.m */; settings = {ASSET_TAGS = (); }; }; + 13EF800E1BCBE015003F47DD /* RCTFileRequestHandler.m in Sources */ = {isa = PBXBuildFile; fileRef = 13EF800D1BCBE015003F47DD /* RCTFileRequestHandler.m */; }; 352DA0BA1B17855800AA15A8 /* RCTHTTPRequestHandler.m in Sources */ = {isa = PBXBuildFile; fileRef = 352DA0B81B17855800AA15A8 /* RCTHTTPRequestHandler.m */; }; - 58B512081A9E6CE300147676 /* RCTNetworking.m in Sources */ = {isa = PBXBuildFile; fileRef = 58B512071A9E6CE300147676 /* RCTNetworking.m */; }; + 58B512081A9E6CE300147676 /* RCTNetworking.mm in Sources */ = {isa = PBXBuildFile; fileRef = 58B512071A9E6CE300147676 /* RCTNetworking.mm */; }; /* End PBXBuildFile section */ /* Begin PBXCopyFilesBuildPhase section */ @@ -40,7 +40,7 @@ 352DA0B81B17855800AA15A8 /* RCTHTTPRequestHandler.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTHTTPRequestHandler.m; sourceTree = ""; }; 58B511DB1A9E6C8500147676 /* libRCTNetwork.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = libRCTNetwork.a; sourceTree = BUILT_PRODUCTS_DIR; }; 58B512061A9E6CE300147676 /* RCTNetworking.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RCTNetworking.h; sourceTree = ""; }; - 58B512071A9E6CE300147676 /* RCTNetworking.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTNetworking.m; sourceTree = ""; }; + 58B512071A9E6CE300147676 /* RCTNetworking.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = RCTNetworking.mm; sourceTree = ""; }; /* End PBXFileReference section */ /* Begin PBXFrameworksBuildPhase section */ @@ -68,7 +68,7 @@ 1372B7351AB03E7B00659ED6 /* RCTNetInfo.h */, 1372B7361AB03E7B00659ED6 /* RCTNetInfo.m */, 58B512061A9E6CE300147676 /* RCTNetworking.h */, - 58B512071A9E6CE300147676 /* RCTNetworking.m */, + 58B512071A9E6CE300147676 /* RCTNetworking.mm */, 58B511DC1A9E6C8500147676 /* Products */, ); indentWidth = 2; @@ -143,7 +143,7 @@ 13EF800E1BCBE015003F47DD /* RCTFileRequestHandler.m in Sources */, 134E969A1BCEB7F800AFFDA1 /* RCTDataRequestHandler.m in Sources */, 1372B7371AB03E7B00659ED6 /* RCTNetInfo.m in Sources */, - 58B512081A9E6CE300147676 /* RCTNetworking.m in Sources */, + 58B512081A9E6CE300147676 /* RCTNetworking.mm in Sources */, 352DA0BA1B17855800AA15A8 /* RCTHTTPRequestHandler.m in Sources */, ); runOnlyForDeploymentPostprocessing = 0; diff --git a/Libraries/Network/RCTNetworking.m b/Libraries/Network/RCTNetworking.mm similarity index 95% rename from Libraries/Network/RCTNetworking.m rename to Libraries/Network/RCTNetworking.mm index df3af6777f9ba4..6c554e638718b9 100644 --- a/Libraries/Network/RCTNetworking.m +++ b/Libraries/Network/RCTNetworking.mm @@ -9,6 +9,8 @@ #import "RCTNetworking.h" +#include + #import "RCTAssert.h" #import "RCTConvert.h" #import "RCTNetworkTask.h" @@ -48,7 +50,7 @@ @implementation RCTHTTPFormDataHelper const size_t boundaryLength = 70; const char *boundaryChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_./"; - char *bytes = malloc(boundaryLength); + char *bytes = (char*)malloc(boundaryLength); size_t charCount = strlen(boundaryChars); for (int i = 0; i < boundaryLength; i++) { bytes[i] = boundaryChars[arc4random_uniform((u_int32_t)charCount)]; @@ -126,6 +128,7 @@ - (RCTURLRequestCancellationBlock)handleResult:(NSDictionary *)r @implementation RCTNetworking { NSMutableDictionary *_tasksByRequestID; + std::mutex _handlersLock; NSArray> *_handlers; } @@ -149,19 +152,23 @@ @implementation RCTNetworking return nil; } - if (!_handlers) { - // Get handlers, sorted in reverse priority order (highest priority first) - _handlers = [[self.bridge modulesConformingToProtocol:@protocol(RCTURLRequestHandler)] sortedArrayUsingComparator:^NSComparisonResult(id a, id b) { - float priorityA = [a respondsToSelector:@selector(handlerPriority)] ? [a handlerPriority] : 0; - float priorityB = [b respondsToSelector:@selector(handlerPriority)] ? [b handlerPriority] : 0; - if (priorityA > priorityB) { - return NSOrderedAscending; - } else if (priorityA < priorityB) { - return NSOrderedDescending; - } else { - return NSOrderedSame; - } - }]; + { + std::lock_guard lock(_handlersLock); + + if (!_handlers) { + // Get handlers, sorted in reverse priority order (highest priority first) + _handlers = [[self.bridge modulesConformingToProtocol:@protocol(RCTURLRequestHandler)] sortedArrayUsingComparator:^NSComparisonResult(id a, id b) { + float priorityA = [a respondsToSelector:@selector(handlerPriority)] ? [a handlerPriority] : 0; + float priorityB = [b respondsToSelector:@selector(handlerPriority)] ? [b handlerPriority] : 0; + if (priorityA > priorityB) { + return NSOrderedAscending; + } else if (priorityA < priorityB) { + return NSOrderedDescending; + } else { + return NSOrderedSame; + } + }]; + } } if (RCT_DEBUG) { diff --git a/React/Base/RCTModuleData.m b/React/Base/RCTModuleData.mm similarity index 87% rename from React/Base/RCTModuleData.m rename to React/Base/RCTModuleData.mm index e56278473853d5..5f49c347a1db13 100644 --- a/React/Base/RCTModuleData.m +++ b/React/Base/RCTModuleData.mm @@ -11,6 +11,8 @@ #import +#include + #import "RCTBridge.h" #import "RCTBridge+Private.h" #import "RCTModuleMethod.h" @@ -23,7 +25,7 @@ @implementation RCTModuleData NSDictionary *_constantsToExport; NSString *_queueName; __weak RCTBridge *_bridge; - NSLock *_instanceLock; + std::mutex _instanceLock; BOOL _setupComplete; } @@ -36,8 +38,6 @@ - (void)setUp _implementsBatchDidComplete = [_moduleClass instancesRespondToSelector:@selector(batchDidComplete)]; _implementsPartialBatchDidFlush = [_moduleClass instancesRespondToSelector:@selector(partialBatchDidFlush)]; - _instanceLock = [NSLock new]; - static IMP objectInitMethod; static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ @@ -84,43 +84,42 @@ - (instancetype)initWithModuleInstance:(id)instance - (void)setUpInstanceAndBridge { RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways, @"[RCTModuleData setUpInstanceAndBridge] [_instanceLock lock]", @{ @"moduleClass": _moduleClass }); - [_instanceLock lock]; - if (!_setupComplete && _bridge.valid) { - if (!_instance) { - if (RCT_DEBUG && _requiresMainQueueSetup) { - RCTAssertMainQueue(); - } - RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways, @"[RCTModuleData setUpInstanceAndBridge] [_moduleClass new]", @{ @"moduleClass": _moduleClass }); - _instance = [_moduleClass new]; - RCT_PROFILE_END_EVENT(RCTProfileTagAlways, @"", nil); + { + std::unique_lock lock(_instanceLock); + + if (!_setupComplete && _bridge.valid) { if (!_instance) { - // Module init returned nil, probably because automatic instantatiation - // of the module is not supported, and it is supposed to be passed in to - // the bridge constructor. Mark setup complete to avoid doing more work. - _setupComplete = YES; - RCTLogWarn(@"The module %@ is returning nil from its constructor. You " - "may need to instantiate it yourself and pass it into the " - "bridge.", _moduleClass); + if (RCT_DEBUG && _requiresMainQueueSetup) { + RCTAssertMainQueue(); + } + RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways, @"[RCTModuleData setUpInstanceAndBridge] [_moduleClass new]", @{ @"moduleClass": _moduleClass }); + _instance = [_moduleClass new]; + RCT_PROFILE_END_EVENT(RCTProfileTagAlways, @"", nil); + if (!_instance) { + // Module init returned nil, probably because automatic instantatiation + // of the module is not supported, and it is supposed to be passed in to + // the bridge constructor. Mark setup complete to avoid doing more work. + _setupComplete = YES; + RCTLogWarn(@"The module %@ is returning nil from its constructor. You " + "may need to instantiate it yourself and pass it into the " + "bridge.", _moduleClass); + } + } + + if (_instance && RCTProfileIsProfiling()) { + RCTProfileHookInstance(_instance); } - } - if (_instance && RCTProfileIsProfiling()) { - RCTProfileHookInstance(_instance); + // Bridge must be set before methodQueue is set up, as methodQueue + // initialization requires it (View Managers get their queue by calling + // self.bridge.uiManager.methodQueue) + [self setBridgeForInstance]; } - // Bridge must be set before methodQueue is set up, as methodQueue - // initialization requires it (View Managers get their queue by calling - // self.bridge.uiManager.methodQueue) - [self setBridgeForInstance]; + [self setUpMethodQueue]; } - [_instanceLock unlock]; RCT_PROFILE_END_EVENT(RCTProfileTagAlways, @"", nil); - // This is called outside of the lock in order to prevent deadlock issues - // because the logic in `setUpMethodQueue` can cause `moduleData.instance` - // to be accessed re-entrantly. - [self setUpMethodQueue]; - // This is called outside of the lock in order to prevent deadlock issues // because the logic in `finishSetupForInstance` can cause // `moduleData.instance` to be accessed re-entrantly. diff --git a/React/React.xcodeproj/project.pbxproj b/React/React.xcodeproj/project.pbxproj index 3beb5e66361e0a..a52f195b6f9ba4 100644 --- a/React/React.xcodeproj/project.pbxproj +++ b/React/React.xcodeproj/project.pbxproj @@ -68,7 +68,7 @@ 1450FF8A1BCFF28A00208362 /* RCTProfileTrampoline-x86_64.S in Sources */ = {isa = PBXBuildFile; fileRef = 1450FF851BCFF28A00208362 /* RCTProfileTrampoline-x86_64.S */; }; 14A43DF31C20B1C900794BC8 /* RCTJSCProfiler.m in Sources */ = {isa = PBXBuildFile; fileRef = 14A43DF21C20B1C900794BC8 /* RCTJSCProfiler.m */; }; 14C2CA711B3AC63800E6CBB2 /* RCTModuleMethod.m in Sources */ = {isa = PBXBuildFile; fileRef = 14C2CA701B3AC63800E6CBB2 /* RCTModuleMethod.m */; }; - 14C2CA741B3AC64300E6CBB2 /* RCTModuleData.m in Sources */ = {isa = PBXBuildFile; fileRef = 14C2CA731B3AC64300E6CBB2 /* RCTModuleData.m */; }; + 14C2CA741B3AC64300E6CBB2 /* RCTModuleData.mm in Sources */ = {isa = PBXBuildFile; fileRef = 14C2CA731B3AC64300E6CBB2 /* RCTModuleData.mm */; }; 14C2CA761B3AC64F00E6CBB2 /* RCTFrameUpdate.m in Sources */ = {isa = PBXBuildFile; fileRef = 14C2CA751B3AC64F00E6CBB2 /* RCTFrameUpdate.m */; }; 14C2CA781B3ACB0400E6CBB2 /* RCTBatchedBridge.m in Sources */ = {isa = PBXBuildFile; fileRef = 14C2CA771B3ACB0400E6CBB2 /* RCTBatchedBridge.m */; }; 14F3620D1AABD06A001CE568 /* RCTSwitch.m in Sources */ = {isa = PBXBuildFile; fileRef = 14F362081AABD06A001CE568 /* RCTSwitch.m */; }; @@ -256,7 +256,7 @@ 14C2CA6F1B3AC63800E6CBB2 /* RCTModuleMethod.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RCTModuleMethod.h; sourceTree = ""; }; 14C2CA701B3AC63800E6CBB2 /* RCTModuleMethod.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTModuleMethod.m; sourceTree = ""; }; 14C2CA721B3AC64300E6CBB2 /* RCTModuleData.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RCTModuleData.h; sourceTree = ""; }; - 14C2CA731B3AC64300E6CBB2 /* RCTModuleData.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTModuleData.m; sourceTree = ""; }; + 14C2CA731B3AC64300E6CBB2 /* RCTModuleData.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = RCTModuleData.mm; sourceTree = ""; }; 14C2CA751B3AC64F00E6CBB2 /* RCTFrameUpdate.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTFrameUpdate.m; sourceTree = ""; }; 14C2CA771B3ACB0400E6CBB2 /* RCTBatchedBridge.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTBatchedBridge.m; sourceTree = ""; }; 14F362071AABD06A001CE568 /* RCTSwitch.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RCTSwitch.h; sourceTree = ""; }; @@ -606,7 +606,7 @@ 83CBBA4D1A601E3B00E9B192 /* RCTLog.h */, 83CBBA4E1A601E3B00E9B192 /* RCTLog.m */, 14C2CA721B3AC64300E6CBB2 /* RCTModuleData.h */, - 14C2CA731B3AC64300E6CBB2 /* RCTModuleData.m */, + 14C2CA731B3AC64300E6CBB2 /* RCTModuleData.mm */, 14C2CA6F1B3AC63800E6CBB2 /* RCTModuleMethod.h */, 14C2CA701B3AC63800E6CBB2 /* RCTModuleMethod.m */, 13A6E20F1C19ABC700845B82 /* RCTNullability.h */, @@ -773,7 +773,7 @@ 14F484561AABFCE100FDF6B9 /* RCTSliderManager.m in Sources */, 13D033631C1837FE0021DC29 /* RCTClipboard.m in Sources */, 85C199EE1CD2407900DAD810 /* RCTJSCWrapper.mm in Sources */, - 14C2CA741B3AC64300E6CBB2 /* RCTModuleData.m in Sources */, + 14C2CA741B3AC64300E6CBB2 /* RCTModuleData.mm in Sources */, 142014191B32094000CC17BA /* RCTPerformanceLogger.m in Sources */, 83CBBA981A6020BB00E9B192 /* RCTTouchHandler.m in Sources */, 3EDCA8A51D3591E700450C31 /* RCTErrorInfo.m in Sources */,