[execution, heap] Lock global safepoint mutex in Isolate::Deinit

This CL locks the global safepoint mutex during Isolate::Deinit when
the shared heap is used. This prevents any shared GC between starting
isolate tear down and detaching from the shared heap isolate.

Not doing that resulted in deadlocks when the isolate's main thread
was blocking until background tasks finished while still being in
the running state.

It also solves the heap verification failures when one client isolate
stopped right before detaching from the shared heap isolate for a
shared GC. In this case the external string table was already
finalized. This CL ensures that there is no GC in-between these two
operations anymore.

Bug: v8:13267, chromium:1401078
Change-Id: I131bcf1506eb8d756e0092139b638fae051b902d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4120442
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#85136}

src/execution/isolate.cc

@@ -3554,7 +3554,20 @@ void Isolate::UpdateLogObjectRelocation() {
 
 void Isolate::Deinit() {
   TRACE_ISOLATE(deinit);
-  DisallowGarbageCollection disallow_gc;
+
+  // All client isolates should already be detached when the shared heap isolate
+  // tears down.
+  if (is_shared_heap_isolate()) {
+    global_safepoint()->AssertNoClientsOnTearDown();
+  }
+
+  if (has_shared_heap()) {
+    IgnoreLocalGCRequests ignore_gc_requests(heap());
+    ParkedScope parked_scope(main_thread_local_heap());
+    shared_heap_isolate()->global_safepoint()->clients_mutex_.Lock();
+  }
+
+  DisallowGarbageCollection no_gc;
 
   tracing_cpu_profiler_.reset();
   if (v8_flags.stress_sampling_allocation_profiler > 0) {
@@ -3590,11 +3603,6 @@ void Isolate::Deinit() {
     optimizing_compile_dispatcher_ = nullptr;
   }
 
-  // All client isolates should already be detached.
-  if (is_shared()) {
-    global_safepoint()->AssertNoClientsOnTearDown();
-  }
-
   if (v8_flags.print_deopt_stress) {
     PrintF(stdout, "=== Stress deopt counter: %u\n", stress_deopt_count_);
   }
@@ -3631,20 +3639,15 @@ void Isolate::Deinit() {
   // At this point there are no more background threads left in this isolate.
   heap_.safepoint()->AssertMainThreadIsOnlyThread();
 
-  // Tear down data using the shared heap before detaching.
+  // Tear down data that requires the shared heap before detaching.
   heap_.TearDownWithSharedHeap();
 
-  {
+  // Detach from the shared heap isolate and then unlock the mutex.
-    // This isolate might have to park for a shared GC initiated by another
+  if (has_shared_heap()) {
-    // client isolate before it can actually detach from the shared isolate.
+    Isolate* shared_heap_isolate = this->shared_heap_isolate();
-    AllowGarbageCollection allow_shared_gc;
     DetachFromSharedIsolate();
     DetachFromSharedSpaceIsolate();
-  }
+    shared_heap_isolate->global_safepoint()->clients_mutex_.Unlock();
-
-  // All client isolates should already be detached.
-  if (is_shared_space_isolate()) {
-    global_safepoint()->AssertNoClientsOnTearDown();
   }
 
   // Since there are no other threads left, we can lock this mutex without any

src/heap/safepoint.cc

@@ -312,12 +312,7 @@ void GlobalSafepoint::AppendClient(Isolate* client) {
 
 void GlobalSafepoint::RemoveClient(Isolate* client) {
   DCHECK_EQ(client->heap()->gc_state(), Heap::TEAR_DOWN);
-
+  AssertActive();
-  // A shared heap may have already acquired the client mutex to perform a
-  // shared GC. We need to park the Isolate here to allow for a shared GC.
-  IgnoreLocalGCRequests ignore_gc_requests(client->heap());
-  ParkedRecursiveMutexGuard guard(client->main_thread_local_heap(),
-                                  &clients_mutex_);
 
   if (client->global_safepoint_next_client_isolate_) {
     client->global_safepoint_next_client_isolate_
@@ -338,12 +333,18 @@ void GlobalSafepoint::RemoveClient(Isolate* client) {
 }
 
 void GlobalSafepoint::AssertNoClientsOnTearDown() {
+  if (v8_flags.shared_space) {
+    DCHECK_EQ(clients_head_, shared_heap_isolate_);
+    DCHECK_NULL(shared_heap_isolate_->global_safepoint_prev_client_isolate_);
+    DCHECK_NULL(shared_heap_isolate_->global_safepoint_next_client_isolate_);
+  } else {
     DCHECK_WITH_MSG(
         clients_head_ == nullptr,
         "Shared heap must not have clients at teardown. The first isolate that "
-      "is created (in a process that has no isolates) owns the lifetime of the "
+        "is created (in a process that has no isolates) owns the lifetime of "
-      "shared heap and is considered the main isolate. The main isolate must "
+        "the shared heap and is considered the main isolate. The main isolate "
-      "outlive all other isolates.");
+        "must outlive all other isolates.");
+  }
 }
 
 void GlobalSafepoint::EnterGlobalSafepointScope(Isolate* initiator) {