Closed Bug 1317376 Opened 5 years ago Closed 5 years ago

Promise self-resolution not detected

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- affected
firefox53 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
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.
Assignee: nobody → till
Flags: needinfo?(till)
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.
Attachment #8825017 - Flags: review?(till)
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).
Attachment #8825018 - Flags: review?(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.
Attachment #8825017 - Flags: review?(till) → review+
Assignee: till → andrebargull
Status: NEW → ASSIGNED
Flags: needinfo?(till)
Comment on attachment 8825018 [details] [diff] [review]
bug1317376-part2.patch

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

Thank you.
Attachment #8825018 - Flags: review?(till) → review+
(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. :-)
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/def2163ef32c
https://hg.mozilla.org/mozilla-central/rev/342e9369b936
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
André, is it something that you are planning to uplift in 52? Thanks
Flags: needinfo?(andrebargull)
(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.
Flags: needinfo?(andrebargull)
You need to log in before you can comment on or make changes to this bug.