cppgc: Check same thread accesses to PersistentRegion

Bug: chromium:1056170
Change-Id: I355187177d062bf7117bcbd402821f2b9dd739de
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3194267
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77163}

include/cppgc/internal/persistent-node.h

@@ -75,16 +75,16 @@ class PersistentNode final {
   TraceCallback trace_ = nullptr;
 };
 
-class V8_EXPORT PersistentRegion {
+class V8_EXPORT PersistentRegionBase {
   using PersistentNodeSlots = std::array<PersistentNode, 256u>;
 
  public:
-  PersistentRegion() = default;
+  PersistentRegionBase() = default;
   // Clears Persistent fields to avoid stale pointers after heap teardown.
-  ~PersistentRegion();
+  ~PersistentRegionBase();
 
-  PersistentRegion(const PersistentRegion&) = delete;
-  PersistentRegion& operator=(const PersistentRegion&) = delete;
+  PersistentRegionBase(const PersistentRegionBase&) = delete;
+  PersistentRegionBase& operator=(const PersistentRegionBase&) = delete;
 
   PersistentNode* AllocateNode(void* owner, TraceCallback trace) {
     if (!free_list_head_) {
@@ -126,8 +126,39 @@ class V8_EXPORT PersistentRegion {
   friend class CrossThreadPersistentRegion;
 };
 
-// CrossThreadPersistent uses PersistentRegion but protects it using this lock
-// when needed.
+// Variant of PersistentRegionBase that checks whether the allocation and
+// freeing happens only on the thread that created the region.
+class V8_EXPORT PersistentRegion final : public PersistentRegionBase {
+ public:
+  PersistentRegion();
+  // Clears Persistent fields to avoid stale pointers after heap teardown.
+  ~PersistentRegion() = default;
+
+  PersistentRegion(const PersistentRegion&) = delete;
+  PersistentRegion& operator=(const PersistentRegion&) = delete;
+
+  V8_INLINE PersistentNode* AllocateNode(void* owner, TraceCallback trace) {
+#if V8_ENABLE_CHECKS
+    CheckIsCreationThread();
+#endif  // V8_ENABLE_CHECKS
+    return PersistentRegionBase::AllocateNode(owner, trace);
+  }
+
+  V8_INLINE void FreeNode(PersistentNode* node) {
+#if V8_ENABLE_CHECKS
+    CheckIsCreationThread();
+#endif  // V8_ENABLE_CHECKS
+    PersistentRegionBase::FreeNode(node);
+  }
+
+ private:
+  void CheckIsCreationThread();
+
+  int creation_thread_id_;
+};
+
+// CrossThreadPersistent uses PersistentRegionBase but protects it using this
+// lock when needed.
 class V8_EXPORT PersistentRegionLock final {
  public:
   PersistentRegionLock();
@@ -136,9 +167,10 @@ class V8_EXPORT PersistentRegionLock final {
   static void AssertLocked();
 };
 
-// Variant of PersistentRegion that checks whether the PersistentRegionLock is
-// locked.
-class V8_EXPORT CrossThreadPersistentRegion final : protected PersistentRegion {
+// Variant of PersistentRegionBase that checks whether the PersistentRegionLock
+// is locked.
+class V8_EXPORT CrossThreadPersistentRegion final
+    : protected PersistentRegionBase {
  public:
   CrossThreadPersistentRegion() = default;
   // Clears Persistent fields to avoid stale pointers after heap teardown.
@@ -150,12 +182,12 @@ class V8_EXPORT CrossThreadPersistentRegion final : protected PersistentRegion {
 
   V8_INLINE PersistentNode* AllocateNode(void* owner, TraceCallback trace) {
     PersistentRegionLock::AssertLocked();
-    return PersistentRegion::AllocateNode(owner, trace);
+    return PersistentRegionBase::AllocateNode(owner, trace);
   }
 
   V8_INLINE void FreeNode(PersistentNode* node) {
     PersistentRegionLock::AssertLocked();
-    PersistentRegion::FreeNode(node);
+    PersistentRegionBase::FreeNode(node);
   }
 
   void Trace(Visitor*);

include/cppgc/persistent.h

@@ -45,7 +45,7 @@ class PersistentBase {
   mutable const void* raw_ = nullptr;
   mutable PersistentNode* node_ = nullptr;
 
-  friend class PersistentRegion;
+  friend class PersistentRegionBase;
 };
 
 // The basic class from which all Persistent classes are generated.

src/heap/cppgc/persistent-node.cc

@@ -9,15 +9,16 @@
 
 #include "include/cppgc/cross-thread-persistent.h"
 #include "include/cppgc/persistent.h"
+#include "src/base/platform/platform.h"
 #include "src/heap/cppgc/process-heap.h"
 
 namespace cppgc {
 namespace internal {
 
-PersistentRegion::~PersistentRegion() { ClearAllUsedNodes(); }
+PersistentRegionBase::~PersistentRegionBase() { ClearAllUsedNodes(); }
 
 template <typename PersistentBaseClass>
-void PersistentRegion::ClearAllUsedNodes() {
+void PersistentRegionBase::ClearAllUsedNodes() {
   for (auto& slots : nodes_) {
     for (auto& node : *slots) {
       if (!node.IsUsed()) continue;
@@ -35,14 +36,15 @@ void PersistentRegion::ClearAllUsedNodes() {
   CPPGC_DCHECK(0u == nodes_in_use_);
 }
 
-template void PersistentRegion::ClearAllUsedNodes<CrossThreadPersistentBase>();
-template void PersistentRegion::ClearAllUsedNodes<PersistentBase>();
+template void
+PersistentRegionBase::ClearAllUsedNodes<CrossThreadPersistentBase>();
+template void PersistentRegionBase::ClearAllUsedNodes<PersistentBase>();
 
-void PersistentRegion::ClearAllUsedNodes() {
+void PersistentRegionBase::ClearAllUsedNodes() {
   ClearAllUsedNodes<PersistentBase>();
 }
 
-size_t PersistentRegion::NodesInUse() const {
+size_t PersistentRegionBase::NodesInUse() const {
 #ifdef DEBUG
   const size_t accumulated_nodes_in_use_ = std::accumulate(
       nodes_.cbegin(), nodes_.cend(), 0u, [](size_t acc, const auto& slots) {
@@ -56,7 +58,7 @@ size_t PersistentRegion::NodesInUse() const {
   return nodes_in_use_;
 }
 
-void PersistentRegion::EnsureNodeSlots() {
+void PersistentRegionBase::EnsureNodeSlots() {
   nodes_.push_back(std::make_unique<PersistentNodeSlots>());
   for (auto& node : *nodes_.back()) {
     node.InitializeAsFreeNode(free_list_head_);
@@ -64,7 +66,7 @@ void PersistentRegion::EnsureNodeSlots() {
   }
 }
 
-void PersistentRegion::Trace(Visitor* visitor) {
+void PersistentRegionBase::Trace(Visitor* visitor) {
   free_list_head_ = nullptr;
   for (auto& slots : nodes_) {
     bool is_empty = true;
@@ -92,6 +94,15 @@ void PersistentRegion::Trace(Visitor* visitor) {
                nodes_.end());
 }
 
+PersistentRegion::PersistentRegion()
+    : creation_thread_id_(v8::base::OS::GetCurrentThreadId()) {
+  USE(creation_thread_id_);
+}
+
+void PersistentRegion::CheckIsCreationThread() {
+  DCHECK_EQ(creation_thread_id_, v8::base::OS::GetCurrentThreadId());
+}
+
 PersistentRegionLock::PersistentRegionLock() {
   g_process_mutex.Pointer()->Lock();
 }
@@ -107,24 +118,24 @@ void PersistentRegionLock::AssertLocked() {
 
 CrossThreadPersistentRegion::~CrossThreadPersistentRegion() {
   PersistentRegionLock guard;
-  PersistentRegion::ClearAllUsedNodes<CrossThreadPersistentBase>();
+  PersistentRegionBase::ClearAllUsedNodes<CrossThreadPersistentBase>();
   nodes_.clear();
-  // PersistentRegion destructor will be a noop.
+  // PersistentRegionBase destructor will be a noop.
 }
 
 void CrossThreadPersistentRegion::Trace(Visitor* visitor) {
   PersistentRegionLock::AssertLocked();
-  PersistentRegion::Trace(visitor);
+  PersistentRegionBase::Trace(visitor);
 }
 
 size_t CrossThreadPersistentRegion::NodesInUse() const {
   // This method does not require a lock.
-  return PersistentRegion::NodesInUse();
+  return PersistentRegionBase::NodesInUse();
 }
 
 void CrossThreadPersistentRegion::ClearAllUsedNodes() {
   PersistentRegionLock::AssertLocked();
-  PersistentRegion::ClearAllUsedNodes<CrossThreadPersistentBase>();
+  PersistentRegionBase::ClearAllUsedNodes<CrossThreadPersistentBase>();
 }
 
 }  // namespace internal