cppgc: Check mark bit on assignment from prefinalizer.

Check that the marked bit of an object is set if assigned during a
prefinalizer to a Member in a live object or a Persistent.

Bug: v8:11749
Change-Id: I993c0d226a4157698591e1f7bc0c55e5c79239b6
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2897093
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74672}

BUILD.gn

@@ -283,6 +283,10 @@ declare_args() {
   # TODO(v8:11785): Enable by default when running with the verifier.
   cppgc_enable_verify_live_bytes = false
 
+  # Enable assignment checks for Members/Persistents during prefinalizer invocations.
+  # TODO(v8:11749): Enable by default after fixing any existing issues in Blink.
+  cppgc_enable_check_assignments_in_prefinalizers = false
+
   # Enable young generation in cppgc.
   cppgc_enable_young_generation = false
 
@@ -711,6 +715,10 @@ config("features") {
     defines += [ "CPPGC_VERIFY_LIVE_BYTES" ]
   }
 
+  if (cppgc_enable_check_assignments_in_prefinalizers) {
+    defines += [ "CPPGC_CHECK_ASSIGNMENTS_IN_PREFINALIZERS" ]
+  }
+
   if (v8_embedder_string != "") {
     defines += [ "V8_EMBEDDER_STRING=\"$v8_embedder_string\"" ]
   }

include/cppgc/internal/pointer-policies.h

@@ -82,7 +82,7 @@ class V8_EXPORT EnabledCheckingPolicy {
 
 class DisabledCheckingPolicy {
  protected:
-  void CheckPointer(const void* raw) {}
+  void CheckPointer(const void*) {}
 };
 
 #if V8_ENABLE_CHECKS
@@ -92,6 +92,10 @@ using DefaultPersistentCheckingPolicy = EnabledCheckingPolicy;
 using DefaultMemberCheckingPolicy = DisabledCheckingPolicy;
 using DefaultPersistentCheckingPolicy = DisabledCheckingPolicy;
 #endif
+// For CT(W)P neither marking information (for value), nor objectstart bitmap
+// (for slot) are guaranteed to be present because there's no synchonization
+// between heaps after marking.
+using DefaultCrossThreadPersistentCheckingPolicy = DisabledCheckingPolicy;
 
 class KeepLocationPolicy {
  public:
@@ -154,7 +158,7 @@ struct WeakCrossThreadPersistentPolicy {
 // Forward declarations setting up the default policies.
 template <typename T, typename WeaknessPolicy,
           typename LocationPolicy = DefaultLocationPolicy,
-          typename CheckingPolicy = DisabledCheckingPolicy>
+          typename CheckingPolicy = DefaultCrossThreadPersistentCheckingPolicy>
 class BasicCrossThreadPersistent;
 template <typename T, typename WeaknessPolicy,
           typename LocationPolicy = DefaultLocationPolicy,

src/heap/cppgc/heap-base.h

@@ -114,6 +114,9 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
   PreFinalizerHandler* prefinalizer_handler() {
     return prefinalizer_handler_.get();
   }
+  const PreFinalizerHandler* prefinalizer_handler() const {
+    return prefinalizer_handler_.get();
+  }
 
   MarkerBase* marker() const { return marker_.get(); }
 

src/heap/cppgc/pointer-policies.cc

@@ -13,6 +13,7 @@
 #include "src/heap/cppgc/heap-page.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/page-memory.h"
+#include "src/heap/cppgc/prefinalizer-handler.h"
 #include "src/heap/cppgc/process-heap.h"
 
 namespace cppgc {
@@ -67,7 +68,20 @@ void EnabledCheckingPolicy::CheckPointerImpl(const void* ptr,
     DCHECK(!header->IsFree());
   }
 
-  // TODO(v8:11749): Check mark bits when during pre-finalizer phase.
+#ifdef CPPGC_CHECK_ASSIGNMENTS_IN_PREFINALIZERS
+  if (heap_->prefinalizer_handler()->IsInvokingPreFinalizers()) {
+    // During prefinalizers invocation, check that |ptr| refers to a live object
+    // and that it is assigned to a live slot.
+    DCHECK(header->IsMarked());
+    // Slot can be in a large object.
+    const auto* slot_page = BasePage::FromInnerAddress(heap_, this);
+    // Off-heap slots (from other heaps or on-stack) are considered live.
+    bool slot_is_live =
+        !slot_page || slot_page->ObjectHeaderFromInnerAddress(this).IsMarked();
+    DCHECK(slot_is_live);
+    USE(slot_is_live);
+  }
+#endif  // CPPGC_CHECK_ASSIGNMENTS_IN_PREFINALIZERS
 }
 
 PersistentRegion& StrongPersistentPolicy::GetPersistentRegion(

src/heap/cppgc/prefinalizer-handler.cc

@@ -53,6 +53,7 @@ void PreFinalizerHandler::InvokePreFinalizers() {
 
   DCHECK(CurrentThreadIsCreationThread());
   LivenessBroker liveness_broker = LivenessBrokerFactory::Create();
+  is_invoking_ = true;
   ordered_pre_finalizers_.erase(
       ordered_pre_finalizers_.begin(),
       std::remove_if(ordered_pre_finalizers_.rbegin(),
@@ -61,6 +62,7 @@ void PreFinalizerHandler::InvokePreFinalizers() {
                        return (pf.callback)(liveness_broker, pf.object);
                      })
           .base());
+  is_invoking_ = false;
   ordered_pre_finalizers_.shrink_to_fit();
 }
 

src/heap/cppgc/prefinalizer-handler.h

@@ -25,6 +25,8 @@ class PreFinalizerHandler final {
 
   void InvokePreFinalizers();
 
+  bool IsInvokingPreFinalizers() const { return is_invoking_; }
+
  private:
   // Checks that the current thread is the thread that created the heap.
   bool CurrentThreadIsCreationThread();
@@ -36,6 +38,7 @@ class PreFinalizerHandler final {
   std::vector<PreFinalizer> ordered_pre_finalizers_;
 
   HeapBase& heap_;
+  bool is_invoking_ = false;
 #ifdef DEBUG
   int creation_thread_id_;
 #endif

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

@@ -226,22 +226,10 @@ TEST_F(PrefinalizerTest, PrefinalizerCanRewireGraphWithLiveObjects) {
   PreciseGC();
 }
 
-TEST_F(PrefinalizerTest, PrefinalizerCanRewireGraphWithDeadObjects) {
-  Persistent<LinkedNode> root{MakeGarbageCollected<LinkedNode>(
-      GetAllocationHandle(),
-      MakeGarbageCollected<LinkedNode>(
-          GetAllocationHandle(),
-          MakeGarbageCollected<LinkedNode>(GetAllocationHandle(), nullptr)))};
-  CHECK(root->next());
-  MakeGarbageCollected<MutatingPrefinalizer>(GetAllocationHandle(), root.Get());
-  // All LinkedNode objects will die on the following GC. The pre-finalizer may
-  // still operate with them but not add them to a live object.
-  root.Clear();
-  PreciseGC();
-}
-
 namespace {
 
+class PrefinalizerDeathTest : public testing::TestWithHeap {};
+
 class AllocatingPrefinalizer : public GarbageCollected<AllocatingPrefinalizer> {
   CPPGC_USING_PRE_FINALIZER(AllocatingPrefinalizer, PreFinalizer);
 
@@ -260,7 +248,7 @@ class AllocatingPrefinalizer : public GarbageCollected<AllocatingPrefinalizer> {
 
 #ifdef DEBUG
 
-TEST_F(PrefinalizerTest, PrefinalizerFailsOnAllcoation) {
+TEST_F(PrefinalizerDeathTest, PrefinalizerFailsOnAllcoation) {
   auto* object = MakeGarbageCollected<AllocatingPrefinalizer>(
       GetAllocationHandle(), GetHeap());
   USE(object);
@@ -269,5 +257,69 @@ TEST_F(PrefinalizerTest, PrefinalizerFailsOnAllcoation) {
 
 #endif  // DEBUG
 
+namespace {
+
+template <template <typename T> class RefType>
+class RessurectingPrefinalizer
+    : public GarbageCollected<RessurectingPrefinalizer<RefType>> {
+  CPPGC_USING_PRE_FINALIZER(RessurectingPrefinalizer, PreFinalizer);
+
+ public:
+  explicit RessurectingPrefinalizer(RefType<GCed>& ref, GCed* obj)
+      : ref_(ref), obj_(obj) {}
+  void Trace(Visitor*) const {}
+  void PreFinalizer() { ref_ = obj_; }
+
+ private:
+  RefType<GCed>& ref_;
+  GCed* obj_;
+};
+
+class GCedHolder : public GarbageCollected<GCedHolder> {
+ public:
+  void Trace(Visitor* v) const { v->Trace(member_); }
+
+  Member<GCed> member_;
+};
+
+}  // namespace
+
+#if V8_ENABLE_CHECKS
+#ifdef CPPGC_CHECK_ASSIGNMENTS_IN_PREFINALIZERS
+
+TEST_F(PrefinalizerDeathTest, PrefinalizerCantRewireGraphWithDeadObjects) {
+  Persistent<LinkedNode> root{MakeGarbageCollected<LinkedNode>(
+      GetAllocationHandle(),
+      MakeGarbageCollected<LinkedNode>(
+          GetAllocationHandle(),
+          MakeGarbageCollected<LinkedNode>(GetAllocationHandle(), nullptr)))};
+  CHECK(root->next());
+  MakeGarbageCollected<MutatingPrefinalizer>(GetAllocationHandle(), root.Get());
+  // All LinkedNode objects will die on the following GC. The pre-finalizer may
+  // still operate with them but not add them to a live object.
+  root.Clear();
+  EXPECT_DEATH_IF_SUPPORTED(PreciseGC(), "");
+}
+
+TEST_F(PrefinalizerDeathTest, PrefinalizerCantRessurectObjectOnStack) {
+  Persistent<GCed> persistent;
+  MakeGarbageCollected<RessurectingPrefinalizer<Persistent>>(
+      GetAllocationHandle(), persistent,
+      MakeGarbageCollected<GCed>(GetAllocationHandle()));
+  EXPECT_DEATH_IF_SUPPORTED(PreciseGC(), "");
+}
+
+TEST_F(PrefinalizerDeathTest, PrefinalizerCantRessurectObjectOnHeap) {
+  Persistent<GCedHolder> persistent(
+      MakeGarbageCollected<GCedHolder>(GetAllocationHandle()));
+  MakeGarbageCollected<RessurectingPrefinalizer<Member>>(
+      GetAllocationHandle(), persistent->member_,
+      MakeGarbageCollected<GCed>(GetAllocationHandle()));
+  EXPECT_DEATH_IF_SUPPORTED(PreciseGC(), "");
+}
+
+#endif  // CPPGC_CHECK_ASSIGNMENTS_IN_PREFINALIZERS
+#endif  // V8_ENABLE_CHECKS
+
 }  // namespace internal
 }  // namespace cppgc

tools/cppgc/gen_cmake.py

@@ -245,6 +245,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON)
 option(CPPGC_ENABLE_OBJECT_NAMES "Enable object names in cppgc for debug purposes" OFF)
 option(CPPGC_ENABLE_CAGED_HEAP "Enable heap reservation of size 4GB, only possible for 64bit archs" OFF)
 option(CPPGC_ENABLE_VERIFY_LIVE_BYTES " Enable verification of live bytes in the marking verifier" OFF)
+option(CPPGC_CHECK_ASSIGNMENTS_IN_PREFINALIZERS " Enable assignment checks for Members/Persistents during prefinalizer invocations" OFF)
 option(CPPGC_ENABLE_YOUNG_GENERATION "Enable young generation in cppgc" OFF)
 set(CPPGC_TARGET_ARCH "x64" CACHE STRING "Target architecture, possible options: x64, x86, arm, arm64, ppc64, s390x, mipsel, mips64el")
 
@@ -437,6 +438,9 @@ endif()
 if(CPPGC_ENABLE_VERIFY_LIVE_BYTES)
   target_compile_definitions({target.name} PRIVATE "-DCPPGC_VERIFY_LIVE_BYTES")
 endif()
+if(CPPGC_CHECK_ASSIGNMENTS_IN_PREFINALIZERS)
+  target_compile_definitions({target.name} PRIVATE "-DCPPGC_CHECK_ASSIGNMENTS_IN_PREFINALIZERS")
+endif()
 if(CPPGC_ENABLE_YOUNG_GENERATION)
   target_compile_definitions({target.name} PRIVATE "-DCPPGC_YOUNG_GENERATION")
 endif()"""