1062323 - Promise sequencing incorrect

Status: RESOLVED FIXED

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

Description

[Kyle Simpson]

Attachment: 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.

Comment 1

[Jan de Mooij [:jandem]]

Promise is currently implemented outside the JS engine, moving this bug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

WrapperPromiseCallback::Call currently does a sync resolve of its following promise.  Should that just be async?  Why exactly is it sync?

Comment 3

[Andrea Marchesini [:baku]]

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."

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Domenic Denicola]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

At a SW shindig this week. I'll probably look at this later. Seems like a good first bug though.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Chained promises should resolve asynchronously

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8493435 [details] [diff] [review]
Chained promises should resolve asynchronously

r=me

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e449addcdf75

Comment 11

[Ed Morley [:emorley]]

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

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 13

[Julien Wajsberg [:julienw]]

(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

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24391

Comment 15

[Julien Wajsberg [:julienw]]

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

Comment 16

[Julien Wajsberg [:julienw]]

Gaia master: 922855fd0d984a164ce4f1d87eaa33384338dc1b

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd8df8e8631

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(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

[Ryan VanderMeulen [:RyanVM]]

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

[Julien Wajsberg [:julienw]]

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

[Julien Wajsberg [:julienw]]

Attachment: github PR

Pass without the patch, I'm testing with the patch now.

Comment 22

[Julien Wajsberg [:julienw]]

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

Comment 23

[Etienne Segonzac (:etienne)]

Comment on attachment 8495070 [details] [review]
github PR

lgtm :)

Comment 24

[:gerard-majax]

Comment on attachment 8495070 [details] [review]
github PR

I think this is waht we wanted to do from scratch :)

Comment 25

[Julien Wajsberg [:julienw]]

gaia master for the voicemail fix: f34925055ae0a520a0bbbae8c64698391691a06f

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c84cee4a85b6

Comment 27

[Julien Wajsberg [:julienw]]

Main patch has not landed yet.

Comment 28

[Julien Wajsberg [:julienw]]

Oh sorry, it actually did :)