Closed
Bug 1009569
Opened 11 years ago
Closed 11 years ago
Promise then() must always be called on a clean stack
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: getify, Assigned: nsm)
References
()
Details
(Keywords: testcase)
Attachments
(1 file, 1 obsolete file)
16.85 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
Comment 1•11 years ago
|
||
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.
Reporter | ||
Comment 2•11 years ago
|
||
> 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)
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Keywords: testcase
Product: Firefox → Core
![]() |
||
Comment 3•11 years ago
|
||
Nikhil, do you have the bandwidth for this?
![]() |
||
Updated•11 years ago
|
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 4•11 years ago
|
||
Yes, I should be able to have a fix this week.
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
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)
![]() |
||
Updated•11 years ago
|
Attachment #8424952 -
Attachment is obsolete: true
Attachment #8424952 -
Flags: review?(bzbarsky)
![]() |
||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
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)
![]() |
||
Comment 9•11 years ago
|
||
> 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•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
Comment 12•11 years ago
|
||
Assignee: nobody → nsm.nikhil
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•11 years ago
|
Flags: in-testsuite+
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
•