[heap] SafepointScope needs GCs to be allowed

A SafepointScope might need to block for a shared GC initiated from
another client isolate. This means that anytime we create a
SafepointScope a shared GC may run. This CL adds a DCHECK to ensure
AllowGarbageCollected::IsAllowed() holds for each SafepointScope.

So far this DCHECK was only run in the less likely event that a
SafepointScope actually runs a shared GC. Which is technically good
enough but it is easy to miss use cases of SafepointScope where this
does not hold.

Bug: v8:11708, v8:12377
Change-Id: I30cc33c05ebe4835430e1d699a86079810523858
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3289625Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77976}

src/api/api.cc

@@ -60,6 +60,7 @@
 #include "src/heap/embedder-tracing.h"
 #include "src/heap/heap-inl.h"
 #include "src/heap/heap-write-barrier.h"
+#include "src/heap/safepoint.h"
 #include "src/init/bootstrapper.h"
 #include "src/init/icu_util.h"
 #include "src/init/startup-data-util.h"
@@ -677,6 +678,7 @@ StartupData SnapshotCreator::CreateBlob(
   i::Snapshot::ClearReconstructableDataForSerialization(
       isolate, function_code_handling == FunctionCodeHandling::kClear);
 
+  i::GlobalSafepointScope global_safepoint(isolate);
   i::DisallowGarbageCollection no_gc_from_here_on;
 
   // Create a vector with all contexts and clear associated Persistent fields.

src/debug/debug-coverage.cc

@@ -7,6 +7,7 @@
 #include "src/ast/ast-source-ranges.h"
 #include "src/ast/ast.h"
 #include "src/base/hashmap.h"
+#include "src/common/assert-scope.h"
 #include "src/common/globals.h"
 #include "src/debug/debug.h"
 #include "src/deoptimizer/deoptimizer.h"
@@ -527,6 +528,7 @@ void CollectAndMaybeResetCounts(Isolate* isolate,
                   ->feedback_vectors_for_profiling_tools()
                   ->IsArrayList());
       DCHECK_EQ(v8::debug::CoverageMode::kBestEffort, coverage_mode);
+      AllowGarbageCollection allow_gc;
       HeapObjectIterator heap_iterator(isolate->heap());
       for (HeapObject current_obj = heap_iterator.Next();
            !current_obj.is_null(); current_obj = heap_iterator.Next()) {

src/debug/debug.cc

@@ -14,6 +14,7 @@
 #include "src/codegen/assembler-inl.h"
 #include "src/codegen/compilation-cache.h"
 #include "src/codegen/compiler.h"
+#include "src/common/assert-scope.h"
 #include "src/common/globals.h"
 #include "src/common/message-template.h"
 #include "src/debug/debug-evaluate.h"
@@ -1281,6 +1282,7 @@ class DiscardBaselineCodeVisitor : public ThreadVisitor {
   DiscardBaselineCodeVisitor() : shared_(SharedFunctionInfo()) {}
 
   void VisitThread(Isolate* isolate, ThreadLocalTop* top) override {
+    DisallowGarbageCollection diallow_gc;
     bool deopt_all = shared_ == SharedFunctionInfo();
     for (JavaScriptFrameIterator it(isolate, top); !it.done(); it.Advance()) {
       if (!deopt_all && it.frame()->function().shared() != shared_) continue;
@@ -1319,7 +1321,6 @@ class DiscardBaselineCodeVisitor : public ThreadVisitor {
 
  private:
   SharedFunctionInfo shared_;
-  DISALLOW_GARBAGE_COLLECTION(no_gc_)
 };
 }  // namespace
 

src/execution/isolate.cc

@@ -3728,10 +3728,6 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
   // LocalHeaps and not wait until this thread is ready for a GC.
   AttachToSharedIsolate();
 
-  // We need to ensure that we do not let a shared GC run before this isolate is
-  // fully set up.
-  DisallowSafepoints no_shared_gc;
-
   // SetUp the object heap.
   DCHECK(!heap_.HasBeenSetUp());
   heap_.SetUp(main_thread_local_heap());
@@ -4948,7 +4944,6 @@ void Isolate::CollectSourcePositionsForAllBytecodeArrays() {
   HandleScope scope(this);
   std::vector<Handle<SharedFunctionInfo>> sfis;
   {
-    DisallowGarbageCollection no_gc;
     HeapObjectIterator iterator(heap());
     for (HeapObject obj = iterator.Next(); !obj.is_null();
          obj = iterator.Next()) {

src/heap/heap.cc

@@ -2143,12 +2143,20 @@ size_t Heap::PerformGarbageCollection(
 
   TRACE_GC_EPOCH(tracer(), CollectorScopeId(collector), ThreadKind::kMain);
 
-  SafepointScope safepoint_scope(this);
+  base::Optional<SafepointScope> safepoint_scope;
+
+  {
+    AllowGarbageCollection allow_shared_gc;
+    safepoint_scope.emplace(this);
+  }
 
   collection_barrier_->StopTimeToCollectionTimer();
 
 #ifdef VERIFY_HEAP
   if (FLAG_verify_heap) {
+    // We don't really perform a GC here but need this scope for the nested
+    // SafepointScope inside Verify().
+    AllowGarbageCollection allow_gc;
     Verify();
   }
 #endif
@@ -2219,6 +2227,9 @@ size_t Heap::PerformGarbageCollection(
 
 #ifdef VERIFY_HEAP
   if (FLAG_verify_heap) {
+    // We don't really perform a GC here but need this scope for the nested
+    // SafepointScope inside Verify().
+    AllowGarbageCollection allow_gc;
     Verify();
   }
 #endif
@@ -3394,7 +3405,13 @@ FixedArrayBase Heap::LeftTrimFixedArray(FixedArrayBase object,
   if (FLAG_enable_slow_asserts) {
     // Make sure the stack or other roots (e.g., Handles) don't contain pointers
     // to the original FixedArray (which is now the filler object).
-    SafepointScope scope(this);
+    base::Optional<SafepointScope> safepoint_scope;
+
+    {
+      AllowGarbageCollection allow_gc;
+      safepoint_scope.emplace(this);
+    }
+
     LeftTrimmerVerifierRootVisitor root_visitor(object);
     ReadOnlyRoots(this).Iterate(&root_visitor);
     IterateRoots(&root_visitor, {});
@@ -5891,7 +5908,7 @@ void Heap::StartTearDown() {
   // a good time to run heap verification (if requested), before starting to
   // tear down parts of the Isolate.
   if (FLAG_verify_heap) {
-    SafepointScope scope(this);
+    AllowGarbageCollection allow_gc;
     Verify();
   }
 #endif

src/heap/heap.h

@@ -2757,8 +2757,6 @@ class V8_EXPORT_PRIVATE HeapObjectIterator {
  private:
   HeapObject NextObject();
 
-  DISALLOW_GARBAGE_COLLECTION(no_heap_allocation_)
-
   Heap* heap_;
   std::unique_ptr<SafepointScope> safepoint_scope_;
   HeapObjectsFiltering filtering_;
@@ -2767,6 +2765,8 @@ class V8_EXPORT_PRIVATE HeapObjectIterator {
   SpaceIterator* space_iterator_;
   // Object iterator for the space currently being iterated.
   std::unique_ptr<ObjectIterator> object_iterator_;
+
+  DISALLOW_GARBAGE_COLLECTION(no_heap_allocation_)
 };
 
 // Abstract base class for checking whether a weak object should be retained.

src/heap/safepoint.cc

@@ -7,6 +7,7 @@
 #include <atomic>
 
 #include "src/base/logging.h"
+#include "src/common/assert-scope.h"
 #include "src/handles/handles.h"
 #include "src/handles/local-handles.h"
 #include "src/handles/persistent-handles.h"
@@ -27,6 +28,7 @@ void IsolateSafepoint::EnterSafepointScope(StopMainThread stop_main_thread) {
   // Safepoints need to be initiated on the main thread.
   DCHECK_EQ(ThreadId::Current(), heap_->isolate()->thread_id());
   DCHECK_NULL(LocalHeap::Current());
+  DCHECK(AllowGarbageCollection::IsAllowed());
 
   if (++active_safepoint_scopes_ > 1) return;
 
@@ -261,11 +263,21 @@ void GlobalSafepoint::LeaveGlobalSafepointScope(Isolate* initiator) {
 
 GlobalSafepointScope::GlobalSafepointScope(Isolate* initiator)
     : initiator_(initiator), shared_isolate_(initiator->shared_isolate()) {
-  shared_isolate_->global_safepoint()->EnterGlobalSafepointScope(initiator_);
+  if (shared_isolate_) {
+    shared_isolate_->global_safepoint()->EnterGlobalSafepointScope(initiator_);
+  } else {
+    initiator_->heap()->safepoint()->EnterSafepointScope(
+        IsolateSafepoint::StopMainThread::kNo);
+  }
 }
 
 GlobalSafepointScope::~GlobalSafepointScope() {
-  shared_isolate_->global_safepoint()->LeaveGlobalSafepointScope(initiator_);
+  if (shared_isolate_) {
+    shared_isolate_->global_safepoint()->LeaveGlobalSafepointScope(initiator_);
+  } else {
+    initiator_->heap()->safepoint()->LeaveSafepointScope(
+        IsolateSafepoint::StopMainThread::kNo);
+  }
 }
 
 }  // namespace internal

src/heap/safepoint.h

@@ -120,6 +120,7 @@ class IsolateSafepoint final {
 
   friend class Heap;
   friend class GlobalSafepoint;
+  friend class GlobalSafepointScope;
   friend class LocalHeap;
   friend class PersistentHandles;
   friend class SafepointScope;

src/logging/log.cc

@@ -1976,7 +1976,6 @@ void Logger::LogAccessorCallbacks() {
 }
 
 void Logger::LogAllMaps() {
-  DisallowGarbageCollection no_gc;
   Heap* heap = isolate_->heap();
   CombinedHeapObjectIterator iterator(heap);
   for (HeapObject obj = iterator.Next(); !obj.is_null();

src/snapshot/snapshot.cc

@@ -253,8 +253,6 @@ void Snapshot::ClearReconstructableDataForSerialization(
     HandleScope scope(isolate);
     std::vector<i::Handle<i::SharedFunctionInfo>> sfis_to_clear;
     {
-      // Heap allocation is disallowed within this scope.
-      DisallowGarbageCollection disallow_gc;
       i::HeapObjectIterator it(isolate->heap());
       for (i::HeapObject o = it.Next(); !o.is_null(); o = it.Next()) {
         if (o.IsSharedFunctionInfo(cage_base)) {
@@ -334,6 +332,7 @@ void Snapshot::SerializeDeserializeAndVerifyForTesting(
 
   // Test serialization.
   {
+    GlobalSafepointScope global_safepoint(isolate);
     DisallowGarbageCollection no_gc;
 
     Snapshot::SerializerFlags flags(
@@ -386,12 +385,9 @@ v8::StartupData Snapshot::Create(
   DCHECK_GT(contexts->size(), 0);
   HandleScope scope(isolate);
 
-  // Enter a safepoint so that the heap is safe to iterate.
-  // TODO(leszeks): This safepoint's scope could be tightened to just string
-  // table iteration, as that iteration relies on there not being any concurrent
-  // threads mutating the string table. But, there's currently no harm in
-  // holding it for the entire snapshot serialization.
-  SafepointScope safepoint(isolate->heap());
+  // Ensure we are in a safepoint scope so that the string table is safe to
+  // iterate. Unlike mksnapshot, embedders may have background threads running.
+  isolate->heap()->safepoint()->AssertActive();
 
   ReadOnlySerializer read_only_serializer(isolate, flags);
   read_only_serializer.SerializeReadOnlyRoots();