Closed
Bug 1237762
Opened 9 years ago
Closed 8 years ago
Promise resolved with itself fails to reject
Categories
(Core :: JavaScript: Standard Library, defect)
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: ljharb, Assigned: evilpie)
References
Details
Attachments
(1 file)
(deleted),
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
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
Updated•9 years ago
|
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Comment 1•9 years ago
|
||
> 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•9 years ago
|
||
> 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
Comment 3•9 years ago
|
||
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 5•9 years ago
|
||
(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 :-(
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•9 years ago
|
||
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?
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•9 years ago
|
||
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•9 years ago
|
||
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.
Updated•8 years ago
|
Blocks: es6promises
Comment 11•8 years ago
|
||
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•8 years ago
|
Component: DOM → JavaScript: Standard Library
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → evilpies
Assignee | ||
Comment 12•8 years ago
|
||
So I just tried to test this and somehow I don't see a TypeError?
Comment 13•8 years ago
|
||
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•8 years ago
|
||
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 15•8 years ago
|
||
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•8 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 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.
Description
•