Fix ThreadSanitizer problem with debugger tests

Summary:
Run with ThreadSanitizer gives us additional confidence on thread
safety on top of the compile time check of Thread-Safety Analysis.

Reviewed By: mattbfb

Differential Revision: D53628862

fbshipit-source-id: 5657f00b0c6d40b1c3701338ffa7b0d089ef7b0f

unittests/API/AsyncDebuggerAPITest.cpp

@@ -315,6 +315,11 @@ TEST_F(AsyncDebuggerAPITest, ResumeFromPausedTest) {
   EXPECT_EQ(finalEvent, DebuggerEventType::Resumed);
 }
 
+// This test seemingly has a data race because it calls resumeFromPaused() from
+// a non-runtime thread. But that's being done to ensure the only thing being
+// signaled is due to DebuggerEventCallback being removed. So disable this test
+// when running with ThreadSanitizer.
+#if !defined(__has_feature) || !__has_feature(thread_sanitizer)
 TEST_F(AsyncDebuggerAPITest, NotifyDueToEventCallbacksTest) {
   // This needs to be a while-loop because Explicit AsyncBreak will only happen
   // while there is JS to run
@@ -365,6 +370,7 @@ TEST_F(AsyncDebuggerAPITest, NotifyDueToEventCallbacksTest) {
 
   asyncDebuggerAPI_->removeDebuggerEventCallback_TS(callbackID);
 }
+#endif
 
 TEST_F(AsyncDebuggerAPITest, NoDebuggerEventCallbackTest) {
   scheduleScript("debugger;");

unittests/API/CDPAgentTest.cpp

@@ -511,12 +511,18 @@ TEST_F(CDPAgentTest, TestEnableWhenAlreadyPaused) {
             [promise](
                 HermesRuntime &runtime,
                 AsyncDebuggerAPI &asyncDebugger,
-                DebuggerEventType event) { promise->set_value(true); });
+                DebuggerEventType event) {
+              if (event == DebuggerEventType::ExplicitPause) {
+                promise->set_value(true);
+              }
+            });
         runtime_->getDebugger().triggerAsyncPause(AsyncPauseKind::Explicit);
       },
       "wait on explicit pause");
 
-  // At this point, the runtime thread is paused due to Explicit AsyncBreak
+  // At this point, the runtime thread is paused due to Explicit AsyncBreak. Now
+  // we'll test if we can perform Debugger.enable while the runtime is in that
+  // state.
 
   sendAndCheckResponse("Debugger.enable", msgId++);
   ensureNotification(
@@ -529,16 +535,17 @@ TEST_F(CDPAgentTest, TestEnableWhenAlreadyPaused) {
       "other",
       {FrameInfo("global", 0, 1).setLineNumberMax(9)});
 
-  // At this point we know the runtime thread is paused, so it should be safe to
-  // call resumeFromPaused() directly.
-  asyncDebuggerAPI_->resumeFromPaused(AsyncDebugCommand::Continue);
-
   // After removing this callback, AsyncDebuggerAPI will still have another
   // callback registered by CDPAgent. Therefore, JS will not continue by itself.
-  // But because of the resumeFromPaused() in the previous line, removing the
-  // callback should wake up the runtime thread and check that a next command
-  // has been set.
   asyncDebuggerAPI_->removeDebuggerEventCallback_TS(eventCallbackID);
+  // Have to manually resume it:
+  waitFor<bool>([this](auto promise) {
+    asyncDebuggerAPI_->triggerInterrupt_TS(
+        [this, promise](HermesRuntime &runtime) {
+          asyncDebuggerAPI_->resumeFromPaused(AsyncDebugCommand::Continue);
+          promise->set_value(true);
+        });
+  });
 
   ensureNotification(waitForMessage("Debugger.resumed"), "Debugger.resumed");