Fix for issue 28606 - Wait for thread that runs the interrupted isolate to exit completely before e…

runtime/vm/debugger_api_impl_test.cc

Index: runtime/vm/debugger_api_impl_test.cc
diff --git a/runtime/vm/debugger_api_impl_test.cc b/runtime/vm/debugger_api_impl_test.cc
index beb26e467b270b470d7b8116732727c5de4ef278..534757edb2ef1f687080178cbd5a51b9c975802f 100644
--- a/runtime/vm/debugger_api_impl_test.cc
+++ b/runtime/vm/debugger_api_impl_test.cc
@@ -1446,6 +1446,7 @@ VM_UNIT_TEST_CASE(Debug_IsolateID) {
 static Monitor* sync = NULL;
 static bool isolate_interrupted = false;
 static bool pause_event_handled = false;
+static bool interrupt_thread_stopped = false;
 static Dart_IsolateId interrupt_isolate_id = ILLEGAL_ISOLATE_ID;
 static volatile bool continue_isolate_loop = true;
 
@@ -1533,6 +1534,12 @@ static void InterruptIsolateRun(uword unused) {
   EXPECT_VALID(retval);
   Dart_ExitScope();
   Dart_ShutdownIsolate();
+  {
+    // Notify the waiting thread that we are done.
+    MonitorLocker ml(sync);
+    interrupt_thread_stopped = true;
+    ml.Notify();
+  }
 }
 
 
@@ -1543,6 +1550,7 @@ TEST_CASE(Debug_InterruptIsolate) {
   Dart_SetIsolateEventHandler(&TestInterruptIsolate);
   EXPECT(interrupt_isolate_id == ILLEGAL_ISOLATE_ID);
   Dart_SetPausedEventHandler(InterruptIsolateHandler);
+  interrupt_thread_stopped = false;

[Florian Schneider, 2017/02/06 21:02:28]

Could the other thread see an uninitialized value here? To be safe I'd also wrap
this access with the locker:

{
  MonitorLocker ml(sync);
  interrupt_thread_stopped = false;
}

[siva, 2017/02/06 21:17:41]

The other thread starts after this assignment so should be ok, but I have added
the lock to be safe and ensure there are no memory barrier issues.

   int result = OSThread::Start("DebugInterruptIsolate", InterruptIsolateRun, 0);
   EXPECT_EQ(0, result);
 
@@ -1580,6 +1588,17 @@ TEST_CASE(Debug_InterruptIsolate) {
     }
   }
   EXPECT(interrupt_isolate_id == ILLEGAL_ISOLATE_ID);

[hausner, 2017/02/04 06:18:37]

I guess we could eliminate waiting for the isolate to be shutdown and move this
EXPECT after the wait loop below (after isolate has been shut down).

[siva, 2017/02/06 20:15:16]

The test wants to test the three stages that the handler reacts to (start of an
isolate, interrupting an isolate while it is running, stopping an isolate). So
we should retain the shutdown part as it tests the shutdown handler callback
mechanism.

+
+  // Wait for the OSThread that we started above, if we do
+  // not wait we end up with a race between the process
+  // exiting and cleaning up while the thread above is cleaning
+  // up stuff from the isolate leading to flaky crashes.
+  {
+    MonitorLocker ml(sync);
+    while (!interrupt_thread_stopped) {
+      ml.Wait();
+    }
+  }
   OS::PrintErr("Complete\n");
   FLAG_trace_shutdown = saved_flag;
 }