[vm/shared] Ensure mutator thread is registered without possibilty of a race.

Registration of the mutator thread in isolate group should be done as thread is added, under the same atomic operation.
Mutator count changes should be done separately as before, under the same mutex used by `ContainsOnlyOneIsolate`.

Follow-up to https://dart.googlesource.com/sdk/+/acc4ee0b1e3fc94a25bdf24431e7567e20cc663d

Fixes #61459
TEST=isolate_group_bound_init_test

Change-Id: I48bf4121bd097d3745915294c04d1eaae3879e0d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/448503
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Alexander Aprelev <[email protected]>

Author: aam

runtime/vm/isolate.cc

@@ -474,18 +474,24 @@ bool IsolateGroup::UnregisterIsolateDecrementCount() {
   return isolate_count_ == 0;
 }
 
-void IsolateGroup::RegisterIsolateGroupMutator(Thread* mutator) {
+void IsolateGroup::IncrementIsolateGroupMutatorCount() {
   SafepointWriteRwLocker ml(Thread::Current(), isolates_lock_.get());
-  mutators_.Append(mutator);
   group_mutator_count_++;
 }
 
-void IsolateGroup::UnregisterIsolateGroupMutator(Thread* mutator) {
+void IsolateGroup::DecrementIsolateGroupMutatorCount() {
   SafepointWriteRwLocker ml(Thread::Current(), isolates_lock_.get());
-  mutators_.Remove(mutator);
   group_mutator_count_--;
 }
 
+void IsolateGroup::RegisterIsolateGroupMutator(Thread* mutator) {
+  mutators_.Append(mutator);
+}
+
+void IsolateGroup::UnregisterIsolateGroupMutator(Thread* mutator) {
+  mutators_.Remove(mutator);
+}
+
 void IsolateGroup::CreateHeap(bool is_vm_isolate,
                               bool is_service_or_kernel_isolate) {
   Heap::Init(this, is_vm_isolate,

runtime/vm/isolate.h

@@ -374,11 +374,12 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
   // Returns `true` if this was the last isolate and the caller is responsible
   // for deleting the isolate group.
   bool UnregisterIsolateDecrementCount();
+  void IncrementIsolateGroupMutatorCount();
+  void DecrementIsolateGroupMutatorCount();
+  bool ContainsOnlyOneIsolate();
   void RegisterIsolateGroupMutator(Thread* mutator);
   void UnregisterIsolateGroupMutator(Thread* mutator);
 
-  bool ContainsOnlyOneIsolate();
-
   Dart_Port interrupt_port() { return interrupt_port_; }
 
   ThreadRegistry* thread_registry() const { return thread_registry_.get(); }

runtime/vm/thread.cc

@@ -537,9 +537,9 @@ void Thread::EnterIsolateGroupAsMutator(IsolateGroup* isolate_group,
   isolate_group->IncreaseMutatorCount(/*thread=*/nullptr,
                                       /*is_nested_reenter=*/true,
                                       /*was_stolen=*/false);
+  isolate_group->IncrementIsolateGroupMutatorCount();
   Thread* thread = AddActiveThread(isolate_group, /*isolate=*/nullptr,
                                    kMutatorTask, bypass_safepoint);
-  isolate_group->RegisterIsolateGroupMutator(thread);
 
   RELEASE_ASSERT(thread != nullptr);
   // Even if [bypass_safepoint] is true, a thread may need mutator state (e.g.
@@ -583,8 +583,8 @@ void Thread::ExitIsolateGroupAsMutator(bool bypass_safepoint) {
   thread->ClearStackLimit();
   SuspendThreadInternal(thread, VMTag::kInvalidTagId);
   auto group = thread->isolate_group();
-  group->UnregisterIsolateGroupMutator(thread);
   FreeActiveThread(thread, /*isolate=*/nullptr, bypass_safepoint);
+  group->DecrementIsolateGroupMutatorCount();
   group->DecreaseMutatorCount(/*is_nested_exit=*/true);
 }
 
@@ -693,9 +693,12 @@ Thread* Thread::AddActiveThread(IsolateGroup* group,
   thread->isolate_ = isolate;  // May be nullptr.
   thread->isolate_group_ = group;
   thread->scheduled_dart_mutator_isolate_ = isolate;
-  if (isolate != nullptr && task_kind == kMutatorTask) {
-    ASSERT(thread_registry->threads_lock()->IsOwnedByCurrentThread());
-    isolate->mutator_thread_ = thread;
+  if (task_kind == kMutatorTask) {
+    if (isolate != nullptr) {
+      isolate->mutator_thread_ = thread;
+    } else {
+      group->RegisterIsolateGroupMutator(thread);
+    }
   }
 
   // We start at being at-safepoint (in case any safepoint operation is
@@ -750,9 +753,12 @@ void Thread::FreeActiveThread(Thread* thread,
   thread->isolate_ = nullptr;
   thread->isolate_group_ = nullptr;
   thread->scheduled_dart_mutator_isolate_ = nullptr;
-  if (isolate != nullptr && thread->task_kind() == kMutatorTask) {
-    ASSERT(thread_registry->threads_lock()->IsOwnedByCurrentThread());
-    isolate->mutator_thread_ = nullptr;
+  if (thread->task_kind() == kMutatorTask) {
+    if (isolate != nullptr) {
+      isolate->mutator_thread_ = nullptr;
+    } else {
+      group->UnregisterIsolateGroupMutator(thread);
+    }
   }
   thread->set_execution_state(Thread::kThreadInNative);
   thread->stack_limit_.store(0);