Bug 1755823 - Lock the ThreadRegistry non-exclusively when only reading the thread list, exclusively when adding/removing threads - r=canaltinova

This allows multiple threads to access ThreadRegistrations through the thread registry, as long as the registry itself is not modified. So in particular, ThreadRegistrations are guaranteed to stay alive as long as a non-exclusive lock is held.
This ensures cross-thread markers are not blocked while the sampler is running.

Differential Revision: https://phabricator.services.mozilla.com/D139917

tools/profiler/core/platform.cpp

@@ -6068,7 +6068,7 @@ void ThreadRegistry::Register(ThreadRegistration::OnThreadRef aOnThreadRef) {
   PSAutoLock lock;
 
   {
-    LockedRegistry lock;
+    RegistryLockExclusive lock{sRegistryMutex};
     MOZ_RELEASE_ASSERT(sRegistryContainer.append(OffThreadRef{aOnThreadRef}));
   }
 
@@ -6120,7 +6120,7 @@ void ThreadRegistry::Unregister(ThreadRegistration::OnThreadRef aOnThreadRef) {
   PSAutoLock psLock;
   locked_unregister_thread(psLock, aOnThreadRef);
 
-  LockedRegistry registryLock;
+  RegistryLockExclusive lock{sRegistryMutex};
   for (OffThreadRef& thread : sRegistryContainer) {
     if (thread.IsPointingAt(*aOnThreadRef.mThreadRegistration)) {
       sRegistryContainer.erase(&thread);

tools/profiler/public/ProfilerThreadRegistration.h

@@ -346,6 +346,9 @@ class ThreadRegistration {
   // is on the stack.
   bool mIsOnHeap = false;
 
+  // Only accessed by ThreadRegistry on this thread.
+  bool mIsRegistryLockedSharedOnThisThread = false;
+
   static MOZ_THREAD_LOCAL(ThreadRegistration*) tlsThreadRegistration;
 
   [[nodiscard]] static decltype(tlsThreadRegistration)* GetTLS() {

tools/profiler/public/ProfilerThreadRegistry.h

@@ -15,8 +15,10 @@ namespace mozilla::profiler {
 
 class ThreadRegistry {
  private:
-  using RegistryMutex = baseprofiler::detail::BaseProfilerMutex;
-  using RegistryLock = baseprofiler::detail::BaseProfilerAutoLock;
+  using RegistryMutex = baseprofiler::detail::BaseProfilerSharedMutex;
+  using RegistryLockExclusive =
+      baseprofiler::detail::BaseProfilerAutoLockExclusive;
+  using RegistryLockShared = baseprofiler::detail::BaseProfilerAutoLockShared;
 
  public:
   // Aliases to data accessors (removing the ThreadRegistration prefix).
@@ -205,7 +207,7 @@ class ThreadRegistry {
     ThreadRegistration* mThreadRegistration;
   };
 
-  // Lock the registry and allow iteration. E.g.:
+  // Lock the registry non-exclusively and allow iteration. E.g.:
   // `for (OffThreadRef thread : LockedRegistry{}) { ... }`
   // Do *not* export copies/references, as they could become dangling.
   // Locking order: Profiler, ThreadRegistry, ThreadRegistration.
@@ -213,12 +215,28 @@ class ThreadRegistry {
    public:
     LockedRegistry()
         : mRegistryLock([]() -> RegistryMutex& {
+            MOZ_ASSERT(!IsRegistryMutexLockedOnCurrentThread(),
+                       "Recursive locking detected");
             // In DEBUG builds, *before* we attempt to lock sRegistryMutex, we
             // want to check that the ThreadRegistration mutex is *not* locked
             // on this thread, to avoid inversion deadlocks.
             MOZ_ASSERT(!ThreadRegistration::IsDataMutexLockedOnCurrentThread());
             return sRegistryMutex;
-          }()) {}
+          }()) {
+      ThreadRegistration::WithOnThreadRef(
+          [](ThreadRegistration::OnThreadRef aOnThreadRef) {
+            aOnThreadRef.mThreadRegistration
+                ->mIsRegistryLockedSharedOnThisThread = true;
+          });
+    }
+
+    ~LockedRegistry() {
+      ThreadRegistration::WithOnThreadRef(
+          [](ThreadRegistration::OnThreadRef aOnThreadRef) {
+            aOnThreadRef.mThreadRegistration
+                ->mIsRegistryLockedSharedOnThisThread = false;
+          });
+    }
 
     [[nodiscard]] const OffThreadRef* begin() const {
       return sRegistryContainer.begin();
@@ -230,7 +248,7 @@ class ThreadRegistry {
     [[nodiscard]] OffThreadRef* end() { return sRegistryContainer.end(); }
 
    private:
-    RegistryLock mRegistryLock;
+    RegistryLockShared mRegistryLock;
   };
 
   // Call `F(OffThreadRef)` for the given aThreadId.
@@ -274,7 +292,13 @@ class ThreadRegistry {
   }
 
   [[nodiscard]] static bool IsRegistryMutexLockedOnCurrentThread() {
-    return sRegistryMutex.IsLockedOnCurrentThread();
+    return sRegistryMutex.IsLockedExclusiveOnCurrentThread() ||
+           ThreadRegistration::WithOnThreadRefOr(
+               [](ThreadRegistration::OnThreadRef aOnThreadRef) {
+                 return aOnThreadRef.mThreadRegistration
+                     ->mIsRegistryLockedSharedOnThisThread;
+               },
+               false);
   }
 
  private:

tools/profiler/tests/gtest/GeckoProfiler.cpp

@@ -1062,7 +1062,8 @@ TEST(GeckoProfiler, ThreadRegistry_DataAccess)
       ranTest = 0;
       EXPECT_FALSE(TRy::IsRegistryMutexLockedOnCurrentThread());
       for (TRy::OffThreadRef offThreadRef : TRy::LockedRegistry{}) {
-        EXPECT_TRUE(TRy::IsRegistryMutexLockedOnCurrentThread());
+        EXPECT_TRUE(TRy::IsRegistryMutexLockedOnCurrentThread() ||
+                    !TR::IsRegistered());
         if (offThreadRef.UnlockedConstReaderCRef().Info().ThreadId() ==
             testThreadId) {
           TestOffThreadRef(offThreadRef);
@@ -1076,7 +1077,8 @@ TEST(GeckoProfiler, ThreadRegistry_DataAccess)
         ranTest = 0;
         EXPECT_FALSE(TRy::IsRegistryMutexLockedOnCurrentThread());
         TRy::LockedRegistry lockedRegistry{};
-        EXPECT_TRUE(TRy::IsRegistryMutexLockedOnCurrentThread());
+        EXPECT_TRUE(TRy::IsRegistryMutexLockedOnCurrentThread() ||
+                    !TR::IsRegistered());
         for (TRy::OffThreadRef offThreadRef : lockedRegistry) {
           if (offThreadRef.UnlockedConstReaderCRef().Info().ThreadId() ==
               testThreadId) {