Bug 1569458 - Unify Base Profiler mutexes into one class - r=gregtatum

`BaeProfilerMutex` is a concrete mutex based on MutexImpl, which was previously
implemented twice in both platform.h and BlocksRingBuffer.h.
This combined mutex has some DEBUG code (when MOZ_BASE_PROFILER is #defined) to
catch recursive locking, and to assert that the mutex is held (for code that
cannot easily use the "proof of lock" pattern; e.g., going through user-provided
callbacks).

This class needs to be public (because it is used in public headers), but is an
implementation detail, so it is located in a new header
"mozilla/BaseProfilerDetail.h" that will collect `mozilla::baseprofiler::detail`
code that may be useful to a few files in Base Profiler.

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

--HG--
extra : moz-landing-system : lando

mozglue/baseprofiler/core/platform.cpp

@@ -46,6 +46,7 @@
 #  include "mozilla/ArrayUtils.h"
 #  include "mozilla/Atomics.h"
 #  include "mozilla/AutoProfilerLabel.h"
+#  include "mozilla/BaseProfilerDetail.h"
 #  include "mozilla/Printf.h"
 #  include "mozilla/Services.h"
 #  include "mozilla/StackWalk.h"
@@ -215,10 +216,10 @@ class MOZ_RAII PSAutoLock {
   void operator=(const PSAutoLock&) = delete;
 
  private:
-  static PSMutex gPSMutex;
+  static detail::BaseProfilerMutex gPSMutex;
 };
 
-PSMutex PSAutoLock::gPSMutex;
+detail::BaseProfilerMutex PSAutoLock::gPSMutex;
 
 // Only functions that take a PSLockRef arg can access CorePS's and ActivePS's
 // fields.

mozglue/baseprofiler/core/platform.h

@@ -34,7 +34,6 @@
 #include "BaseProfiler.h"
 
 #include "mozilla/Logging.h"
-#include "mozilla/PlatformMutex.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/Vector.h"
 
@@ -83,17 +82,6 @@ class JSONWriter;
 
 namespace baseprofiler {
 
-// Thin shell around mozglue PlatformMutex, for Base Profiler internal use.
-// Does not preserve behavior in JS record/replay.
-class PSMutex : private mozilla::detail::MutexImpl {
- public:
-  PSMutex()
-      : mozilla::detail::MutexImpl(
-            mozilla::recordreplay::Behavior::DontPreserve) {}
-  void Lock() { mozilla::detail::MutexImpl::lock(); }
-  void Unlock() { mozilla::detail::MutexImpl::unlock(); }
-};
-
 typedef uint8_t* Address;
 
 class PlatformData;

mozglue/baseprofiler/core/shared-libraries-macos.cc

@@ -59,10 +59,11 @@ class MOZ_RAII SharedLibrariesLock {
   void operator=(const SharedLibrariesLock&) = delete;
 
  private:
-  static mozilla::baseprofiler::PSMutex sSharedLibrariesMutex;
+  static mozilla::baseprofiler::detail::BaseProfilerMutex sSharedLibrariesMutex;
 };
 
-mozilla::baseprofiler::PSMutex SharedLibrariesLock::sSharedLibrariesMutex;
+mozilla::baseprofiler::detail::BaseProfilerMutex
+    SharedLibrariesLock::sSharedLibrariesMutex;
 
 static void SharedLibraryAddImage(const struct mach_header* mh,
                                   intptr_t vmaddr_slide) {

mozglue/baseprofiler/moz.build

@@ -83,6 +83,7 @@ EXPORTS += [
 
 EXPORTS.mozilla += [
     'public/BaseProfilerCounts.h',
+    'public/BaseProfilerDetail.h',
     'public/BlocksRingBuffer.h',
     'public/leb128iterator.h',
     'public/ModuloBuffer.h',

mozglue/baseprofiler/public/BaseProfilerDetail.h

@@ -0,0 +1,88 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+// Internal Base Profiler utilities.
+
+#ifndef BaseProfilerDetail_h
+#define BaseProfilerDetail_h
+
+#include "mozilla/PlatformMutex.h"
+
+#ifdef DEBUG
+#  include "BaseProfiler.h"
+#  ifdef MOZ_BASE_PROFILER
+#    include "mozilla/Atomics.h"
+// When #defined, safety-checking code is added. By default: DEBUG builds,
+// and we need `MOZ_BASE_PROFILER` to use `profiler_current_thread_id()`.
+#    define MOZ_BASE_PROFILER_DEBUG
+#  endif  // MOZ_BASE_PROFILER
+#endif    // DEBUG
+
+namespace mozilla {
+namespace baseprofiler {
+namespace detail {
+
+// Thin shell around mozglue PlatformMutex, for Base Profiler internal use.
+// Does not preserve behavior in JS record/replay.
+class BaseProfilerMutex : private ::mozilla::detail::MutexImpl {
+ public:
+  BaseProfilerMutex()
+      : ::mozilla::detail::MutexImpl(
+            ::mozilla::recordreplay::Behavior::DontPreserve) {}
+  void Lock() {
+#ifdef MOZ_BASE_PROFILER_DEBUG
+    // This is only designed to catch recursive locking.
+    int tid = baseprofiler::profiler_current_thread_id();
+    MOZ_ASSERT(mOwningThreadId != tid);
+#endif  // MOZ_BASE_PROFILER_DEBUG
+    ::mozilla::detail::MutexImpl::lock();
+#ifdef MOZ_BASE_PROFILER_DEBUG
+    MOZ_ASSERT(mOwningThreadId != tid);
+    mOwningThreadId = tid;
+#endif  // MOZ_BASE_PROFILER_DEBUG
+  }
+
+  void Unlock() {
+#ifdef MOZ_BASE_PROFILER_DEBUG
+    // This should never trigger! But check just in case something has gone
+    // very wrong.
+    MOZ_ASSERT(mOwningThreadId == baseprofiler::profiler_current_thread_id());
+    // We're still holding the mutex here, so it's safe to just reset
+    // `mOwningThreadId`.
+    mOwningThreadId = 0;
+#endif  // MOZ_BASE_PROFILER_DEBUG
+    ::mozilla::detail::MutexImpl::unlock();
+  }
+
+  void AssertCurrentThreadOwns() const {
+#ifdef MOZ_BASE_PROFILER_DEBUG
+    MOZ_ASSERT(mOwningThreadId == baseprofiler::profiler_current_thread_id());
+#endif  // MOZ_BASE_PROFILER_DEBUG
+  }
+
+#ifdef MOZ_BASE_PROFILER_DEBUG
+ private:
+  Atomic<int> mOwningThreadId{0};
+#endif  // MOZ_BASE_PROFILER_DEBUG
+};
+
+// RAII class to lock a mutex.
+class MOZ_RAII BPAutoLock {
+ public:
+  explicit BPAutoLock(BaseProfilerMutex& aMutex) : mMutex(aMutex) {
+    mMutex.Lock();
+  }
+  ~BPAutoLock() { mMutex.Unlock(); }
+
+ private:
+  BaseProfilerMutex& mMutex;
+};
+
+}  // namespace detail
+}  // namespace baseprofiler
+}  // namespace mozilla
+
+#endif  // BaseProfilerDetail_h

mozglue/baseprofiler/public/BlocksRingBuffer.h

@@ -7,9 +7,9 @@
 #ifndef BlocksRingBuffer_h
 #define BlocksRingBuffer_h
 
+#include "mozilla/BaseProfilerDetail.h"
 #include "mozilla/ModuloBuffer.h"
 #include "mozilla/Pair.h"
-#include "mozilla/PlatformMutex.h"
 
 #include "mozilla/Maybe.h"
 
@@ -181,7 +181,7 @@ class BlocksRingBuffer {
   // Note that these may change right after this thread-safe call, so they
   // should only be used for statistical purposes.
   Pair<uint64_t, uint64_t> GetPushedAndClearedCounts() const {
-    RBAutoLock lock(*this);
+    baseprofiler::detail::BPAutoLock lock(mMutex);
     return {mPushedBlockCount, mClearedBlockCount};
   }
 
@@ -383,7 +383,7 @@ class BlocksRingBuffer {
   // call.
   template <typename Callback>
   auto Read(Callback&& aCallback) const {
-    RBAutoLock lock(*this);
+    baseprofiler::detail::BPAutoLock lock(mMutex);
     return std::forward<Callback>(aCallback)(Reader(*this));
   }
 
@@ -403,7 +403,7 @@ class BlocksRingBuffer {
   // this thread-safe call.
   template <typename Callback>
   auto ReadAt(BlockIndex aBlockIndex, Callback&& aCallback) const {
-    RBAutoLock lock(*this);
+    baseprofiler::detail::BPAutoLock lock(mMutex);
     MOZ_ASSERT(aBlockIndex <= mNextWriteIndex);
     Maybe<EntryReader> maybeReader;
     if (aBlockIndex >= mFirstReadIndex && aBlockIndex < mNextWriteIndex) {
@@ -607,7 +607,7 @@ class BlocksRingBuffer {
     // fast writers going around the ring cannot trample on this entry until it
     // is fully written.
     // TODO: Investigate this potential improvement as part of bug 1562604.
-    RBAutoLock lock(*this);
+    baseprofiler::detail::BPAutoLock lock(mMutex);
     return std::forward<Callback>(aCallback)(EntryReserver(*this));
   }
 
@@ -638,7 +638,7 @@ class BlocksRingBuffer {
   // Clear all entries, calling entry destructor (if any), and move read index
   // to the end so that these entries cannot be read anymore.
   void Clear() {
-    RBAutoLock lock(*this);
+    baseprofiler::detail::BPAutoLock lock(mMutex);
     ClearAllEntries();
   }
 
@@ -646,7 +646,7 @@ class BlocksRingBuffer {
   // destructor (if any), and move read index to the end so that these entries
   // cannot be read anymore.
   void ClearBefore(BlockIndex aBlockIndex) {
-    RBAutoLock lock(*this);
+    baseprofiler::detail::BPAutoLock lock(mMutex);
     // Don't accept a not-yet-written index. One-past-the-end is ok.
     MOZ_ASSERT(aBlockIndex <= mNextWriteIndex);
     if (aBlockIndex <= mFirstReadIndex) {
@@ -687,6 +687,7 @@ class BlocksRingBuffer {
 
 #ifdef DEBUG
   void Dump() const {
+    baseprofiler::detail::BPAutoLock lock(mMutex);
     using ULL = unsigned long long;
     printf("start=%llu (%llu) end=%llu (%llu) - ", ULL(Index(mFirstReadIndex)),
            ULL(Index(mFirstReadIndex) & (BufferLength().Value() - 1)),
@@ -761,31 +762,8 @@ class BlocksRingBuffer {
     mFirstReadIndex = mNextWriteIndex;
   }
 
-  // Thin shell around mozglue PlatformMutex, for Base Profiler internal use.
-  // Does not preserve behavior in JS record/replay.
-  class RBMutex : private mozilla::detail::MutexImpl {
-   public:
-    RBMutex()
-        : mozilla::detail::MutexImpl(
-              mozilla::recordreplay::Behavior::DontPreserve) {}
-    void Lock() { mozilla::detail::MutexImpl::lock(); }
-    void Unlock() { mozilla::detail::MutexImpl::unlock(); }
-  };
-
-  // RAII class to lock the mutex.
-  class MOZ_RAII RBAutoLock {
-   public:
-    explicit RBAutoLock(const BlocksRingBuffer& aBuffer) : mBuffer(aBuffer) {
-      mBuffer.mMutex.Lock();
-    }
-    ~RBAutoLock() { mBuffer.mMutex.Unlock(); }
-
-   private:
-    const BlocksRingBuffer& mBuffer;
-  };
-
   // Mutex guarding the following members.
-  mutable RBMutex mMutex;
+  mutable baseprofiler::detail::BaseProfilerMutex mMutex;
 
   // Underlying circular byte buffer.
   Buffer mBuffer;