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
--- 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;
}