Revert "cppgc: Add regression test and check for object start bitmap"

This reverts commit 164a040a2a.

Reason for revert: roll failure: https://ci.chromium.org/ui/p/chromium/builders/try/cast_shell_linux/1164753/overview

Original change's description:
> 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/+/3535782
> Reviewed-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}

Bug: chromium:1307471
Change-Id: I181e63a34eae9369184fb86112bc64e53b8bfad5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3545317
Owners-Override: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79590}

src/heap/cppgc/heap-base.cc

@@ -195,15 +195,6 @@ 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,11 +214,6 @@ 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(
@@ -289,10 +284,6 @@ 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,7 +106,8 @@ const HeapObjectHeader* BasePage::TryObjectHeaderFromInnerAddress(
   }
 
   // |address| is on the heap, so we FromInnerAddress can get the header.
-  const HeapObjectHeader* header = ObjectHeaderFromInnerAddressImpl(address);
+  const HeapObjectHeader* header =
+      ObjectHeaderFromInnerAddressImpl(this, address);
   if (header->IsFree()) return nullptr;
   DCHECK_NE(kFreeListGCInfoIndex, header->GetGCInfoIndex());
   return header;

src/heap/cppgc/heap-page.h

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

src/heap/cppgc/trace-trait.cc

@@ -5,7 +5,6 @@
 #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 {
@@ -16,7 +15,6 @@ 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,7 +222,6 @@ 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());
@@ -239,7 +238,6 @@ 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,7 +99,6 @@ 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,14 +50,10 @@ class TestMarkingVisitor : public MutatorMarkingVisitor {
  public:
   explicit TestMarkingVisitor(Marker* marker)
       : MutatorMarkingVisitor(marker->heap(),
-                              marker->MutatorMarkingStateForTesting()),
-        allow_access_(marker->heap()) {}
+                              marker->MutatorMarkingStateForTesting()) {}
   ~TestMarkingVisitor() { marking_state_.Publish(); }
 
   BasicMarkingState& marking_state() { return marking_state_; }
-
- private:
-  testing::AllowLookupOfObjectStartInBitmap allow_access_;
 };
 
 }  // namespace

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

@@ -369,56 +369,5 @@ 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,
-         cppgc::Heap::MarkingType::kAtomic,
-         cppgc::Heap::SweepingType::kIncrementalAndConcurrent,
-         internal::GarbageCollector::Config::FreeMemoryHandling::
-             kDiscardWherePossible,
-         internal::GarbageCollector::Config::IsForcedGC::kForced});
-  });
-}
-
 }  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/tests.cc

@@ -38,21 +38,6 @@ 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,16 +125,6 @@ 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,7 +68,6 @@ 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);
@@ -103,7 +102,6 @@ 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);
@@ -121,7 +119,6 @@ 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(
@@ -141,8 +138,8 @@ namespace {
 
 class DispatchingVisitor final : public VisitorBase {
  public:
-  DispatchingVisitor(cppgc::Heap& heap, const void* object, const void* payload)
-      : allow_access_(heap), object_(object), payload_(payload) {}
+  DispatchingVisitor(const void* object, const void* payload)
+      : object_(object), payload_(payload) {}
 
  protected:
   void Visit(const void* t, TraceDescriptor desc) final {
@@ -160,7 +157,6 @@ class DispatchingVisitor final : public VisitorBase {
   }
 
  private:
-  testing::AllowLookupOfObjectStartInBitmap allow_access_;
   const void* object_;
   const void* payload_;
 };
@@ -169,7 +165,7 @@ class DispatchingVisitor final : public VisitorBase {
 
 TEST_F(VisitorTest, DispatchTraceGCed) {
   Member<GCed> ref = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  DispatchingVisitor visitor(*GetHeap(), ref, ref);
+  DispatchingVisitor visitor(ref, ref);
   EXPECT_EQ(0u, GCed::trace_callcount);
   visitor.Trace(ref);
   EXPECT_EQ(1u, GCed::trace_callcount);
@@ -182,7 +178,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(*GetHeap(), gced_mixin, gced_mixin_app);
+  DispatchingVisitor visitor(gced_mixin, gced_mixin_app);
   EXPECT_EQ(0u, GCed::trace_callcount);
   visitor.Trace(ref);
   EXPECT_EQ(1u, GCed::trace_callcount);
@@ -190,7 +186,7 @@ TEST_F(VisitorTest, DispatchTraceGCedMixin) {
 
 TEST_F(VisitorTest, DispatchTraceWeakGCed) {
   WeakMember<GCed> ref = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  DispatchingVisitor visitor(*GetHeap(), ref, ref);
+  DispatchingVisitor visitor(ref, ref);
   visitor.Trace(ref);
   // No marking, so reference should be cleared.
   EXPECT_EQ(nullptr, ref.Get());
@@ -203,7 +199,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(*GetHeap(), gced_mixin, gced_mixin_app);
+  DispatchingVisitor visitor(gced_mixin, gced_mixin_app);
   visitor.Trace(ref);
   // No marking, so reference should be cleared.
   EXPECT_EQ(nullptr, ref.Get());
@@ -294,7 +290,7 @@ class GCedWithComposite final : public GarbageCollected<GCedWithComposite> {
 TEST_F(VisitorTest, DispatchToCompositeObject) {
   Member<GCedWithComposite> ref =
       MakeGarbageCollected<GCedWithComposite>(GetAllocationHandle());
-  DispatchingVisitor visitor(*GetHeap(), ref, ref);
+  DispatchingVisitor visitor(ref, ref);
   EXPECT_EQ(0u, Composite::callback_callcount);
   visitor.Trace(ref);
   EXPECT_EQ(1u, Composite::callback_callcount);