Bug 1339677 - Add sanity checks to debug MozPromise crashes. r=gerald, a=lizzard
authorJW Wang <jwwang@mozilla.com>
Fri, 17 Feb 2017 10:33:19 +0800
changeset 376285 104592a2b9be438203cfc489e60e0db851466ce5
parent 376284 4a02468e067257db5c8f6cefc330bb3c30d1d541
child 376286 891902fa999f3c8bb2fd41735cd6d6ceea6d5fb0
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgerald, lizzard
bugs1339677
milestone53.0a2
Bug 1339677 - Add sanity checks to debug MozPromise crashes. r=gerald, a=lizzard Bug 1339677. Part 1 - assert DoResolveOrReject() is called on the target thread and use stronger assertions. r=gerald Bug 1339677. Part 2 - some code cleanup and remove unnecessary scope qualifiers. r=gerald Bug 1339677. Part 3 - add some sanity checks. r=gerald Bug 1339677. Part 4 - check if mMutex.mLock is tampered. r=gerald
xpcom/glue/Mutex.h
xpcom/threads/MozPromise.h
--- a/xpcom/glue/Mutex.h
+++ b/xpcom/glue/Mutex.h
@@ -105,16 +105,20 @@ public:
 private:
   OffTheBooksMutex();
   OffTheBooksMutex(const OffTheBooksMutex&);
   OffTheBooksMutex& operator=(const OffTheBooksMutex&);
 
   PRLock* mLock;
 
   friend class CondVar;
+
+  // MozPromise needs to access mLock for debugging purpose.
+  template<typename, typename, bool>
+  friend class MozPromise;
 };
 
 /**
  * Mutex
  * When possible, use MutexAutoLock/MutexAutoUnlock to lock/unlock this
  * mutex within a scope, instead of calling Lock/Unlock directly.
  */
 class Mutex : public OffTheBooksMutex
--- a/xpcom/threads/MozPromise.h
+++ b/xpcom/threads/MozPromise.h
@@ -14,16 +14,26 @@
 #include "mozilla/Mutex.h"
 #include "mozilla/Monitor.h"
 #include "mozilla/Tuple.h"
 #include "mozilla/TypeTraits.h"
 
 #include "nsTArray.h"
 #include "nsThreadUtils.h"
 
+#if defined(DEBUG) || !defined(RELEASE_OR_BETA)
+#define PROMISE_DEBUG
+#endif
+
+#ifdef PROMISE_DEBUG
+#define PROMISE_ASSERT MOZ_RELEASE_ASSERT
+#else
+#define PROMISE_ASSERT(...) do { } while (0)
+#endif
+
 namespace mozilla {
 
 extern LazyLogModule gMozPromiseLog;
 
 #define PROMISE_LOG(x, ...) \
   MOZ_LOG(gMozPromiseLog, mozilla::LogLevel::Debug, (x, ##__VA_ARGS__))
 
 namespace detail {
@@ -115,16 +125,18 @@ protected:
   virtual ~MozPromiseRefcountable() {}
 };
 
 template<typename T> class MozPromiseHolder;
 template<typename T> class MozPromiseRequestHolder;
 template<typename ResolveValueT, typename RejectValueT, bool IsExclusive>
 class MozPromise : public MozPromiseRefcountable
 {
+  static const uint32_t sMagic = 0xcecace11;
+
 public:
   typedef ResolveValueT ResolveValueType;
   typedef RejectValueT RejectValueType;
   class ResolveOrRejectValue
   {
   public:
     template<typename ResolveValueType_>
     void SetResolve(ResolveValueType_&& aResolveValue)
@@ -171,16 +183,19 @@ public:
 protected:
   // MozPromise is the public type, and never constructed directly. Construct
   // a MozPromise::Private, defined below.
   MozPromise(const char* aCreationSite, bool aIsCompletionPromise)
     : mCreationSite(aCreationSite)
     , mMutex("MozPromise Mutex")
     , mHaveRequest(false)
     , mIsCompletionPromise(aIsCompletionPromise)
+#ifdef PROMISE_DEBUG
+    , mMagic4(mMutex.mLock)
+#endif
   {
     PROMISE_LOG("%s creating MozPromise (%p)", mCreationSite, this);
   }
 
 public:
   // MozPromise::Private allows us to separate the public interface (upon which
   // consumers of the promise may invoke methods like Then()) from the private
   // interface (upon which the creator of the promise may invoke Resolve() or
@@ -273,21 +288,16 @@ public:
     }
     return holder->Promise();
   }
 
   class Request : public MozPromiseRefcountable
   {
   public:
     virtual void Disconnect() = 0;
-
-    // MSVC complains when an inner class (ThenValueBase::{Resolve,Reject}Runnable)
-    // tries to access an inherited protected member.
-    bool IsDisconnected() const { return mDisconnected; }
-
     virtual void AssertIsDead() = 0;
 
   protected:
     Request() : mComplete(false), mDisconnected(false) {}
     virtual ~Request() {}
 
     bool mComplete;
     bool mDisconnected;
@@ -299,16 +309,17 @@ protected:
    * A ThenValue tracks a single consumer waiting on the promise. When a consumer
    * invokes promise->Then(...), a ThenValue is created. Once the Promise is
    * resolved or rejected, a {Resolve,Reject}Runnable is dispatched, which
    * invokes the resolve/reject method and then deletes the ThenValue.
    */
   class ThenValueBase : public Request
   {
     friend class MozPromise;
+    static const uint32_t sMagic = 0xfadece11;
 
   public:
     class ResolveOrRejectRunnable : public Runnable
     {
     public:
       ResolveOrRejectRunnable(ThenValueBase* aThenValue, MozPromise* aPromise)
         : mThenValue(aThenValue)
         , mPromise(aPromise)
@@ -338,68 +349,79 @@ protected:
     };
 
     ThenValueBase(AbstractThread* aResponseTarget,
                   const char* aCallSite)
       : mResponseTarget(aResponseTarget)
       , mCallSite(aCallSite)
     { }
 
+#ifdef PROMISE_DEBUG
+    ~ThenValueBase()
+    {
+      mMagic1 = 0;
+      mMagic2 = 0;
+    }
+#endif
+
     void AssertIsDead() override
     {
+      PROMISE_ASSERT(mMagic1 == sMagic && mMagic2 == sMagic);
       // We want to assert that this ThenValues is dead - that is to say, that
       // there are no consumers waiting for the result. In the case of a normal
       // ThenValue, we check that it has been disconnected, which is the way
       // that the consumer signals that it no longer wishes to hear about the
       // result. If this ThenValue has a completion promise (which is mutually
       // exclusive with being disconnectable), we recursively assert that every
       // ThenValue associated with the completion promise is dead.
       if (mCompletionPromise) {
         mCompletionPromise->AssertIsDead();
       } else {
         MOZ_DIAGNOSTIC_ASSERT(Request::mDisconnected);
       }
     }
 
     void Dispatch(MozPromise *aPromise)
     {
+      PROMISE_ASSERT(mMagic1 == sMagic && mMagic2 == sMagic);
       aPromise->mMutex.AssertCurrentThreadOwns();
       MOZ_ASSERT(!aPromise->IsPending());
 
-      RefPtr<Runnable> runnable =
-        static_cast<Runnable*>(new (typename ThenValueBase::ResolveOrRejectRunnable)(this, aPromise));
+      nsCOMPtr<nsIRunnable> r = new ResolveOrRejectRunnable(this, aPromise);
       PROMISE_LOG("%s Then() call made from %s [Runnable=%p, Promise=%p, ThenValue=%p]",
-                  aPromise->mValue.IsResolve() ? "Resolving" : "Rejecting", ThenValueBase::mCallSite,
-                  runnable.get(), aPromise, this);
+                  aPromise->mValue.IsResolve() ? "Resolving" : "Rejecting", mCallSite,
+                  r.get(), aPromise, this);
 
       // Promise consumers are allowed to disconnect the Request object and
       // then shut down the thread or task queue that the promise result would
       // be dispatched on. So we unfortunately can't assert that promise
       // dispatch succeeds. :-(
-      mResponseTarget->Dispatch(runnable.forget(), AbstractThread::DontAssertDispatchSuccess);
+      mResponseTarget->Dispatch(r.forget(), AbstractThread::DontAssertDispatchSuccess);
     }
 
     void Disconnect() override
     {
-      MOZ_ASSERT(ThenValueBase::mResponseTarget->IsCurrentThreadIn());
+      MOZ_DIAGNOSTIC_ASSERT(mResponseTarget->IsCurrentThreadIn());
       MOZ_DIAGNOSTIC_ASSERT(!Request::mComplete);
       Request::mDisconnected = true;
 
       // We could support rejecting the completion promise on disconnection, but
       // then we'd need to have some sort of default reject value. The use cases
       // of disconnection and completion promise chaining seem pretty orthogonal,
       // so let's use assert against it.
       MOZ_DIAGNOSTIC_ASSERT(!mCompletionPromise);
     }
 
   protected:
     virtual already_AddRefed<MozPromise> DoResolveOrRejectInternal(const ResolveOrRejectValue& aValue) = 0;
 
     void DoResolveOrReject(const ResolveOrRejectValue& aValue)
     {
+      PROMISE_ASSERT(mMagic1 == sMagic && mMagic2 == sMagic);
+      MOZ_DIAGNOSTIC_ASSERT(mResponseTarget->IsCurrentThreadIn());
       Request::mComplete = true;
       if (Request::mDisconnected) {
         PROMISE_LOG("ThenValue::DoResolveOrReject disconnected - bailing out [this=%p]", this);
         return;
       }
 
       // Invoke the resolve or reject method.
       RefPtr<MozPromise> p = DoResolveOrRejectInternal(aValue);
@@ -417,23 +439,27 @@ protected:
           p->ChainTo(completionPromise.forget(), "<chained completion promise>");
         } else {
           completionPromise->ResolveOrReject(aValue, "<completion of non-promise-returning method>");
         }
       }
     }
 
     RefPtr<AbstractThread> mResponseTarget; // May be released on any thread.
-
+#ifdef PROMISE_DEBUG
+    uint32_t mMagic1 = sMagic;
+#endif
     // Declaring RefPtr<MozPromise::Private> here causes build failures
     // on MSVC because MozPromise::Private is only forward-declared at this
     // point. This hack can go away when we inline-declare MozPromise::Private,
     // which is blocked on the B2G ICS compiler being too old.
     RefPtr<MozPromise> mCompletionPromise;
-
+#ifdef PROMISE_DEBUG
+    uint32_t mMagic2 = sMagic;
+#endif
     const char* mCallSite;
   };
 
   /*
    * We create two overloads for invoking Resolve/Reject Methods so as to
    * make the resolve/reject value argument "optional".
    */
 
@@ -673,16 +699,17 @@ protected:
   private:
     Maybe<ResolveRejectFunction> mResolveRejectFunction; // Only accessed and deleted on dispatch thread.
   };
 
 public:
   void ThenInternal(AbstractThread* aResponseThread, ThenValueBase* aThenValue,
                     const char* aCallSite)
   {
+    PROMISE_ASSERT(mMagic1 == sMagic && mMagic2 == sMagic && mMagic3 == sMagic && mMagic4 == mMutex.mLock);
     MutexAutoLock lock(mMutex);
     MOZ_ASSERT(aResponseThread->IsDispatchReliable());
     MOZ_DIAGNOSTIC_ASSERT(!IsExclusive || !mHaveRequest);
     mHaveRequest = true;
     PROMISE_LOG("%s invoking Then() [this=%p, aThenValue=%p, isPending=%d]",
                 aCallSite, this, aThenValue, (int) IsPending());
     if (!IsPending()) {
       aThenValue->Dispatch(this);
@@ -826,16 +853,17 @@ public:
   }
 
   // Note we expose the function AssertIsDead() instead of IsDead() since
   // checking IsDead() is a data race in the situation where the request is not
   // dead. Therefore we enforce the form |Assert(IsDead())| by exposing
   // AssertIsDead() only.
   void AssertIsDead()
   {
+    PROMISE_ASSERT(mMagic1 == sMagic && mMagic2 == sMagic && mMagic3 == sMagic && mMagic4 == mMutex.mLock);
     MutexAutoLock lock(mMutex);
     for (auto&& then : mThenValues) {
       then->AssertIsDead();
     }
     for (auto&& chained : mChainedPromises) {
       chained->AssertIsDead();
     }
   }
@@ -880,58 +908,79 @@ protected:
     AssertIsDead();
     // We can't guarantee a completion promise will always be revolved or
     // rejected since ResolveOrRejectRunnable might not run when dispatch fails.
     if (!mIsCompletionPromise) {
       MOZ_ASSERT(!IsPending());
       MOZ_ASSERT(mThenValues.IsEmpty());
       MOZ_ASSERT(mChainedPromises.IsEmpty());
     }
+#ifdef PROMISE_DEBUG
+    mMagic1 = 0;
+    mMagic2 = 0;
+    mMagic3 = 0;
+    mMagic4 = nullptr;
+#endif
   };
 
   const char* mCreationSite; // For logging
   Mutex mMutex;
   ResolveOrRejectValue mValue;
+#ifdef PROMISE_DEBUG
+  uint32_t mMagic1 = sMagic;
+#endif
   nsTArray<RefPtr<ThenValueBase>> mThenValues;
+#ifdef PROMISE_DEBUG
+  uint32_t mMagic2 = sMagic;
+#endif
   nsTArray<RefPtr<Private>> mChainedPromises;
+#ifdef PROMISE_DEBUG
+  uint32_t mMagic3 = sMagic;
+#endif
   bool mHaveRequest;
   const bool mIsCompletionPromise;
+#ifdef PROMISE_DEBUG
+  void* mMagic4;
+#endif
 };
 
 template<typename ResolveValueT, typename RejectValueT, bool IsExclusive>
 class MozPromise<ResolveValueT, RejectValueT, IsExclusive>::Private
   : public MozPromise<ResolveValueT, RejectValueT, IsExclusive>
 {
 public:
   explicit Private(const char* aCreationSite, bool aIsCompletionPromise = false)
     : MozPromise(aCreationSite, aIsCompletionPromise) {}
 
   template<typename ResolveValueT_>
   void Resolve(ResolveValueT_&& aResolveValue, const char* aResolveSite)
   {
+    PROMISE_ASSERT(mMagic1 == sMagic && mMagic2 == sMagic && mMagic3 == sMagic && mMagic4 == mMutex.mLock);
     MutexAutoLock lock(mMutex);
     MOZ_ASSERT(IsPending());
     PROMISE_LOG("%s resolving MozPromise (%p created at %s)", aResolveSite, this, mCreationSite);
     mValue.SetResolve(Forward<ResolveValueT_>(aResolveValue));
     DispatchAll();
   }
 
   template<typename RejectValueT_>
   void Reject(RejectValueT_&& aRejectValue, const char* aRejectSite)
   {
+    PROMISE_ASSERT(mMagic1 == sMagic && mMagic2 == sMagic && mMagic3 == sMagic && mMagic4 == mMutex.mLock);
     MutexAutoLock lock(mMutex);
     MOZ_ASSERT(IsPending());
     PROMISE_LOG("%s rejecting MozPromise (%p created at %s)", aRejectSite, this, mCreationSite);
     mValue.SetReject(Forward<RejectValueT_>(aRejectValue));
     DispatchAll();
   }
 
   template<typename ResolveOrRejectValue_>
   void ResolveOrReject(ResolveOrRejectValue_&& aValue, const char* aSite)
   {
+    PROMISE_ASSERT(mMagic1 == sMagic && mMagic2 == sMagic && mMagic3 == sMagic && mMagic4 == mMutex.mLock);
     MutexAutoLock lock(mMutex);
     MOZ_ASSERT(IsPending());
     PROMISE_LOG("%s resolveOrRejecting MozPromise (%p created at %s)", aSite, this, mCreationSite);
     mValue = Forward<ResolveOrRejectValue_>(aValue);
     DispatchAll();
   }
 };
 
@@ -1319,12 +1368,14 @@ InvokeAsync(AbstractThread* aTarget, con
                 "Function object must not be passed by lvalue-ref (to avoid "
                 "unplanned copies); Consider move()ing the object.");
   return detail::InvokeAsync(aTarget, aCallerName,
                              detail::AllowInvokeAsyncFunctionLVRef(),
                              Forward<Function>(aFunction));
 }
 
 #undef PROMISE_LOG
+#undef PROMISE_ASSERT
+#undef PROMISE_DEBUG
 
 } // namespace mozilla
 
 #endif