Bug 1568410 - (part 2) Call the pre/post lock code from ConditionVariable r=jandem

Condition variables lock and unlock mutexes. The code in the next patch
would find assertion failures because this case wasn't handled.

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

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

js/src/threading/ConditionVariable.h

@@ -46,7 +46,17 @@ class ConditionVariable {
 
   // Block the current thread of execution until this condition variable is
   // woken from another thread via notify_one or notify_all.
-  void wait(UniqueLock<Mutex>& lock) { impl_.wait(lock.lock); }
+  void wait(Mutex& lock) {
+#ifdef DEBUG
+    lock.preUnlockChecks();
+#endif
+    impl_.wait(lock.impl_);
+#ifdef DEBUG
+    lock.preLockChecks();
+    lock.postLockChecks();
+#endif
+  }
+  void wait(UniqueLock<Mutex>& lock) { wait(lock.lock); }
 
   // As with |wait|, block the current thread of execution until woken from
   // another thread. This method will resume waiting once woken until the given
@@ -90,9 +100,18 @@ class ConditionVariable {
   // encounter substantially longer delays, depending on system load.
   CVStatus wait_for(UniqueLock<Mutex>& lock,
                     const mozilla::TimeDuration& rel_time) {
-    return impl_.wait_for(lock.lock, rel_time) == mozilla::CVStatus::Timeout
-               ? CVStatus::Timeout
-               : CVStatus::NoTimeout;
+#ifdef DEBUG
+    lock.lock.preUnlockChecks();
+#endif
+    CVStatus res =
+        impl_.wait_for(lock.lock.impl_, rel_time) == mozilla::CVStatus::Timeout
+            ? CVStatus::Timeout
+            : CVStatus::NoTimeout;
+#ifdef DEBUG
+    lock.lock.preLockChecks();
+    lock.lock.postLockChecks();
+#endif
+    return res;
   }
 
   // As with |wait_for|, block the current thread of execution until woken from

js/src/threading/ExclusiveData.h

@@ -202,7 +202,7 @@ class ExclusiveWaitableData : public ExclusiveData<T> {
 
     void wait() {
       auto* parent = static_cast<const ExclusiveWaitableData*>(this->parent());
-      parent->condVar_.impl_.wait(parent->lock_);
+      parent->condVar_.wait(parent->lock_);
     }
 
     void notify_one() {

js/src/threading/Mutex.cpp

@@ -38,6 +38,12 @@ void js::Mutex::ShutDown() {
 }
 
 void js::Mutex::lock() {
+  preLockChecks();
+  impl_.lock();
+  postLockChecks();
+}
+
+void js::Mutex::preLockChecks() const {
   auto& stack = heldMutexStack();
   if (!stack.empty()) {
     const Mutex& prev = *stack.back();
@@ -49,19 +55,24 @@ void js::Mutex::lock() {
       MOZ_CRASH("Mutex ordering violation");
     }
   }
+}
 
-  MutexImpl::lock();
-
+void js::Mutex::postLockChecks() {
   AutoEnterOOMUnsafeRegion oomUnsafe;
+  auto& stack = heldMutexStack();
   if (!stack.append(this)) {
     oomUnsafe.crash("js::Mutex::lock");
   }
 }
 
 void js::Mutex::unlock() {
+  preUnlockChecks();
+  impl_.unlock();
+}
+
+void js::Mutex::preUnlockChecks() {
   auto& stack = heldMutexStack();
   MOZ_ASSERT(stack.back() == this);
-  MutexImpl::unlock();
   stack.popBack();
 }
 

js/src/threading/Mutex.h

@@ -25,12 +25,29 @@ struct MutexId {
   uint32_t order;
 };
 
+// The Mutex class below wraps mozilla::detail::MutexImpl, but we don't want to
+// use public inheritance, and private inheritance is problematic because
+// Mutex's friends can access the private parent class as if it was public
+// inheritance.  So use a data member, but for Mutex to access the data member
+// we must override it and make Mutex a friend.
+class MutexImpl : public mozilla::detail::MutexImpl {
+ protected:
+  MutexImpl()
+      : mozilla::detail::MutexImpl(
+            mozilla::recordreplay::Behavior::DontPreserve) {}
+
+  friend class Mutex;
+};
+
 // In debug builds, js::Mutex is a wrapper over MutexImpl that checks correct
 // locking order is observed.
 //
 // The class maintains a per-thread stack of currently-held mutexes to enable it
 // to check this.
-class Mutex : public mozilla::detail::MutexImpl {
+class Mutex {
+ private:
+  MutexImpl impl_;
+
  public:
 #ifdef DEBUG
   static bool Init();
@@ -41,10 +58,8 @@ class Mutex : public mozilla::detail::MutexImpl {
 #endif
 
   explicit Mutex(const MutexId& id)
-      : mozilla::detail::MutexImpl(
-            mozilla::recordreplay::Behavior::DontPreserve)
 #ifdef DEBUG
-        , id_(id)
+      : id_(id)
 #endif
   {
     MOZ_ASSERT(id_.order != 0);
@@ -54,8 +69,8 @@ class Mutex : public mozilla::detail::MutexImpl {
   void lock();
   void unlock();
 #else
-  using MutexImpl::lock;
-  using MutexImpl::unlock;
+  void lock() { impl_.lock(); }
+  void unlock() { impl_.unlock(); }
 #endif
 
 #ifdef DEBUG
@@ -69,6 +84,15 @@ class Mutex : public mozilla::detail::MutexImpl {
   static MOZ_THREAD_LOCAL(MutexVector*) HeldMutexStack;
   static MutexVector& heldMutexStack();
 #endif
+
+ private:
+#ifdef DEBUG
+  void preLockChecks() const;
+  void postLockChecks();
+  void preUnlockChecks();
+#endif
+
+  friend class ConditionVariable;
 };
 
 }  // namespace js