nagai/fune
3
0
Fork 0
forked from mirrors/gecko-dev
Code Pull requests Activity

Bug 1917242 - Destroy callbacks before chaining completion promise; r=xpcom-reviewers,nika a=RyanVM

Browse source 
Differential Revision: https://phabricator.services.mozilla.com/D221321
This commit is contained in:
Jan Varga 2024-09-11 15:30:00 +00:00
parent cdefc8416d
commit 80194fbe0b
2 changed files with 62 additions and 59 deletions
repo.diff.show_diff_stats Download patch file Download diff file Expand all files Collapse all files

2
xpcom/tests/gtest/TestMozPromise.cpp
View file

@ -817,7 +817,7 @@ TEST(MozPromise, MapErr)
EXPECT_EQ(ran_ok, false); EXPECT_EQ(ran_ok, false);
} }
TEST(MozPromise, DISABLED_ObjectDestructionOrder) TEST(MozPromise, ObjectDestructionOrder)
{ {
AutoTaskQueue atq; AutoTaskQueue atq;
RefPtr<TaskQueue> queue = atq.Queue(); RefPtr<TaskQueue> queue = atq.Queue();

119
xpcom/threads/MozPromise.h
View file

@ -642,45 +642,41 @@ class MozPromise : public MozPromiseBase {
}; };
/* /*
* We create two overloads for invoking Resolve/Reject Methods so as to * Helper to make the resolve/reject value argument "optional".
* make the resolve/reject value argument "optional".
*/ */
template <typename ThisType, typename MethodType, typename ValueType> template <typename ThisType, typename MethodType, typename ValueType>
static std::enable_if_t<TakesAnyArguments<MethodType>, static MethodReturnType<MethodType> InvokeMethod(ThisType* aThisVal,
MethodReturnType<MethodType>> MethodType aMethod,
InvokeMethod(ThisType* aThisVal, MethodType aMethod, ValueType&& aValue) { ValueType&& aValue) {
return (aThisVal->*aMethod)(std::forward<ValueType>(aValue)); if constexpr (TakesAnyArguments<MethodType>) {
} return (aThisVal->*aMethod)(std::forward<ValueType>(aValue));
} else {
template <typename ThisType, typename MethodType, typename ValueType> return (aThisVal->*aMethod)();
static std::enable_if_t<!TakesAnyArguments<MethodType>,
MethodReturnType<MethodType>>
InvokeMethod(ThisType* aThisVal, MethodType aMethod, ValueType&& aValue) {
return (aThisVal->*aMethod)();
}
// Called when promise chaining is supported.
template <bool SupportChaining, typename ThisType, typename MethodType,
typename ValueType, typename CompletionPromiseType>
static std::enable_if_t<SupportChaining, void> InvokeCallbackMethod(
ThisType* aThisVal, MethodType aMethod, ValueType&& aValue,
CompletionPromiseType&& aCompletionPromise) {
auto p = InvokeMethod(aThisVal, aMethod, std::forward<ValueType>(aValue));
if (aCompletionPromise) {
p->ChainTo(aCompletionPromise.forget(), "<chained completion promise>");
} }
} }
// Called when promise chaining is not supported. template <bool SupportChaining, typename PromiseType, typename ThisType,
template <bool SupportChaining, typename ThisType, typename MethodType, typename MethodType, typename ValueType>
typename ValueType, typename CompletionPromiseType> static RefPtr<PromiseType> InvokeCallbackMethod(ThisType* aThisVal,
static std::enable_if_t<!SupportChaining, void> InvokeCallbackMethod( MethodType aMethod,
ThisType* aThisVal, MethodType aMethod, ValueType&& aValue, ValueType&& aValue) {
CompletionPromiseType&& aCompletionPromise) { if constexpr (SupportChaining) {
MOZ_DIAGNOSTIC_ASSERT( return InvokeMethod(aThisVal, aMethod, std::forward<ValueType>(aValue));
!aCompletionPromise, } else {
"Can't do promise chaining for a non-promise-returning method."); InvokeMethod(aThisVal, aMethod, std::forward<ValueType>(aValue));
InvokeMethod(aThisVal, aMethod, std::forward<ValueType>(aValue)); return nullptr;
}
}
template <typename PromiseType>
static void MaybeChain(PromiseType* aFrom,
RefPtr<typename PromiseType::Private>&& aTo) {
if (aTo) {
MOZ_DIAGNOSTIC_ASSERT(
aFrom,
"Can't do promise chaining for a non-promise-returning method.");
aFrom->ChainTo(aTo.forget(), "<chained completion promise>");
}
} }
template <typename> template <typename>
@ -729,21 +725,22 @@ class MozPromise : public MozPromiseBase {
} }
void DoResolveOrRejectInternal(ResolveOrRejectValue& aValue) override { void DoResolveOrRejectInternal(ResolveOrRejectValue& aValue) override {
if (aValue.IsResolve()) { RefPtr<PromiseType> result =
InvokeCallbackMethod<SupportChaining>(mThisVal.get(), mResolveMethod, aValue.IsResolve()
MaybeMove(aValue.ResolveValue()), ? InvokeCallbackMethod<SupportChaining, PromiseType>(
std::move(mCompletionPromise)); mThisVal.get(), mResolveMethod,
} else { MaybeMove(aValue.ResolveValue()))
InvokeCallbackMethod<SupportChaining>(mThisVal.get(), mRejectMethod, : InvokeCallbackMethod<SupportChaining, PromiseType>(
MaybeMove(aValue.RejectValue()), mThisVal.get(), mRejectMethod,
std::move(mCompletionPromise)); MaybeMove(aValue.RejectValue()));
}
// Null out mThisVal after invoking the callback so that any references // Null out mThisVal after invoking the callback so that any references
// are released predictably on the dispatch thread. Otherwise, it would be // are released predictably on the dispatch thread. Otherwise, it would be
// released on whatever thread last drops its reference to the ThenValue, // released on whatever thread last drops its reference to the ThenValue,
// which may or may not be ok. // which may or may not be ok.
mThisVal = nullptr; mThisVal = nullptr;
MaybeChain<PromiseType>(result, std::move(mCompletionPromise));
} }
private: private:
@ -789,15 +786,17 @@ class MozPromise : public MozPromiseBase {
} }
void DoResolveOrRejectInternal(ResolveOrRejectValue& aValue) override { void DoResolveOrRejectInternal(ResolveOrRejectValue& aValue) override {
InvokeCallbackMethod<SupportChaining>( RefPtr<PromiseType> result =
mThisVal.get(), mResolveRejectMethod, MaybeMove(aValue), InvokeCallbackMethod<SupportChaining, PromiseType>(
std::move(mCompletionPromise)); mThisVal.get(), mResolveRejectMethod, MaybeMove(aValue));
// Null out mThisVal after invoking the callback so that any references // Null out mThisVal after invoking the callback so that any references
// are released predictably on the dispatch thread. Otherwise, it would be // are released predictably on the dispatch thread. Otherwise, it would be
// released on whatever thread last drops its reference to the ThenValue, // released on whatever thread last drops its reference to the ThenValue,
// which may or may not be ok. // which may or may not be ok.
mThisVal = nullptr; mThisVal = nullptr;
MaybeChain<PromiseType>(result, std::move(mCompletionPromise));
} }
private: private:
@ -853,15 +852,14 @@ class MozPromise : public MozPromiseBase {
// classes with ::operator()), since it allows us to share code more // classes with ::operator()), since it allows us to share code more
// easily. We could fix this if need be, though it's quite easy to work // easily. We could fix this if need be, though it's quite easy to work
// around by just capturing something. // around by just capturing something.
if (aValue.IsResolve()) { RefPtr<PromiseType> result =
InvokeCallbackMethod<SupportChaining>( aValue.IsResolve()
mResolveFunction.ptr(), &ResolveFunction::operator(), ? InvokeCallbackMethod<SupportChaining, PromiseType>(
MaybeMove(aValue.ResolveValue()), std::move(mCompletionPromise)); mResolveFunction.ptr(), &ResolveFunction::operator(),
} else { MaybeMove(aValue.ResolveValue()))
InvokeCallbackMethod<SupportChaining>( : InvokeCallbackMethod<SupportChaining, PromiseType>(
mRejectFunction.ptr(), &RejectFunction::operator(), mRejectFunction.ptr(), &RejectFunction::operator(),
MaybeMove(aValue.RejectValue()), std::move(mCompletionPromise)); MaybeMove(aValue.RejectValue()));
}
// Destroy callbacks after invocation so that any references in closures // Destroy callbacks after invocation so that any references in closures
// are released predictably on the dispatch thread. Otherwise, they would // are released predictably on the dispatch thread. Otherwise, they would
@ -869,6 +867,8 @@ class MozPromise : public MozPromiseBase {
// ThenValue, which may or may not be ok. // ThenValue, which may or may not be ok.
mResolveFunction.reset(); mResolveFunction.reset();
mRejectFunction.reset(); mRejectFunction.reset();
MaybeChain<PromiseType>(result, std::move(mCompletionPromise));
} }
private: private:
@ -919,15 +919,18 @@ class MozPromise : public MozPromiseBase {
// classes with ::operator()), since it allows us to share code more // classes with ::operator()), since it allows us to share code more
// easily. We could fix this if need be, though it's quite easy to work // easily. We could fix this if need be, though it's quite easy to work
// around by just capturing something. // around by just capturing something.
InvokeCallbackMethod<SupportChaining>( RefPtr<PromiseType> result =
mResolveRejectFunction.ptr(), &ResolveRejectFunction::operator(), InvokeCallbackMethod<SupportChaining, PromiseType>(
MaybeMove(aValue), std::move(mCompletionPromise)); mResolveRejectFunction.ptr(), &ResolveRejectFunction::operator(),
MaybeMove(aValue));
// Destroy callbacks after invocation so that any references in closures // Destroy callbacks after invocation so that any references in closures
// are released predictably on the dispatch thread. Otherwise, they would // are released predictably on the dispatch thread. Otherwise, they would
// be released on whatever thread last drops its reference to the // be released on whatever thread last drops its reference to the
// ThenValue, which may or may not be ok. // ThenValue, which may or may not be ok.
mResolveRejectFunction.reset(); mResolveRejectFunction.reset();
MaybeChain<PromiseType>(result, std::move(mCompletionPromise));
} }
private: private: