From 76c25739858e7a927929ba6049ecc2ed0ef19159 Mon Sep 17 00:00:00 2001
From: Ryan Macnak <rmacnak@google.com>
Date: Tue, 3 Dec 2024 23:24:48 +0000
Subject: [PATCH] [vm, isolates] Fix deletion of handles when not entered into
 an isolate group.

Handles may only be allocated/updated/deleted by a thread whose execution state prevents GC from running.

TEST=tsan
Change-Id: Id2853e30d326642acc572dbd1c03345aaefd0f45
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/398606
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>
---
 runtime/lib/isolate.cc      | 21 ++++++++++++---------
 runtime/vm/dart_api_state.h |  8 +++++++-
 runtime/vm/thread.cc        | 18 ------------------
 runtime/vm/thread.h         |  3 +--
 4 files changed, 20 insertions(+), 30 deletions(-)

diff --git a/runtime/lib/isolate.cc b/runtime/lib/isolate.cc
index 9ced70beded..f6279466b8d 100644
--- a/runtime/lib/isolate.cc
+++ b/runtime/lib/isolate.cc
@@ -581,9 +581,6 @@ class IsolateSpawnState {
                     IsolateGroup* group);
   ~IsolateSpawnState();
 
-  Isolate* isolate() const { return isolate_; }
-  void set_isolate(Isolate* value) { isolate_ = value; }
-
   Dart_Port parent_port() const { return parent_port_; }
   Dart_Port origin_id() const { return origin_id_; }
   Dart_Port on_exit_port() const { return on_exit_port_; }
@@ -608,7 +605,6 @@ class IsolateSpawnState {
   IsolateGroup* isolate_group() const { return isolate_group_; }
 
  private:
-  Isolate* isolate_ = nullptr;
   Dart_Port parent_port_;
   Dart_Port origin_id_ = ILLEGAL_PORT;
   Dart_Port on_exit_port_;
@@ -880,12 +876,14 @@ class SpawnIsolateTask : public ThreadPool::Task {
       return;
     }
 
-    state_->set_isolate(child);
     if (state_->origin_id() != ILLEGAL_PORT) {
       // origin_id is set to parent isolate main port id when spawning via
       // spawnFunction.
       child->set_origin_id(state_->origin_id());
     }
+    bool errors_are_fatal = state_->errors_are_fatal();
+    Dart_Port on_error_port = state_->on_error_port();
+    Dart_Port on_exit_port = state_->on_exit_port();
 
     bool success = true;
     {
@@ -895,18 +893,22 @@ class SpawnIsolateTask : public ThreadPool::Task {
       HandleScope hs(thread);
 
       success = EnqueueEntrypointInvocationAndNotifySpawner(thread);
+
+      // Destruction of [IsolateSpawnState] may cause destruction of [Message]
+      // which make need to delete persistent handles, so explicitly delete it
+      // now while we are in the right safepoint state.
+      state_ = nullptr;
     }
 
     if (!success) {
-      state_ = nullptr;
       Dart_ShutdownIsolate();
       return;
     }
 
     // All preconditions are met for this to always succeed.
     char* error = nullptr;
-    if (!Dart_RunLoopAsync(state_->errors_are_fatal(), state_->on_error_port(),
-                           state_->on_exit_port(), &error)) {
+    if (!Dart_RunLoopAsync(errors_are_fatal, on_error_port, on_exit_port,
+                           &error)) {
       FATAL("Dart_RunLoopAsync() failed: %s. Please file a Dart VM bug report.",
             error);
     }
@@ -1030,12 +1032,13 @@ class SpawnIsolateTask : public ThreadPool::Task {
     // isolate group).
     if (has_current_isolate) {
       ASSERT(IsolateGroup::Current() == state_->isolate_group());
+      TransitionNativeToVM transition(Thread::Current());
       state_ = nullptr;
     } else if (state_->isolate_group() != nullptr) {
       ASSERT(IsolateGroup::Current() == nullptr);
       const bool kBypassSafepoint = false;
       const bool result = Thread::EnterIsolateGroupAsHelper(
-          state_->isolate_group(), Thread::kUnknownTask, kBypassSafepoint);
+          state_->isolate_group(), Thread::kSpawnTask, kBypassSafepoint);
       ASSERT(result);
       state_ = nullptr;
       Thread::ExitIsolateGroupAsHelper(kBypassSafepoint);
diff --git a/runtime/vm/dart_api_state.h b/runtime/vm/dart_api_state.h
index 6ada5021a48..544a950cff8 100644
--- a/runtime/vm/dart_api_state.h
+++ b/runtime/vm/dart_api_state.h
@@ -178,7 +178,13 @@ class PersistentHandle {
     ptr_ = static_cast<ObjectPtr>(reinterpret_cast<uword>(free_list));
     ASSERT(!ptr_->IsHeapObject());
   }
-  void FreeHandle(PersistentHandle* free_list) { SetNext(free_list); }
+  void FreeHandle(PersistentHandle* free_list) {
+#if defined(DEBUG)
+    Thread* thread = Thread::Current();
+    ASSERT(thread->MayAllocateHandles());
+#endif  // DEBUG
+    SetNext(free_list);
+  }
 
   ObjectPtr ptr_;
   DISALLOW_ALLOCATION();  // Allocated through AllocateHandle methods.
diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc
index e3ee1a14270..ad38d537181 100644
--- a/runtime/vm/thread.cc
+++ b/runtime/vm/thread.cc
@@ -253,24 +253,6 @@ ErrorPtr Thread::StealStickyError() {
   return return_value;
 }
 
-const char* Thread::TaskKindToCString(TaskKind kind) {
-  switch (kind) {
-    case kUnknownTask:
-      return "kUnknownTask";
-    case kMutatorTask:
-      return "kMutatorTask";
-    case kCompilerTask:
-      return "kCompilerTask";
-    case kSweeperTask:
-      return "kSweeperTask";
-    case kMarkerTask:
-      return "kMarkerTask";
-    default:
-      UNREACHABLE();
-      return "";
-  }
-}
-
 void Thread::AssertNonMutatorInvariants() {
   ASSERT(BypassSafepoints());
   ASSERT(store_buffer_block_ == nullptr);
diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h
index 402e5c3e8bb..42a00336eaf 100644
--- a/runtime/vm/thread.h
+++ b/runtime/vm/thread.h
@@ -365,9 +365,8 @@ class Thread : public ThreadState {
     kScavengerTask,
     kSampleBlockTask,
     kIncrementalCompactorTask,
+    kSpawnTask,
   };
-  // Converts a TaskKind to its corresponding C-String name.
-  static const char* TaskKindToCString(TaskKind kind);
 
   ~Thread();