1317376 - Promise self-resolution not detected

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Test case:
---
var resolve;

var p1 = new Promise(function(_resolve) { resolve = _resolve; });
var p2 = p1.then(function() {
    return p2;
});

p2.then(function() {
    print("Resolved");
}, function(reason) {
    print("Rejected");
});

resolve();
---

Expected: Prints "Rejected" (with a TypeError reason)
Actual: No output at all


Test case is derived from test262, see also:
test262/built-ins/Promise/prototype/then/resolve-pending-fulfilled-self.js
test262/built-ins/Promise/prototype/then/resolve-pending-rejected-self.js
test262/built-ins/Promise/prototype/then/resolve-settled-fulfilled-self.js
test262/built-ins/Promise/prototype/then/resolve-settled-rejected-self.js

Comment 1

[Till Schneidereit [:till]]

Huh, that's weird: I ran the test262 test after all changes to the Promise code and didn't get this. Either I used an outdated checkout of the tests, or I messed this up some other way, clearly.

Comment 2

[André Bargull [:anba]]

Attachment: bug1317376-part1.patch

A bit of clean-up before adding the actual patch:

- Add "enum PromiseHandler" because we no longer need to synchronize the handler values with self-hosted code.
- Remove the unused RejectFunctionSlot_PromiseAllData enum value.
- NewPromiseAllDataHolder is called from
  - GetWaitForAllPromise, which invokes NewPromiseCapability with canOmitResolutionFunctions=false, therefore the resolve function is never null.
  - PerformPromiseAll, which is only called from Promise_static_all, and Promise_static_all also invokes NewPromiseCapability with canOmitResolutionFunctions=false.
  => That means the resolve function for PromiseAllDataHolder is always defined.
- Align already-resolved check in ResolvePromiseFunction with the same check in RejectPromiseFunction.
- The RejectFunctionSlot_ResolveFunction slot of a RejectPromiseFunction always contains a ResolvePromiseFunction, assert this in GetResolveFunctionFromReject.
- Add GetRejectFunctionFromResolve to match GetResolveFunctionFromReject.
- PerformPromiseAll is only called from Promise_static_all, which in turn calls NewPromiseCapability with canOmitResolutionFunctions=false.
  => That means the resolve/reject functions are always defined.
- In PromiseAllResolveElementFunction, the resolve function in a PromiseAllDataHolder is never null, therefore the branch to call ResolvePromiseInternal is unreachable.
- PerformPromiseRace is only called from Promise_static_race, which in turn calls NewPromiseCapability with canOmitResolutionFunctions=false.
  => That means the resolve/reject functions are always defined.

Comment 3

[André Bargull [:anba]]

Attachment: bug1317376-part2.patch

The Promise self-resolution check needed to be moved from ResolvePromiseFunction to ResolvePromiseInternal, so it's also executed when ResolvePromiseInternal is called from PromiseReactionJob (through RunResolutionFunction).

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8825017 [details] [diff] [review]
bug1317376-part1.patch

Review of attachment 8825017 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for doing this. Some of this is just cruft I didn't properly clean up during the rewrite. Some was intended to be used for further optimizations: we can omit the creation of resolution functions in more places in theory. In practice, this turned out to have subtle consequences, so I left it for later. There really isn't a good reason for leaving these half-baked preparations around.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8825018 [details] [diff] [review]
bug1317376-part2.patch

Review of attachment 8825018 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you.

Comment 6

[André Bargull [:anba]]

(In reply to Till Schneidereit [till] from comment #4)
> Thank you for doing this. Some of this is just cruft I didn't properly clean
> up during the rewrite. Some was intended to be used for further
> optimizations: we can omit the creation of resolution functions in more
> places in theory. In practice, this turned out to have subtle consequences,
> so I left it for later. There really isn't a good reason for leaving these
> half-baked preparations around.

Thanks for the explanation. I wasn't sure if this was old or unfinished code and a bit too lazy to read the commit logs. :-)

Comment 7

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=289bac78de16438a759bf23f159104ad03ed4fa8

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/def2163ef32c
Part 1: Remove unreachable code and remnants from the self-hosted implementation. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/342e9369b936
Part 2: Detect Promise self-resolution when resolving through the Promise resolving fast path. r=till

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/def2163ef32c
https://hg.mozilla.org/mozilla-central/rev/342e9369b936

Comment 10

[Sylvestre Ledru [:Sylvestre]]

André, is it something that you are planning to uplift in 52? Thanks

Comment 11

[André Bargull [:anba]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #10)
> André, is it something that you are planning to uplift in 52? Thanks

I asked :till on #jsapi if it makes sense to uplift to this bug and he said it's probably not necessary.