Revert "[vm/concurrency] Remove redundant isolates list, ensure shutdown procedure waits until the isolates actually got deleted"

This reverts commit 40eaf81834.

Reason for revert: This CL seems to cause an OOM error in Flutter tests, the Flutter HHH bot also shows failures starting with this CL. Please see https://github.com/dart-lang/sdk/issues/40627 which has an ASAN dump indicating a double free.

Original change's description:
> [vm/concurrency] Remove redundant isolates list, ensure shutdown procedure waits until the isolates actually got deleted
> 
> Issue https://github.com/dart-lang/sdk/issues/36097
> 
> Change-Id: If24affbb838eff8d80e5d448eac7455b3ffcb3a1
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/135062
> Commit-Queue: Martin Kustermann <kustermann@google.com>
> Reviewed-by: Alexander Aprelev <aam@google.com>

TBR=kustermann@google.com,aam@google.com,rmacnak@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: I7329a9cccc788e7ae2794639e0c76071fd4d9aa2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/135792
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Siva Annamalai <asiva@google.com>

runtime/vm/dart.cc

@@ -246,12 +246,12 @@ char* Dart::Init(const uint8_t* vm_isolate_snapshot,
     // really an isolate itself - it acts more as a container for VM-global
     // objects.
     std::unique_ptr<IsolateGroupSource> source(
-        new IsolateGroupSource(nullptr, kVmIsolateName, vm_isolate_snapshot,
+        new IsolateGroupSource(nullptr, "vm-isolate", vm_isolate_snapshot,
                                instructions_snapshot, nullptr, -1, api_flags));
     auto group = new IsolateGroup(std::move(source), /*embedder_data=*/nullptr);
     IsolateGroup::RegisterIsolateGroup(group);
     vm_isolate_ =
-        Isolate::InitIsolate(kVmIsolateName, group, api_flags, is_vm_isolate);
+        Isolate::InitIsolate("vm-isolate", group, api_flags, is_vm_isolate);
     group->set_initial_spawn_successful();
 
     // Verify assumptions about executing in the VM isolate.
@@ -405,43 +405,31 @@ char* Dart::Init(const uint8_t* vm_isolate_snapshot,
   return NULL;
 }
 
-static void DumpAliveIsolates(intptr_t num_attempts,
-                              bool only_aplication_isolates) {
-  IsolateGroup::ForEach([&](IsolateGroup* group) {
-    group->ForEachIsolate([&](Isolate* isolate) {
-      if (!only_aplication_isolates || !Isolate::IsVMInternalIsolate(isolate)) {
-        OS::PrintErr("Attempt:%" Pd " waiting for isolate %s to check in\n",
-                     num_attempts, isolate->name());
-      }
-    });
-  });
-}
-
-static bool OnlyVmIsolateLeft() {
-  intptr_t count = 0;
-  bool found_vm_isolate = false;
-  IsolateGroup::ForEach([&](IsolateGroup* group) {
-    group->ForEachIsolate([&](Isolate* isolate) {
-      count++;
-      if (isolate == Dart::vm_isolate()) {
-        found_vm_isolate = true;
-      }
-    });
-  });
-  return count == 1 && found_vm_isolate;
+bool Dart::HasApplicationIsolateLocked() {
+  for (Isolate* isolate = Isolate::isolates_list_head_; isolate != NULL;
+       isolate = isolate->next_) {
+    if (!Isolate::IsVMInternalIsolate(isolate)) return true;
+  }
+  return false;
 }
 
 // This waits until only the VM, service and kernel isolates are in the list.
 void Dart::WaitForApplicationIsolateShutdown() {
   ASSERT(!Isolate::creation_enabled_);
-  MonitorLocker ml(Isolate::isolate_creation_monitor_);
+  MonitorLocker ml(Isolate::isolates_list_monitor_);
   intptr_t num_attempts = 0;
-  while (Isolate::application_isolates_count_ > 1) {
+  while (HasApplicationIsolateLocked()) {
     Monitor::WaitResult retval = ml.Wait(1000);
     if (retval == Monitor::kTimedOut) {
       num_attempts += 1;
       if (num_attempts > 10) {
-        DumpAliveIsolates(num_attempts, /*only_application_isolates=*/true);
+        for (Isolate* isolate = Isolate::isolates_list_head_; isolate != NULL;
+             isolate = isolate->next_) {
+          if (!Isolate::IsVMInternalIsolate(isolate)) {
+            OS::PrintErr("Attempt:%" Pd " waiting for isolate %s to check in\n",
+                         num_attempts, isolate->name_);
+          }
+        }
       }
     }
   }
@@ -450,19 +438,22 @@ void Dart::WaitForApplicationIsolateShutdown() {
 // This waits until only the VM isolate remains in the list.
 void Dart::WaitForIsolateShutdown() {
   ASSERT(!Isolate::creation_enabled_);
-  MonitorLocker ml(Isolate::isolate_creation_monitor_);
+  MonitorLocker ml(Isolate::isolates_list_monitor_);
   intptr_t num_attempts = 0;
-  while (Isolate::total_isolates_count_ > 1) {
+  while ((Isolate::isolates_list_head_ != NULL) &&
+         (Isolate::isolates_list_head_->next_ != NULL)) {
     Monitor::WaitResult retval = ml.Wait(1000);
     if (retval == Monitor::kTimedOut) {
       num_attempts += 1;
       if (num_attempts > 10) {
-        DumpAliveIsolates(num_attempts, /*only_application_isolates=*/false);
+        for (Isolate* isolate = Isolate::isolates_list_head_; isolate != NULL;
+             isolate = isolate->next_)
+          OS::PrintErr("Attempt:%" Pd " waiting for isolate %s to check in\n",
+                       num_attempts, isolate->name_);
       }
     }
   }
-
-  ASSERT(OnlyVmIsolateLeft());
+  ASSERT(Isolate::isolates_list_head_ == Dart::vm_isolate());
 }
 
 char* Dart::Cleanup() {
@@ -948,22 +939,11 @@ void Dart::ShutdownIsolate(Isolate* isolate) {
 
 void Dart::ShutdownIsolate() {
   Isolate* isolate = Isolate::Current();
-  const bool is_application_isolate = !Isolate::IsVMInternalIsolate(isolate);
   isolate->Shutdown();
   if (KernelIsolate::IsKernelIsolate(isolate)) {
     KernelIsolate::SetKernelIsolate(NULL);
   }
   delete isolate;
-
-  // Only now do we know for sure that the isolate and all it's resources have
-  // been deleted. So we can let any potential Dart::Cleanup() know it's safe to
-  // proceed shutdown of the VM.
-  Isolate::MarkIsolateDead(is_application_isolate);
-}
-
-bool Dart::VmIsolateNameEquals(const char* name) {
-  ASSERT(name != NULL);
-  return (strcmp(name, kVmIsolateName) == 0);
 }
 
 int64_t Dart::UptimeMicros() {

runtime/vm/dart.h

@@ -66,7 +66,6 @@ class Dart : public AllStatic {
 
   static Isolate* vm_isolate() { return vm_isolate_; }
   static ThreadPool* thread_pool() { return thread_pool_; }
-  static bool VmIsolateNameEquals(const char* name);
 
   static int64_t UptimeMicros();
   static int64_t UptimeMillis() {
@@ -130,10 +129,9 @@ class Dart : public AllStatic {
   static bool non_nullable_flag() { return true; }
 
  private:
-  static constexpr const char* kVmIsolateName = "vm-isolate";
-
   static void WaitForIsolateShutdown();
   static void WaitForApplicationIsolateShutdown();
+  static bool HasApplicationIsolateLocked();
 
   static Isolate* vm_isolate_;
   static int64_t start_time_micros_;

runtime/vm/isolate.cc

@@ -1380,10 +1380,10 @@ void Isolate::InitVM() {
   shutdown_callback_ = nullptr;
   cleanup_callback_ = nullptr;
   cleanup_group_callback_ = nullptr;
-  if (isolate_creation_monitor_ == nullptr) {
-    isolate_creation_monitor_ = new Monitor();
+  if (isolates_list_monitor_ == nullptr) {
+    isolates_list_monitor_ = new Monitor();
   }
-  ASSERT(isolate_creation_monitor_ != nullptr);
+  ASSERT(isolates_list_monitor_ != nullptr);
   EnableIsolateCreation();
 }
 
@@ -1392,7 +1392,6 @@ Isolate* Isolate::InitIsolate(const char* name_prefix,
                               const Dart_IsolateFlags& api_flags,
                               bool is_vm_isolate) {
   Isolate* result = new Isolate(isolate_group, api_flags);
-  result->BuildName(name_prefix);
   ASSERT(result != nullptr);
 
 #if !defined(PRODUCT)
@@ -1459,6 +1458,7 @@ Isolate* Isolate::InitIsolate(const char* name_prefix,
   result->set_pause_capability(result->random()->NextUInt64());
   result->set_terminate_capability(result->random()->NextUInt64());
 
+  result->BuildName(name_prefix);
 #if !defined(PRODUCT)
   result->debugger_ = new Debugger(result);
 #endif
@@ -1476,7 +1476,7 @@ Isolate* Isolate::InitIsolate(const char* name_prefix,
 #endif  // !PRODUCT
 
   // Add to isolate list. Shutdown and delete the isolate on failure.
-  if (!TryMarkIsolateReady(result)) {
+  if (!AddIsolateToList(result)) {
     result->LowLevelShutdown();
     Thread::ExitIsolate();
     if (KernelIsolate::IsKernelIsolate(result)) {
@@ -2224,7 +2224,12 @@ void Isolate::Shutdown() {
   // Don't allow anymore dart code to execution on this isolate.
   thread->ClearStackLimit();
 
+  // Remove this isolate from the list *before* we start tearing it down, to
+  // avoid exposing it in a state of decay.
+  RemoveIsolateFromList(this);
+
   {
+    // After removal from isolate list. Before tearing down the heap.
     StackZone zone(thread);
     HandleScope handle_scope(thread);
     ServiceIsolate::SendIsolateShutdownMessage();
@@ -2260,7 +2265,6 @@ void Isolate::Shutdown() {
 #endif  // !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME)
 
   // Then, proceed with low-level teardown.
-  Isolate::UnMarkIsolateReady(this);
   LowLevelShutdown();
 
 #if defined(DEBUG)
@@ -2291,9 +2295,8 @@ Dart_IsolateShutdownCallback Isolate::shutdown_callback_ = nullptr;
 Dart_IsolateCleanupCallback Isolate::cleanup_callback_ = nullptr;
 Dart_IsolateGroupCleanupCallback Isolate::cleanup_group_callback_ = nullptr;
 
-Monitor* Isolate::isolate_creation_monitor_ = nullptr;
-intptr_t Isolate::application_isolates_count_ = 0;
-intptr_t Isolate::total_isolates_count_ = 0;
+Monitor* Isolate::isolates_list_monitor_ = nullptr;
+Isolate* Isolate::isolates_list_head_ = nullptr;
 bool Isolate::creation_enabled_ = false;
 
 RwLock* IsolateGroup::isolate_groups_rwlock_ = nullptr;
@@ -3023,99 +3026,113 @@ void Isolate::VisitIsolates(IsolateVisitor* visitor) {
   if (visitor == nullptr) {
     return;
   }
-  IsolateGroup::ForEach([&](IsolateGroup* group) {
-    group->ForEachIsolate(
-        [&](Isolate* isolate) { visitor->VisitIsolate(isolate); });
-  });
+  // The visitor could potentially run code that could safepoint so use
+  // SafepointMonitorLocker to ensure the lock has safepoint checks.
+  SafepointMonitorLocker ml(isolates_list_monitor_);
+  Isolate* current = isolates_list_head_;
+  while (current != nullptr) {
+    visitor->VisitIsolate(current);
+    current = current->next_;
+  }
 }
 
 intptr_t Isolate::IsolateListLength() {
-  MonitorLocker ml(isolate_creation_monitor_);
-  return total_isolates_count_;
+  MonitorLocker ml(isolates_list_monitor_);
+  intptr_t count = 0;
+  Isolate* current = isolates_list_head_;
+  while (current != nullptr) {
+    count++;
+    current = current->next_;
+  }
+  return count;
 }
 
 Isolate* Isolate::LookupIsolateByPort(Dart_Port port) {
-  Isolate* match = nullptr;
-  IsolateGroup::ForEach([&](IsolateGroup* group) {
-    group->ForEachIsolate([&](Isolate* isolate) {
-      if (isolate->main_port() == port) {
-        match = isolate;
-      }
-    });
-  });
-  return match;
+  MonitorLocker ml(isolates_list_monitor_);
+  Isolate* current = isolates_list_head_;
+  while (current != nullptr) {
+    if (current->main_port() == port) {
+      return current;
+    }
+    current = current->next_;
+  }
+  return nullptr;
 }
 
 std::unique_ptr<char[]> Isolate::LookupIsolateNameByPort(Dart_Port port) {
-  MonitorLocker ml(isolate_creation_monitor_);
-  std::unique_ptr<char[]> result;
-  IsolateGroup::ForEach([&](IsolateGroup* group) {
-    group->ForEachIsolate([&](Isolate* isolate) {
-      if (isolate->main_port() == port) {
-        const size_t len = strlen(isolate->name()) + 1;
-        result = std::unique_ptr<char[]>(new char[len]);
-        strncpy(result.get(), isolate->name(), len);
-      }
-    });
-  });
-  return result;
+  MonitorLocker ml(isolates_list_monitor_);
+  Isolate* current = isolates_list_head_;
+  while (current != nullptr) {
+    if (current->main_port() == port) {
+      const size_t len = strlen(current->name()) + 1;
+      auto result = std::unique_ptr<char[]>(new char[len]);
+      strncpy(result.get(), current->name(), len);
+      return result;
+    }
+    current = current->next_;
+  }
+  return std::unique_ptr<char[]>();
 }
 
-bool Isolate::TryMarkIsolateReady(Isolate* isolate) {
-  MonitorLocker ml(isolate_creation_monitor_);
+bool Isolate::AddIsolateToList(Isolate* isolate) {
+  MonitorLocker ml(isolates_list_monitor_);
   if (!creation_enabled_) {
     return false;
   }
-  total_isolates_count_++;
-  if (!Isolate::IsVMInternalIsolate(isolate)) {
-    application_isolates_count_++;
-  }
-  isolate->accepts_messages_ = true;
+  ASSERT(isolate != nullptr);
+  ASSERT(isolate->next_ == nullptr);
+  isolate->next_ = isolates_list_head_;
+  isolates_list_head_ = isolate;
   return true;
 }
 
-void Isolate::UnMarkIsolateReady(Isolate* isolate) {
-  MonitorLocker ml(isolate_creation_monitor_);
-  ASSERT(total_isolates_count_ > 0);
-  isolate->accepts_messages_ = false;
-}
-
-void Isolate::MarkIsolateDead(bool is_application_isolate) {
-  MonitorLocker ml(isolate_creation_monitor_);
-  ASSERT(total_isolates_count_ > 0);
-  total_isolates_count_--;
-  if (is_application_isolate) {
-    ASSERT(application_isolates_count_ > 0);
-    application_isolates_count_--;
+void Isolate::RemoveIsolateFromList(Isolate* isolate) {
+  MonitorLocker ml(isolates_list_monitor_);
+  ASSERT(isolate != nullptr);
+  if (isolate == isolates_list_head_) {
+    isolates_list_head_ = isolate->next_;
+    if (!creation_enabled_) {
+      ml.Notify();
+    }
+    return;
   }
-  if (!creation_enabled_) {
-    ml.Notify();
+  Isolate* previous = nullptr;
+  Isolate* current = isolates_list_head_;
+  while (current != nullptr) {
+    if (current == isolate) {
+      ASSERT(previous != nullptr);
+      previous->next_ = current->next_;
+      if (!creation_enabled_) {
+        ml.Notify();
+      }
+      return;
+    }
+    previous = current;
+    current = current->next_;
   }
+  // If we are shutting down the VM, the isolate may not be in the list.
+  ASSERT(!creation_enabled_);
 }
 
 void Isolate::DisableIsolateCreation() {
-  MonitorLocker ml(isolate_creation_monitor_);
+  MonitorLocker ml(isolates_list_monitor_);
   creation_enabled_ = false;
 }
 
 void Isolate::EnableIsolateCreation() {
-  MonitorLocker ml(isolate_creation_monitor_);
+  MonitorLocker ml(isolates_list_monitor_);
   creation_enabled_ = true;
 }
 
 bool Isolate::IsolateCreationEnabled() {
-  MonitorLocker ml(isolate_creation_monitor_);
+  MonitorLocker ml(isolates_list_monitor_);
   return creation_enabled_;
 }
 
 bool Isolate::IsVMInternalIsolate(const Isolate* isolate) {
-  // We use a name comparison here because this method can be called during
-  // shutdown, where the actual isolate pointers might've already been cleared.
-  return Dart::VmIsolateNameEquals(isolate->name()) ||
-#if !defined(DART_PRECOMPILED_RUNTIME)
-         KernelIsolate::NameEquals(isolate->name()) ||
-#endif
-         ServiceIsolate::NameEquals(isolate->name());
+  return (isolate == Dart::vm_isolate()) ||
+         ServiceIsolate::IsServiceIsolateDescendant(isolate) ||
+         KernelIsolate::IsKernelIsolate(isolate);
 }
 
 void Isolate::KillLocked(LibMsgId msg_id) {
@@ -3172,9 +3189,7 @@ class IsolateKillerVisitor : public IsolateVisitor {
   void VisitIsolate(Isolate* isolate) {
     ASSERT(isolate != nullptr);
     if (ShouldKill(isolate)) {
-      if (isolate->AcceptsMessagesLocked()) {
-        isolate->KillLocked(msg_id_);
-      }
+      isolate->KillLocked(msg_id_);
     }
   }
 
@@ -3191,15 +3206,11 @@ class IsolateKillerVisitor : public IsolateVisitor {
 };
 
 void Isolate::KillAllIsolates(LibMsgId msg_id) {
-  MonitorLocker ml(isolate_creation_monitor_);
-
   IsolateKillerVisitor visitor(msg_id);
   VisitIsolates(&visitor);
 }
 
 void Isolate::KillIfExists(Isolate* isolate, LibMsgId msg_id) {
-  MonitorLocker ml(isolate_creation_monitor_);
-
   IsolateKillerVisitor visitor(isolate, msg_id);
   VisitIsolates(&visitor);
 }

runtime/vm/isolate.h

@@ -1087,7 +1087,7 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
                               const Dart_IsolateFlags& api_flags,
                               bool is_vm_isolate = false);
 
-  // The isolate_creation_monitor_ should be held when calling Kill().
+  // The isolates_list_monitor_ should be held when calling Kill().
   void KillLocked(LibMsgId msg_id);
 
   void LowLevelShutdown();
@@ -1277,6 +1277,9 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
 
   RawError* sticky_error_;
 
+  // Isolate list next pointer.
+  Isolate* next_ = nullptr;
+
   // Protect access to boxed_field_list_.
   Mutex field_list_mutex_;
   // List of fields that became boxed and that trigger deoptimization.
@@ -1300,11 +1303,6 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
   std::unique_ptr<WeakTable> forward_table_new_;
   std::unique_ptr<WeakTable> forward_table_old_;
 
-  // Signals whether the isolate can receive messages (e.g. KillAllIsolates can
-  // send a kill message).
-  // This is protected by [isolate_creation_monitor_].
-  bool accepts_messages_ = false;
-
   static Dart_IsolateGroupCreateCallback create_group_callback_;
   static Dart_InitializeIsolateCallback initialize_callback_;
   static Dart_IsolateShutdownCallback shutdown_callback_;
@@ -1316,19 +1314,12 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
 #endif
 
   // Manage list of existing isolates.
-  static bool TryMarkIsolateReady(Isolate* isolate);
-  static void UnMarkIsolateReady(Isolate* isolate);
-  static void MarkIsolateDead(bool is_application_isolate);
-  bool AcceptsMessagesLocked() {
-    ASSERT(isolate_creation_monitor_->IsOwnedByCurrentThread());
-    return accepts_messages_;
-  }
+  static bool AddIsolateToList(Isolate* isolate);
+  static void RemoveIsolateFromList(Isolate* isolate);
 
-  // This monitor protects application_isolates_count_, total_isolates_count_,
-  // creation_enabled_.
-  static Monitor* isolate_creation_monitor_;
-  static intptr_t application_isolates_count_;
-  static intptr_t total_isolates_count_;
+  // This monitor protects isolates_list_head_, and creation_enabled_.
+  static Monitor* isolates_list_monitor_;
+  static Isolate* isolates_list_head_;
   static bool creation_enabled_;
 
 #define REUSABLE_FRIEND_DECLARATION(name)                                      \

runtime/vm/service_isolate.cc

@@ -363,6 +363,7 @@ class RunServiceTask : public ThreadPool::Task {
 
       free(error);
       error = nullptr;
+      ServiceIsolate::SetServiceIsolate(NULL);
       ServiceIsolate::InitializingFailed(formatted_error);
       return;
     }
@@ -418,6 +419,9 @@ class RunServiceTask : public ThreadPool::Task {
       }
       Dart::RunShutdownCallback();
     }
+    ASSERT(ServiceIsolate::IsServiceIsolate(I));
+    ServiceIsolate::SetServiceIsolate(NULL);
+    ServiceIsolate::SetServicePort(ILLEGAL_PORT);
 
     // Shut the isolate down.
     Dart::ShutdownIsolate(I);