Closed Bug 1362912 Opened 3 years ago Closed 3 years ago

Inconsistent behavior with the JS promise in promise chaining

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

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.
Assignee: nobody → jwwang
Blocks: 1362910
Priority: -- → P3
Attachment #8866202 - Flags: review?(gsquelart)
Attachment #8866203 - Flags: review?(gsquelart)
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
Attachment #8866202 - Flags: review?(gsquelart) → review+
Comment on attachment 8866203 [details]
Bug 1362912. P2 - fix the callers.

https://reviewboard.mozilla.org/r/137852/#review140964
Attachment #8866203 - Flags: review?(gsquelart) → review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/54b325c535a7
https://hg.mozilla.org/mozilla-central/rev/86297713a790
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1364662
You need to log in before you can comment on or make changes to this bug.