Implement AP2 Promises

Implement AP2 Promises

This patch implements AP2 Promises, replace the existing Promises with
it and test it with the existing and newly added test cases.
The implementation has almost 1:1 correspondence to the spec.

In the latest draft, ThenableCoercions is used to attach private members
to arbitrary objects without any modification on the objects.
However V8 doesn't expose WeakMap to embedders and it is unavailable
if the harmony flag isn't set. Since V8 GC may relocate the object
addresses, we cannot use them as WeakMap's key. As a result, we cannot
implement O(1) WeakMap in the Blink-side[1].
So instead of using WeakMap, we use V8 hidden properties to attach
private members to the arbitrary objects.

[1]: https://chromiumcodereview.appspot.com/24403002/

BUG=295420
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158915

[yhirano, 2013-09-27 16:43:30]

Yusuke, your internship has ended, and I am not sure how to handle this CL. Do
you want to land this CL by yourself? Or do you want me to take over it? Both
are OK with me.

[yusukesuzuki, 2013-09-27 17:05:19]

On 2013/09/27 16:43:30, yhirano wrote:
> Yusuke, your internship has ended, and I am not sure how to handle this CL. Do
> you want to land this CL by yourself? Or do you want me to take over it? Both
> are OK with me.

I'd like to land it by myself to complete my work :)

[yhirano, 2013-09-27 17:20:06]

On 2013/09/27 17:05:19, yusukesuzuki wrote:
> On 2013/09/27 16:43:30, yhirano wrote:
> > Yusuke, your internship has ended, and I am not sure how to handle this CL.
Do
> > you want to land this CL by yourself? Or do you want me to take over it?
Both
> > are OK with me.
> 
> I'd like to land it by myself to complete my work :)

I see, thank you!

[abarth-chromium, 2013-09-30 19:32:19]

yhirano, did you want to review this CL?

[yhirano, 2013-10-01 02:03:46]

I took vacation yesterday and will look at it from now on.
Anyway, your l-g-t-m is needed for landing.

[yusukesuzuki, 2013-10-01 03:44:03]

Rebased, fixed FIXMEs and added new tests.

https://codereview.chromium.org/24980002/diff/9001/LayoutTests/fast/js/Promis...
File LayoutTests/fast/js/Promise-onFulfilled-deep.html (right):

https://codereview.chromium.org/24980002/diff/9001/LayoutTests/fast/js/Promis...
LayoutTests/fast/js/Promise-onFulfilled-deep.html:19: promise =
promise.then(function (value) { return value; }, function () {
testFailed('rejected'); });
Since AP2 Promise's resolve chain is executed asynchronously, stack overflow
doesn't happen.
This test guarantees that the issue[1] is fixed by the spec change.

[1]: http://crbug.com/272656

https://codereview.chromium.org/24980002/diff/9001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/24980002/diff/9001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8PromiseCustom.cpp:231: v8::Local<v8::Value> value =
V8ScriptRunner::callFunction(m_handler.newLocal(isolate), context,
v8::Undefined(isolate), WTF_ARRAY_LENGTH(args), args, isolate);
Since V8-side change[1] and Blink-side change[2] are landed, we can use
undefined as a receiver.

[1]: https://chromiumcodereview.appspot.com/24920003/
[2]: https://codereview.chromium.org/25313003/

[yhirano, 2013-10-01 08:12:24]

https://codereview.chromium.org/24980002/diff/1/Source/bindings/v8/custom/V8P...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/24980002/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:540: if
(promise->SameValue(valuePromise)) {
We should have a test checking this behavior.

https://codereview.chromium.org/24980002/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:574: return
then(internal->GetInternalField(InternalResultIndex).As<v8::Object>(),
onFulfilled, onRejected, isolate);
Doesn't this function call cause a stack overflow?

https://codereview.chromium.org/24980002/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:605: v8::Local<v8::Array>
derivedPromises =
internal->GetInternalField(InternalDerivedPromiseIndex).As<v8::Array>();
Let's assert that all arrays are of the same length.

https://codereview.chromium.org/24980002/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:624: void
V8PromiseCustom::updateDerived(v8::Handle<v8::Object> derivedPromise,
v8::Handle<v8::Function> onFulfilled, v8::Handle<v8::Function> onRejected,
v8::Handle<v8::Object> originator, v8::Isolate* isolate)
I think stack overflow is introduced, such as:

 UpdateDerived -> UpdateDerivedFromValue -> SetValue -> PropagateToDerived ->
UpdateDerived

Is that correct?
Can you fix that?
If it is hard to fix using C++ and can be solved with JS implementation
(especially with ES6 Proper Tail Calls), you can leave it be and just write
FIXME.

[yusukesuzuki, 2013-10-02 00:20:55]

https://chromiumcodereview.appspot.com/24980002/diff/1/Source/bindings/v8/cus...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://chromiumcodereview.appspot.com/24980002/diff/1/Source/bindings/v8/cus...
Source/bindings/v8/custom/V8PromiseCustom.cpp:540: if
(promise->SameValue(valuePromise)) {
On 2013/10/01 08:12:24, yhirano wrote:
> We should have a test checking this behavior.

Done.

https://chromiumcodereview.appspot.com/24980002/diff/1/Source/bindings/v8/cus...
Source/bindings/v8/custom/V8PromiseCustom.cpp:574: return
then(internal->GetInternalField(InternalResultIndex).As<v8::Object>(),
onFulfilled, onRejected, isolate);
On 2013/10/01 08:12:24, yhirano wrote:
> Doesn't this function call cause a stack overflow?

Done. Fixed. Lookup the promise by while statement.

https://chromiumcodereview.appspot.com/24980002/diff/1/Source/bindings/v8/cus...
Source/bindings/v8/custom/V8PromiseCustom.cpp:605: v8::Local<v8::Array>
derivedPromises =
internal->GetInternalField(InternalDerivedPromiseIndex).As<v8::Array>();
On 2013/10/01 08:12:24, yhirano wrote:
> Let's assert that all arrays are of the same length.

Done in PromisePropagator::propagateToDerived.

https://chromiumcodereview.appspot.com/24980002/diff/1/Source/bindings/v8/cus...
Source/bindings/v8/custom/V8PromiseCustom.cpp:624: void
V8PromiseCustom::updateDerived(v8::Handle<v8::Object> derivedPromise,
v8::Handle<v8::Function> onFulfilled, v8::Handle<v8::Function> onRejected,
v8::Handle<v8::Object> originator, v8::Isolate* isolate)
On 2013/10/01 08:12:24, yhirano wrote:
> I think stack overflow is introduced, such as:
> 
>  UpdateDerived -> UpdateDerivedFromValue -> SetValue -> PropagateToDerived ->
> UpdateDerived
> 
> Is that correct?
> Can you fix that?
> If it is hard to fix using C++ and can be solved with JS implementation
> (especially with ES6 Proper Tail Calls), you can leave it be and just write
> FIXME.

Right.
So I've introduced PromisePropagator. It holds the stack and when propagating
the result to the deriveds, we append them to the stack. After that, we drain
the stack by while statement.

[yhirano, 2013-10-03 03:57:36]

https://codereview.chromium.org/24980002/diff/37001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/24980002/diff/37001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:260: ScopedPersistent<v8::Object>
m_originatorObject;
I think m_originatorValueObject is a better name because this variable
represents originator.[[Value]], not originator itself.

https://codereview.chromium.org/24980002/diff/37001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:363: private:
Please place a blank line before the access specifier.

https://codereview.chromium.org/24980002/diff/37001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:369: while
(!m_derivedStack.isEmpty()) {
How about creating a HandleScope here?

https://codereview.chromium.org/24980002/diff/37001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:444:
ASSERT(V8PromiseCustom::getState(originatorInternal) ==
V8PromiseCustom::Fulfilled || V8PromiseCustom::getState(originatorInternal) ==
V8PromiseCustom::Rejected);
If you define originatorState before the assert, you can use it in the assert.

[yusukesuzuki, 2013-10-03 08:59:41]

https://chromiumcodereview.appspot.com/24980002/diff/37001/Source/bindings/v8...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://chromiumcodereview.appspot.com/24980002/diff/37001/Source/bindings/v8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:260: ScopedPersistent<v8::Object>
m_originatorObject;
On 2013/10/03 03:57:36, yhirano wrote:
> I think m_originatorValueObject is a better name because this variable
> represents originator.[[Value]], not originator itself.

You're right. Renamed.

https://chromiumcodereview.appspot.com/24980002/diff/37001/Source/bindings/v8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:363: private:
On 2013/10/03 03:57:36, yhirano wrote:
> Please place a blank line before the access specifier.

Done.

https://chromiumcodereview.appspot.com/24980002/diff/37001/Source/bindings/v8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:369: while
(!m_derivedStack.isEmpty()) {
On 2013/10/03 03:57:36, yhirano wrote:
> How about creating a HandleScope here?

Sounds nice. I've added it here

https://chromiumcodereview.appspot.com/24980002/diff/37001/Source/bindings/v8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:444:
ASSERT(V8PromiseCustom::getState(originatorInternal) ==
V8PromiseCustom::Fulfilled || V8PromiseCustom::getState(originatorInternal) ==
V8PromiseCustom::Rejected);
On 2013/10/03 03:57:36, yhirano wrote:
> If you define originatorState before the assert, you can use it in the assert.

Done.

[yhirano, 2013-10-03 09:11:57]

lgtm

[abarth-chromium, 2013-10-03 18:49:53]

lgtm

https://codereview.chromium.org/24980002/diff/39001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/24980002/diff/39001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:337: ScopedPersistent<v8::Object>
m_originator;
We could probably just use local handles here because this object appears to be
held only by the PromisePropagator, which is held on the stack.

The risk in using ScopedPersistent here is that we could create a memory leak if
we retain this object in the heap (since it strongly retains these objects).  On
the other hand, if we use Local handles, we'll get memory corruption from that!

It's probably ok to use ScopedPersistent here, but it would be ideal to add an
ASSERT that would trigger if we retain this object in the heap.  I'm not
entirely sure how to write that assert, but maybe you have an idea.

https://codereview.chromium.org/24980002/diff/39001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:373: m_derivedStack.removeLast();
Consider using a Deque instead of a Vector.  Then you can call takeFirst() to
combine these operations (or add a takeLast if you need the last one).

https://codereview.chromium.org/24980002/diff/39001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:736: // FIXME: Currently we
dones't lookup "constructor" property since we limits
dones't -> don't

limits -> limit

[yusukesuzuki, 2013-10-03 20:19:06]

https://codereview.chromium.org/24980002/diff/39001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/24980002/diff/39001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:337: ScopedPersistent<v8::Object>
m_originator;
On 2013/10/03 18:49:54, abarth wrote:
> We could probably just use local handles here because this object appears to
be
> held only by the PromisePropagator, which is held on the stack.
> 
> The risk in using ScopedPersistent here is that we could create a memory leak
if
> we retain this object in the heap (since it strongly retains these objects). 
On
> the other hand, if we use Local handles, we'll get memory corruption from
that!

Since users can create a lot of Derived tuples by using recursive propagation
routines calls and handle stack may be grown infinitely, we inserted
v8::HandleScope to performPropagation loop and made Derived's members
ScopedPersistent.
If it isn't problem, we can replace them with v8::Local and remove
v8::HandleScope in performPropagation loop. In this case, memory corruption
doesn't happen. Is it preferable?

> It's probably ok to use ScopedPersistent here, but it would be ideal to add an
> ASSERT that would trigger if we retain this object in the heap.  I'm not
> entirely sure how to write that assert, but maybe you have an idea.

I cannot come up with any nice idea about checking by ASSERT.

But since only PromisePropagator holds Derived tuples ownership, we can change
Derived to PromisePropagator inner class to hide Derived from other classes.

And by checking the use cases that PromisePropagator is held only on the stack,
I think that we can ensure that Derived isn't retained in the heap. Since
PromisePropagator is defined in the cpp file, it is only used in this file. So
we can check it easily.

I've changed Derived to PromisePropagator inner class and added
"PromisePropagator should be held on the stack" comment.

https://codereview.chromium.org/24980002/diff/39001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:373: m_derivedStack.removeLast();
On 2013/10/03 18:49:54, abarth wrote:
> Consider using a Deque instead of a Vector.  Then you can call takeFirst() to
> combine these operations (or add a takeLast if you need the last one).

Sounds nice! I've added takeLast/removeLast to Deque and used it here.

https://codereview.chromium.org/24980002/diff/39001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:736: // FIXME: Currently we
dones't lookup "constructor" property since we limits
On 2013/10/03 18:49:54, abarth wrote:
> dones't -> don't
> 
> limits -> limit

Done.

[abarth-chromium, 2013-10-03 20:42:52]

On 2013/10/03 20:19:06, yusukesuzuki wrote:
> I've changed Derived to PromisePropagator inner class and added
> "PromisePropagator should be held on the stack" comment.

That sounds like a great approach.  Thank you!

[commit-bot: I haz the power, 2013-10-04 05:50:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukesuzuki@chromium.org/24980002/52001

[commit-bot: I haz the power, 2013-10-04 10:35:47]

Change committed as 158915