Promise sequencing incorrect

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: getify, Assigned: nsm)

Tracking

35 Branch
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
Created attachment 8483500 [details]
screenshot of FF35 incorrect promise sequencing

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

4 years ago
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)

Comment 5

4 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)
> 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)
Created attachment 8493435 [details] [diff] [review]
Chained promises should resolve asynchronously
Attachment #8493435 - Flags: review?(bzbarsky)
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.
Created attachment 8495070 [details] [review]
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
https://hg.mozilla.org/mozilla-central/rev/c84cee4a85b6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.