cppgc: Add checks and locks to (Weak)CrossThreadPersistents

This CL adds missing locks to the PersistentRegions for
(Weak)CrossThreadPersistents.
To make sure no locks are missed in the future, this CL also splits
PersistentRegion and introduces CrossThreadPersistentRegion that checks
whether a lock is taken whenever it is accessed.

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

include/cppgc/cross-thread-persistent.h

@@ -45,7 +45,7 @@ class BasicCrossThreadPersistent final : public PersistentBase,
       : PersistentBase(raw), LocationPolicy(loc) {
     if (!IsValid(raw)) return;
     PersistentRegionLock guard;
-    PersistentRegion& region = this->GetPersistentRegion(raw);
+    CrossThreadPersistentRegion& region = this->GetPersistentRegion(raw);
     SetNode(region.AllocateNode(this, &Trace));
     this->CheckPointer(raw);
   }
@@ -63,7 +63,7 @@ class BasicCrossThreadPersistent final : public PersistentBase,
       const SourceLocation& loc = SourceLocation::Current())
       : PersistentBase(raw), LocationPolicy(loc) {
     if (!IsValid(raw)) return;
-    PersistentRegion& region = this->GetPersistentRegion(raw);
+    CrossThreadPersistentRegion& region = this->GetPersistentRegion(raw);
     SetNode(region.AllocateNode(this, &Trace));
     this->CheckPointer(raw);
   }
@@ -192,7 +192,8 @@ class BasicCrossThreadPersistent final : public PersistentBase,
     const void* old_value = GetValue();
     if (IsValid(old_value)) {
       PersistentRegionLock guard;
-      PersistentRegion& region = this->GetPersistentRegion(old_value);
+      CrossThreadPersistentRegion& region =
+          this->GetPersistentRegion(old_value);
       region.FreeNode(GetNode());
       SetNode(nullptr);
     }
@@ -276,7 +277,8 @@ class BasicCrossThreadPersistent final : public PersistentBase,
     const void* old_value = GetValue();
     if (IsValid(old_value)) {
       PersistentRegionLock guard;
-      PersistentRegion& region = this->GetPersistentRegion(old_value);
+      CrossThreadPersistentRegion& region =
+          this->GetPersistentRegion(old_value);
       if (IsValid(ptr) && (&region == &this->GetPersistentRegion(ptr))) {
         SetValue(ptr);
         this->CheckPointer(ptr);
@@ -296,7 +298,8 @@ class BasicCrossThreadPersistent final : public PersistentBase,
     PersistentRegionLock::AssertLocked();
     const void* old_value = GetValue();
     if (IsValid(old_value)) {
-      PersistentRegion& region = this->GetPersistentRegion(old_value);
+      CrossThreadPersistentRegion& region =
+          this->GetPersistentRegion(old_value);
       if (IsValid(ptr) && (&region == &this->GetPersistentRegion(ptr))) {
         SetValue(ptr);
         this->CheckPointer(ptr);

include/cppgc/internal/persistent-node.h

@@ -19,6 +19,8 @@ class Visitor;
 
 namespace internal {
 
+class CrossThreadPersistentRegion;
+
 // PersistentNode represents a variant of two states:
 // 1) traceable node with a back pointer to the Persistent object;
 // 2) freelist entry.
@@ -116,6 +118,8 @@ class V8_EXPORT PersistentRegion final {
   std::vector<std::unique_ptr<PersistentNodeSlots>> nodes_;
   PersistentNode* free_list_head_ = nullptr;
   size_t nodes_in_use_ = 0;
+
+  friend class CrossThreadPersistentRegion;
 };
 
 // CrossThreadPersistent uses PersistentRegion but protects it using this lock
@@ -128,6 +132,38 @@ class V8_EXPORT PersistentRegionLock final {
   static void AssertLocked();
 };
 
+// Variant of PersistentRegion that checks whether the PersistentRegionLock is
+// locked.
+class V8_EXPORT CrossThreadPersistentRegion final {
+ public:
+  CrossThreadPersistentRegion() = default;
+  // Clears Persistent fields to avoid stale pointers after heap teardown.
+  ~CrossThreadPersistentRegion();
+
+  CrossThreadPersistentRegion(const CrossThreadPersistentRegion&) = delete;
+  CrossThreadPersistentRegion& operator=(const CrossThreadPersistentRegion&) =
+      delete;
+
+  V8_INLINE PersistentNode* AllocateNode(void* owner, TraceCallback trace) {
+    PersistentRegionLock::AssertLocked();
+    return persistent_region_.AllocateNode(owner, trace);
+  }
+
+  V8_INLINE void FreeNode(PersistentNode* node) {
+    PersistentRegionLock::AssertLocked();
+    persistent_region_.FreeNode(node);
+  }
+
+  void Trace(Visitor*);
+
+  size_t NodesInUse() const;
+
+  void ClearAllUsedNodes();
+
+ private:
+  PersistentRegion persistent_region_;
+};
+
 }  // namespace internal
 
 }  // namespace cppgc

include/cppgc/internal/pointer-policies.h

@@ -16,6 +16,7 @@ namespace cppgc {
 namespace internal {
 
 class PersistentRegion;
+class CrossThreadPersistentRegion;
 
 // Tags to distinguish between strong and weak member types.
 class StrongMemberTag;
@@ -115,12 +116,14 @@ struct WeakPersistentPolicy {
 
 struct StrongCrossThreadPersistentPolicy {
   using IsStrongPersistent = std::true_type;
-  static V8_EXPORT PersistentRegion& GetPersistentRegion(const void* object);
+  static V8_EXPORT CrossThreadPersistentRegion& GetPersistentRegion(
+      const void* object);
 };
 
 struct WeakCrossThreadPersistentPolicy {
   using IsStrongPersistent = std::false_type;
-  static V8_EXPORT PersistentRegion& GetPersistentRegion(const void* object);
+  static V8_EXPORT CrossThreadPersistentRegion& GetPersistentRegion(
+      const void* object);
 };
 
 // Forward declarations setting up the default policies.

src/heap/cppgc-js/cpp-snapshot.cc

@@ -704,6 +704,7 @@ void CppGraphBuilderImpl::Run() {
     ParentScope parent_scope(
         states_.CreateRootState(AddRootNode("C++ cross-thread roots")));
     GraphBuildingVisitor object_visitor(*this, parent_scope);
+    cppgc::internal::PersistentRegionLock guard;
     cpp_heap_.GetStrongCrossThreadPersistentRegion().Trace(&object_visitor);
   }
 }

src/heap/cppgc/heap-base.cc

@@ -111,11 +111,12 @@ void HeapBase::Terminate() {
 
     // Clear root sets.
     strong_persistent_region_.ClearAllUsedNodes();
-    strong_cross_thread_persistent_region_.ClearAllUsedNodes();
-    // Clear weak root sets, as the GC below does not execute weakness
-    // callbacks.
     weak_persistent_region_.ClearAllUsedNodes();
+    {
+      PersistentRegionLock guard;
+      strong_cross_thread_persistent_region_.ClearAllUsedNodes();
       weak_cross_thread_persistent_region_.ClearAllUsedNodes();
+    }
 
     stats_collector()->NotifyMarkingStarted(
         GarbageCollector::Config::CollectionType::kMajor,
@@ -131,6 +132,11 @@ void HeapBase::Terminate() {
 
   object_allocator().Terminate();
   disallow_gc_scope_++;
+
+  CHECK_EQ(0u, strong_persistent_region_.NodesInUse());
+  CHECK_EQ(0u, weak_persistent_region_.NodesInUse());
+  CHECK_EQ(0u, strong_cross_thread_persistent_region_.NodesInUse());
+  CHECK_EQ(0u, weak_cross_thread_persistent_region_.NodesInUse());
 }
 
 HeapStatistics HeapBase::CollectStatistics(

src/heap/cppgc/heap-base.h

@@ -130,16 +130,18 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
   const PersistentRegion& GetWeakPersistentRegion() const {
     return weak_persistent_region_;
   }
-  PersistentRegion& GetStrongCrossThreadPersistentRegion() {
+  CrossThreadPersistentRegion& GetStrongCrossThreadPersistentRegion() {
     return strong_cross_thread_persistent_region_;
   }
-  const PersistentRegion& GetStrongCrossThreadPersistentRegion() const {
+  const CrossThreadPersistentRegion& GetStrongCrossThreadPersistentRegion()
+      const {
     return strong_cross_thread_persistent_region_;
   }
-  PersistentRegion& GetWeakCrossThreadPersistentRegion() {
+  CrossThreadPersistentRegion& GetWeakCrossThreadPersistentRegion() {
     return weak_cross_thread_persistent_region_;
   }
-  const PersistentRegion& GetWeakCrossThreadPersistentRegion() const {
+  const CrossThreadPersistentRegion& GetWeakCrossThreadPersistentRegion()
+      const {
     return weak_cross_thread_persistent_region_;
   }
 
@@ -201,8 +203,8 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
 
   PersistentRegion strong_persistent_region_;
   PersistentRegion weak_persistent_region_;
-  PersistentRegion strong_cross_thread_persistent_region_;
-  PersistentRegion weak_cross_thread_persistent_region_;
+  CrossThreadPersistentRegion strong_cross_thread_persistent_region_;
+  CrossThreadPersistentRegion weak_cross_thread_persistent_region_;
 
   ProcessHeapStatisticsUpdater::AllocationObserverImpl
       allocation_observer_for_PROCESS_HEAP_STATISTICS_;

src/heap/cppgc/persistent-node.cc

@@ -95,5 +95,26 @@ void PersistentRegionLock::AssertLocked() {
   return g_process_mutex.Pointer()->AssertHeld();
 }
 
+CrossThreadPersistentRegion::~CrossThreadPersistentRegion() {
+  PersistentRegionLock guard;
+  persistent_region_.ClearAllUsedNodes();
+  persistent_region_.nodes_.clear();
+}
+
+void CrossThreadPersistentRegion::Trace(Visitor* visitor) {
+  PersistentRegionLock::AssertLocked();
+  return persistent_region_.Trace(visitor);
+}
+
+size_t CrossThreadPersistentRegion::NodesInUse() const {
+  // This method does not require a lock.
+  return persistent_region_.NodesInUse();
+}
+
+void CrossThreadPersistentRegion::ClearAllUsedNodes() {
+  PersistentRegionLock::AssertLocked();
+  return persistent_region_.ClearAllUsedNodes();
+}
+
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/pointer-policies.cc

@@ -33,14 +33,14 @@ PersistentRegion& WeakPersistentPolicy::GetPersistentRegion(
   return heap->GetWeakPersistentRegion();
 }
 
-PersistentRegion& StrongCrossThreadPersistentPolicy::GetPersistentRegion(
-    const void* object) {
+CrossThreadPersistentRegion&
+StrongCrossThreadPersistentPolicy::GetPersistentRegion(const void* object) {
   auto* heap = BasePage::FromPayload(object)->heap();
   return heap->GetStrongCrossThreadPersistentRegion();
 }
 
-PersistentRegion& WeakCrossThreadPersistentPolicy::GetPersistentRegion(
-    const void* object) {
+CrossThreadPersistentRegion&
+WeakCrossThreadPersistentPolicy::GetPersistentRegion(const void* object) {
   auto* heap = BasePage::FromPayload(object)->heap();
   return heap->GetWeakCrossThreadPersistentRegion();
 }

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

@@ -52,20 +52,20 @@ struct PersistentRegionTrait<WeakPersistent> {
 
 template <>
 struct PersistentRegionTrait<subtle::CrossThreadPersistent> {
-  static PersistentRegion& Get(cppgc::Heap* heap) {
+  static CrossThreadPersistentRegion& Get(cppgc::Heap* heap) {
     return internal::Heap::From(heap)->GetStrongCrossThreadPersistentRegion();
   }
 };
 
 template <>
 struct PersistentRegionTrait<subtle::WeakCrossThreadPersistent> {
-  static PersistentRegion& Get(cppgc::Heap* heap) {
+  static CrossThreadPersistentRegion& Get(cppgc::Heap* heap) {
     return internal::Heap::From(heap)->GetWeakCrossThreadPersistentRegion();
   }
 };
 
 template <template <typename> class PersistentType>
-PersistentRegion& GetRegion(cppgc::Heap* heap) {
+auto& GetRegion(cppgc::Heap* heap) {
   return PersistentRegionTrait<PersistentType>::Get(heap);
 }
 
@@ -114,31 +114,31 @@ class PersistentTest : public testing::TestSupportingAllocationOnly {};
 
 template <template <typename> class PersistentType>
 void NullStateCtor(cppgc::Heap* heap) {
-  EXPECT_EQ(0u, GetRegion<Persistent>(heap).NodesInUse());
+  EXPECT_EQ(0u, GetRegion<PersistentType>(heap).NodesInUse());
   {
     PersistentType<GCed> empty;
     EXPECT_EQ(nullptr, empty.Get());
     EXPECT_EQ(nullptr, empty.Release());
-    EXPECT_EQ(0u, GetRegion<Persistent>(heap).NodesInUse());
+    EXPECT_EQ(0u, GetRegion<PersistentType>(heap).NodesInUse());
   }
   {
     PersistentType<GCed> empty = nullptr;
     EXPECT_EQ(nullptr, empty.Get());
     EXPECT_EQ(nullptr, empty.Release());
-    EXPECT_EQ(0u, GetRegion<Persistent>(heap).NodesInUse());
+    EXPECT_EQ(0u, GetRegion<PersistentType>(heap).NodesInUse());
   }
   {
     PersistentType<GCed> empty = kSentinelPointer;
     EXPECT_EQ(kSentinelPointer, empty);
     EXPECT_EQ(kSentinelPointer, empty.Release());
-    EXPECT_EQ(0u, GetRegion<Persistent>(heap).NodesInUse());
+    EXPECT_EQ(0u, GetRegion<PersistentType>(heap).NodesInUse());
   }
   {
     // Runtime null must not allocated associated node.
     PersistentType<GCed> empty = static_cast<GCed*>(nullptr);
     EXPECT_EQ(nullptr, empty.Get());
     EXPECT_EQ(nullptr, empty.Release());
-    EXPECT_EQ(0u, GetRegion<Persistent>(heap).NodesInUse());
+    EXPECT_EQ(0u, GetRegion<PersistentType>(heap).NodesInUse());
   }
   EXPECT_EQ(0u, GetRegion<PersistentType>(heap).NodesInUse());
 }