1332785 - MozPromiseHolder::Resolve and Reject should take references

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

MozPromiseHolder::Resolve and Reject currently take their argument by-value, which could add an unwanted copy.
In particular, it would prevent moving a temporary value into the function, if the value type prohibit implicit copies (e.g.: nsTArray).

So we should allow both copy and moves, by providing Resolve/Reject functions taking both 'const T&' and 'T&&'.

(Note that I won't just take forwarding references of any type, as this could lead to implicit conversions by bypassing the 'explicit' marker of the value type constructors, like in bug 1331137.)

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1332785 - Optimize MozPromiseHolder::Resolve and Reject -

Instead of lvalues, MozPromiseHolder::Resolve and Reject now take either
const& or && values of the expected types, to allow move semantics all the way
to the ResolveOrRejectValue object.

Note that we won't just take forwarding references of any type, as this could
lead to implicit conversions by bypassing the 'explicit' marker of the value
type constructors, like in bug 1331137.

Review commit: https://reviewboard.mozilla.org/r/106246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106246/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Gerald Squelart [:gerald] from comment #0)
> (Note that I won't just take forwarding references of any type, as this
> could lead to implicit conversions by bypassing the 'explicit' marker of the
> value type constructors, like in bug 1331137.)

Can you give an example? I can't see how this is possible in MozPromise.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Here's a little test, just to see how things build:
https://hg.mozilla.org/try/rev/2cb8bd56627b

The most interesting lines are:
>  struct Other {};
>  struct Explicit { Explicit() {}; explicit Explicit(const Other&) {} };
>  typedef MozPromise<Explicit, Explicit, true> ExplicitPromise;
>  MozPromiseHolder<ExplicitPromise> holder;
>  holder.Resolve(Other{}, __func__);
The last line (and more similar ones under it) should fail to compile, because we are trying to build an ExplicitPromise::ResolveOrRejectValue::ResolveType (aka ExplicitPromise) from an 'Other{}', but the conversion constructor for that is marked 'explicit' and therefore should be ignored.

Here's a try with just that test on top of the current code (which takes ResolveType by value) :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42a6dbce20b1a345f67070b63a238bee914d4b9c
It fails as expected.

Here's a try with my proposed patch (which takes Resolve/RejectType by const&& or &&) :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c418b3a0e1aea2c5eb17b5b245919626d7d9f2be
It also fails for the same reason.

And this is a try with a patch where Resolve/Reject take forwarding references of any type:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfceedabb6b095bbd5121f5959bebf1cd918dd00
It doesn't fail anymore, meaning the 'explicit' for the conversion constructor was ignored!
(Well, there's a failure in the static analysis build, but it's unrelated.)

Does that convince you?

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Here's a little test, just to see how things build:
https://hg.mozilla.org/try/rev/2cb8bd56627b

The most interesting lines are:
>  struct Other {};
>  struct Explicit { Explicit() {}; explicit Explicit(const Other&) {} };
>  typedef MozPromise<Explicit, Explicit, true> ExplicitPromise;
>  MozPromiseHolder<ExplicitPromise> holder;
>  holder.Resolve(Other{}, __func__);
The last line (and more similar ones under it) should fail to compile, because we are trying to build an ExplicitPromise::ResolveOrRejectValue::ResolveType (aka Explicit) from an 'Other{}', but the conversion constructor for that is marked 'explicit' and therefore should be ignored.

Here's a try with just that test on top of the current code (which takes ResolveType by value) :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42a6dbce20b1a345f67070b63a238bee914d4b9c
It fails as expected.

Here's a try with my proposed patch (which takes Resolve/RejectType by const&& or &&) :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c418b3a0e1aea2c5eb17b5b245919626d7d9f2be
It also fails for the same reason.

And this is a try with a patch where Resolve/Reject take forwarding references of any type:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfceedabb6b095bbd5121f5959bebf1cd918dd00
It doesn't fail anymore, meaning the 'explicit' for the conversion constructor was ignored!
(Well, there's a failure in the static analysis build, but it's happening later and is unrelated to our 'explicit' issue.)

Does that convince you?

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the detailed explanation.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8829046 [details]
Bug 1332785 - Optimize MozPromiseHolder::Resolve and Reject -

https://reviewboard.mozilla.org/r/106246/#review107380

::: xpcom/threads/MozPromise.h:1028
(Diff revision 1)
>    {
>      if (!IsEmpty()) {
>        Resolve(aResolveValue, aMethodName);
>      }
>    }
> +  void ResolveIfExists(typename PromiseType::ResolveValueType&& aResolveValue,

I think we can use forwarding reference there because Resolve() takes either ResolveValueType&& or const ResolveValueType& and prevents implicit conversions.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/839e34051bf9
Optimize MozPromiseHolder::Resolve and Reject - r=jwwang

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8829046 [details]
Bug 1332785 - Optimize MozPromiseHolder::Resolve and Reject -

https://reviewboard.mozilla.org/r/106246/#review107380

> I think we can use forwarding reference there because Resolve() takes either ResolveValueType&& or const ResolveValueType& and prevents implicit conversions.

(Sorry, forgot to publish this comment before pushing.)

Thank you for the review & suggestions.

You are correct that forwarding references would work in Resolve/RejectIfExists and would still prevent implicit conversions.
I even gave it a Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fccce0eb1b0eefd987806f55560e1cc03266fbe

As I feared, it makes it harder to spot the location of the error, as the compiler first points at Resolve/Reject inside MozPromise.h, instead of the call site, which is only mentioned 13 lines after the error message!

So I'll keep the patch as-is: I prefer a small maintenance cost in one location (with the duplicated const&/&& functions) as a trade-off for more obvious error messages whenever something wrong is done.

(There may be a way to use templated functions that only accept the correct type, but it'd be quite a bit of work to get right; we can do that in a future bug if really deemed useful...)

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/839e34051bf9