Closed Bug 1237762 Opened 9 years ago Closed 8 years ago

Promise resolved with itself fails to reject

Categories

(Core :: JavaScript: Standard Library, defect)

46 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: ljharb, Assigned: evilpie)

References

Details

Attachments

(1 file)

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
Blocks: 911216
OS: Unspecified → All
Hardware: Unspecified → All
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.
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 :-(
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?
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
Component: DOM → JavaScript: Standard Library
Assignee: nobody → evilpies
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
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+
Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bc5cd00a3250 Test: Promise resolved with itself rejects.r=till
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: