[heap] Verify the shared heap before tearing down a client heap

In the case of bugs creating shared->local edges, this lets us catch
dangling pointers via CHECKs before they happen.

Also removed some redundant checks in the shared struct verifier.
Existing heap verification already checks that all of a Heap's pointers
are contained within it.

Bug: v8:12547
Change-Id: Ic7a007b3b6559e3dfd0286fbf869586023c6f801
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3704911
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81335}

src/diagnostics/objects-debug.cc

@@ -327,6 +327,8 @@ void HeapObject::HeapObjectVerify(Isolate* isolate) {
 // static
 void HeapObject::VerifyHeapPointer(Isolate* isolate, Object p) {
   CHECK(p.IsHeapObject());
+  // If you crashed here and {isolate->is_shared()}, there is a bug causing the
+  // host of {p} to point to a non-shared object.
   CHECK(IsValidHeapObject(isolate->heap(), HeapObject::cast(p)));
   CHECK_IMPLIES(V8_EXTERNAL_CODE_SPACE_BOOL, !p.IsCode());
 }
@@ -1238,6 +1240,7 @@ USE_TORQUE_VERIFIER(JSWrappedFunction)
 
 void JSSharedStruct::JSSharedStructVerify(Isolate* isolate) {
   CHECK(IsJSSharedStruct());
+  CHECK(InSharedWritableHeap());
   JSObjectVerify(isolate);
   CHECK(HasFastProperties());
   // Shared structs can only point to primitives or other shared HeapObjects,
@@ -1258,10 +1261,9 @@ void JSSharedStruct::JSSharedStructVerify(Isolate* isolate) {
 
 void JSAtomicsMutex::JSAtomicsMutexVerify(Isolate* isolate) {
   CHECK(IsJSAtomicsMutex());
-  CHECK(InSharedHeap());
+  CHECK(InSharedWritableHeap());
   JSObjectVerify(isolate);
   Map mutex_map = map();
-  CHECK(mutex_map.InSharedHeap());
   CHECK(mutex_map.GetBackPointer().IsUndefined(isolate));
   CHECK(!mutex_map.is_extensible());
   CHECK(!mutex_map.is_prototype_map());

src/heap/heap.cc

@@ -4609,6 +4609,26 @@ void Heap::VerifyReadOnlyHeap() {
   read_only_space_->Verify(isolate());
 }
 
+void Heap::VerifySharedHeap(Isolate* initiator) {
+  DCHECK(IsShared());
+
+  // Stop all client isolates attached to this isolate.
+  GlobalSafepointScope global_safepoint(initiator);
+
+  // Migrate shared isolate to the main thread of the initiator isolate.
+  v8::Locker locker(reinterpret_cast<v8::Isolate*>(isolate()));
+  v8::Isolate::Scope isolate_scope(reinterpret_cast<v8::Isolate*>(isolate()));
+
+  DCHECK_NOT_NULL(isolate()->global_safepoint());
+
+  // Free all shared LABs to make the shared heap iterable.
+  isolate()->global_safepoint()->IterateClientIsolates([](Isolate* client) {
+    client->heap()->FreeSharedLinearAllocationAreas();
+  });
+
+  Verify();
+}
+
 class SlotVerifyingVisitor : public ObjectVisitorWithCageBases {
  public:
   SlotVerifyingVisitor(Isolate* isolate, std::set<Address>* untyped,
@@ -6037,9 +6057,14 @@ void Heap::StartTearDown() {
   // tear down parts of the Isolate.
   if (FLAG_verify_heap) {
     AllowGarbageCollection allow_gc;
-    IgnoreLocalGCRequests ignore_gc_requests(this);
-    SafepointScope scope(this);
     Verify();
+
+    // If this is a client Isolate of a shared Isolate, verify that there are no
+    // shared-to-local pointers before tearing down the client Isolate and
+    // creating dangling pointers.
+    if (Isolate* shared_isolate = isolate()->shared_isolate()) {
+      shared_isolate->heap()->VerifySharedHeap(isolate());
+    }
   }
 #endif
 }

src/heap/heap.h

@@ -1617,9 +1617,15 @@ class Heap {
 #ifdef VERIFY_HEAP
   // Verify the heap is in its normal state before or after a GC.
   V8_EXPORT_PRIVATE void Verify();
+
   // Verify the read-only heap after all read-only heap objects have been
   // created.
   void VerifyReadOnlyHeap();
+
+  // Verify the shared heap, initiating from a client heap. This performs a
+  // global safepoint, then the normal heap verification.
+  void VerifySharedHeap(Isolate* initiator);
+
   void VerifyRememberedSetFor(HeapObject object);
 
   // Verify that cached size of invalidated object is up-to-date.

test/cctest/test-serialize.cc

@@ -40,6 +40,7 @@
 #include "src/common/assert-scope.h"
 #include "src/debug/debug.h"
 #include "src/heap/heap-inl.h"
+#include "src/heap/parked-scope.h"
 #include "src/heap/read-only-heap.h"
 #include "src/heap/safepoint.h"
 #include "src/heap/spaces.h"
@@ -5040,7 +5041,13 @@ UNINITIALIZED_TEST(SharedStrings) {
   CheckObjectsAreInSharedHeap(i_isolate1);
   CheckObjectsAreInSharedHeap(i_isolate2);
 
-  isolate1->Dispose();
+  {
+    // Because both isolate1 and isolate2 are considered running on the main
+    // thread, one must be parked to avoid deadlock in the shared heap
+    // verification that may happen on client heap disposal.
+    ParkedScope parked(i_isolate2->main_thread_local_isolate());
+    isolate1->Dispose();
+  }
   isolate2->Dispose();
   Isolate::Delete(reinterpret_cast<Isolate*>(shared_isolate));
 

test/cctest/test-shared-strings.cc

@@ -21,6 +21,27 @@ struct V8_NODISCARD IsolateWrapper {
   v8::Isolate* const isolate;
 };
 
+// Some tests in this file allocate two Isolates in the same thread to directly
+// test shared string behavior. Because both are considered running, when
+// disposing these Isolates, one must be parked to not cause a deadlock in the
+// shared heap verification that happens on client Isolate disposal.
+struct V8_NODISCARD IsolatePairWrapper {
+  IsolatePairWrapper(v8::Isolate* isolate1, v8::Isolate* isolate2)
+      : isolate1(isolate1), isolate2(isolate2) {}
+
+  ~IsolatePairWrapper() {
+    {
+      i::ParkedScope parked(
+          reinterpret_cast<Isolate*>(isolate1)->main_thread_local_isolate());
+      isolate2->Dispose();
+    }
+    isolate1->Dispose();
+  }
+
+  v8::Isolate* const isolate1;
+  v8::Isolate* const isolate2;
+};
+
 class MultiClientIsolateTest {
  public:
   MultiClientIsolateTest() {
@@ -111,10 +132,10 @@ UNINITIALIZED_TEST(InPlaceInternalization) {
   FLAG_shared_string_table = true;
 
   MultiClientIsolateTest test;
-  IsolateWrapper isolate1_wrapper(test.NewClientIsolate());
-  IsolateWrapper isolate2_wrapper(test.NewClientIsolate());
-  v8::Isolate* isolate1 = isolate1_wrapper.isolate;
-  v8::Isolate* isolate2 = isolate2_wrapper.isolate;
+  IsolatePairWrapper isolates_wrapper(test.NewClientIsolate(),
+                                      test.NewClientIsolate());
+  v8::Isolate* isolate1 = isolates_wrapper.isolate1;
+  v8::Isolate* isolate2 = isolates_wrapper.isolate2;
   Isolate* i_isolate1 = reinterpret_cast<Isolate*>(isolate1);
   Factory* factory1 = i_isolate1->factory();
   Isolate* i_isolate2 = reinterpret_cast<Isolate*>(isolate2);
@@ -179,10 +200,10 @@ UNINITIALIZED_TEST(YoungInternalization) {
   FLAG_shared_string_table = true;
 
   MultiClientIsolateTest test;
-  IsolateWrapper isolate1_wrapper(test.NewClientIsolate());
-  IsolateWrapper isolate2_wrapper(test.NewClientIsolate());
-  v8::Isolate* isolate1 = isolate1_wrapper.isolate;
-  v8::Isolate* isolate2 = isolate2_wrapper.isolate;
+  IsolatePairWrapper isolates_wrapper(test.NewClientIsolate(),
+                                      test.NewClientIsolate());
+  v8::Isolate* isolate1 = isolates_wrapper.isolate1;
+  v8::Isolate* isolate2 = isolates_wrapper.isolate2;
   Isolate* i_isolate1 = reinterpret_cast<Isolate*>(isolate1);
   Factory* factory1 = i_isolate1->factory();
   Isolate* i_isolate2 = reinterpret_cast<Isolate*>(isolate2);