Revert "cppgc: Rework testing GC infrastructure"

This reverts commit eb453679.

Reason for revert: Breaks MSAN: https://ci.chromium.org/p/v8/builders/ci/V8%20Linux%20-%20arm64%20-%20sim%20-%20MSAN/37053

Original change's description:
> cppgc: Rework testing GC infrastructure
>
> Instead of moving the stand-alone logic to the base heap, allows
> specific heaps to override their stand-alone GC behavior. This allows
> CppHeap to reuse the unified heap bottlenecks and visitors for
> testing. This works as long as any v8 references are empty as there is
> no Isolate attached to the heap in this case.
>
> - Reverts parts of https://crrev.com/c/2716291
> - Relands parts of https://crrev.com/c/2718146
>
> In addition, add tests covering v8::CppHeap and cppgc::Heap.
>
> Bug: chromium:1056170
> Change-Id: I47dc88c7f0e4961a1aadd60da9b05bff4dcfb27a
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2718612
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Omer Katz <omerkatz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#73077}

Bug: chromium:1056170
Change-Id: Ieda44c07d08f837a6632f96b8db6d5bec87dd521
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2723216
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Francis McCabe <fgm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73078}

include/cppgc/testing.h

@@ -18,6 +18,20 @@ class HeapHandle;
  */
 namespace testing {
 
+/**
+ * Testing helper used to acces heap internals.
+ */
+class V8_EXPORT Heap final {
+ public:
+  /**
+   * Atomically collects garbage on the C++ heap.
+   *
+   * \param heap_handle The corresponding heap.
+   * \param stack_state The stack state to assume for the garbage collection.
+   */
+  void CollectGarbage(HeapHandle& heap_handle, EmbedderStackState stack_state);
+};
+
 /**
  * Overrides the state of the stack with the provided value. Takes precedence
  * over other parameters that set the stack state. Must no be nested.

include/v8-cppgc.h

@@ -9,7 +9,6 @@
 #include <memory>
 #include <vector>
 
-#include "cppgc/common.h"
 #include "cppgc/custom-space.h"
 #include "cppgc/heap-statistics.h"
 #include "cppgc/internal/write-barrier.h"
@@ -119,20 +118,6 @@ class V8_EXPORT CppHeap {
   cppgc::HeapStatistics CollectStatistics(
       cppgc::HeapStatistics::DetailLevel detail_level);
 
-  /**
-   * Enables a detached mode that allows testing garbage collection using
-   * `cppgc::testing` APIs. Once used, the heap cannot be attached to an
-   * `Isolate` anymore.
-   */
-  void EnableDetachedGarbageCollectionsForTesting();
-
-  /**
-   * Performs a stop-the-world garbage collection for testing purposes.
-   *
-   * \param stack_state The stack state to assume for the garbage collection.
-   */
-  void CollectGarbageForTesting(cppgc::EmbedderStackState stack_state);
-
  private:
   CppHeap() = default;
 

src/heap/cppgc-js/cpp-heap.cc

@@ -64,15 +64,6 @@ cppgc::HeapStatistics CppHeap::CollectStatistics(
       detail_level);
 }
 
-void CppHeap::EnableDetachedGarbageCollectionsForTesting() {
-  return internal::CppHeap::From(this)
-      ->EnableDetachedGarbageCollectionsForTesting();
-}
-
-void CppHeap::CollectGarbageForTesting(cppgc::EmbedderStackState stack_state) {
-  return internal::CppHeap::From(this)->CollectGarbageForTesting(stack_state);
-}
-
 void JSHeapConsistency::DijkstraMarkingBarrierSlow(
     cppgc::HeapHandle& heap_handle, const TracedReferenceBase& ref) {
   auto& heap_base = cppgc::internal::HeapBase::From(heap_handle);
@@ -153,7 +144,7 @@ UnifiedHeapConcurrentMarker::CreateConcurrentMarkingVisitor(
 
 class UnifiedHeapMarker final : public cppgc::internal::MarkerBase {
  public:
-  UnifiedHeapMarker(Key, Heap* v8_heap, cppgc::internal::HeapBase& cpp_heap,
+  UnifiedHeapMarker(Key, Heap& v8_heap, cppgc::internal::HeapBase& cpp_heap,
                     cppgc::Platform* platform, MarkingConfig config);
 
   ~UnifiedHeapMarker() final = default;
@@ -175,7 +166,7 @@ class UnifiedHeapMarker final : public cppgc::internal::MarkerBase {
   cppgc::internal::ConservativeMarkingVisitor conservative_marking_visitor_;
 };
 
-UnifiedHeapMarker::UnifiedHeapMarker(Key key, Heap* v8_heap,
+UnifiedHeapMarker::UnifiedHeapMarker(Key key, Heap& v8_heap,
                                      cppgc::internal::HeapBase& heap,
                                      cppgc::Platform* platform,
                                      MarkingConfig config)
@@ -230,7 +221,6 @@ void CppHeap::Terminate() {
 }
 
 void CppHeap::AttachIsolate(Isolate* isolate) {
-  CHECK(!in_detached_testing_mode);
   CHECK_NULL(isolate_);
   isolate_ = isolate;
   static_cast<CppgcPlatformAdapter*>(platform())
@@ -297,8 +287,7 @@ void CppHeap::TracePrologue(TraceFlags flags) {
   }
   marker_ =
       cppgc::internal::MarkerFactory::CreateAndStartMarking<UnifiedHeapMarker>(
-          isolate_ ? isolate_->heap() : nullptr, AsBase(), platform_.get(),
-          marking_config);
+          *isolate_->heap(), AsBase(), platform_.get(), marking_config);
   marking_done_ = false;
 }
 
@@ -401,32 +390,5 @@ void CppHeap::ReportBufferedAllocationSizeIfPossible() {
   buffered_allocated_bytes_ = 0;
 }
 
-void CppHeap::CollectGarbageForTesting(
-    cppgc::internal::GarbageCollector::Config::StackState stack_state) {
-  if (in_no_gc_scope()) return;
-
-  // Finish sweeping in case it is still running.
-  sweeper().FinishIfRunning();
-
-  if (isolate_) {
-    // Go through EmbedderHeapTracer API and perform a unified heap collection.
-    GarbageCollectionForTesting(stack_state);
-  } else {
-    // Perform an atomic GC, with starting incremental/concurrent marking and
-    // immediately finalizing the garbage collection.
-    TracePrologue(TraceFlags::kForced);
-    EnterFinalPause(stack_state);
-    AdvanceTracing(std::numeric_limits<double>::infinity());
-    TraceEpilogue(nullptr);
-  }
-}
-
-void CppHeap::EnableDetachedGarbageCollectionsForTesting() {
-  CHECK(!in_detached_testing_mode);
-  CHECK_NULL(isolate_);
-  no_gc_scope_--;
-  in_detached_testing_mode = true;
-}
-
 }  // namespace internal
 }  // namespace v8

src/heap/cppgc-js/cpp-heap.h

@@ -50,11 +50,6 @@ class V8_EXPORT_PRIVATE CppHeap final
 
   void Terminate();
 
-  void EnableDetachedGarbageCollectionsForTesting();
-
-  void CollectGarbageForTesting(
-      cppgc::internal::GarbageCollector::Config::StackState);
-
   // v8::EmbedderHeapTracer interface.
   void RegisterV8References(
       const std::vector<std::pair<void*, void*> >& embedder_fields) final;
@@ -88,8 +83,6 @@ class V8_EXPORT_PRIVATE CppHeap final
   int64_t buffered_allocated_bytes_ = 0;
 
   v8::WrapperDescriptor wrapper_descriptor_;
-
-  bool in_detached_testing_mode = false;
 };
 
 }  // namespace internal

src/heap/cppgc-js/unified-heap-marking-state.h

@@ -7,7 +7,6 @@
 
 #include "include/v8-cppgc.h"
 #include "include/v8.h"
-#include "src/base/logging.h"
 #include "src/heap/heap.h"
 
 namespace v8 {
@@ -23,7 +22,7 @@ class BasicTracedReferenceExtractor {
 
 class UnifiedHeapMarkingState {
  public:
-  explicit UnifiedHeapMarkingState(Heap* heap) : heap_(heap) {}
+  explicit UnifiedHeapMarkingState(Heap& heap) : heap_(heap) {}
 
   UnifiedHeapMarkingState(const UnifiedHeapMarkingState&) = delete;
   UnifiedHeapMarkingState& operator=(const UnifiedHeapMarkingState&) = delete;
@@ -31,16 +30,11 @@ class UnifiedHeapMarkingState {
   inline void MarkAndPush(const TracedReferenceBase&);
 
  private:
-  Heap* heap_;
+  Heap& heap_;
 };
 
 void UnifiedHeapMarkingState::MarkAndPush(const TracedReferenceBase& ref) {
-  // The same visitor is used in testing scenarios without attaching the heap to
-  // an Isolate under the assumption that no non-empty v8 references are found.
-  // Having the following DCHECK crash means that the heap is in detached mode
-  // but we find traceable pointers into an Isolate.
-  DCHECK_NOT_NULL(heap_);
-  heap_->RegisterExternallyReferencedObject(
+  heap_.RegisterExternallyReferencedObject(
       BasicTracedReferenceExtractor::ObjectReference(ref));
 }
 

src/heap/cppgc/heap-base.cc

@@ -147,5 +147,75 @@ HeapStatistics HeapBase::CollectStatistics(
   return HeapStatisticsCollector().CollectStatistics(this);
 }
 
+void HeapBase::StandAloneGarbageCollectionForTesting(
+    GarbageCollector::Config::StackState stack_state) {
+  GarbageCollector::Config config = {
+      GarbageCollector::Config::CollectionType::kMajor, stack_state,
+      GarbageCollector::Config::MarkingType::kAtomic,
+      GarbageCollector::Config::SweepingType::kAtomic};
+
+  if (in_no_gc_scope()) return;
+
+  if (!IsMarking()) {
+    StartStandAloneGarbageCollection(config);
+  }
+  DCHECK(IsMarking());
+  FinalizeStandAloneGarbageCollection(config);
+}
+
+void HeapBase::StartStandAloneGarbageCollection(
+    GarbageCollector::Config config) {
+  DCHECK(!IsMarking());
+  DCHECK(!in_no_gc_scope());
+
+  // Finish sweeping in case it is still running.
+  sweeper_.FinishIfRunning();
+
+  epoch_++;
+
+#if defined(CPPGC_YOUNG_GENERATION)
+  if (config.collection_type == Config::CollectionType::kMajor)
+    Unmarker unmarker(&raw_heap());
+#endif
+
+  const Marker::MarkingConfig marking_config{
+      config.collection_type, config.stack_state, config.marking_type,
+      config.is_forced_gc};
+  marker_ = MarkerFactory::CreateAndStartMarking<Marker>(*this, platform_.get(),
+                                                         marking_config);
+}
+
+void HeapBase::FinalizeStandAloneGarbageCollection(
+    GarbageCollector::Config config) {
+  DCHECK(IsMarking());
+  DCHECK(!in_no_gc_scope());
+  CHECK(!in_disallow_gc_scope());
+  if (override_stack_state_) {
+    config.stack_state = *override_stack_state_;
+  }
+  in_atomic_pause_ = true;
+  {
+    // This guards atomic pause marking, meaning that no internal method or
+    // external callbacks are allowed to allocate new objects.
+    cppgc::subtle::DisallowGarbageCollectionScope no_gc_scope(*this);
+    marker_->FinishMarking(config.stack_state);
+  }
+  marker_.reset();
+  ExecutePreFinalizers();
+  // TODO(chromium:1056170): replace build flag with dedicated flag.
+#if DEBUG
+  MarkingVerifier verifier(*this);
+  verifier.Run(config.stack_state);
+#endif
+
+  subtle::NoGarbageCollectionScope no_gc(*this);
+  const Sweeper::SweepingConfig sweeping_config{
+      config.sweeping_type,
+      Sweeper::SweepingConfig::CompactableSpaceHandling::kSweep};
+  sweeper_.Start(sweeping_config);
+  in_atomic_pause_ = false;
+  sweeper_.NotifyDoneIfNeeded();
+}
+
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/heap-base.h

@@ -163,6 +163,8 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
 
   HeapStatistics CollectStatistics(HeapStatistics::DetailLevel);
 
+  size_t epoch() const { return epoch_; }
+
   EmbedderStackState stack_state_of_prev_gc() const {
     return stack_state_of_prev_gc_;
   }
@@ -171,6 +173,10 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
   }
 
  protected:
+  // Starts and finalizes stand-alone garbage collections.
+  void StartStandAloneGarbageCollection(GarbageCollector::Config);
+  void FinalizeStandAloneGarbageCollection(GarbageCollector::Config);
+
   // Used by the incremental scheduler to finalize a GC if supported.
   virtual void FinalizeIncrementalGarbageCollectionIfNeeded(
       cppgc::Heap::StackState) = 0;
@@ -218,6 +224,12 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
 
   bool in_atomic_pause_ = false;
 
+  size_t epoch_ = 0;
+
+ private:
+  void StandAloneGarbageCollectionForTesting(
+      GarbageCollector::Config::StackState);
+
   friend class MarkerBase::IncrementalMarkingTask;
   friend class testing::TestWithHeap;
   friend class cppgc::subtle::DisallowGarbageCollectionScope;

src/heap/cppgc/heap.cc

@@ -116,10 +116,10 @@ void Heap::CollectGarbage(Config config) {
   config_ = config;
 
   if (!IsMarking()) {
-    StartGarbageCollection(config);
+    StartStandAloneGarbageCollection(config);
   }
   DCHECK(IsMarking());
-  FinalizeGarbageCollection(config.stack_state);
+  FinalizeStandAloneGarbageCollection(config);
 }
 
 void Heap::StartIncrementalGarbageCollection(Config config) {
@@ -131,7 +131,7 @@ void Heap::StartIncrementalGarbageCollection(Config config) {
 
   config_ = config;
 
-  StartGarbageCollection(config);
+  StartStandAloneGarbageCollection(config);
 }
 
 void Heap::FinalizeIncrementalGarbageCollectionIfRunning(Config config) {
@@ -144,60 +144,7 @@ void Heap::FinalizeIncrementalGarbageCollectionIfRunning(Config config) {
 
   DCHECK_NE(Config::MarkingType::kAtomic, config_.marking_type);
   config_ = config;
-  FinalizeGarbageCollection(config.stack_state);
-}
-
-void Heap::StartGarbageCollection(Config config) {
-  DCHECK(!IsMarking());
-  DCHECK(!in_no_gc_scope());
-
-  // Finish sweeping in case it is still running.
-  sweeper_.FinishIfRunning();
-
-  epoch_++;
-
-#if defined(CPPGC_YOUNG_GENERATION)
-  if (config.collection_type == Config::CollectionType::kMajor)
-    Unmarker unmarker(&raw_heap());
-#endif
-
-  const Marker::MarkingConfig marking_config{
-      config.collection_type, config.stack_state, config.marking_type,
-      config.is_forced_gc};
-  marker_ = MarkerFactory::CreateAndStartMarking<Marker>(
-      AsBase(), platform_.get(), marking_config);
-}
-
-void Heap::FinalizeGarbageCollection(Config::StackState stack_state) {
-  DCHECK(IsMarking());
-  DCHECK(!in_no_gc_scope());
-  CHECK(!in_disallow_gc_scope());
-  config_.stack_state = stack_state;
-  if (override_stack_state_) {
-    config_.stack_state = *override_stack_state_;
-  }
-  in_atomic_pause_ = true;
-  {
-    // This guards atomic pause marking, meaning that no internal method or
-    // external callbacks are allowed to allocate new objects.
-    cppgc::subtle::DisallowGarbageCollectionScope no_gc_scope(*this);
-    marker_->FinishMarking(config_.stack_state);
-  }
-  marker_.reset();
-  ExecutePreFinalizers();
-  // TODO(chromium:1056170): replace build flag with dedicated flag.
-#if DEBUG
-  MarkingVerifier verifier(*this);
-  verifier.Run(config_.stack_state);
-#endif
-
-  subtle::NoGarbageCollectionScope no_gc(*this);
-  const Sweeper::SweepingConfig sweeping_config{
-      config_.sweeping_type,
-      Sweeper::SweepingConfig::CompactableSpaceHandling::kSweep};
-  sweeper_.Start(sweeping_config);
-  in_atomic_pause_ = false;
-  sweeper_.NotifyDoneIfNeeded();
+  FinalizeStandAloneGarbageCollection(config_);
 }
 
 void Heap::DisableHeapGrowingForTesting() { growing_.DisableForTesting(); }
@@ -206,8 +153,10 @@ void Heap::FinalizeIncrementalGarbageCollectionIfNeeded(
     Config::StackState stack_state) {
   StatsCollector::EnabledScope stats_scope(
       stats_collector(), StatsCollector::kMarkIncrementalFinalize);
-  FinalizeGarbageCollection(stack_state);
+  FinalizeStandAloneGarbageCollection(config_);
 }
 
+size_t Heap::epoch() const { return HeapBase::epoch(); }
+
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/heap.h

@@ -36,14 +36,11 @@ class V8_EXPORT_PRIVATE Heap final : public HeapBase,
   void StartIncrementalGarbageCollection(Config) final;
   void FinalizeIncrementalGarbageCollectionIfRunning(Config);
 
-  size_t epoch() const final { return epoch_; }
+  size_t epoch() const final;
 
   void DisableHeapGrowingForTesting();
 
  private:
-  void StartGarbageCollection(Config);
-  void FinalizeGarbageCollection(Config::StackState);
-
   void FinalizeIncrementalGarbageCollectionIfNeeded(Config::StackState) final;
 
   Config config_;
@@ -52,8 +49,6 @@ class V8_EXPORT_PRIVATE Heap final : public HeapBase,
 
   const MarkingType marking_support_;
   const SweepingType sweeping_support_;
-
-  size_t epoch_;
 };
 
 }  // namespace internal

src/heap/cppgc/marking-verifier.h

@@ -37,7 +37,7 @@ class V8_EXPORT_PRIVATE MarkingVerifierBase
   MarkingVerifierBase(const MarkingVerifierBase&) = delete;
   MarkingVerifierBase& operator=(const MarkingVerifierBase&) = delete;
 
-  void Run(Heap::Config::StackState);
+  void Run(GarbageCollector::Config::StackState);
 
  protected:
   MarkingVerifierBase(HeapBase&, std::unique_ptr<cppgc::Visitor>);

src/heap/cppgc/testing.cc

@@ -10,6 +10,12 @@
 namespace cppgc {
 namespace testing {
 
+void Heap::CollectGarbage(HeapHandle& heap_handle,
+                          EmbedderStackState stack_state) {
+  auto& heap = internal::HeapBase::From(heap_handle);
+  heap.StandAloneGarbageCollectionForTesting(stack_state);
+}
+
 OverrideEmbedderStackStateScope::OverrideEmbedderStackStateScope(
     HeapHandle& heap_handle, EmbedderStackState state)
     : heap_handle_(heap_handle) {
@@ -19,7 +25,8 @@ OverrideEmbedderStackStateScope::OverrideEmbedderStackStateScope(
 }
 
 OverrideEmbedderStackStateScope::~OverrideEmbedderStackStateScope() {
-  internal::HeapBase::From(heap_handle_).override_stack_state_.reset();
+  auto& heap = internal::HeapBase::From(heap_handle_);
+  heap.override_stack_state_.reset();
 }
 
 }  // namespace testing

test/unittests/heap/unified-heap-unittest.cc

@@ -6,7 +6,6 @@
 #include "include/cppgc/garbage-collected.h"
 #include "include/cppgc/persistent.h"
 #include "include/cppgc/platform.h"
-#include "include/cppgc/testing.h"
 #include "include/v8-cppgc.h"
 #include "include/v8.h"
 #include "src/api/api-inl.h"
@@ -141,7 +140,6 @@ TEST_F(UnifiedHeapDetachedTest, AllocationBeforeConfigureHeap) {
     cpp_heap.AsBase().sweeper().FinishIfRunning();
     EXPECT_TRUE(weak_holder);
   }
-  USE(object);
   {
     js_heap.SetEmbedderStackStateForNextFinalization(
         EmbedderHeapTracer::EmbedderStackState::kNoHeapPointers);
@@ -151,29 +149,5 @@ TEST_F(UnifiedHeapDetachedTest, AllocationBeforeConfigureHeap) {
   }
 }
 
-TEST_F(UnifiedHeapDetachedTest, StandAloneCppGC) {
-  // Test ensures that stand-alone C++ GC are possible when using CppHeap. This
-  // works even in the presence of wrappables using TracedReference as long
-  // as the reference is empty.
-  auto heap = v8::CppHeap::Create(
-      V8::GetCurrentPlatform(),
-      CppHeapCreateParams{{}, WrapperHelper::DefaultWrapperDescriptor()});
-  auto* object =
-      cppgc::MakeGarbageCollected<Wrappable>(heap->GetAllocationHandle());
-  cppgc::WeakPersistent<Wrappable> weak_holder{object};
-
-  heap->EnableDetachedGarbageCollectionsForTesting();
-  {
-    heap->CollectGarbageForTesting(
-        cppgc::EmbedderStackState::kMayContainHeapPointers);
-    EXPECT_TRUE(weak_holder);
-  }
-  USE(object);
-  {
-    heap->CollectGarbageForTesting(cppgc::EmbedderStackState::kNoHeapPointers);
-    EXPECT_FALSE(weak_holder);
-  }
-}
-
 }  // namespace internal
 }  // namespace v8