From c256774ffc60b5b44d3964c512bda8b059b7590c Mon Sep 17 00:00:00 2001 From: Christopher Hogan Date: Mon, 4 Nov 2019 13:24:10 -0800 Subject: [PATCH 1/5] Add memory management and concurrency tests plans to our fork of React Native --- RNTester/RNTester.xcodeproj/project.pbxproj | 2 + .../xcshareddata/xcschemes/RNTester.xcscheme | 30 ++++---- .../RNTesterTestPlan_iOS.xctestplan | 74 +++++++++++++++++++ 3 files changed, 92 insertions(+), 14 deletions(-) create mode 100644 RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan diff --git a/RNTester/RNTester.xcodeproj/project.pbxproj b/RNTester/RNTester.xcodeproj/project.pbxproj index 6ba93508ba4fd6..af9d0c04348345 100644 --- a/RNTester/RNTester.xcodeproj/project.pbxproj +++ b/RNTester/RNTester.xcodeproj/project.pbxproj @@ -915,6 +915,7 @@ 8385CF031B87479200C6273E /* RCTImageLoaderHelpers.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTImageLoaderHelpers.m; sourceTree = ""; }; 8385CF051B8747A000C6273E /* RCTImageLoaderHelpers.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = RCTImageLoaderHelpers.h; sourceTree = ""; }; 9F1534BD233AB44F006DFE44 /* JavaScriptCore.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = JavaScriptCore.framework; path = Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/JavaScriptCore.framework; sourceTree = DEVELOPER_DIR; }; + 9F1C4D00236CB06B0022EC0D /* RNTesterTestPlan_iOS.xctestplan */ = {isa = PBXFileReference; lastKnownFileType = text; path = RNTesterTestPlan_iOS.xctestplan; sourceTree = ""; }; 9F5C1923230F46CB00E3E5A7 /* JavaScriptCore.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = JavaScriptCore.framework; path = Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/JavaScriptCore.framework; sourceTree = DEVELOPER_DIR; }; 9FBFA513233C7E4C003D9A8D /* RNTesterBundle-macOS.bundle */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = "RNTesterBundle-macOS.bundle"; sourceTree = BUILT_PRODUCTS_DIR; }; 9FBFA515233C7E4C003D9A8D /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = ""; }; @@ -1452,6 +1453,7 @@ 3D13F83F1D6F6AE000E69E0E /* RNTesterBundle */ = { isa = PBXGroup; children = ( + 9F1C4D00236CB06B0022EC0D /* RNTesterTestPlan_iOS.xctestplan */, 3D13F8401D6F6AE000E69E0E /* Info.plist */, 3D13F8441D6F6AF200E69E0E /* ImageInBundle.png */, 3D13F8451D6F6AF200E69E0E /* OtherImages.xcassets */, diff --git a/RNTester/RNTester.xcodeproj/xcshareddata/xcschemes/RNTester.xcscheme b/RNTester/RNTester.xcodeproj/xcshareddata/xcschemes/RNTester.xcscheme index 3e545b616a909d..2202cf9c8b5de0 100644 --- a/RNTester/RNTester.xcodeproj/xcshareddata/xcschemes/RNTester.xcscheme +++ b/RNTester/RNTester.xcodeproj/xcshareddata/xcschemes/RNTester.xcscheme @@ -1,7 +1,7 @@ + version = "1.7"> @@ -83,6 +83,21 @@ selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB" selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB" shouldUseLaunchSchemeArgsEnv = "YES"> + + + + + + + + @@ -110,17 +125,6 @@ - - - - - - - - Date: Tue, 5 Nov 2019 15:06:42 -0800 Subject: [PATCH 2/5] temporarily disable a few more tests that are failing for us --- .../RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan | 2 ++ 1 file changed, 2 insertions(+) diff --git a/RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan b/RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan index 042687ffca8bdf..aeaef85a71f4e4 100644 --- a/RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan +++ b/RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan @@ -61,6 +61,8 @@ "parallelizable" : true, "skippedTests" : [ "RNTesterIntegrationTests\/testAccessibilityManagerTest", + "RNTesterIntegrationTests\/testAppEventsTest", + "RNTesterIntegrationTests\/testSyncMethodTest", "RNTesterIntegrationTests\/testWebSocketTest" ], "target" : { From 88edf5ae43d2af6cf27c82675c6ffac18dfab5cb Mon Sep 17 00:00:00 2001 From: Christopher Hogan Date: Wed, 6 Nov 2019 14:53:45 -0800 Subject: [PATCH 3/5] reenable tests that are building/testing again --- .../RNTesterTestPlan_iOS.xctestplan | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan b/RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan index aeaef85a71f4e4..0757e2bffbf24d 100644 --- a/RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan +++ b/RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan @@ -1,12 +1,5 @@ { "configurations" : [ - { - "id" : "8505E0BC-7E4D-4320-A0AB-AA632872F9B6", - "name" : "Default", - "options" : { - - } - }, { "id" : "37D31EEF-8FEF-4078-89E7-6BA48A640995", "name" : "Memory Checking", @@ -47,10 +40,6 @@ "testTargets" : [ { "parallelizable" : true, - "skippedTests" : [ - "RCTModuleInitTests\/testCustomInitModuleInitializedAtBridgeStartup", - "RCTModuleInitTests\/testLazyInitModuleNotInitializedDuringBridgeInit" - ], "target" : { "containerPath" : "container:RNTester.xcodeproj", "identifier" : "004D289D1AAF61C70097A701", @@ -60,10 +49,7 @@ { "parallelizable" : true, "skippedTests" : [ - "RNTesterIntegrationTests\/testAccessibilityManagerTest", - "RNTesterIntegrationTests\/testAppEventsTest", - "RNTesterIntegrationTests\/testSyncMethodTest", - "RNTesterIntegrationTests\/testWebSocketTest" + "RNTesterIntegrationTests\/testAccessibilityManagerTest" ], "target" : { "containerPath" : "container:RNTester.xcodeproj", From 45a316a42730cbfa800268dc53c35ccc774f3ad8 Mon Sep 17 00:00:00 2001 From: Tom Underhill Date: Thu, 7 Nov 2019 10:17:04 -0800 Subject: [PATCH 4/5] Build break fix: when running parallized xctestplan builds, non-deterministic errors can occur in the header maps of RCTText. Fix by forward declaring @protocols in headers, and #import the necessary headers in the .m files that need them. --- Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.h | 5 +++-- Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.m | 2 ++ Libraries/Text/TextInput/Singleline/RCTUITextField.m | 1 + 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.h b/Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.h index d6d84239162e43..8c3555b50c400a 100644 --- a/Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.h +++ b/Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.h @@ -5,14 +5,15 @@ * LICENSE file in the root directory of this source tree. */ -#import "RCTBackedTextInputViewProtocol.h" -#import "RCTBackedTextInputDelegate.h" #import "../RCTTextUIKit.h" // TODO(macOS ISS#2323203) NS_ASSUME_NONNULL_BEGIN #pragma mark - RCTBackedTextFieldDelegateAdapter (for UITextField) +@protocol RCTBackedTextInputViewProtocol; // TODO(OSS Candidate ISS#2710739) +@protocol RCTBackedTextInputDelegate; // TODO(OSS Candidate ISS#2710739) + @interface RCTBackedTextFieldDelegateAdapter : NSObject - (instancetype)initWithTextField:(UITextField *)backedTextInputView; diff --git a/Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.m b/Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.m index d9aaf7fbf9eb1c..59a800b66587ad 100644 --- a/Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.m +++ b/Libraries/Text/TextInput/RCTBackedTextInputDelegateAdapter.m @@ -6,6 +6,8 @@ */ #import "RCTBackedTextInputDelegateAdapter.h" +#import "RCTBackedTextInputViewProtocol.h" // TODO(OSS Candidate ISS#2710739) +#import "RCTBackedTextInputDelegate.h" // TODO(OSS Candidate ISS#2710739) #import "../RCTTextUIKit.h" // TODO(macOS ISS#2323203) #pragma mark - RCTBackedTextFieldDelegateAdapter (for UITextField) diff --git a/Libraries/Text/TextInput/Singleline/RCTUITextField.m b/Libraries/Text/TextInput/Singleline/RCTUITextField.m index 08fb76749b7c83..4c86eebc3ad1df 100644 --- a/Libraries/Text/TextInput/Singleline/RCTUITextField.m +++ b/Libraries/Text/TextInput/Singleline/RCTUITextField.m @@ -11,6 +11,7 @@ #import #import "RCTBackedTextInputDelegateAdapter.h" +#import "RCTBackedTextInputDelegate.h" // TODO(OSS Candidate ISS#2710739) #import "RCTTextAttributes.h" #if TARGET_OS_OSX // [TODO(macOS ISS#2323203) From c2884968fdee8b8ee65f53258c749b371a1cf903 Mon Sep 17 00:00:00 2001 From: Tom Underhill Date: Thu, 7 Nov 2019 11:46:23 -0800 Subject: [PATCH 5/5] ASAN testing revealed that RCTMessageThread can be deleted before the RunLoop has executed a block during shutdown, which results in this->m_shutdown being deferenced to deallocated memory. Capture a shared_ptr to this instead of a raw `this` in the lambdas. --- React/CxxBridge/RCTMessageThread.h | 2 +- React/CxxBridge/RCTMessageThread.mm | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/React/CxxBridge/RCTMessageThread.h b/React/CxxBridge/RCTMessageThread.h index 1e1f8cd28b6f97..90150bf67625ad 100644 --- a/React/CxxBridge/RCTMessageThread.h +++ b/React/CxxBridge/RCTMessageThread.h @@ -15,7 +15,7 @@ namespace facebook { namespace react { -class RCTMessageThread : public MessageQueueThread { +class RCTMessageThread : public MessageQueueThread, public std::enable_shared_from_this { // TODO(OSS Candidate ISS#2710739) public: RCTMessageThread(NSRunLoop *runLoop, RCTJavaScriptCompleteBlock errorBlock); ~RCTMessageThread() override; diff --git a/React/CxxBridge/RCTMessageThread.mm b/React/CxxBridge/RCTMessageThread.mm index 5017bbb4e35fdc..b98d83e3bffcad 100644 --- a/React/CxxBridge/RCTMessageThread.mm +++ b/React/CxxBridge/RCTMessageThread.mm @@ -67,9 +67,10 @@ return; } - runAsync([this, func=std::make_shared>(std::move(func))] { - if (!m_shutdown) { - tryFunc(*func); + auto sharedThis = shared_from_this(); // TODO(OSS Candidate ISS#2710739): `this` can be deleted before the RunLoop executes the block as revealed by ASAN test runs. + runAsync([sharedThis, func=std::make_shared>(std::move(func))] { + if (sharedThis->m_shutdown == false) { + sharedThis->tryFunc(*func); } }); } @@ -78,9 +79,11 @@ if (m_shutdown) { return; } - runSync([this, func=std::move(func)] { - if (!m_shutdown) { - tryFunc(func); + + auto sharedThis = shared_from_this(); // TODO(OSS Candidate ISS#2710739): `this` can be deleted before the RunLoop executes the block as revealed by ASAN test runs. + runSync([sharedThis, func=std::move(func)] { + if (sharedThis->m_shutdown == false) { + sharedThis->tryFunc(func); } }); }