Closed Bug 1062323 Opened 10 years ago Closed 10 years ago

Promise sequencing incorrect

Categories

(Core :: DOM: Core & HTML, defect)

35 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: getify, Assigned: nsm)

References

Details

Attachments

(4 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:35.0) Gecko/20100101 Firefox/35.0 Build ID: 20140903030206 Steps to reproduce: Test case: var p = new Promise(function(resolve, reject){ console.log("start"); resolve(""); }); p.then(function () { console.log("1-1"); }).then(function () { console.log("1-2"); }).then(function () { console.log("1-3"); }); p.then(function () { console.log("2-1"); }).then(function () { console.log("2-2"); }).then(function () { console.log("2-3"); }); Actual results: "start" "1-1" "1-2" "1-3" "2-1" "2-2" "2-3" Expected results: "start" "1-1" "2-1" "1-2" "2-2" "1-3" "2-3" The ES6 spec queues then notification tasks into a FIFO order, so the implication is that this is the correct order (interleaving these two chains back-and-forth). Chrome, as well as the <a href="https://github.com/domenic/promises-unwrapping/tree/master/reference-implementation">Reference Implementation</a> for `Promise` all agree on the ordering. Note: AFAICT, there's not (yet) an ES6 conformance test for this inter-promise ordering, so the assertion here is being made purely off reading of the spec draft itself.
OS: Mac OS X → All
Hardware: x86 → All
Promise is currently implemented outside the JS engine, moving this bug.
Component: JavaScript: Standard Library → DOM
WrapperPromiseCallback::Call currently does a sync resolve of its following promise. Should that just be async? Why exactly is it sync?
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(amarchesini)
When that code was implemented the spec was saying: https://github.com/whatwg/dom/blob/3e2ee7152caf9b69a5179d11edf8965acb68c115/Overview.src.html: "If invoking callback threw an exception, catch it and run resolver's reject with the thrown exception as argument and the synchronous flag set. The exception is not re-thrown and does not reach window.onerror. Otherwise, run resolver's resolve with value and the synchronous flag set."
Flags: needinfo?(amarchesini)
Ah, so the spec just changed on us, without the test suite getting updated in the process? Domenic, are there plans to update the test suite to reflect current spec reality here?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(domenic)
Oh, that old spec... :( Pretty much unrelated to ES6 promises. There are vague plans to create a test suite for the ES6 promises spec, but they are mostly unofficial, e.g. https://github.com/promises-es6/promises-es6 plus some plans to convert that to test262 format. There are bootstrapping problems however since due to the lack of other async functionality in ES you kind of have to assume promises work to test promises. See https://github.com/tc39/test262/issues/56. In terms of my personal time commitment, I have not been able to prioritize writing a test suite high enough to overcome other priorities, and have been hoping efforts like smikes's above will be able to carry the day without me.
Flags: needinfo?(domenic)
> Oh, that old spec... :( That's what our implementation was initially based on; it predates ES6 promises. That said, we did test our implementation against the then-existing test suite at the time.
At a SW shindig this week. I'll probably look at this later. Seems like a good first bug though.
Flags: needinfo?(nsm.nikhil)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment on attachment 8493435 [details] [diff] [review] Chained promises should resolve asynchronously r=me
Attachment #8493435 - Flags: review?(bzbarsky) → review+
Julien, could you take a look? I'm having some trouble understanding the tests. It seems waitForSegmentInfo() calls the same Promise resolution function multiple times, which shouldn't even work.
Flags: needinfo?(felash)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #12) > Julien, > > could you take a look? I'm having some trouble understanding the tests. It > seems waitForSegmentInfo() calls the same Promise resolution function > multiple times, which shouldn't even work. No, it doesn't do this. "waitForSegmentInfo" really use "waitForComposeEvent" defined in [1]. You see that it calls "resolve()" only once "count" reaches 0. So it's not possible that "resolve" is called twice. The exercised code is [2]. I don't exactly remember what the code is testing (the test evolved when we switched to Promises, and when I converted it I wasn't _really_ sure it was still useful). Basically, the test ensures that when I get the "segmentinfochange" event, I get the "segmentInfo" value change that triggered that event. Maybe we don't need to be that strict and we can simply ensure that the last value in the results is the latest received. According to the test output this would make the test pass, and this is still correct from the application point of view. In the past, before we used Promises, we had races where sometimes we didn't have the latest value. I think such races are not possible with Promises anyway, even when they're more asynchronous like here. Do you think you'll be able to make the change to the tests and I can review it, or do you prefer that I change it myself and ask a review to another peer? The change would replace assert.deepEqual(results, [ segmentInfo1, segmentInfo2 ]); by assert.equal(results[1], segmentInfo2); (and similar for the second test). [1] https://github.com/mozilla-b2g/gaia/blob/9fb6c954d2143a21f79e8eae36ea531067802735/apps/sms/test/unit/compose_test.js#L79-L89 [2] https://github.com/mozilla-b2g/gaia/blob/9fb6c954d2143a21f79e8eae36ea531067802735/apps/sms/js/compose.js#L350-L366
Flags: needinfo?(felash)
Comment on attachment 8494612 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24391 green both with and without the patch, r=me
Attachment #8494612 - Flags: review?(felash) → review+
Gaia master: 922855fd0d984a164ce4f1d87eaa33384338dc1b
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #17) > https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd8df8e8631 In the future, please wait until the Gaia patch has had a chance to propagate around before pushing the Gecko patch that depends on it. Now I've got perma-failing Gu on inbound again...
The issue in voicemail is in apps/system/js/voicemail.js: in "setupNotifications", we return promise where (I think) we should return the result of the promise chain. In this case, the test is good and shows a code issue, yay :) I'll prepare a patch and ask someone to review.
Attached file github PR
Pass without the patch, I'm testing with the patch now.
Comment on attachment 8495070 [details] [review] github PR Fixes the issue with the patch. Hey Etienne and Alexandre, can you please have a look to this fix? The Gecko patch here is changing how "then" callbacks are called. Previously it was synchronous, and now it's asynchronous. As a result, the test is failing (see [1]) because we try to trigger the event before it's been really added, despite that the "setup" function waits for the promise to call "done" :) But maybe you had a good reason to return the original promise instead of the chain? [1] https://tbpl.mozilla.org/php/getParsedLog.php?id=48653081&tree=Mozilla-Inbound#error2
Attachment #8495070 - Flags: review?(etienne)
Attachment #8495070 - Flags: feedback?(lissyx+mozillians)
Comment on attachment 8495070 [details] [review] github PR lgtm :)
Attachment #8495070 - Flags: review?(etienne) → review+
Comment on attachment 8495070 [details] [review] github PR I think this is waht we wanted to do from scratch :)
Attachment #8495070 - Flags: feedback?(lissyx+mozillians) → feedback+
gaia master for the voicemail fix: f34925055ae0a520a0bbbae8c64698391691a06f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Main patch has not landed yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Oh sorry, it actually did :)
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
QA Whiteboard: [good first verify]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: