Closed Bug 1009569 Opened 6 years ago Closed 6 years ago

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

Categories

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

32 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: getify, Assigned: nsm)

References

()

Details

(Keywords: testcase)

Attachments

(1 file, 1 obsolete file)

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.
OS: Mac OS X → All
Hardware: x86 → All
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.
> 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)
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Keywords: testcase
Product: Firefox → Core
Nikhil, do you have the bandwidth for this?
Flags: needinfo?(nsm.nikhil)
Yes, I should be able to have a fix this week.
Flags: needinfo?(nsm.nikhil)
The anonymous namespace has just been moved around and does not need reviewing.

Does JS::Rooted<JSObject*> rootedThenable in ThenableResolverMixin::RunInternal() need wrapping?
Attachment #8424952 - Flags: review?(bzbarsky)
The anonymous namespace has just been moved around and does not need reviewing.

Does JS::Rooted<JSObject*> rootedThenable in ThenableResolverMixin::RunInternal() need wrapping?
Attachment #8424955 - Flags: review?(bzbarsky)
Attachment #8424952 - Attachment is obsolete: true
Attachment #8424952 - Flags: review?(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?
Flags: needinfo?(nsm.nikhil)
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.
Flags: needinfo?(nsm.nikhil)
> 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 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.
Attachment #8424955 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/d489e0bc4e5d
Assignee: nobody → nsm.nikhil
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Flags: in-testsuite+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.