1362912 - Inconsistent behavior with the JS promise in promise chaining

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P3)

Description

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

For the JS code below:

Promise.resolve(333)
.then(val => {
  log(val);
})
.then(val => {
  log(`resolved: ${val}`);
})

The second then will print "resolved: undefined" instead of "resolved: 333". This is inconsistent with our MozPromise where the 2nd then is resolved with 333 when the 1st then doesn't return a MozPromise.

Since there is no easy way to pass an 'undefined' value in C++, we should just assert again it.

Note this bug is a prerequisite for bug 1362910 where we should not pass the resolved value to more than one Then values.

Comment 1

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

Attachment: Bug 1362912. P1 - disallow promise chaining when any of the Then callbacks doesn't return a promise.

A template won't be instatiated until used. We make it a compile error
to call Then() on ThenCommand<false> to disallow promise chaining.

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

Comment 2

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

Attachment: Bug 1362912. P2 - fix the callers.

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

Comment 3

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

Comment on attachment 8866202 [details]
Bug 1362912. P1 - disallow promise chaining when any of the Then callbacks doesn't return a promise.

https://reviewboard.mozilla.org/r/137850/#review140962

::: xpcom/threads/MozPromise.h:441
(Diff revision 1)
>  
>        // Invoke the resolve or reject method.
>        RefPtr<MozPromise> result = DoResolveOrRejectInternal(aValue);
>  
> -      // If there's a completion promise, resolve it appropriately with the
> -      // result of the method.
> +      MOZ_DIAGNOSTIC_ASSERT(!mCompletionPromise || result,
> +        "Can't do promising chaining for a non-promise-returning method.");

promising -> promise

::: xpcom/threads/MozPromise.h:765
(Diff revision 1)
> +    template <typename...>
>      operator RefPtr<MozPromise>()
>      {
> +      static_assert(SupportChaining,
> +        "The resolve/reject callback needs to return a RefPtr<MozPromise> "
> +        "in order to do promising chaining.");

promising -> promise

Comment 4

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

Comment on attachment 8866203 [details]
Bug 1362912. P2 - fix the callers.

https://reviewboard.mozilla.org/r/137852/#review140964

Comment 5

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

Thanks for the review!

Comment 6

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

Comment on attachment 8866202 [details]
Bug 1362912. P1 - disallow promise chaining when any of the Then callbacks doesn't return a promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137850/diff/1-2/

Comment 7

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

Comment on attachment 8866203 [details]
Bug 1362912. P2 - fix the callers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137852/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54b325c535a7
P1 - disallow promise chaining when any of the Then callbacks doesn't return a promise. r=gerald
https://hg.mozilla.org/integration/autoland/rev/86297713a790
P2 - fix the callers. r=gerald

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54b325c535a7
https://hg.mozilla.org/mozilla-central/rev/86297713a790