1237762 - Promise resolved with itself fails to reject

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[ljharb]

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

Comment 1

[Masatoshi Kimura [:emk]]

> 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.

Comment 2

[Till Schneidereit [:till]]

> 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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[ljharb]

Yes, sorry - my autocorrect turned ( p ) into ℗ :-p

Comment 5

[Till Schneidereit [:till]]

(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 :-(

Comment 6

[ljharb]

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.

Comment 7

[Masatoshi Kimura [:emk]]

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?

Comment 8

[ljharb]

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.

Comment 9

[Till Schneidereit [:till]]

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.

Comment 10

[Masatoshi Kimura [:emk]]

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.

Comment 11

[Tooru Fujisawa [:arai]]

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

Comment 12

[Tom S [:evilpie]]

So I just tried to test this and somehow I don't see a TypeError?

Comment 13

[Tooru Fujisawa [:arai]]

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

Comment 14

[Tom S [:evilpie]]

Attachment: 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?

Comment 15

[Till Schneidereit [: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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc5cd00a3250
Test: Promise resolved with itself rejects.r=till

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bc5cd00a3250