Add memory management and concurrency tests plans to our fork of Reac… (#186)

HeyImChris · tom-un · commit ea7767a211e5 · 2019-11-07T12:33:24.000-08:00
* Add memory management and concurrency tests plans to our fork of React Native * temporarily disable a few more tests that are failing for us * reenable tests that are building/testing again * 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. * 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.
diff --git 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<RCTBackedTextInputViewProtocol> *)backedTextInputView;
diff --git 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
@@ -11,6 +11,7 @@
 #import <React/UIView+React.h>
 
 #import "RCTBackedTextInputDelegateAdapter.h"
+#import "RCTBackedTextInputDelegate.h" // TODO(OSS Candidate ISS#2710739)
 #import "RCTTextAttributes.h"
 
 #if TARGET_OS_OSX // [TODO(macOS ISS#2323203)
diff --git 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 = "<group>"; };
 		8385CF051B8747A000C6273E /* RCTImageLoaderHelpers.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = RCTImageLoaderHelpers.h; sourceTree = "<group>"; };
 		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 = "<group>"; };
 		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 = "<group>"; };
@@ -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
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <Scheme
    LastUpgradeVersion = "0940"
-   version = "1.3">
+   version = "1.7">
    <BuildAction
       parallelizeBuildables = "NO"
       buildImplicitDependencies = "YES">
@@ -83,6 +83,21 @@
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
       shouldUseLaunchSchemeArgsEnv = "YES">
+      <MacroExpansion>
+         <BuildableReference
+            BuildableIdentifier = "primary"
+            BlueprintIdentifier = "13B07F861A680F5B00A75B9A"
+            BuildableName = "RNTester.app"
+            BlueprintName = "RNTester"
+            ReferencedContainer = "container:RNTester.xcodeproj">
+         </BuildableReference>
+      </MacroExpansion>
+      <TestPlans>
+         <TestPlanReference
+            reference = "container:RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan"
+            default = "YES">
+         </TestPlanReference>
+      </TestPlans>
       <Testables>
          <TestableReference
             skipped = "NO">
@@ -110,17 +125,6 @@
             </SkippedTests>
          </TestableReference>
       </Testables>
-      <MacroExpansion>
-         <BuildableReference
-            BuildableIdentifier = "primary"
-            BlueprintIdentifier = "13B07F861A680F5B00A75B9A"
-            BuildableName = "RNTester.app"
-            BlueprintName = "RNTester"
-            ReferencedContainer = "container:RNTester.xcodeproj">
-         </BuildableReference>
-      </MacroExpansion>
-      <AdditionalOptions>
-      </AdditionalOptions>
    </TestAction>
    <LaunchAction
       buildConfiguration = "Debug"
@@ -154,8 +158,6 @@
             isEnabled = "YES">
          </EnvironmentVariable>
       </EnvironmentVariables>
-      <AdditionalOptions>
-      </AdditionalOptions>
    </LaunchAction>
    <ProfileAction
       buildConfiguration = "Release"
diff --git a/RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan b/RNTester/RNTester/RNTesterBundle/RNTesterTestPlan_iOS.xctestplan
@@ -0,0 +1,62 @@
+{
+  "configurations" : [
+    {
+      "id" : "37D31EEF-8FEF-4078-89E7-6BA48A640995",
+      "name" : "Memory Checking",
+      "options" : {
+        "addressSanitizer" : {
+          "detectStackUseAfterReturn" : true,
+          "enabled" : true
+        },
+        "nsZombieEnabled" : true
+      }
+    },
+    {
+      "id" : "B79DCEC6-8538-44AF-B29F-2F45036E8EE2",
+      "name" : "Concurrency",
+      "options" : {
+        "testExecutionOrdering" : "random",
+        "undefinedBehaviorSanitizerEnabled" : true
+      }
+    }
+  ],
+  "defaultOptions" : {
+    "environmentVariableEntries" : [
+      {
+        "key" : "CI_USE_PACKAGER",
+        "value" : "1"
+      },
+      {
+        "key" : "RN_BUNDLE_PREFIX",
+        "value" : ""
+      }
+    ],
+    "targetForVariableExpansion" : {
+      "containerPath" : "container:RNTester.xcodeproj",
+      "identifier" : "13B07F861A680F5B00A75B9A",
+      "name" : "RNTester"
+    }
+  },
+  "testTargets" : [
+    {
+      "parallelizable" : true,
+      "target" : {
+        "containerPath" : "container:RNTester.xcodeproj",
+        "identifier" : "004D289D1AAF61C70097A701",
+        "name" : "RNTesterUnitTests"
+      }
+    },
+    {
+      "parallelizable" : true,
+      "skippedTests" : [
+        "RNTesterIntegrationTests\/testAccessibilityManagerTest"
+      ],
+      "target" : {
+        "containerPath" : "container:RNTester.xcodeproj",
+        "identifier" : "143BC5941B21E3E100462512",
+        "name" : "RNTesterIntegrationTests"
+      }
+    }
+  ],
+  "version" : 1
+}
diff --git 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<RCTMessageThread> { // 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
@@ -67,9 +67,10 @@
     return;
   }
 
-  runAsync([this, func=std::make_shared<std::function<void()>>(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::function<void()>>(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);
     }
   });
 }

Original file line number	Diff line number	Diff line change
`@@ -67,9 +67,10 @@`
`67`	`67`	`return;`
`68`	`68`	`}`
`69`	`69`
`70`		`- runAsync([this, func=std::make_shared<std::function<void()>>(std::move(func))] {`
`71`		`- if (!m_shutdown) {`
`72`		`- tryFunc(*func);`
	`70`	+ 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.
	`71`	`+ runAsync([sharedThis, func=std::make_shared<std::function<void()>>(std::move(func))] {`
	`72`	`+ if (sharedThis->m_shutdown == false) {`
	`73`	`+ sharedThis->tryFunc(*func);`
`73`	`74`	`}`
`74`	`75`	`});`
`75`	`76`	`}`
`@@ -78,9 +79,11 @@`
`78`	`79`	`if (m_shutdown) {`
`79`	`80`	`return;`
`80`	`81`	`}`
`81`		`- runSync([this, func=std::move(func)] {`
`82`		`- if (!m_shutdown) {`
`83`		`- tryFunc(func);`
	`82`	`+`
	`83`	+ 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.
	`84`	`+ runSync([sharedThis, func=std::move(func)] {`
	`85`	`+ if (sharedThis->m_shutdown == false) {`
	`86`	`+ sharedThis->tryFunc(func);`
`84`	`87`	`}`
`85`	`88`	`});`
`86`	`89`	`}`