[vm/concurrency] Fix inconsistency in Dart's spawn implementation

When Dart invokes Isolate.spawn the Dart VM calls out to the embedder to
create an isolate (group) or initialize it (in case of lightweight
isolates).

  * If the embedder doesn't implement this functionality or fails to
    perform the creation/initialization, it will signal this failure
    via the return value it gives to the VM in the call back
    implementation (i.e. Dart_InitializeParams.create_group and
    Dart_InitializeParams.initialize_isolate}).

  * If the embeder sucessfully completed it's work, the VM owns the
    isolate after the embedder call returns. The VM is then responsible
    for running it.

It is an undocumented invariant atm that the embedder is responsible
to make the isolate runnable if it signals the VM that isolate was
sucessfully created/initialized.
  => If it does not do that, we have effectively an isolate leak.
  => Though right now seemingly all of our embedders to that correctly.

To avoid this unintentional isolate leak we make the isolate runnable
ourselves if embedder callback was successful but did not make it
runnable itself.

  => This also avoids unnecessary ceremony code in the embedders that
     look like this:

      Dart_ExitIsolate();
      Dart_IsolateMakeRunnable(child);
      Dart_EnterIsolate(child);

The implementation of `Dart_IsolateMakeRunnable()` had an untested and
unused code path that caused running the isolate (on a thread pool).
This is not documented and can lead to bugs because often the
embedders enter the isolate right after making it runnable (e.g. above
code sequence) - which would be racing with the spawned message handler
thread.

  => Furtunately our embedders are well behaved and make the isolate
     runnable during the callback.
  => As a safeguard we'll make `Dart_IsolateMakeRunnable()` return an
     error if it's called outside the callback (which we can detect by
     the presense of a spawn state)

Issue #44088
Issue #36097

TEST=Changes internal details of implementation which is already covered by tests.

Change-Id: Ieef316cc0807d99f2fcb1fbd56bbc9c41e904ba8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/170882
Commit-Queue: Martin Kustermann <[email protected]>
Reviewed-by: Alexander Aprelev <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>

Author: mkustermann

runtime/bin/main.cc

@@ -245,12 +245,8 @@ static bool OnIsolateInitialize(void** child_callback_data, char** error) {
     if (Dart_IsError(result)) goto failed;
   }
 
-  // Make the isolate runnable so that it is ready to handle messages.
   Dart_ExitScope();
-  Dart_ExitIsolate();
-  *error = Dart_IsolateMakeRunnable(isolate);
-  Dart_EnterIsolate(isolate);
-  return *error == nullptr;
+  return true;
 
 failed:
   *error = Utils::StrDup(Dart_GetError(result));

runtime/lib/isolate.cc

@@ -352,6 +352,13 @@ class SpawnIsolateTask : public ThreadPool::Task {
       return;
     }
 
+    if (isolate->object_store()->root_library() == Library::null()) {
+      Dart_ShutdownIsolate();
+      FailedSpawn(
+          "The embedder has to ensure there is a root library (e.g. by calling "
+          "Dart_LoadScriptFromKernel).");
+    }
+
     Run(isolate);
   }
 
@@ -401,15 +408,19 @@ class SpawnIsolateTask : public ThreadPool::Task {
     state_->set_isolate(child);
 
     MutexLocker ml(child->mutex());
-    child->set_origin_id(state_->origin_id());
-    child->set_spawn_state(std::move(state_));
 
-    // If the isolate is not marked as runnable, then the embedder might do so
-    // later on and the launch of the isolate will happen inside
-    // `Dart_IsolateMakeRunnable`.
-    if (child->is_runnable()) {
-      child->Run();
+    // We called out to the embedder to create/initialize a new isolate. The
+    // embedder callback sucessfully did so. It is now our responsibility to
+    // run the isolate.
+    // If the isolate was not marked as runnable, we'll do so here and run it
+    if (!child->is_runnable()) {
+      child->MakeRunnableLocked();
     }
+    ASSERT(child->is_runnable());
+
+    child->set_origin_id(state_->origin_id());
+    child->set_spawn_state(std::move(state_));
+    child->Run();
   }
 
   void FailedSpawn(const char* error) {

runtime/vm/dart_api_impl.cc

@@ -2001,18 +2001,11 @@ DART_EXPORT char* Dart_IsolateMakeRunnable(Dart_Isolate isolate) {
     FATAL1("%s expects argument 'isolate' to be non-null.", CURRENT_FUNC);
   }
   // TODO(16615): Validate isolate parameter.
-  Isolate* iso = reinterpret_cast<Isolate*>(isolate);
-  const char* error;
-  if (iso->object_store()->root_library() == Library::null()) {
-    // The embedder should have called Dart_LoadScriptFromKernel by now.
-    error = "Missing root library";
-  } else {
-    error = iso->MakeRunnable();
-  }
-  if (error != NULL) {
+  const char* error = reinterpret_cast<Isolate*>(isolate)->MakeRunnable();
+  if (error != nullptr) {
     return Utils::StrDup(error);
   }
-  return NULL;
+  return nullptr;
 }
 
 // --- Messages and Ports ---

runtime/vm/isolate.cc

@@ -2055,9 +2055,26 @@ const char* Isolate::MakeRunnable() {
   if (is_runnable() == true) {
     return "Isolate is already runnable";
   }
+  if (spawn_state() != nullptr) {
+    return "The embedder has to make the isolate runnable during isolate "
+           "creation / initialization callback.";
+  }
+  if (object_store()->root_library() == Library::null()) {
+    return "The embedder has to ensure there is a root library (e.g. by "
+           "calling Dart_LoadScriptFromKernel ).";
+  }
+  MakeRunnableLocked();
+  return nullptr;
+}
+
+void Isolate::MakeRunnableLocked() {
+  ASSERT(mutex_.IsOwnedByCurrentThread());
+  ASSERT(!is_runnable());
+  ASSERT(spawn_state() == nullptr);
+  ASSERT(object_store()->root_library() != Library::null());
+
   // Set the isolate as runnable and if we are being spawned schedule
   // isolate on thread pool for execution.
-  ASSERT(object_store()->root_library() != Library::null());
   set_is_runnable(true);
 #ifndef PRODUCT
   if (!Isolate::IsSystemIsolate(this)) {
@@ -2066,16 +2083,6 @@ const char* Isolate::MakeRunnable() {
     }
   }
 #endif  // !PRODUCT
-  IsolateSpawnState* state = spawn_state();
-  if (state != nullptr) {
-    // If the embedder does not make the isolate runnable during the
-    // `create_isolate_group`/`initialize_isolate` embedder callbacks but rather
-    // some time in the future, we'll hit this case.
-    // WARNING: This is currently untested - we might consider changing our APIs
-    // to disallow two different flows.
-    ASSERT(this == state->isolate());
-    Run();
-  }
 #if defined(SUPPORT_TIMELINE)
   TimelineStream* stream = Timeline::GetIsolateStream();
   ASSERT(stream != nullptr);
@@ -2092,7 +2099,6 @@ const char* Isolate::MakeRunnable() {
   }
   GetRunnableLatencyMetric()->set_value(UptimeMicros());
 #endif  // !PRODUCT
-  return nullptr;
 }
 
 bool Isolate::VerifyPauseCapability(const Object& capability) const {

runtime/vm/isolate.h

@@ -923,6 +923,7 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
   void ScheduleInterrupts(uword interrupt_bits);
 
   const char* MakeRunnable();
+  void MakeRunnableLocked();
   void Run();
 
   MessageHandler* message_handler() const { return message_handler_; }