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

runtime/vm/debugger_api_impl_test.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "include/dart_mirrors_api.h"
 #include "include/dart_tools_api.h"
 #include "platform/assert.h"
 #include "vm/dart_api_impl.h"
 #include "vm/lockers.h"
 #include "vm/unit_test.h"
@@ skipping 1428 matching lines @@
   Dart_ExitScope();
   Dart_ShutdownIsolate();
   Dart_SetIsolateEventHandler(NULL);
   EXPECT(verify_callback == 0x5);  // Only created and shutdown events.
 }
 
 
 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;
 
 
 static void InterruptIsolateHandler(Dart_IsolateId isolateId,
                                     intptr_t breakpointId,
                                     const Dart_CodeLocation& location) {
   MonitorLocker ml(sync);
   pause_event_handled = true;
   ml.Notify();
@@ skipping 67 matching lines @@
   LoadScript(kScriptChars);
 
   Dart_Handle result =
       Dart_SetNativeResolver(script_lib, &InterruptNativeResolver, NULL);
   EXPECT_VALID(result);
 
   Dart_Handle retval = Invoke("main");
   EXPECT_VALID(retval);
   Dart_ExitScope();
   Dart_ShutdownIsolate();
+  {
+    // Notify the waiting thread that we are done.
+    MonitorLocker ml(sync);
+    interrupt_thread_stopped = true;
+    ml.Notify();
+  }
 }
 
 
 TEST_CASE(Debug_InterruptIsolate) {
   bool saved_flag = FLAG_trace_shutdown;
   FLAG_trace_shutdown = true;
   sync = new Monitor();
   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);
 
   // Wait for the test isolate to be created.
   {
     OS::PrintErr("Waiting for isolate to be created\n");
     MonitorLocker ml(sync);
     while (interrupt_isolate_id == ILLEGAL_ISOLATE_ID) {
       ml.Wait();
     }
@@ skipping 16 matching lines @@
   EXPECT(pause_event_handled);
 
   // Wait for the test isolate to shutdown.
   {
     OS::PrintErr("Waiting for isolate to be shut down\n");
     MonitorLocker ml(sync);
     while (interrupt_isolate_id != ILLEGAL_ISOLATE_ID) {
       ml.Wait();
     }
   }
   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;
 }
 
 
 static void StackTraceDump1BreakpointHandler(
     Dart_IsolateId isolate_id,
     intptr_t bp_id,
     const Dart_CodeLocation& location) {
   Dart_StackTrace trace;
@@ skipping 741 matching lines @@
       " null, 5, 17, 5, 18, 9, 19, 12,"
       " null, 6, 21, 1,"
       " null, 8, 24, 1, 25, 5, 26, 6, 27, 8,"
       " null, 9, 29, 1]",
       tokens_cstr);
 }
 
 #endif
 
 }  // namespace dart