cppgc: Persistent: Check thread usage on slow path

Checks whether a Persistent is used from the creation thread on slow
path allocations. In practice, these currently happen every 256
Persistent allocations. This is a best effort check that may help to
flush out issues that are missed with DCHECK builds.

Bug: chromium:1276570
Change-Id: Ia868ca436341b1b5ef427d5b3ec04926c1394e41
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3318658
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78276}

include/cppgc/internal/persistent-node.h

@@ -80,23 +80,31 @@ class V8_EXPORT PersistentRegionBase {
   using PersistentNodeSlots = std::array<PersistentNode, 256u>;
 
  public:
-  explicit PersistentRegionBase(const FatalOutOfMemoryHandler& oom_handler);
   // Clears Persistent fields to avoid stale pointers after heap teardown.
   ~PersistentRegionBase();
 
   PersistentRegionBase(const PersistentRegionBase&) = delete;
   PersistentRegionBase& operator=(const PersistentRegionBase&) = delete;
 
-  PersistentNode* AllocateNode(void* owner, TraceCallback trace) {
-    if (!free_list_head_) {
-      EnsureNodeSlots();
-      CPPGC_DCHECK(free_list_head_);
-    }
-    PersistentNode* node = free_list_head_;
+  void Trace(Visitor*);
+
+  size_t NodesInUse() const;
+
+  void ClearAllUsedNodes();
+
+ protected:
+  explicit PersistentRegionBase(const FatalOutOfMemoryHandler& oom_handler);
+
+  PersistentNode* TryAllocateNodeFromFreeList(void* owner,
+                                              TraceCallback trace) {
+    PersistentNode* node = nullptr;
+    if (V8_LIKELY(free_list_head_)) {
+      node = free_list_head_;
       free_list_head_ = free_list_head_->FreeListNext();
       CPPGC_DCHECK(!node->IsUsed());
       node->InitializeAsUsedNode(owner, trace);
       nodes_in_use_++;
+    }
     return node;
   }
 
@@ -109,18 +117,15 @@ class V8_EXPORT PersistentRegionBase {
     nodes_in_use_--;
   }
 
-  void Trace(Visitor*);
-
-  size_t NodesInUse() const;
-
-  void ClearAllUsedNodes();
+  PersistentNode* RefillFreeListAndAllocateNode(void* owner,
+                                                TraceCallback trace);
 
  private:
-  void EnsureNodeSlots();
-
   template <typename PersistentBaseClass>
   void ClearAllUsedNodes();
 
+  void RefillFreeList();
+
   std::vector<std::unique_ptr<PersistentNodeSlots>> nodes_;
   PersistentNode* free_list_head_ = nullptr;
   size_t nodes_in_use_ = 0;
@@ -142,7 +147,12 @@ class V8_EXPORT PersistentRegion final : public PersistentRegionBase {
 
   V8_INLINE PersistentNode* AllocateNode(void* owner, TraceCallback trace) {
     CPPGC_DCHECK(IsCreationThread());
-    return PersistentRegionBase::AllocateNode(owner, trace);
+    auto* node = TryAllocateNodeFromFreeList(owner, trace);
+    if (V8_LIKELY(node)) return node;
+
+    // Slow path allocation allows for checking thread correspondence.
+    CPPGC_CHECK(IsCreationThread());
+    return RefillFreeListAndAllocateNode(owner, trace);
   }
 
   V8_INLINE void FreeNode(PersistentNode* node) {
@@ -181,7 +191,10 @@ class V8_EXPORT CrossThreadPersistentRegion final
 
   V8_INLINE PersistentNode* AllocateNode(void* owner, TraceCallback trace) {
     PersistentRegionLock::AssertLocked();
-    return PersistentRegionBase::AllocateNode(owner, trace);
+    auto* node = TryAllocateNodeFromFreeList(owner, trace);
+    if (V8_LIKELY(node)) return node;
+
+    return RefillFreeListAndAllocateNode(owner, trace);
   }
 
   V8_INLINE void FreeNode(PersistentNode* node) {

src/heap/cppgc/persistent-node.cc

@@ -63,10 +63,10 @@ size_t PersistentRegionBase::NodesInUse() const {
   return nodes_in_use_;
 }
 
-void PersistentRegionBase::EnsureNodeSlots() {
+void PersistentRegionBase::RefillFreeList() {
   auto node_slots = std::make_unique<PersistentNodeSlots>();
   if (!node_slots.get()) {
-    oom_handler_("Oilpan: PersistentRegionBase::EnsureNodeSlots()");
+    oom_handler_("Oilpan: PersistentRegionBase::RefillFreeList()");
   }
   nodes_.push_back(std::move(node_slots));
   for (auto& node : *nodes_.back()) {
@@ -75,6 +75,14 @@ void PersistentRegionBase::EnsureNodeSlots() {
   }
 }
 
+PersistentNode* PersistentRegionBase::RefillFreeListAndAllocateNode(
+    void* owner, TraceCallback trace) {
+  RefillFreeList();
+  auto* node = TryAllocateNodeFromFreeList(owner, trace);
+  CPPGC_DCHECK(node);
+  return node;
+}
+
 void PersistentRegionBase::Trace(Visitor* visitor) {
   free_list_head_ = nullptr;
   for (auto& slots : nodes_) {

test/unittests/heap/cppgc/persistent-family-unittest.cc

@@ -7,12 +7,15 @@
 #include "include/cppgc/allocation.h"
 #include "include/cppgc/cross-thread-persistent.h"
 #include "include/cppgc/garbage-collected.h"
+#include "include/cppgc/internal/persistent-node.h"
 #include "include/cppgc/internal/pointer-policies.h"
 #include "include/cppgc/member.h"
 #include "include/cppgc/persistent.h"
 #include "include/cppgc/source-location.h"
 #include "include/cppgc/type-traits.h"
 #include "src/base/logging.h"
+#include "src/base/platform/platform.h"
+#include "src/heap/cppgc/globals.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/liveness-broker.h"
 #include "src/heap/cppgc/visitor.h"
@@ -110,6 +113,7 @@ class RootVisitor final : public VisitorBase {
 };
 
 class PersistentTest : public testing::TestWithHeap {};
+class PersistentDeathTest : public testing::TestWithHeap {};
 
 }  // namespace
 
@@ -1042,5 +1046,39 @@ TEST_F(PersistentTest, ObjectReclaimedAfterClearedPersistent) {
   EXPECT_FALSE(weak_finalized);
 }
 
+namespace {
+
+class PersistentAccessOnBackgroundThread : public v8::base::Thread {
+ public:
+  explicit PersistentAccessOnBackgroundThread(GCed* raw_gced)
+      : v8::base::Thread(v8::base::Thread::Options(
+            "PersistentAccessOnBackgroundThread", 2 * kMB)),
+        raw_gced_(raw_gced) {}
+
+  void Run() override {
+    EXPECT_DEATH_IF_SUPPORTED(
+        Persistent<GCed> gced(static_cast<GCed*>(raw_gced_)), "");
+  }
+
+ private:
+  void* raw_gced_;
+};
+
+}  // namespace
+
+TEST_F(PersistentDeathTest, CheckCreationThread) {
+#ifdef DEBUG
+  // In DEBUG mode, every Persistent creation should check whether the handle
+  // is created on the right thread. In release mode, this check is only
+  // performed on slow path allocations.
+  Persistent<GCed> first_persistent_triggers_slow_path(
+      MakeGarbageCollected<GCed>(GetAllocationHandle()));
+#endif  // DEBUG
+  PersistentAccessOnBackgroundThread thread(
+      MakeGarbageCollected<GCed>(GetAllocationHandle()));
+  CHECK(thread.StartSynchronously());
+  thread.Join();
+}
+
 }  // namespace internal
 }  // namespace cppgc