Use a non-recursive mutex for ProcessHeap::CrossThreadPersistentMutex.

This may enable us to remove support for recursive mutexes in the near
future.

Bugs in this CL are likely to manifest as threads hanging while
trying to acquire this mutex recursively (while manipulating GC objects).

Bug: 856641
Change-Id: Icfc3982f7489fb53ddd4df319c405002ee1c3ea8
Reviewed-on: https://chromium-review.googlesource.com/1112197Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570884}

third_party/blink/renderer/platform/heap/heap.cc

@@ -459,8 +459,6 @@ void ThreadHeap::PromptlyFreed(size_t gc_info_index) {
 
 #if defined(ADDRESS_SANITIZER)
 void ThreadHeap::PoisonAllHeaps() {
-  RecursiveMutexLocker persistent_lock(
-      ProcessHeap::CrossThreadPersistentMutex());
   // Poisoning all unmarked objects in the other arenas.
   for (int i = 1; i < BlinkGC::kNumberOfArenas; i++)
     arenas_[i]->PoisonArena();
@@ -472,8 +470,6 @@ void ThreadHeap::PoisonAllHeaps() {
 }
 
 void ThreadHeap::PoisonEagerArena() {
-  RecursiveMutexLocker persistent_lock(
-      ProcessHeap::CrossThreadPersistentMutex());
   arenas_[BlinkGC::kEagerSweepArenaIndex]->PoisonArena();
   // CrossThreadPersistents in unmarked objects may be accessed from other
   // threads (e.g. in CrossThreadPersistentRegion::shouldTracePersistent) and

third_party/blink/renderer/platform/heap/heap_test.cc

@@ -365,8 +365,7 @@ class TestGCMarkingScope : public TestGCCollectGarbageScope {
  public:
   explicit TestGCMarkingScope(BlinkGC::StackState state)
       : TestGCCollectGarbageScope(state),
-        atomic_pause_scope_(ThreadState::Current()),
-        persistent_lock_(ProcessHeap::CrossThreadPersistentMutex()) {
+        atomic_pause_scope_(ThreadState::Current()) {
     ThreadState::Current()->Heap().stats_collector()->NotifyMarkingStarted(
         BlinkGC::GCReason::kTesting);
     ThreadState::Current()->AtomicPausePrologue(state, BlinkGC::kAtomicMarking,
@@ -380,7 +379,6 @@ class TestGCMarkingScope : public TestGCCollectGarbageScope {
 
  private:
   ThreadState::AtomicPauseScope atomic_pause_scope_;
-  RecursiveMutexLocker persistent_lock_;
 };
 
 class TestGCScope : public TestGCMarkingScope {

third_party/blink/renderer/platform/heap/persistent.h

@@ -175,6 +175,22 @@ class PersistentBase {
     return this;
   }
 
+  NO_SANITIZE_ADDRESS
+  void ClearWithLockHeld() {
+    static_assert(
+        crossThreadnessConfiguration == kCrossThreadPersistentConfiguration,
+        "This Persistent does not require the cross-thread lock.");
+#if DCHECK_IS_ON()
+    DCHECK(ProcessHeap::CrossThreadPersistentMutex().Locked());
+#endif
+    raw_ = nullptr;
+    CrossThreadPersistentRegion& region =
+        weaknessConfiguration == kWeakPersistentConfiguration
+            ? ProcessHeap::GetCrossThreadWeakPersistentRegion()
+            : ProcessHeap::GetCrossThreadPersistentRegion();
+    region.FreePersistentNode(persistent_node_);
+  }
+
  protected:
   NO_SANITIZE_ADDRESS
   T* AtomicGet() {
@@ -186,8 +202,7 @@ class PersistentBase {
   NO_SANITIZE_ADDRESS
   void Assign(T* ptr) {
     if (crossThreadnessConfiguration == kCrossThreadPersistentConfiguration) {
-      RecursiveMutexLocker persistent_lock(
-          ProcessHeap::CrossThreadPersistentMutex());
+      MutexLocker persistent_lock(ProcessHeap::CrossThreadPersistentMutex());
       raw_ = ptr;
     } else {
       raw_ = ptr;
@@ -227,6 +242,7 @@ class PersistentBase {
           weaknessConfiguration == kWeakPersistentConfiguration
               ? ProcessHeap::GetCrossThreadWeakPersistentRegion()
               : ProcessHeap::GetCrossThreadPersistentRegion();
+      MutexLocker lock(ProcessHeap::CrossThreadPersistentMutex());
       region.AllocatePersistentNode(persistent_node_, this, trace_callback);
       return;
     }
@@ -250,6 +266,7 @@ class PersistentBase {
             weaknessConfiguration == kWeakPersistentConfiguration
                 ? ProcessHeap::GetCrossThreadWeakPersistentRegion()
                 : ProcessHeap::GetCrossThreadPersistentRegion();
+        MutexLocker lock(ProcessHeap::CrossThreadPersistentMutex());
         region.FreePersistentNode(persistent_node_);
       }
       return;
@@ -312,7 +329,29 @@ class PersistentBase {
     Base* persistent = reinterpret_cast<Base*>(persistent_pointer);
     T* object = persistent->Get();
     if (object && !ObjectAliveTrait<T>::IsHeapObjectAlive(object))
-      persistent->Clear();
+      ClearWeakPersistent(persistent);
+  }
+
+  static void ClearWeakPersistent(
+      PersistentBase<std::remove_const_t<T>,
+                     kWeakPersistentConfiguration,
+                     kCrossThreadPersistentConfiguration>* persistent) {
+#if DCHECK_IS_ON()
+    DCHECK(ProcessHeap::CrossThreadPersistentMutex().Locked());
+#endif
+    persistent->ClearWithLockHeld();
+  }
+
+  static void ClearWeakPersistent(
+      PersistentBase<std::remove_const_t<T>,
+                     kWeakPersistentConfiguration,
+                     kSingleThreadPersistentConfiguration>* persistent) {
+    persistent->Clear();
+  }
+
+  template <typename BadPersistent>
+  static void ClearWeakPersistent(BadPersistent* non_weak_persistent) {
+    NOTREACHED();
   }
 
   // m_raw is accessed most, so put it at the first field.
@@ -852,8 +891,6 @@ template <typename T>
 struct BindUnwrapTraits<blink::CrossThreadWeakPersistent<T>> {
   static blink::CrossThreadPersistent<T> Unwrap(
       const blink::CrossThreadWeakPersistent<T>& wrapped) {
-    WTF::RecursiveMutexLocker persistent_lock(
-        blink::ProcessHeap::CrossThreadPersistentMutex());
     return blink::CrossThreadPersistent<T>(wrapped.Get());
   }
 };

third_party/blink/renderer/platform/heap/persistent_node.cc

@@ -167,7 +167,7 @@ void CrossThreadPersistentRegion::PrepareForThreadStateTermination(
   // For heaps belonging to a thread that's detaching, any cross-thread
   // persistents pointing into them needs to be disabled. Do that by clearing
   // out the underlying heap reference.
-  RecursiveMutexLocker lock(ProcessHeap::CrossThreadPersistentMutex());
+  MutexLocker lock(ProcessHeap::CrossThreadPersistentMutex());
 
   PersistentNodeSlots* slots = persistent_region_.slots_;
   while (slots) {
@@ -187,7 +187,7 @@ void CrossThreadPersistentRegion::PrepareForThreadStateTermination(
       BasePage* page = PageFromObject(raw_object);
       DCHECK(page);
       if (page->Arena()->GetThreadState() == thread_state) {
-        persistent->Clear();
+        persistent->ClearWithLockHeld();
         DCHECK(slots->slot_[i].IsUnused());
       }
     }
@@ -197,7 +197,7 @@ void CrossThreadPersistentRegion::PrepareForThreadStateTermination(
 
 #if defined(ADDRESS_SANITIZER)
 void CrossThreadPersistentRegion::UnpoisonCrossThreadPersistents() {
-  RecursiveMutexLocker lock(ProcessHeap::CrossThreadPersistentMutex());
+  MutexLocker lock(ProcessHeap::CrossThreadPersistentMutex());
   int persistent_count = 0;
   for (PersistentNodeSlots* slots = persistent_region_.slots_; slots;
        slots = slots->next_) {

third_party/blink/renderer/platform/heap/persistent_node.h

@@ -165,6 +165,7 @@ class PLATFORM_EXPORT PersistentRegion final {
 #endif
 };
 
+// Protected by ProcessHeap::CrossThreadPersistentMutex.
 class CrossThreadPersistentRegion final {
   USING_FAST_MALLOC(CrossThreadPersistentRegion);
 
@@ -172,14 +173,18 @@ class CrossThreadPersistentRegion final {
   void AllocatePersistentNode(PersistentNode*& persistent_node,
                               void* self,
                               TraceCallback trace) {
-    RecursiveMutexLocker lock(ProcessHeap::CrossThreadPersistentMutex());
+#if DCHECK_IS_ON()
+    DCHECK(ProcessHeap::CrossThreadPersistentMutex().Locked());
+#endif
     PersistentNode* node =
         persistent_region_.AllocatePersistentNode(self, trace);
     ReleaseStore(reinterpret_cast<void* volatile*>(&persistent_node), node);
   }
 
   void FreePersistentNode(PersistentNode*& persistent_node) {
-    RecursiveMutexLocker lock(ProcessHeap::CrossThreadPersistentMutex());
+#if DCHECK_IS_ON()
+    DCHECK(ProcessHeap::CrossThreadPersistentMutex().Locked());
+#endif
     // When the thread that holds the heap object that the cross-thread
     // persistent shuts down, prepareForThreadStateTermination() will clear out
     // the associated CrossThreadPersistent<> and PersistentNode so as to avoid

third_party/blink/renderer/platform/heap/process_heap.cc

@@ -54,8 +54,8 @@ CrossThreadPersistentRegion& ProcessHeap::GetCrossThreadWeakPersistentRegion() {
   return persistent_region;
 }
 
-RecursiveMutex& ProcessHeap::CrossThreadPersistentMutex() {
-  DEFINE_THREAD_SAFE_STATIC_LOCAL(RecursiveMutex, mutex, ());
+Mutex& ProcessHeap::CrossThreadPersistentMutex() {
+  DEFINE_THREAD_SAFE_STATIC_LOCAL(Mutex, mutex, ());
   return mutex;
 }
 

third_party/blink/renderer/platform/heap/process_heap.h

@@ -32,12 +32,7 @@ class PLATFORM_EXPORT ProcessHeap {
   // - Iteration and processing of weak cross-thread Persistents. The lock
   //   needs to span both operations as iteration of weak persistents only
   //   registers memory regions that are then processed afterwards.
-  //
-  // Recursive as PrepareForThreadStateTermination() clears a PersistentNode's
-  // associated Persistent<> -- it in turn freeing the PersistentNode. And both
-  // CrossThreadPersistentRegion operations need a lock on the region before
-  // mutating.
-  static RecursiveMutex& CrossThreadPersistentMutex();
+  static Mutex& CrossThreadPersistentMutex();
 
   static void IncreaseTotalAllocatedObjectSize(size_t delta) {
     AtomicAdd(&total_allocated_object_size_, static_cast<long>(delta));

third_party/blink/renderer/platform/heap/thread_state.cc

@@ -362,8 +362,7 @@ void ThreadState::VisitPersistents(Visitor* visitor) {
         Heap().stats_collector(),
         ThreadHeapStatsCollector::kVisitCrossThreadPersistents);
     // See ProcessHeap::CrossThreadPersistentMutex().
-    RecursiveMutexLocker persistent_lock(
-        ProcessHeap::CrossThreadPersistentMutex());
+    MutexLocker persistent_lock(ProcessHeap::CrossThreadPersistentMutex());
     ProcessHeap::GetCrossThreadPersistentRegion().TracePersistentNodes(visitor);
   }
   {
@@ -1706,8 +1705,7 @@ void ThreadState::MarkPhaseEpilogue(BlinkGC::MarkingType marking_type) {
 
   {
     // See ProcessHeap::CrossThreadPersistentMutex().
-    RecursiveMutexLocker persistent_lock(
-        ProcessHeap::CrossThreadPersistentMutex());
+    MutexLocker persistent_lock(ProcessHeap::CrossThreadPersistentMutex());
     VisitWeakPersistents(visitor);
     Heap().WeakProcessing(visitor);
   }