1009569 - Promise then() must always be called on a clean stack

Status: RESOLVED FIXED

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

Description

[Kyle Simpson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140508030203

Steps to reproduce:

See test cases.

Promise.resolve(): https://gist.github.com/getify/d64bb01751b50ed6b281#file-bug1-js

Also, these two test cases are probably (but not definitely) the same bug, but affecting other API calls.

Promise.race(): https://gist.github.com/getify/d64bb01751b50ed6b281#file-bug2-js
Promise.all(): https://gist.github.com/getify/d64bb01751b50ed6b281#file-bug3-js




Actual results:

The three test cases linked above produce these outputs:

[1,2]
[5,6]
[5,6]


Expected results:

The test cases linked above should have produced these outputs (as Chrome does):

[0,2]
[0,6]
[0,6]

--------------

The common denominator is that the `thenable.then(..)` is being called synchronously when passed to Promise.resolve(), Promise.race(), and Promise.all(), but the spec calls for `then()` to always be called on a clear stack (aka, asynchronously).

When this bug was discovered recently, it was found that the spec was unclear on this point, so the spec was clarified here: https://bugs.ecmascript.org/show_bug.cgi?id=2837

The reason I am guessing that all 3 bugs are the same bug is because the spec suggests (does not strictly require) that Promise.race() and Promise.all() should use Promise.resolve().

So I think the problem is with Promise.resolve(), that upon receiving a thenable (not a genuine promise), it synchronously calls `.then(..)` on it, which is incorrect.

Comment 1

[Domenic Denicola]

To clarify, this is a pretty recent fix to the spec (hasn't even made it into the official Word document yet), so until recently Firefox was compliant :).

Also:

> the spec suggests (does not strictly require) that Promise.race() and Promise.all() should use Promise.resolve().


The spec does strictly require this, or more precisely, it strictly requires that `this.resolve` is called.

Comment 2

[Kyle Simpson]

> The spec does strictly require this

IIUC, I only meant that the spec doesn't require, implementation-wise, that you have to share the code. FF theoretically could have internally 3 separate implementations of the logic, one for each of those API methods, such that this bug needs to be fixed in 3 places, not 1.

(side note: my promise polyfill was doing exactly that until a recent revision where I consolidated)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Nikhil, do you have the bandwidth for this?

Comment 4

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

Yes, I should be able to have a fix this week.

Comment 5

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

Attachment: Promise then() must be called on a clean stack.

The anonymous namespace has just been moved around and does not need reviewing.

Does JS::Rooted<JSObject*> rootedThenable in ThenableResolverMixin::RunInternal() need wrapping?

Comment 6

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

Attachment: Promise then() must be called on a clean stack.

The anonymous namespace has just been moved around and does not need reviewing.

Does JS::Rooted<JSObject*> rootedThenable in ThenableResolverMixin::RunInternal() need wrapping?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8424955 [details] [diff] [review]
Promise then() must be called on a clean stack.

Why did we need to move all that code, exactly?  If there's a good reason, we can do it, but otherwise, it seems like it confuses blame for no gain?

Comment 8

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

I wanted to keep ThenableResolverMixin close to PromiseResolverMixin since they have very similar structures (maybe even refactorable?). Had to move the namespace before it for C++ name lookup reasons.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Had to move the namespace before it for C++ name lookup reasons.

Couldn't you just forward-declare in the new spot?  I guess that has its own confusion issues... OK.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8424955 [details] [diff] [review]
Promise then() must be called on a clean stack.

>+      new PromiseInit(mThen, mozilla::dom::GetIncumbentGlobal());

Shouldn't you have saved the incumbent global from when we posted the task in this object instead?  That seems like it woud be more likely to do the right thing....

Most simply, this could be done by creating the PromiseInit in the caller and passing in that instead of a raw JSObject mThen.

And of course the spec needs a spec issue here, since it doesn't know about incumbent globals.

>+class WorkerThenableResolverTask MOZ_FINAL : public WorkerSameThreadRunnable,
>+                                            public ThenableResolverMixin

Fix the indent?

r=me with those bits fixed.

Comment 11

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

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

Comment 12

[Carsten Book [:Tomcat]]

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