cppgc: Avoid recursive GC during sweeping

Destructors are allowed to allocate without triggering recursive
garbage collections.

This changes NoGCScope to provide a soft-bailout for garbage
collections to avoid introducing yet another scope.

Bug: chromium:1056170
Change-Id: I0fe51a21977ae954221b6b64b2f6e938ff6d3264
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2185131
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67632}

src/heap/cppgc/heap.cc

@@ -98,13 +98,15 @@ Heap::Heap()
       prefinalizer_handler_(std::make_unique<PreFinalizerHandler>()) {}
 
 Heap::~Heap() {
+  NoGCScope no_gc(this);
   // Finish already running GC if any, but don't finalize live objects.
   sweeper_.Finish();
 }
 
 void Heap::CollectGarbage(GCConfig config) {
-  // No GC calls when in NoGCScope.
-  CHECK(!in_no_gc_scope());
+  if (in_no_gc_scope()) return;
+
+  epoch_++;
 
   // TODO(chromium:1056170): Replace with proper mark-sweep algorithm.
   // "Marking".
@@ -118,7 +120,10 @@ void Heap::CollectGarbage(GCConfig config) {
     NoAllocationScope no_allocation_scope_(this);
     prefinalizer_handler_->InvokePreFinalizers();
   }
-  sweeper_.Start(Sweeper::Config::kAtomic);
+  {
+    NoGCScope no_gc(this);
+    sweeper_.Start(Sweeper::Config::kAtomic);
+  }
 }
 
 size_t Heap::ObjectPayloadSize() const {

src/heap/cppgc/heap.h

@@ -32,28 +32,35 @@ class V8_EXPORT_PRIVATE LivenessBrokerFactory {
 
 class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
  public:
+  // NoGCScope allows going over limits and avoids triggering garbage
+  // collection triggered through allocations or even explicitly.
   class V8_EXPORT_PRIVATE NoGCScope final {
+    CPPGC_STACK_ALLOCATED();
+
    public:
     explicit NoGCScope(Heap* heap);
     ~NoGCScope();
 
+    NoGCScope(const NoGCScope&) = delete;
+    NoGCScope& operator=(const NoGCScope&) = delete;
+
    private:
-    Heap* heap_;
+    Heap* const heap_;
   };
 
-  // The NoAllocationScope class is used in debug mode to catch unwanted
-  // allocations. E.g. allocations during GC.
+  // NoAllocationScope is used in debug mode to catch unwanted allocations. E.g.
+  // allocations during GC.
   class V8_EXPORT_PRIVATE NoAllocationScope final {
     CPPGC_STACK_ALLOCATED();
-
    public:
     explicit NoAllocationScope(Heap* heap);
     ~NoAllocationScope();
 
+    NoAllocationScope(const NoAllocationScope&) = delete;
+    NoAllocationScope& operator=(const NoAllocationScope&) = delete;
+
    private:
     Heap* const heap_;
-
-    DISALLOW_COPY_AND_ASSIGN(NoAllocationScope);
   };
 
   struct GCConfig {
@@ -98,6 +105,8 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
 
   Sweeper& sweeper() { return sweeper_; }
 
+  size_t epoch() const { return epoch_; }
+
   size_t ObjectPayloadSize() const;
 
  private:
@@ -118,6 +127,8 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
   PersistentRegion strong_persistent_region_;
   PersistentRegion weak_persistent_region_;
 
+  size_t epoch_ = 0;
+
   size_t no_gc_scope_ = 0;
   size_t no_allocation_scope_ = 0;
 };

test/unittests/heap/cppgc/sweeper_unittest.cc

@@ -189,5 +189,30 @@ TEST_F(SweeperTest, CoalesceFreeListEntries) {
   EXPECT_TRUE(freelist.Contains(coalesced_block));
 }
 
+namespace {
+
+class GCInDestructor final : public GarbageCollected<GCInDestructor> {
+ public:
+  explicit GCInDestructor(Heap* heap) : heap_(heap) {}
+  ~GCInDestructor() {
+    // Instead of directly calling GC, allocations should be supported here as
+    // well.
+    heap_->CollectGarbage(internal::Heap::GCConfig::Default());
+  }
+
+ private:
+  Heap* heap_;
+};
+
+}  // namespace
+
+TEST_F(SweeperTest, SweepDoesNotTriggerRecursiveGC) {
+  auto* internal_heap = internal::Heap::From(GetHeap());
+  size_t saved_epoch = internal_heap->epoch();
+  MakeGarbageCollected<GCInDestructor>(GetHeap(), internal_heap);
+  PreciseGC();
+  EXPECT_EQ(saved_epoch + 1, internal_heap->epoch());
+}
+
 }  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/tests.cc

@@ -7,6 +7,7 @@
 #include <memory>
 
 namespace cppgc {
+namespace internal {
 namespace testing {
 
 // static
@@ -27,8 +28,8 @@ void TestWithPlatform::TearDownTestSuite() {
 TestWithHeap::TestWithHeap() : heap_(Heap::Create()) {}
 
 TestSupportingAllocationOnly::TestSupportingAllocationOnly()
-    : no_gc_scope_(std::make_unique<internal::Heap::NoGCScope>(
-          internal::Heap::From(GetHeap()))) {}
+    : no_gc_scope_(internal::Heap::From(GetHeap())) {}
 
 }  // namespace testing
+}  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/tests.h

@@ -12,6 +12,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace cppgc {
+namespace internal {
 namespace testing {
 
 class TestWithPlatform : public ::testing::Test {
@@ -29,10 +30,10 @@ class TestWithHeap : public TestWithPlatform {
 
   void PreciseGC() {
     heap_->ForceGarbageCollectionSlow("TestWithHeap", "Testing",
-                                      Heap::StackState::kEmpty);
+                                      Heap::GCConfig::StackState::kEmpty);
   }
 
-  Heap* GetHeap() const { return heap_.get(); }
+  cppgc::Heap* GetHeap() const { return heap_.get(); }
 
  private:
   std::unique_ptr<cppgc::Heap> heap_;
@@ -47,10 +48,11 @@ class TestSupportingAllocationOnly : public TestWithHeap {
   TestSupportingAllocationOnly();
 
  private:
-  std::unique_ptr<cppgc::internal::Heap::NoGCScope> no_gc_scope_;
+  Heap::NoGCScope no_gc_scope_;
 };
 
 }  // namespace testing
+}  // namespace internal
 }  // namespace cppgc
 
 #endif  // V8_UNITTESTS_HEAP_CPPGC_TESTS_H_