Bug 1508312 - Add assert to catch accidental re-enter of Slim RW lock r=ccorcoran,aklotz
authorChris Martin <cmartin@mozilla.com>
Wed, 16 Jan 2019 01:21:16 +0000
changeset 511154 493f7ee0ee0069fb0005ad61002e04ed3ff80fe8
parent 511153 54b883fea35a02c8e7d332a5a6c0904318e598e1
child 511155 79c6f7924680f978c9205df25653a16cec378e21
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersccorcoran, aklotz
bugs1508312, 1402282
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1508312 - Add assert to catch accidental re-enter of Slim RW lock r=ccorcoran,aklotz WindowsDllBlocklist installs a callback function that fires whenever a DLL is loaded. The installer function shares an SRWLock with the callback function. SRWLock is not re-entrant, so if the installer function accidently causes a DLL load before releasing the lock, the callback function will deadlock. This occured trying to solve Bug 1402282, where the installer function used "new" to allocate memory, which called the Win32 "RtlGenRandom()" function, which loaded bcrypt.dll, which caused the callback to fire off, which tried to lock the mutex that was already locked by the installer function. Hopefully this will save another developer lots of debug time in the future by turning a difficult-to-debug deadlock into a nice, loud assertion. Differential Revision: https://phabricator.services.mozilla.com/D15840
mozglue/build/MozglueUtils.h
mozglue/build/WindowsDllBlocklist.cpp
--- a/mozglue/build/MozglueUtils.h
+++ b/mozglue/build/MozglueUtils.h
@@ -4,51 +4,137 @@
  * 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/. */
 
 #ifndef mozilla_glue_MozglueUtils_h
 #define mozilla_glue_MozglueUtils_h
 
 #include <windows.h>
 
+#include "mozilla/Atomics.h"
 #include "mozilla/Attributes.h"
 
 namespace mozilla {
 namespace glue {
 
+#ifdef DEBUG
+
+class MOZ_STATIC_CLASS Win32SRWLock final {
+ public:
+  // Microsoft guarantees that '0' is never a valid thread id
+  // https://docs.microsoft.com/en-ca/windows/desktop/ProcThread/thread-handles-and-identifiers
+  static const DWORD kInvalidThreadId = 0;
+
+  constexpr Win32SRWLock()
+      : mExclusiveThreadId(kInvalidThreadId), mLock(SRWLOCK_INIT) {}
+
+  ~Win32SRWLock() { MOZ_ASSERT(mExclusiveThreadId == kInvalidThreadId); }
+
+  void LockShared() {
+    MOZ_ASSERT(
+        mExclusiveThreadId != GetCurrentThreadId(),
+        "Deadlock detected - A thread attempted to acquire a shared lock on "
+        "a SRWLOCK when it already owns the exclusive lock on it.");
+
+    ::AcquireSRWLockShared(&mLock);
+  }
+
+  void UnlockShared() { ::ReleaseSRWLockShared(&mLock); }
+
+  void LockExclusive() {
+    MOZ_ASSERT(
+        mExclusiveThreadId != GetCurrentThreadId(),
+        "Deadlock detected - A thread attempted to acquire an exclusive lock "
+        "on a SRWLOCK when it already owns the exclusive lock on it.");
+
+    ::AcquireSRWLockExclusive(&mLock);
+    mExclusiveThreadId = GetCurrentThreadId();
+  }
+
+  void UnlockExclusive() {
+    MOZ_ASSERT(mExclusiveThreadId == GetCurrentThreadId());
+
+    mExclusiveThreadId = kInvalidThreadId;
+    ::ReleaseSRWLockExclusive(&mLock);
+  }
+
+  Win32SRWLock(const Win32SRWLock&) = delete;
+  Win32SRWLock(Win32SRWLock&&) = delete;
+  Win32SRWLock& operator=(const Win32SRWLock&) = delete;
+  Win32SRWLock& operator=(Win32SRWLock&&) = delete;
+
+ private:
+  // "Relaxed" memory ordering is fine. Threads will see other thread IDs
+  // appear here in some non-deterministic ordering (or not at all) and simply
+  // ignore them.
+  //
+  // But a thread will only read its own ID if it previously wrote it, and a
+  // single thread doesn't need a memory barrier to read its own write.
+
+  Atomic<DWORD, Relaxed> mExclusiveThreadId;
+  SRWLOCK mLock;
+};
+
+#else  // DEBUG
+
+class MOZ_STATIC_CLASS Win32SRWLock final {
+ public:
+  constexpr Win32SRWLock() : mLock(SRWLOCK_INIT) {}
+
+  void LockShared() { ::AcquireSRWLockShared(&mLock); }
+
+  void UnlockShared() { ::ReleaseSRWLockShared(&mLock); }
+
+  void LockExclusive() { ::AcquireSRWLockExclusive(&mLock); }
+
+  void UnlockExclusive() { ::ReleaseSRWLockExclusive(&mLock); }
+
+  ~Win32SRWLock() = default;
+
+  Win32SRWLock(const Win32SRWLock&) = delete;
+  Win32SRWLock(Win32SRWLock&&) = delete;
+  Win32SRWLock& operator=(const Win32SRWLock&) = delete;
+  Win32SRWLock& operator=(Win32SRWLock&&) = delete;
+
+ private:
+  SRWLOCK mLock;
+};
+
+#endif
+
 class MOZ_RAII AutoSharedLock final {
  public:
-  explicit AutoSharedLock(SRWLOCK& aLock) : mLock(aLock) {
-    ::AcquireSRWLockShared(&aLock);
+  explicit AutoSharedLock(Win32SRWLock& aLock) : mLock(aLock) {
+    mLock.LockShared();
   }
 
-  ~AutoSharedLock() { ::ReleaseSRWLockShared(&mLock); }
+  ~AutoSharedLock() { mLock.UnlockShared(); }
 
   AutoSharedLock(const AutoSharedLock&) = delete;
   AutoSharedLock(AutoSharedLock&&) = delete;
   AutoSharedLock& operator=(const AutoSharedLock&) = delete;
   AutoSharedLock& operator=(AutoSharedLock&&) = delete;
 
  private:
-  SRWLOCK& mLock;
+  Win32SRWLock& mLock;
 };
 
 class MOZ_RAII AutoExclusiveLock final {
  public:
-  explicit AutoExclusiveLock(SRWLOCK& aLock) : mLock(aLock) {
-    ::AcquireSRWLockExclusive(&aLock);
+  explicit AutoExclusiveLock(Win32SRWLock& aLock) : mLock(aLock) {
+    mLock.LockExclusive();
   }
 
-  ~AutoExclusiveLock() { ::ReleaseSRWLockExclusive(&mLock); }
+  ~AutoExclusiveLock() { mLock.UnlockExclusive(); }
 
   AutoExclusiveLock(const AutoExclusiveLock&) = delete;
   AutoExclusiveLock(AutoExclusiveLock&&) = delete;
   AutoExclusiveLock& operator=(const AutoExclusiveLock&) = delete;
   AutoExclusiveLock& operator=(AutoExclusiveLock&&) = delete;
 
  private:
-  SRWLOCK& mLock;
+  Win32SRWLock& mLock;
 };
 
 }  // namespace glue
 }  // namespace mozilla
 
 #endif  //  mozilla_glue_MozglueUtils_h
--- a/mozglue/build/WindowsDllBlocklist.cpp
+++ b/mozglue/build/WindowsDllBlocklist.cpp
@@ -42,17 +42,17 @@
 #include "mozilla/AutoProfilerLabel.h"
 #include "mozilla/glue/WindowsDllServices.h"
 
 using namespace mozilla;
 
 using CrashReporter::Annotation;
 using CrashReporter::AnnotationToString;
 
-static SRWLOCK gDllServicesLock = SRWLOCK_INIT;
+static glue::Win32SRWLock gDllServicesLock;
 static glue::detail::DllServicesBase* gDllServices;
 
 #define DLL_BLOCKLIST_ENTRY(name, ...) {name, __VA_ARGS__},
 #define DLL_BLOCKLIST_STRING_TYPE const char*
 #include "mozilla/WindowsDllBlocklistDefs.h"
 
 // define this for very verbose dll load debug spew
 #undef DEBUG_very_verbose