Promise resolved with itself fails to reject

RESOLVED FIXED in Firefox 51

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ljharb, Assigned: evilpie)

Tracking

46 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/601.2.7 (KHTML, like Gecko) Version/9.0.1 Safari/601.2.7

Steps to reproduce:

var resolve;
p = new Promise(function (y) { resolve = y; });
resolve℗;
p.then(console.log.bind(console, 'resolved')).catch(console.log.bind(console, 'rejected'));


Actual results:

Nothing - the promise is forever pending.


Expected results:

It should have been rejected per step 6 of http://www.ecma-international.org/ecma-262/6.0/index.html#sec-promise-resolve-functions
(Reporter)

Updated

3 years ago
Blocks: 911216
OS: Unspecified → All
Hardware: Unspecified → All

Updated

3 years ago
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
> resolve℗;

Did you mean |resolve(p);|?

And is it impossible to fix this without implementing Promise in JavaScript Engine? If it is not, the Component should be DOM.
> And is it impossible to fix this without implementing Promise in JavaScript Engine? If it is not, the Component should be DOM.

The bug exists in our current Promise implementation. The one in SpiderMonkey shouldn't have it, but having this bug block that will help me keep track of it.
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → DOM
Ever confirmed: true
I could have sworn we had code for this, but we clearly don't.  It's possible this was added to the spec after our initial Promise implementation and we never updated to it...

In any case, it's simple to fix this in the DOM promise impl if we want to.
(Reporter)

Comment 4

3 years ago
Yes, sorry - my autocorrect turned ( p ) into ℗ :-p
(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #3)
> In any case, it's simple to fix this in the DOM promise impl if we want to.

Based on our IRC conversation, at lesat ljharb would specifically *not* want us to fix it in the existing implementation:

22:49 <ljharb> the primary reason i'm concerned is that your new implementation actually *fixes* enough bugs that the es6-shim can no longer synchronously determine that it's buggy (to replace it)
22:49 <ljharb> the only remaining bug i have tests for is this one
22:50 <ljharb> so if this ships, then i'd have to do fingerprinting to determine if it's an affected version of firefox, rather than feature testing :-(
(Reporter)

Comment 6

3 years ago
I would definitely prefer to remove the broken DOM implementation very quickly, and my concern is *not* in the DOM implementation - it has a plethora of bugs so it's very easy for the es6-shim to detect and overwrite it. This is a concern with the new implementation in Nightly.
Why are you overwriting the native implementation in the first place?
The new implementation should fix bugs so that shims/polyfills or whatever don't have to overwrite it. We shouldn't rely on shims forever, no?
(Reporter)

Comment 8

3 years ago
You can see some of the native failures here: https://rawgit.com/es-shims/es6-shim/master/test/native.html

My goal is correctness - first I want an implementation to be correct, then, if it's incorrect, I want to be able to synchronously detect that it's incorrect, so I can overwrite it.

This is the last remaining Promise bug I test for (although the enumerability of the static methods seems to have recently flipped back to broken), and it's not synchronously detectable, so Firefox Nightly is in a state right now (when I filed this bug, at least) that I can't reliably patch with feature-testing. If this is fixed quickly, I won't have to shim Promises in Firefox at all moving forward.
Ah, thanks for the explanation. That speaks to fixing the bug in the current implementation, given that we'll probably not ship the new one until the next release train.

I should also clarify: what's in Nightly right now is still our old implementation. bz just fixed many issues when he introduced subclassing support in bug 1170760 recently. The new implementation in SpiderMonkey is tracked by the bug blocked by this.
In conclusion, we should fix this bug in our DOM implementation so that es6-shim doesn't resort to sniffing because the new implementation will not ship in this release.
Tested on Nightly 50.0a1 (2016-07-17) (with bug 911216 fixed), now it's rejected.

> var resolve;
undefined
> p = new Promise(function (y) { resolve = y; });
Promise { <state>: "pending" }
> resolve(p);
undefined
TypeError: A promise cannot be resolved with itself.
> p.then(console.log.bind(console, 'resolved')).catch(console.log.bind(console, 'rejected'));
Promise { <state>: "pending" }
rejected TypeError: A promise cannot be resolved with itself.
Stack trace:
@debugger eval code:1:1
(Assignee)

Updated

2 years ago
Component: DOM → JavaScript: Standard Library
(Assignee)

Updated

2 years ago
Assignee: nobody → evilpies
(Assignee)

Comment 12

2 years ago
So I just tried to test this and somehow I don't see a TypeError?
comment #11 is the input and output for the case when I enter each line one by one in web console.
if I enter those lines at once, "TypeError: A promise cannot be resolved with itself." is not logged
(Assignee)

Comment 14

2 years ago
Created attachment 8778436 [details] [diff] [review]
Promise resolved with itself rejects

Ah turns out I had to call drainJobQueue otherwise the program would end before the promise reject handler is invoked. Should we maybe check that the job queue is empty at the end of a test?
Attachment #8778436 - Flags: review?(till)
Comment on attachment 8778436 [details] [diff] [review]
Promise resolved with itself rejects

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

Thanks.

::: js/src/tests/ecma_6/Promise/self-resolve.js
@@ +1,4 @@
> +// |reftest| skip-if(!xulRuntime.shell) -- needs drainJobQueue
> +
> +if (!this.Promise) {
> +    reportCompare(true,true);

Guard this with `this.reportCompare &&` or something like it, so the test can be run outside the harness.

@@ +19,5 @@
> +
> +assertEq(results.length, 1);
> +assertEq(results[0], "rejected");
> +
> +reportCompare(0, 0, "ok");

Same here.
Attachment #8778436 - Flags: review?(till) → review+

Comment 16

2 years ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc5cd00a3250
Test: Promise resolved with itself rejects.r=till

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc5cd00a3250
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.