cppgc: Make tests use idiomatic allocation

Neither Member, nor GarbageCollected objects (and friends) should be
allocated on the stack. Create a special test fixture that allows for
writing idiomatic unit tests that depend on allocation but do not pull
in garbage collection.

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

src/heap/cppgc/heap.cc

@@ -49,7 +49,17 @@ class StackMarker final : public StackVisitor {
 Heap::Heap()
     : stack_(std::make_unique<Stack>(v8::base::Stack::GetStackStart())) {}
 
+Heap::~Heap() {
+  for (HeapObjectHeader* header : objects_) {
+    header->Finalize();
+    free(header);
+  }
+}
+
 void Heap::CollectGarbage(GCConfig config) {
+  // No GC calls when in NoGCScope.
+  CHECK(!in_no_gc_scope());
+
   // TODO(chromium:1056170): Replace with proper mark-sweep algorithm.
   // "Marking".
   if (NeedsConservativeStackScan(config)) {
@@ -70,5 +80,9 @@ void Heap::CollectGarbage(GCConfig config) {
   }
 }
 
+Heap::NoGCScope::NoGCScope(Heap* heap) : heap_(heap) { heap_->no_gc_scope_++; }
+
+Heap::NoGCScope::~NoGCScope() { heap_->no_gc_scope_--; }
+
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/heap.h

@@ -19,6 +19,15 @@ class Stack;
 
 class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
  public:
+  class V8_EXPORT_PRIVATE NoGCScope final {
+   public:
+    explicit NoGCScope(Heap* heap);
+    ~NoGCScope();
+
+   private:
+    Heap* heap_;
+  };
+
   struct GCConfig {
     enum class StackState : uint8_t {
       kEmpty,
@@ -33,15 +42,19 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
   static Heap* From(cppgc::Heap* heap) { return static_cast<Heap*>(heap); }
 
   Heap();
-  ~Heap() final = default;
+  ~Heap() final;
 
   inline void* Allocate(size_t size, GCInfoIndex index);
 
   void CollectGarbage(GCConfig config = GCConfig::Default());
 
  private:
+  bool in_no_gc_scope() { return no_gc_scope_ > 0; }
+
   std::unique_ptr<Stack> stack_;
   std::vector<HeapObjectHeader*> objects_;
+
+  size_t no_gc_scope_ = 0;
 };
 
 }  // namespace internal

test/unittests/heap/cppgc/garbage-collected_unittest.cc

@@ -37,12 +37,8 @@ class GCWithMergedMixins : public GCed, public MergedMixins {
   void Trace(cppgc::Visitor* visitor) override { MergedMixins::Trace(visitor); }
 };
 
-class GarbageCollectedTestWithHeap : public testing::TestWithHeap {
-  void TearDown() override {
-    internal::Heap::From(GetHeap())->CollectGarbage();
-    TestWithHeap::TearDown();
-  }
-};
+class GarbageCollectedTestWithHeap
+    : public testing::TestSupportingAllocationOnly {};
 
 }  // namespace
 
@@ -66,8 +62,6 @@ TEST(GarbageCollectedTest, GarbageCollectedMixinTrait) {
   STATIC_ASSERT(IsGarbageCollectedMixinType<GCWithMergedMixins>::value);
 }
 
-#ifdef CPPGC_SUPPORTS_CONSERVATIVE_STACK_SCAN
-
 TEST_F(GarbageCollectedTestWithHeap, GetObjectStartReturnsCorrentAddress) {
   GCed* gced = MakeGarbageCollected<GCed>(GetHeap());
   GCedWithMixin* gced_with_mixin =
@@ -80,7 +74,5 @@ TEST_F(GarbageCollectedTestWithHeap, GetObjectStartReturnsCorrentAddress) {
                       .base_object_payload);
 }
 
-#endif  // CPPGC_SUPPORTS_CONSERVATIVE_STACK_SCAN
-
 }  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/tests.cc

@@ -4,6 +4,8 @@
 
 #include "test/unittests/heap/cppgc/tests.h"
 
+#include <memory>
+
 namespace cppgc {
 namespace testing {
 
@@ -12,7 +14,7 @@ std::unique_ptr<cppgc::PageAllocator> TestWithPlatform::page_allocator_;
 
 // static
 void TestWithPlatform::SetUpTestSuite() {
-  page_allocator_.reset(new v8::base::PageAllocator());
+  page_allocator_ = std::make_unique<v8::base::PageAllocator>();
   cppgc::InitializePlatform(page_allocator_.get());
 }
 
@@ -22,15 +24,11 @@ void TestWithPlatform::TearDownTestSuite() {
   page_allocator_.reset();
 }
 
-void TestWithHeap::SetUp() {
-  heap_ = Heap::Create();
-  TestWithPlatform::SetUp();
-}
+TestWithHeap::TestWithHeap() : heap_(Heap::Create()) {}
 
-void TestWithHeap::TearDown() {
-  heap_.reset();
-  TestWithPlatform::TearDown();
-}
+TestSupportingAllocationOnly::TestSupportingAllocationOnly()
+    : no_gc_scope_(std::make_unique<internal::Heap::NoGCScope>(
+          internal::Heap::From(GetHeap()))) {}
 
 }  // namespace testing
 }  // namespace cppgc

test/unittests/heap/cppgc/tests.h

@@ -8,6 +8,7 @@
 #include "include/cppgc/heap.h"
 #include "include/cppgc/platform.h"
 #include "src/base/page-allocator.h"
+#include "src/heap/cppgc/heap.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace cppgc {
@@ -24,8 +25,7 @@ class TestWithPlatform : public ::testing::Test {
 
 class TestWithHeap : public TestWithPlatform {
  protected:
-  void SetUp() override;
-  void TearDown() override;
+  TestWithHeap();
 
   Heap* GetHeap() const { return heap_.get(); }
 
@@ -33,6 +33,18 @@ class TestWithHeap : public TestWithPlatform {
   std::unique_ptr<cppgc::Heap> heap_;
 };
 
+// Restrictive test fixture that supports allocation but will make sure no
+// garbage collection is triggered. This is useful for writing idiomatic
+// tests where object are allocated on the managed heap while still avoiding
+// far reaching test consquences of full garbage collection calls.
+class TestSupportingAllocationOnly : public TestWithHeap {
+ protected:
+  TestSupportingAllocationOnly();
+
+ private:
+  std::unique_ptr<cppgc::internal::Heap::NoGCScope> no_gc_scope_;
+};
+
 }  // namespace testing
 }  // namespace cppgc
 

test/unittests/heap/cppgc/visitor_unittest.cc

@@ -3,9 +3,11 @@
 // found in the LICENSE file.
 
 #include "src/heap/cppgc/visitor.h"
+#include "include/cppgc/allocation.h"
 #include "include/cppgc/garbage-collected.h"
 #include "include/cppgc/trace-trait.h"
 #include "src/base/macros.h"
+#include "test/unittests/heap/cppgc/tests.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace cppgc {
@@ -13,6 +15,9 @@ namespace internal {
 
 namespace {
 
+class TraceTraitTest : public testing::TestSupportingAllocationOnly {};
+class VisitorTest : public testing::TestSupportingAllocationOnly {};
+
 class GCed : public GarbageCollected<GCed> {
  public:
   static size_t trace_callcount;
@@ -62,71 +67,83 @@ class DispatchingVisitor final : public VisitorBase {
 
 }  // namespace
 
-TEST(TraceTraitTest, GetObjectStartGCed) {
-  GCed gced;
-  EXPECT_EQ(&gced,
-            TraceTrait<GCed>::GetTraceDescriptor(&gced).base_object_payload);
+TEST_F(TraceTraitTest, GetObjectStartGCed) {
+  auto* gced = MakeGarbageCollected<GCed>(GetHeap());
+  EXPECT_EQ(gced,
+            TraceTrait<GCed>::GetTraceDescriptor(gced).base_object_payload);
 }
 
-TEST(TraceTraitTest, GetObjectStartGCedMixin) {
-  GCedMixinApplication gced_mixin_app;
-  GCedMixin* gced_mixin = static_cast<GCedMixin*>(&gced_mixin_app);
-  EXPECT_EQ(&gced_mixin_app,
+TEST_F(TraceTraitTest, GetObjectStartGCedMixin) {
+  auto* gced_mixin_app = MakeGarbageCollected<GCedMixinApplication>(GetHeap());
+  auto* gced_mixin = static_cast<GCedMixin*>(gced_mixin_app);
+  EXPECT_EQ(gced_mixin_app,
             TraceTrait<GCedMixin>::GetTraceDescriptor(gced_mixin)
                 .base_object_payload);
 }
 
-TEST(TraceTraitTest, TraceGCed) {
-  GCed gced;
+TEST_F(TraceTraitTest, TraceGCed) {
+  auto* gced = MakeGarbageCollected<GCed>(GetHeap());
   EXPECT_EQ(0u, GCed::trace_callcount);
-  TraceTrait<GCed>::Trace(nullptr, &gced);
+  TraceTrait<GCed>::Trace(nullptr, gced);
   EXPECT_EQ(1u, GCed::trace_callcount);
 }
 
-TEST(TraceTraitTest, TraceGCedMixin) {
-  GCedMixinApplication gced_mixin_app;
-  GCedMixin* gced_mixin = static_cast<GCedMixin*>(&gced_mixin_app);
+TEST_F(TraceTraitTest, TraceGCedMixin) {
+  auto* gced_mixin_app = MakeGarbageCollected<GCedMixinApplication>(GetHeap());
+  auto* gced_mixin = static_cast<GCedMixin*>(gced_mixin_app);
   EXPECT_EQ(0u, GCed::trace_callcount);
   TraceTrait<GCedMixin>::Trace(nullptr, gced_mixin);
   EXPECT_EQ(1u, GCed::trace_callcount);
 }
 
-TEST(TraceTraitTest, TraceGCedThroughTraceDescriptor) {
-  GCed gced;
+TEST_F(TraceTraitTest, TraceGCedThroughTraceDescriptor) {
+  auto* gced = MakeGarbageCollected<GCed>(GetHeap());
   EXPECT_EQ(0u, GCed::trace_callcount);
-  TraceDescriptor desc = TraceTrait<GCed>::GetTraceDescriptor(&gced);
+  TraceDescriptor desc = TraceTrait<GCed>::GetTraceDescriptor(gced);
   desc.callback(nullptr, desc.base_object_payload);
   EXPECT_EQ(1u, GCed::trace_callcount);
 }
 
-TEST(TraceTraitTest, TraceGCedMixinThroughTraceDescriptor) {
-  GCedMixinApplication gced_mixin_app;
-  GCedMixin* gced_mixin = static_cast<GCedMixin*>(&gced_mixin_app);
+TEST_F(TraceTraitTest, TraceGCedMixinThroughTraceDescriptor) {
+  auto* gced_mixin_app = MakeGarbageCollected<GCedMixinApplication>(GetHeap());
+  auto* gced_mixin = static_cast<GCedMixin*>(gced_mixin_app);
   EXPECT_EQ(0u, GCed::trace_callcount);
   TraceDescriptor desc = TraceTrait<GCedMixin>::GetTraceDescriptor(gced_mixin);
   desc.callback(nullptr, desc.base_object_payload);
   EXPECT_EQ(1u, GCed::trace_callcount);
 }
 
-TEST(VisitorTest, DispatchTraceGCed) {
-  GCed gced;
-  Member<GCed> ref(&gced);
-  DispatchingVisitor visitor(&gced, &gced);
+namespace {
+
+template <typename T>
+class MemberHolder final : public GarbageCollected<MemberHolder<T>> {
+ public:
+  void Trace(Visitor* visitor) { visitor->Trace(ref); }
+  Member<T> ref;
+};
+
+}  // namespace
+
+TEST_F(VisitorTest, DispatchTraceGCed) {
+  auto* gced = MakeGarbageCollected<GCed>(GetHeap());
+  auto* holder = MakeGarbageCollected<MemberHolder<GCed>>(GetHeap());
+  holder->ref = gced;
+  DispatchingVisitor visitor(gced, gced);
   EXPECT_EQ(0u, GCed::trace_callcount);
-  visitor.Trace(ref);
+  visitor.Trace(holder->ref);
   EXPECT_EQ(1u, GCed::trace_callcount);
 }
 
-TEST(VisitorTest, DispatchTraceGCedMixin) {
-  GCedMixinApplication gced_mixin_app;
-  GCedMixin* gced_mixin = static_cast<GCedMixin*>(&gced_mixin_app);
+TEST_F(VisitorTest, DispatchTraceGCedMixin) {
+  auto* gced_mixin_app = MakeGarbageCollected<GCedMixinApplication>(GetHeap());
+  auto* gced_mixin = static_cast<GCedMixin*>(gced_mixin_app);
   // Ensure that we indeed test dispatching an inner object.
-  EXPECT_NE(static_cast<void*>(&gced_mixin_app),
-            static_cast<void*>(gced_mixin));
-  Member<GCedMixin> ref(gced_mixin);
-  DispatchingVisitor visitor(gced_mixin, &gced_mixin_app);
+  EXPECT_NE(static_cast<void*>(gced_mixin_app), static_cast<void*>(gced_mixin));
+  auto* holder = MakeGarbageCollected<MemberHolder<GCedMixin>>(GetHeap());
+  holder->ref = gced_mixin;
+  DispatchingVisitor visitor(gced_mixin, gced_mixin_app);
   EXPECT_EQ(0u, GCed::trace_callcount);
-  visitor.Trace(ref);
+  visitor.Trace(holder->ref);
   EXPECT_EQ(1u, GCed::trace_callcount);
 }