Bug 1250829 - add customized assertions for completion promises to facilitate promise chaining. r=bobbyholley.
authorJW Wang <jwwang@mozilla.com>
Fri, 18 Mar 2016 11:27:19 +0800
changeset 342021 0edbdee8e573cc8a7810b12b046375355f18a7c3
parent 342020 26f9c1949e1c479d7102eeebe7c3f565bb531f09
child 342022 c7914f20970a34cfcf5a878466a13cc842fa50e4
push id13342
push userbmo:james@hoppipolla.co.uk
push dateFri, 18 Mar 2016 09:55:58 +0000
reviewersbobbyholley
bugs1250829
milestone48.0a1
Bug 1250829 - add customized assertions for completion promises to facilitate promise chaining. r=bobbyholley. MozReview-Commit-ID: 5IbbAz0KIBY
dom/media/gtest/TestMozPromise.cpp
xpcom/threads/MozPromise.h
--- a/dom/media/gtest/TestMozPromise.cpp
+++ b/dom/media/gtest/TestMozPromise.cpp
@@ -215,9 +215,43 @@ TEST(MozPromise, PromiseAllReject)
       [queue] (float aRejectValue) -> void {
         EXPECT_EQ(aRejectValue, 32.0);
         queue->BeginShutdown();
       }
     );
   });
 }
 
+// Test we don't hit the assertions in MozPromise when exercising promise
+// chaining upon task queue shutdown.
+TEST(MozPromise, Chaining)
+{
+  AutoTaskQueue atq;
+  RefPtr<TaskQueue> queue = atq.Queue();
+  MozPromiseRequestHolder<TestPromise> holder;
+
+  RunOnTaskQueue(queue, [queue, &holder] () {
+    auto p = TestPromise::CreateAndResolve(42, __func__);
+    const size_t kIterations = 100;
+    for (size_t i = 0; i < kIterations; ++i) {
+      p = p->Then(queue, __func__,
+        [] (int aVal) {
+          EXPECT_EQ(aVal, 42);
+        },
+        [] () {}
+      )->CompletionPromise();
+
+      if (i == kIterations / 2) {
+        p->Then(queue, __func__,
+          [queue, &holder] () {
+            holder.Disconnect();
+            queue->BeginShutdown();
+          },
+          DO_FAIL);
+      }
+    }
+    // We will hit the assertion if we don't disconnect the leaf Request
+    // in the promise chain.
+    holder.Begin(p->Then(queue, __func__, [] () {}, [] () {}));
+  });
+}
+
 #undef DO_FAIL
--- a/xpcom/threads/MozPromise.h
+++ b/xpcom/threads/MozPromise.h
@@ -160,20 +160,21 @@ public:
   private:
     Maybe<ResolveValueType> mResolveValue;
     Maybe<RejectValueType> mRejectValue;
   };
 
 protected:
   // MozPromise is the public type, and never constructed directly. Construct
   // a MozPromise::Private, defined below.
-  explicit MozPromise(const char* aCreationSite)
+  MozPromise(const char* aCreationSite, bool aIsCompletionPromise)
     : mCreationSite(aCreationSite)
     , mMutex("MozPromise Mutex")
     , mHaveRequest(false)
+    , mIsCompletionPromise(aIsCompletionPromise)
   {
     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,16 +274,18 @@ 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 MozPromise* CompletionPromise() = 0;
 
+    virtual void AssertIsDead() = 0;
+
   protected:
     Request() : mComplete(false), mDisconnected(false) {}
     virtual ~Request() {}
 
     bool mComplete;
     bool mDisconnected;
   };
 
@@ -304,17 +307,19 @@ protected:
         : mThenValue(aThenValue)
         , mPromise(aPromise)
       {
         MOZ_DIAGNOSTIC_ASSERT(!mPromise->IsPending());
       }
 
       ~ResolveOrRejectRunnable()
       {
-        MOZ_DIAGNOSTIC_ASSERT(!mThenValue || mThenValue->IsDisconnected());
+        if (mThenValue) {
+          mThenValue->AssertIsDead();
+        }
       }
 
       NS_IMETHODIMP Run()
       {
         PROMISE_LOG("ResolveOrRejectRunnable::Run() [this=%p]", this);
         mThenValue->DoResolveOrReject(mPromise->Value());
         mThenValue = nullptr;
         mPromise = nullptr;
@@ -329,21 +334,38 @@ protected:
     explicit ThenValueBase(AbstractThread* aResponseTarget, const char* aCallSite)
       : mResponseTarget(aResponseTarget), mCallSite(aCallSite) {}
 
     MozPromise* CompletionPromise() override
     {
       MOZ_DIAGNOSTIC_ASSERT(mResponseTarget->IsCurrentThreadIn());
       MOZ_DIAGNOSTIC_ASSERT(!Request::mComplete);
       if (!mCompletionPromise) {
-        mCompletionPromise = new MozPromise::Private("<completion promise>");
+        mCompletionPromise = new MozPromise::Private(
+          "<completion promise>", true /* aIsCompletionPromise */);
       }
       return mCompletionPromise;
     }
 
+    void AssertIsDead() override
+    {
+      // 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)
     {
       aPromise->mMutex.AssertCurrentThreadOwns();
       MOZ_ASSERT(!aPromise->IsPending());
 
       RefPtr<nsRunnable> runnable =
         static_cast<nsRunnable*>(new (typename ThenValueBase::ResolveOrRejectRunnable)(this, aPromise));
       PROMISE_LOG("%s Then() call made from %s [Runnable=%p, Promise=%p, ThenValue=%p]",
@@ -608,16 +630,31 @@ public:
                 aCallSite, this, chainedPromise.get(), (int) IsPending());
     if (!IsPending()) {
       ForwardTo(chainedPromise);
     } else {
       mChainedPromises.AppendElement(chainedPromise);
     }
   }
 
+  // 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()
+  {
+    MutexAutoLock lock(mMutex);
+    for (auto&& then : mThenValues) {
+      then->AssertIsDead();
+    }
+    for (auto&& chained : mChainedPromises) {
+      chained->AssertIsDead();
+    }
+  }
+
 protected:
   bool IsPending() const { return mValue.IsNothing(); }
   const ResolveOrRejectValue& Value() const
   {
     // This method should only be called once the value has stabilized. As
     // such, we don't need to acquire the lock here.
     MOZ_DIAGNOSTIC_ASSERT(!IsPending());
     return mValue;
@@ -645,35 +682,42 @@ protected:
     } else {
       aOther->Reject(mValue.RejectValue(), "<chained promise>");
     }
   }
 
   virtual ~MozPromise()
   {
     PROMISE_LOG("MozPromise::~MozPromise [this=%p]", this);
-    MOZ_ASSERT(!IsPending());
-    MOZ_ASSERT(mThenValues.IsEmpty());
-    MOZ_ASSERT(mChainedPromises.IsEmpty());
+    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());
+    }
   };
 
   const char* mCreationSite; // For logging
   Mutex mMutex;
   ResolveOrRejectValue mValue;
   nsTArray<RefPtr<ThenValueBase>> mThenValues;
   nsTArray<RefPtr<Private>> mChainedPromises;
   bool mHaveRequest;
+  const bool mIsCompletionPromise;
 };
 
 template<typename ResolveValueT, typename RejectValueT, bool IsExclusive>
 class MozPromise<ResolveValueT, RejectValueT, IsExclusive>::Private
   : public MozPromise<ResolveValueT, RejectValueT, IsExclusive>
 {
 public:
-  explicit Private(const char* aCreationSite) : MozPromise(aCreationSite) {}
+  explicit Private(const char* aCreationSite, bool aIsCompletionPromise = false)
+    : MozPromise(aCreationSite, aIsCompletionPromise) {}
 
   template<typename ResolveValueT_>
   void Resolve(ResolveValueT_&& aResolveValue, const char* aResolveSite)
   {
     MutexAutoLock lock(mMutex);
     MOZ_ASSERT(IsPending());
     PROMISE_LOG("%s resolving MozPromise (%p created at %s)", aResolveSite, this, mCreationSite);
     mValue.SetResolve(Forward<ResolveValueT_>(aResolveValue));