Bug 1370453 - fix potential race condition in ThenCommand<>::Track(). draft
authorJW Wang <jwwang@mozilla.com>
Tue, 06 Jun 2017 14:19:59 +0800
changeset 589986 be568cefc7b997133bb0f15352d52671ae30a43a
parent 589337 30c22d38fb736e939566c923856b5d61314fa7c4
child 632060 fa8ca433b1b9af552ea3639ef89a534414802b37
push id62564
push userjwwang@mozilla.com
push dateWed, 07 Jun 2017 03:00:42 +0000
bugs1370453
milestone55.0a1
Bug 1370453 - fix potential race condition in ThenCommand<>::Track(). http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/xpcom/threads/MozPromise.h#953-958 MozPromiseRequestHolder is not thread-safe and it is possible for mReceiver->ThenInternal() to trigger resolve/reject callbacks before aRequestHolder.Track() is run. We should call aRequestHolder.Track() before mReceiver->ThenInternal() to avoid the race condition. MozReview-Commit-ID: K2R09m9UFBF
xpcom/threads/MozPromise.h
--- a/xpcom/threads/MozPromise.h
+++ b/xpcom/threads/MozPromise.h
@@ -855,30 +855,35 @@ protected:
     }
 
   private:
     Maybe<ResolveRejectFunction> mResolveRejectFunction; // Only accessed and deleted on dispatch thread.
     RefPtr<typename PromiseType::Private> mCompletionPromise;
   };
 
 public:
-  void ThenInternal(AbstractThread* aResponseThread, ThenValueBase* aThenValue,
+  void ThenInternal(AbstractThread* aResponseThread,
+                    already_AddRefed<ThenValueBase> aThenValue,
                     const char* aCallSite)
   {
     PROMISE_ASSERT(mMagic1 == sMagic && mMagic2 == sMagic && mMagic3 == sMagic && mMagic4 == &mMutex);
     MOZ_ASSERT(aResponseThread);
+    RefPtr<ThenValueBase> thenValue = aThenValue;
     MutexAutoLock lock(mMutex);
     MOZ_DIAGNOSTIC_ASSERT(!IsExclusive || !mHaveRequest);
     mHaveRequest = true;
     PROMISE_LOG("%s invoking Then() [this=%p, aThenValue=%p, isPending=%d]",
-                aCallSite, this, aThenValue, (int) IsPending());
+                aCallSite,
+                this,
+                thenValue.get(),
+                (int)IsPending());
     if (!IsPending()) {
-      aThenValue->Dispatch(this);
+      thenValue->Dispatch(this);
     } else {
-      mThenValues.AppendElement(aThenValue);
+      mThenValues.AppendElement(thenValue.forget());
     }
   }
 
 protected:
   /*
    * A command object to store all information needed to make a request to
    * the promise. This allows us to delay the request until further use is
    * known (whether it is ->Then() again for more promise chaining or ->Track()
@@ -912,54 +917,53 @@ protected:
 
     ThenCommand(ThenCommand&& aOther) = default;
 
   public:
     ~ThenCommand()
     {
       // Issue the request now if the return value of Then() is not used.
       if (mThenValue) {
-        mReceiver->ThenInternal(mResponseThread, mThenValue, mCallSite);
+        mReceiver->ThenInternal(
+          mResponseThread, mThenValue.forget(), mCallSite);
       }
     }
 
     // Allow RefPtr<MozPromise> p = somePromise->Then();
     //       p->Then(thread1, ...);
     //       p->Then(thread2, ...);
     operator RefPtr<PromiseType>()
     {
       static_assert(
         ThenValueType::SupportChaining::value,
         "The resolve/reject callback needs to return a RefPtr<MozPromise> "
         "in order to do promise chaining.");
 
-      RefPtr<ThenValueType> thenValue = mThenValue.forget();
       // mCompletionPromise must be created before ThenInternal() to avoid race.
       RefPtr<Private> p =
         new Private("<completion promise>", true /* aIsCompletionPromise */);
-      thenValue->mCompletionPromise = p;
+      mThenValue->mCompletionPromise = p;
       // Note ThenInternal() might nullify mCompletionPromise before return.
       // So we need to return p instead of mCompletionPromise.
-      mReceiver->ThenInternal(mResponseThread, thenValue, mCallSite);
+      mReceiver->ThenInternal(mResponseThread, mThenValue.forget(), mCallSite);
       return p;
     }
 
     template<typename... Ts>
     auto Then(Ts&&... aArgs)
       -> decltype(DeclVal<PromiseType>().Then(Forward<Ts>(aArgs)...))
     {
       return static_cast<RefPtr<PromiseType>>(*this)->Then(
         Forward<Ts>(aArgs)...);
     }
 
     void Track(MozPromiseRequestHolder<MozPromise>& aRequestHolder)
     {
-      RefPtr<ThenValueType> thenValue = mThenValue.forget();
-      mReceiver->ThenInternal(mResponseThread, thenValue, mCallSite);
-      aRequestHolder.Track(thenValue.forget());
+      aRequestHolder.Track(do_AddRef(mThenValue));
+      mReceiver->ThenInternal(mResponseThread, mThenValue.forget(), mCallSite);
     }
 
     // Allow calling ->Then() again for more promise chaining or ->Track() to
     // end chaining and track the request for future disconnection.
     ThenCommand* operator->()
     {
       return this;
     }
@@ -1302,20 +1306,20 @@ private:
  */
 template<typename PromiseType>
 class MozPromiseRequestHolder
 {
 public:
   MozPromiseRequestHolder() {}
   ~MozPromiseRequestHolder() { MOZ_ASSERT(!mRequest); }
 
-  void Track(RefPtr<typename PromiseType::Request>&& aRequest)
+  void Track(already_AddRefed<typename PromiseType::Request> aRequest)
   {
     MOZ_DIAGNOSTIC_ASSERT(!Exists());
-    mRequest = Move(aRequest);
+    mRequest = aRequest;
   }
 
   void Complete()
   {
     MOZ_DIAGNOSTIC_ASSERT(Exists());
     mRequest = nullptr;
   }