Closed
Bug 1062323
Opened 10 years ago
Closed 10 years ago
Promise sequencing incorrect
Categories
(Core :: DOM: Core & HTML, defect)
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.
Reporter | ||
Updated•10 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 1•10 years ago
|
||
Promise is currently implemented outside the JS engine, moving this bug.
Component: JavaScript: Standard Library → DOM
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
> 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.
Assignee | ||
Comment 7•10 years ago
|
||
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 | ||
Comment 8•10 years ago
|
||
Attachment #8493435 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment 9•10 years ago
|
||
Comment on attachment 8493435 [details] [diff] [review]
Chained promises should resolve asynchronously
r=me
Attachment #8493435 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
Reverted for gaia-unit test failures in compose_test.js:
https://tbpl.mozilla.org/php/getParsedLog.php?id=48653081&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=48659855&tree=Mozilla-Inbound
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f086e7f4420e
Assignee | ||
Comment 12•10 years ago
|
||
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)
Comment 13•10 years ago
|
||
(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)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8494612 -
Flags: review?(felash)
Comment 15•10 years ago
|
||
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+
Comment 16•10 years ago
|
||
Gaia master: 922855fd0d984a164ce4f1d87eaa33384338dc1b
Assignee | ||
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
(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...
Comment 19•10 years ago
|
||
Kind of moot because it still has failures in voicemail_test.js anyway. Backed out again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1735ff2bb23e
https://tbpl.mozilla.org/php/getParsedLog.php?id=48805140&tree=Mozilla-Inbound
Comment 20•10 years ago
|
||
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.
Comment 21•10 years ago
|
||
Pass without the patch, I'm testing with the patch now.
Comment 22•10 years ago
|
||
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 23•10 years ago
|
||
Comment on attachment 8495070 [details] [review]
github PR
lgtm :)
Attachment #8495070 -
Flags: review?(etienne) → review+
Comment 24•10 years ago
|
||
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+
Comment 25•10 years ago
|
||
gaia master for the voicemail fix: f34925055ae0a520a0bbbae8c64698391691a06f
Comment 26•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 27•10 years ago
|
||
Main patch has not landed yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 28•10 years ago
|
||
Oh sorry, it actually did :)
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
QA Whiteboard: [good first verify]
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•