cppgc: Add regression test and check for object start bitmap

Access to the object start bitmap is only safe during marking until
sweeping is started as the concurrent sweeper may clear and rebuild
the bitmap at any time during sweeping.

Adds a DCHECK and an additional test for a previously broken
pre-finalizer scenario.

Bug: chromium:1307471
Change-Id: If67ade43f7cdad6de4720c0efeac11bfe8c22b3c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3535782Reviewed-by: Nikolaos Papaspyrou <nikolaos@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79550}

src/heap/cppgc/heap-base.cc

@@ -192,6 +192,15 @@ void HeapBase::Terminate() {
   CHECK_EQ(0u, weak_cross_thread_persistent_region_.NodesInUse());
 }
 
+bool HeapBase::CanLookupObjectStartInBitmap() const {
+  // Object start bitmap access is only safe during marking and until the
+  // sweeper has been started. User code cannot distinguish between sweeping or
+  // not sweeping, which is why it's only valid to use the bitmap during
+  // marking.
+  return marker() || (in_atomic_pause() && !sweeper().IsSweepingInProgress()) ||
+         allow_lookup_of_object_start_in_bitmap_;
+}
+
 HeapStatistics HeapBase::CollectStatistics(
     HeapStatistics::DetailLevel detail_level) {
   if (detail_level == HeapStatistics::DetailLevel::kBrief) {

src/heap/cppgc/heap-base.h

@@ -214,6 +214,11 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
   MarkingType marking_support() const { return marking_support_; }
   SweepingType sweeping_support() const { return sweeping_support_; }
 
+  bool CanLookupObjectStartInBitmap() const;
+  void set_allow_lookup_of_object_start_in_bitmap(bool value) {
+    allow_lookup_of_object_start_in_bitmap_ = value;
+  }
+
  protected:
   // Used by the incremental scheduler to finalize a GC if supported.
   virtual void FinalizeIncrementalGarbageCollectionIfNeeded(
@@ -284,6 +289,10 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
 
   int creation_thread_id_ = v8::base::OS::GetCurrentThreadId();
 
+  // Used to explicitly allow lookups to the object start bitmap for testing
+  // code or internal code that otherwise verifies that access is safe.
+  bool allow_lookup_of_object_start_in_bitmap_ = false;
+
   const MarkingType marking_support_;
   const SweepingType sweeping_support_;
 

src/heap/cppgc/heap-page.cc

@@ -106,8 +106,7 @@ const HeapObjectHeader* BasePage::TryObjectHeaderFromInnerAddress(
   }
 
   // |address| is on the heap, so we FromInnerAddress can get the header.
-  const HeapObjectHeader* header =
-      ObjectHeaderFromInnerAddressImpl(this, address);
+  const HeapObjectHeader* header = ObjectHeaderFromInnerAddressImpl(address);
   if (header->IsFree()) return nullptr;
   DCHECK_NE(kFreeListGCInfoIndex, header->GetGCInfoIndex());
   return header;

src/heap/cppgc/heap-page.h

@@ -90,6 +90,10 @@ class V8_EXPORT_PRIVATE BasePage {
   BasePage(HeapBase&, BaseSpace&, PageType);
 
  private:
+  template <AccessMode mode = AccessMode::kNonAtomic>
+  const HeapObjectHeader* ObjectHeaderFromInnerAddressImpl(
+      const void* address) const;
+
   HeapBase& heap_;
   BaseSpace& space_;
   PageType type_;
@@ -271,14 +275,14 @@ const BasePage* BasePage::FromPayload(const void* payload) {
       kGuardPageSize);
 }
 
-template <AccessMode mode = AccessMode::kNonAtomic>
-const HeapObjectHeader* ObjectHeaderFromInnerAddressImpl(const BasePage* page,
-                                                         const void* address) {
-  if (page->is_large()) {
-    return LargePage::From(page)->ObjectHeader();
+template <AccessMode mode>
+const HeapObjectHeader* BasePage::ObjectHeaderFromInnerAddressImpl(
+    const void* address) const {
+  if (is_large()) {
+    return LargePage::From(this)->ObjectHeader();
   }
   const PlatformAwareObjectStartBitmap& bitmap =
-      NormalPage::From(page)->object_start_bitmap();
+      NormalPage::From(this)->object_start_bitmap();
   const HeapObjectHeader* header =
       bitmap.FindHeader<mode>(static_cast<ConstAddress>(address));
   DCHECK_LT(address, reinterpret_cast<ConstAddress>(header) +
@@ -303,7 +307,7 @@ const HeapObjectHeader& BasePage::ObjectHeaderFromInnerAddress(
   // reference to a mixin type
   SynchronizedLoad();
   const HeapObjectHeader* header =
-      ObjectHeaderFromInnerAddressImpl<mode>(this, address);
+      ObjectHeaderFromInnerAddressImpl<mode>(address);
   DCHECK_NE(kFreeListGCInfoIndex, header->GetGCInfoIndex<mode>());
   return *header;
 }

src/heap/cppgc/trace-trait.cc

@@ -5,6 +5,7 @@
 #include "include/cppgc/trace-trait.h"
 
 #include "src/heap/cppgc/gc-info-table.h"
+#include "src/heap/cppgc/heap-base.h"
 #include "src/heap/cppgc/heap-page.h"
 
 namespace cppgc {
@@ -15,6 +16,7 @@ TraceDescriptor TraceTraitFromInnerAddressImpl::GetTraceDescriptor(
   // address is guaranteed to be on a normal page because this is used only for
   // mixins.
   const BasePage* page = BasePage::FromPayload(address);
+  DCHECK(page->heap().CanLookupObjectStartInBitmap());
   page->SynchronizedLoad();
   const HeapObjectHeader& header =
       page->ObjectHeaderFromInnerAddress<AccessMode::kAtomic>(address);

test/unittests/heap/cppgc/ephemeron-pair-unittest.cc

@@ -222,6 +222,7 @@ TEST_F(EphemeronPairTest, EphemeronPairWithMixinKey) {
                                                       key, value);
   EXPECT_NE(static_cast<void*>(key), holder->ephemeron_pair().key.Get());
   EXPECT_NE(static_cast<void*>(value), holder->ephemeron_pair().value.Get());
+  testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap());
   InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get());
   FinishSteps();
   EXPECT_FALSE(HeapObjectHeader::FromObject(value).IsMarked());
@@ -238,6 +239,7 @@ TEST_F(EphemeronPairTest, EphemeronPairWithEmptyMixinValue) {
                                                       key, nullptr);
   EXPECT_NE(static_cast<void*>(key), holder->ephemeron_pair().key.Get());
   EXPECT_TRUE(HeapObjectHeader::FromObject(key).TryMarkAtomic());
+  testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap());
   InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get());
   FinishSteps();
   FinishMarking();

test/unittests/heap/cppgc/garbage-collected-unittest.cc

@@ -99,6 +99,7 @@ TEST_F(GarbageCollectedTestWithHeap, GetObjectStartReturnsCurrentAddress) {
   GCed* gced = MakeGarbageCollected<GCed>(GetAllocationHandle());
   GCedWithMixin* gced_with_mixin =
       MakeGarbageCollected<GCedWithMixin>(GetAllocationHandle());
+  testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap());
   const void* base_object_payload = TraceTrait<Mixin>::GetTraceDescriptor(
                                         static_cast<Mixin*>(gced_with_mixin))
                                         .base_object_payload;

test/unittests/heap/cppgc/marking-visitor-unittest.cc

@@ -50,10 +50,14 @@ class TestMarkingVisitor : public MutatorMarkingVisitor {
  public:
   explicit TestMarkingVisitor(Marker* marker)
       : MutatorMarkingVisitor(marker->heap(),
-                              marker->MutatorMarkingStateForTesting()) {}
+                              marker->MutatorMarkingStateForTesting()),
+        allow_access_(marker->heap()) {}
   ~TestMarkingVisitor() { marking_state_.Publish(); }
 
   BasicMarkingState& marking_state() { return marking_state_; }
+
+ private:
+  testing::AllowLookupOfObjectStartInBitmap allow_access_;
 };
 
 }  // namespace

test/unittests/heap/cppgc/prefinalizer-unittest.cc

@@ -369,5 +369,56 @@ TEST_F(PrefinalizerTest, VirtualPrefinalizer) {
   EXPECT_LT(0u, GCedInherited::prefinalizer_count_);
 }
 
+namespace {
+
+class MixinCallingGC : public GarbageCollectedMixin {
+ public:
+  template <typename GCFunction>
+  explicit MixinCallingGC(GCFunction gc) {
+    gc();
+  }
+
+  void Trace(Visitor*) const override {}
+};
+
+class MixinWithPrefinalizer : public GarbageCollectedMixin {
+  CPPGC_USING_PRE_FINALIZER(MixinWithPrefinalizer, PreFinalize);
+
+ public:
+  MixinWithPrefinalizer() = default;
+  void PreFinalize() {}
+
+  void Trace(Visitor*) const override {}
+};
+
+class BaseWithMixins : public GarbageCollected<BaseWithMixins>,
+                       public MixinCallingGC,
+                       public MixinWithPrefinalizer {
+ public:
+  template <typename GCFunction>
+  explicit BaseWithMixins(GCFunction gc) : MixinCallingGC(gc) {}
+
+  void Trace(Visitor* v) const override {
+    MixinCallingGC::Trace(v);
+    MixinWithPrefinalizer::Trace(v);
+  }
+};
+
+}  // namespace
+
+TEST_F(PrefinalizerTest, GCBeforePrefinalizerRegistration) {
+  // Regression test: https://crbug.com/1307471
+  MakeGarbageCollected<BaseWithMixins>(GetAllocationHandle(), [this]() {
+    internal::Heap::From(GetHeap())->CollectGarbage(
+        {internal::GarbageCollector::Config::CollectionType::kMajor,
+         cppgc::Heap::StackState::kMayContainHeapPointers,
+         Heap::MarkingType::kAtomic,
+         Heap::SweepingType::kIncrementalAndConcurrent,
+         internal::GarbageCollector::Config::FreeMemoryHandling::
+             kDiscardWherePossible,
+         internal::GarbageCollector::Config::IsForcedGC::kForced});
+  });
+}
+
 }  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/tests.cc

@@ -38,6 +38,21 @@ void TestWithHeap::ResetLinearAllocationBuffers() {
 TestSupportingAllocationOnly::TestSupportingAllocationOnly()
     : no_gc_scope_(GetHeap()->GetHeapHandle()) {}
 
+AllowLookupOfObjectStartInBitmap::AllowLookupOfObjectStartInBitmap(
+    cppgc::Heap& heap)
+    : AllowLookupOfObjectStartInBitmap(
+          *static_cast<HeapBase*>(Heap::From(&heap))) {}
+
+AllowLookupOfObjectStartInBitmap::AllowLookupOfObjectStartInBitmap(
+    HeapBase& heap)
+    : heap_(heap) {
+  heap_.set_allow_lookup_of_object_start_in_bitmap(true);
+}
+
+AllowLookupOfObjectStartInBitmap::~AllowLookupOfObjectStartInBitmap() {
+  heap_.set_allow_lookup_of_object_start_in_bitmap(false);
+}
+
 }  // namespace testing
 }  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/tests.h

@@ -125,6 +125,16 @@ class TestSupportingAllocationOnly : public TestWithHeap {
   subtle::NoGarbageCollectionScope no_gc_scope_;
 };
 
+class AllowLookupOfObjectStartInBitmap final {
+ public:
+  explicit AllowLookupOfObjectStartInBitmap(cppgc::Heap&);
+  explicit AllowLookupOfObjectStartInBitmap(HeapBase&);
+  ~AllowLookupOfObjectStartInBitmap();
+
+ private:
+  HeapBase& heap_;
+};
+
 }  // namespace testing
 }  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/visitor-unittest.cc

@@ -68,6 +68,7 @@ TEST_F(TraceTraitTest, GetObjectStartGCedMixin) {
   auto* gced_mixin_app =
       MakeGarbageCollected<GCedMixinApplication>(GetAllocationHandle());
   auto* gced_mixin = static_cast<GCedMixin*>(gced_mixin_app);
+  testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap());
   EXPECT_EQ(gced_mixin_app,
             TraceTrait<GCedMixin>::GetTraceDescriptor(gced_mixin)
                 .base_object_payload);
@@ -102,6 +103,7 @@ TEST_F(TraceTraitTest, TraceGCedMixinThroughTraceDescriptor) {
       MakeGarbageCollected<GCedMixinApplication>(GetAllocationHandle());
   auto* gced_mixin = static_cast<GCedMixin*>(gced_mixin_app);
   EXPECT_EQ(0u, GCed::trace_callcount);
+  testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap());
   TraceDescriptor desc = TraceTrait<GCedMixin>::GetTraceDescriptor(gced_mixin);
   desc.callback(nullptr, desc.base_object_payload);
   EXPECT_EQ(1u, GCed::trace_callcount);
@@ -119,6 +121,7 @@ TEST_F(TraceTraitTest, MixinInstanceWithoutTrace) {
   auto* mixin_without_trace =
       MakeGarbageCollected<MixinInstanceWithoutTrace>(GetAllocationHandle());
   auto* mixin = static_cast<GCedMixin*>(mixin_without_trace);
+  testing::AllowLookupOfObjectStartInBitmap allow_access(*GetHeap());
   EXPECT_EQ(0u, GCedMixin::trace_callcount);
   TraceDescriptor mixin_without_trace_desc =
       TraceTrait<MixinInstanceWithoutTrace>::GetTraceDescriptor(
@@ -138,8 +141,8 @@ namespace {
 
 class DispatchingVisitor final : public VisitorBase {
  public:
-  DispatchingVisitor(const void* object, const void* payload)
-      : object_(object), payload_(payload) {}
+  DispatchingVisitor(cppgc::Heap& heap, const void* object, const void* payload)
+      : allow_access_(heap), object_(object), payload_(payload) {}
 
  protected:
   void Visit(const void* t, TraceDescriptor desc) final {
@@ -157,6 +160,7 @@ class DispatchingVisitor final : public VisitorBase {
   }
 
  private:
+  testing::AllowLookupOfObjectStartInBitmap allow_access_;
   const void* object_;
   const void* payload_;
 };
@@ -165,7 +169,7 @@ class DispatchingVisitor final : public VisitorBase {
 
 TEST_F(VisitorTest, DispatchTraceGCed) {
   Member<GCed> ref = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  DispatchingVisitor visitor(ref, ref);
+  DispatchingVisitor visitor(*GetHeap(), ref, ref);
   EXPECT_EQ(0u, GCed::trace_callcount);
   visitor.Trace(ref);
   EXPECT_EQ(1u, GCed::trace_callcount);
@@ -178,7 +182,7 @@ TEST_F(VisitorTest, DispatchTraceGCedMixin) {
   // 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);
+  DispatchingVisitor visitor(*GetHeap(), gced_mixin, gced_mixin_app);
   EXPECT_EQ(0u, GCed::trace_callcount);
   visitor.Trace(ref);
   EXPECT_EQ(1u, GCed::trace_callcount);
@@ -186,7 +190,7 @@ TEST_F(VisitorTest, DispatchTraceGCedMixin) {
 
 TEST_F(VisitorTest, DispatchTraceWeakGCed) {
   WeakMember<GCed> ref = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  DispatchingVisitor visitor(ref, ref);
+  DispatchingVisitor visitor(*GetHeap(), ref, ref);
   visitor.Trace(ref);
   // No marking, so reference should be cleared.
   EXPECT_EQ(nullptr, ref.Get());
@@ -199,7 +203,7 @@ TEST_F(VisitorTest, DispatchTraceWeakGCedMixin) {
   // Ensure that we indeed test dispatching an inner object.
   EXPECT_NE(static_cast<void*>(gced_mixin_app), static_cast<void*>(gced_mixin));
   WeakMember<GCedMixin> ref = gced_mixin;
-  DispatchingVisitor visitor(gced_mixin, gced_mixin_app);
+  DispatchingVisitor visitor(*GetHeap(), gced_mixin, gced_mixin_app);
   visitor.Trace(ref);
   // No marking, so reference should be cleared.
   EXPECT_EQ(nullptr, ref.Get());
@@ -290,7 +294,7 @@ class GCedWithComposite final : public GarbageCollected<GCedWithComposite> {
 TEST_F(VisitorTest, DispatchToCompositeObject) {
   Member<GCedWithComposite> ref =
       MakeGarbageCollected<GCedWithComposite>(GetAllocationHandle());
-  DispatchingVisitor visitor(ref, ref);
+  DispatchingVisitor visitor(*GetHeap(), ref, ref);
   EXPECT_EQ(0u, Composite::callback_callcount);
   visitor.Trace(ref);
   EXPECT_EQ(1u, Composite::callback_callcount);