Bug 1370453 - fix potential race condition in ThenCommand<>::Track(). r=gerald
authorJW Wang <jwwang@mozilla.com>
Tue, 06 Jun 2017 14:19:59 +0800
changeset 410896 4d5d122b04ea7867ecf84c4a354c3bd43c5a6c91
parent 410895 0fbdafd2214dee63b4bf31c7891ed8c075c23cb5
child 410897 a12adeb205ce703c8f904cab08751bf7c4a054d2
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgerald
bugs1370453
milestone55.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 1370453 - fix potential race condition in ThenCommand<>::Track(). r=gerald 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;
   }