[es6] refactor Promise resolution

[es6] refactor Promise resolution

Several changes are included here:

1. Each resolution callback references shared data indicating whether
it has already been resolved or not, as described in 25.4.1.3
http://tc39.github.io/ecma262/#sec-createresolvingfunctions.
Previously this was handled exclusively by the Promise's status,
which does not work correctly with the current chaining behaviour.

2. During fulfillment, When a Promise is resolved with a thenable, the
spec chains the promises together by invoking the thenable's `then`
function with the original Promise's resolve and reject methods (per
section 25.4.2.2, or
http://tc39.github.io/ecma262/#sec-promiseresolvethenablejob, on the
next tick, regardless of whether or not there are pending tasks.

3. Adds a spec compliance fix to ensure that the Promise constructor
is only loaded once when `then()` is called, solving v8:4539 as well.
This involves refactoring PromiseChain to accept a constructor
argument. PromiseChain/PromiseDeferred will hopefully be removed soon,
simplifying the process.

BUG=v8:4162, v8:4539, v8:3237
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org

Committed: https://crrev.com/24ff30b740236619e911106573ee5cde0cd16dc2
Cr-Commit-Position: refs/heads/master@{#32046}

[caitp (gmail), 2015-10-07 20:51:39]

This is just a quick WIP. Fixing this breaks the current behaviour of
PromiseChain(), and per v8:3237 it sounds like that one will be difficult to get
rid of. However, it does match existing promise libraries.

If anyone thinks it's worth carrying on with it, I'll fix up tests + debugging
support, and clean up/improve perf. If keeping PromiseChain is really needed, an
alternative implementation could be supplied.

If it's not worth working on, that's fine too.

[stefan.penner, 2015-10-07 21:09:54]

On 2015/10/07 at 20:51:39, caitpotter88 wrote:

> This is just a quick WIP

Implementation looks correct.

> it sounds like that one will be difficult to get rid of. 

Hopefully that doesn't prevent it (and `Promise.prototype.accept`) from,
hopefully soon, being removed entirely. Carrying around unspecified public API
feels like a forward compatibility trap.

[stefan.penner, 2015-10-07 21:17:42]

On 2015/10/07 at 21:09:54, stefan.penner wrote:
> On 2015/10/07 at 20:51:39, caitpotter88 wrote:
> 
> > This is just a quick WIP
> 
> Implementation looks correct.
> 
> > it sounds like that one will be difficult to get rid of. 
> 
> Hopefully that doesn't prevent it (and `Promise.prototype.accept`) from,
hopefully soon, being removed entirely. Carrying around unspecified public API
feels like a forward compatibility trap.

I believe this may indicate a missing scenario in the promise/a+ suite. Although
the a+ spec does imply this behavior, the related tests only appear to test
foreign thenables. So own-thenables, with tampered `then`'s appear to sneak
through.

[rossberg, 2015-10-12 14:18:59]

It looks like it would be possible to not interfere with chain and friends by
making this a separate wrapper around PromiseResolve, say,
PromiseResolveForThenables, and invoke that in the promise constructor. Or am I
missing some other implications?

[caitp (gmail), 2015-10-12 14:22:03]

On 2015/10/12 14:18:59, rossberg wrote:
> It looks like it would be possible to not interfere with chain and friends by
> making this a separate wrapper around PromiseResolve, say,
> PromiseResolveForThenables, and invoke that in the promise constructor. Or am
I
> missing some other implications?

Yes, it should be possible to do that

[caitp (gmail), 2015-10-12 19:19:10]

On 2015/10/12 14:22:03, caitp wrote:
> On 2015/10/12 14:18:59, rossberg wrote:
> > It looks like it would be possible to not interfere with chain and friends
by
> > making this a separate wrapper around PromiseResolve, say,
> > PromiseResolveForThenables, and invoke that in the promise constructor. Or
am
> I
> > missing some other implications?
> 
> Yes, it should be possible to do that

I'm mistaken.

Consider the following snippet:

```
var res;

var p = new Promise(function(resolve, reject) { res = resolve; });
res({ then() { print("thenable called..."); throw "oops"; }});

p = new Promise(function(resolve, reject) {
  res = resolve;
});

p = p.then(function(v) {
    return {
        then(resolve, reject) { resolve({ then() { print(v); }}); }
    };
});

res({ then(resolve) { resolve(2); } });
```

This should print

```
thenable called...
2
```

to the terminal, but if PromiseDeferred-created promises thenables are not using
the "PromiseResolveForThenable" wrapper as suggested, the bug doesn't get fixed.

And, changing PromiseDeferred breaks chain and friends.

To accomodate them, it's probably necessary to have another hidden property or
something, to mark whether the promise is a chaining promise or not.

[Dan Ehrenberg, 2015-10-13 08:07:45]

On 2015/10/12 at 19:19:10, caitpotter88 wrote:
> On 2015/10/12 14:22:03, caitp wrote:
> > On 2015/10/12 14:18:59, rossberg wrote:
> > > It looks like it would be possible to not interfere with chain and friends
by
> > > making this a separate wrapper around PromiseResolve, say,
> > > PromiseResolveForThenables, and invoke that in the promise constructor. Or
am
> > I
> > > missing some other implications?
> > 
> > Yes, it should be possible to do that
> 
> I'm mistaken.
> 
> Consider the following snippet:
> 
> ```
> var res;
> 
> var p = new Promise(function(resolve, reject) { res = resolve; });
> res({ then() { print("thenable called..."); throw "oops"; }});
> 
> p = new Promise(function(resolve, reject) {
>   res = resolve;
> });
> 
> p = p.then(function(v) {
>     return {
>         then(resolve, reject) { resolve({ then() { print(v); }}); }
>     };
> });
> 
> res({ then(resolve) { resolve(2); } });
> ```
> 
> This should print
> 
> ```
> thenable called...
> 2
> ```
> 
> to the terminal, but if PromiseDeferred-created promises thenables are not
using the "PromiseResolveForThenable" wrapper as suggested, the bug doesn't get
fixed.
> 
> And, changing PromiseDeferred breaks chain and friends.
> 
> To accomodate them, it's probably necessary to have another hidden property or
something, to mark whether the promise is a chaining promise or not.

I don't understand. Presumably you should be able call both .then() and .chain()
on the same Promise and when it resolves it will continue both of them, right?

Maybe we need a spec for how chain and other V8-specific methods work, in the
style of the ES2015 spec, if we want to keep them around while bringing the
other things up to spec compliance.

[caitp (gmail), 2015-10-13 15:17:14]

On 2015/10/13 08:07:45, Dan Ehrenberg wrote:
> On 2015/10/12 at 19:19:10, caitpotter88 wrote:
> > On 2015/10/12 14:22:03, caitp wrote:
> > > On 2015/10/12 14:18:59, rossberg wrote:
> > > > It looks like it would be possible to not interfere with chain and
friends
> by
> > > > making this a separate wrapper around PromiseResolve, say,
> > > > PromiseResolveForThenables, and invoke that in the promise constructor.
Or
> am
> > > I
> > > > missing some other implications?
> > > 
> > > Yes, it should be possible to do that
> > 
> > I'm mistaken.
> > 
> > Consider the following snippet:
> > 
> > ```
> > var res;
> > 
> > var p = new Promise(function(resolve, reject) { res = resolve; });
> > res({ then() { print("thenable called..."); throw "oops"; }});
> > 
> > p = new Promise(function(resolve, reject) {
> >   res = resolve;
> > });
> > 
> > p = p.then(function(v) {
> >     return {
> >         then(resolve, reject) { resolve({ then() { print(v); }}); }
> >     };
> > });
> > 
> > res({ then(resolve) { resolve(2); } });
> > ```
> > 
> > This should print
> > 
> > ```
> > thenable called...
> > 2
> > ```
> > 
> > to the terminal, but if PromiseDeferred-created promises thenables are not
> using the "PromiseResolveForThenable" wrapper as suggested, the bug doesn't
get
> fixed.
> > 
> > And, changing PromiseDeferred breaks chain and friends.
> > 
> > To accomodate them, it's probably necessary to have another hidden property
or
> something, to mark whether the promise is a chaining promise or not.
> 
> I don't understand. Presumably you should be able call both .then() and
.chain()
> on the same Promise and when it resolves it will continue both of them, right?
> 

The issue is, chain() currently depends on promise coercion and promise
resolution doing non-standard things. When they are changed to do the right
things, chain() stops working as expected by the tests. Only changing a subset
of the functions which perform resolution doesn't work correctly either, so
putting this under a new wrapper function doesn't quite do what is expected.

I think this could be made to work with some careful re-engineering, but it's
probably not worth it, from my perspective.

> Maybe we need a spec for how chain and other V8-specific methods work, in the
> style of the ES2015 spec, if we want to keep them around while bringing the
> other things up to spec compliance.

If chain really needs to stay around, then this definitely needs to happen.

[caitp (gmail), 2015-10-21 18:45:27]

As suggested in the meeting, I've disabled the offending tests, and added some
new tests verifying the fix.

[caitp (gmail), 2015-10-21 18:50:20]

On 2015/10/21 18:45:27, caitp wrote:
> As suggested in the meeting, I've disabled the offending tests, and added some
> new tests verifying the fix.

there are still some other tests that need correcting, won't be long

[caitp (gmail), 2015-11-09 16:46:30]

Description was changed from

==========
[es6] PromiseResolveThenableJob when promise resolved with thenable

When a Promise is resolved with a thenable, the spec chains the promises
together by invoking the thenable's `then` function with the original
Promise's resolve and reject methods (per section 25.4.2.2, or
http://www.ecma-international.org/ecma-262/6.0/#sec-promiseresolvethenablejob)

BUG=v8:4162
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org
==========

to

==========
[es6] PromiseResolveThenableJob when promise resolved with thenable

When a Promise is resolved with a thenable, the spec chains the promises
together by invoking the thenable's `then` function with the original
Promise's resolve and reject methods (per section 25.4.2.2, or
http://www.ecma-international.org/ecma-262/6.0/#sec-promiseresolvethenablejob)

Adds a spec compliance fix to ensure that the Promise constructor is only loaded
once when `then()` is called, solving v8:4539 as well.

BUG=v8:4162, v8:4539
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org
==========

[caitp (gmail), 2015-11-09 16:47:45]

Added a fix for v8:4539, just making sure all test failures are handled and then
hopefully this can be checked in

[caitp (gmail), 2015-11-09 17:12:18]

On 2015/11/09 16:47:45, caitp wrote:
> Added a fix for v8:4539, just making sure all test failures are handled and
then
> hopefully this can be checked in

Few tests are still broken, and they aren't invalid :( fixin em up.

[caitp (gmail), 2015-11-09 21:04:35]

On 2015/11/09 17:12:18, caitp wrote:
> On 2015/11/09 16:47:45, caitp wrote:
> > Added a fix for v8:4539, just making sure all test failures are handled and
> then
> > hopefully this can be checked in
> 
> Few tests are still broken, and they aren't invalid :( fixin em up.

Added a fix for those tests, also matches the spec a bit more clearly.

[caitp (gmail), 2015-11-09 21:10:24]

https://codereview.chromium.org/1394463003/diff/120001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1394463003/diff/120001/src/js/promise.js#newc...
src/js/promise.js:38: function CreateResolvingFunctions(promise) {
http://tc39.github.io/ecma262/#sec-createresolvingfunctions --- makes sure that
this is all done the same way every time. context-allocated variables could
technically be used all the time, but I think they aren't needed here?

However, I'm not 100% sure that it avoids context-allocating the function
reference, so it might be a wasted effort for now.

https://codereview.chromium.org/1394463003/diff/120001/src/js/promise.js#newc...
src/js/promise.js:41: var resolve = function(value) {
I think the spec mandates that these functions don't have a `name` property,
which will break once names are inferred properly per the spec :(

https://codereview.chromium.org/1394463003/diff/120001/src/js/promise.js#newc...
src/js/promise.js:235: function NewPromiseCapability(C) {
Sort-of (but not quite) matches the spec concept "NewPromiseCapability()", and
allows getting rid of %_Call() to invoke it.

https://codereview.chromium.org/1394463003/diff/120001/src/js/promise.js#newc...
src/js/promise.js:321: return %_Call(PromiseChainInternal, this,
this.constructor,
I think the goal is to get rid of this, but it's been re-arranged to minimize
the number of `Get(this, "constructor");` loads that occur

[caitp (gmail), 2015-11-09 21:35:26]

On 2015/11/09 21:10:24, caitp wrote:
> https://codereview.chromium.org/1394463003/diff/120001/src/js/promise.js
> File src/js/promise.js (right):
> 
>
https://codereview.chromium.org/1394463003/diff/120001/src/js/promise.js#newc...
> src/js/promise.js:38: function CreateResolvingFunctions(promise) {
> http://tc39.github.io/ecma262/#sec-createresolvingfunctions --- makes sure
that
> this is all done the same way every time. context-allocated variables could
> technically be used all the time, but I think they aren't needed here?
> 
> However, I'm not 100% sure that it avoids context-allocating the function
> reference, so it might be a wasted effort for now.
> 
>
https://codereview.chromium.org/1394463003/diff/120001/src/js/promise.js#newc...
> src/js/promise.js:41: var resolve = function(value) {
> I think the spec mandates that these functions don't have a `name` property,
> which will break once names are inferred properly per the spec :(
> 
>
https://codereview.chromium.org/1394463003/diff/120001/src/js/promise.js#newc...
> src/js/promise.js:235: function NewPromiseCapability(C) {
> Sort-of (but not quite) matches the spec concept "NewPromiseCapability()", and
> allows getting rid of %_Call() to invoke it.
> 
>
https://codereview.chromium.org/1394463003/diff/120001/src/js/promise.js#newc...
> src/js/promise.js:321: return %_Call(PromiseChainInternal, this,
> this.constructor,
> I think the goal is to get rid of this, but it's been re-arranged to minimize
> the number of `Get(this, "constructor");` loads that occur

Anyways, tests passing now, PTAL

[Dan Ehrenberg, 2015-11-10 00:42:37]

Does this fix the issue with chain?

https://codereview.chromium.org/1394463003/diff/160001/test/mjsunit/es6/promi...
File test/mjsunit/es6/promises.js (right):

https://codereview.chromium.org/1394463003/diff/160001/test/mjsunit/es6/promi...
test/mjsunit/es6/promises.js:236: /* DISABLED
Were you hoping to check the code in like this?

[caitp (gmail), 2015-11-10 00:49:35]

On 2015/11/10 00:42:37, Dan Ehrenberg wrote:
> Does this fix the issue with chain?
> 

I think PromiseChain needs to be removed, because it just doesn't work with the
way ES6 Promise is specd. I don't really have a plan for "fixing" it without
causing weird behaviours in the spec'd API when mixed with chain().

If people are okay with getting rid of it, I can do that in this CL, I'm just
worried it could be used by node modules or Chrome extensions or something (in
which case breaking it here isn't really any better).

>
https://codereview.chromium.org/1394463003/diff/160001/test/mjsunit/es6/promi...
> File test/mjsunit/es6/promises.js (right):
> 
>
https://codereview.chromium.org/1394463003/diff/160001/test/mjsunit/es6/promi...
> test/mjsunit/es6/promises.js:236: /* DISABLED
> Were you hoping to check the code in like this?

[Dan Ehrenberg, 2015-11-10 00:58:21]

If PromiseChain is removed, I think it should be in a different CL. Maybe it
should be driven by a flag, since there are possible web compatibility concerns.
Andreas, do you have any idea how to keep PromiseChain working in the presence
of ES6-compatible promises?

[rossberg, 2015-11-10 08:20:43]

On 2015/11/10 00:58:21, Dan Ehrenberg wrote:
> If PromiseChain is removed, I think it should be in a different CL. Maybe it
> should be driven by a flag, since there are possible web compatibility
concerns.
> Andreas, do you have any idea how to keep PromiseChain working in the presence
> of ES6-compatible promises?

[rossberg, 2015-11-10 08:30:10]

On 2015/11/10 08:20:43, rossberg wrote:
> On 2015/11/10 00:58:21, Dan Ehrenberg wrote:
> > If PromiseChain is removed, I think it should be in a different CL. Maybe it
> > should be driven by a flag, since there are possible web compatibility
> concerns.
> > Andreas, do you have any idea how to keep PromiseChain working in the
presence
> > of ES6-compatible promises?

[Oops, wrong button before.]

No, as discussed in one of the recent meetings, I don't have any idea how to
salvage it. And given other developments, typing JS properly is a lost cause
anyway, so I'm ready to give up on this.

Commenting out the tests for now is fine, I think, and what I had suggested
earlier. However, a suitable TODO comment would be preferable to DISABLED.

[caitp (gmail), 2015-11-10 12:44:12]

Description was changed from

==========
[es6] PromiseResolveThenableJob when promise resolved with thenable

When a Promise is resolved with a thenable, the spec chains the promises
together by invoking the thenable's `then` function with the original
Promise's resolve and reject methods (per section 25.4.2.2, or
http://www.ecma-international.org/ecma-262/6.0/#sec-promiseresolvethenablejob)

Adds a spec compliance fix to ensure that the Promise constructor is only loaded
once when `then()` is called, solving v8:4539 as well.

BUG=v8:4162, v8:4539
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org
==========

to

==========
[es6] refactor Promise resolution

Several changes are included here:

1. Each resolution callback references shared data indicating whether it has
already been resolved or not. Previously this was handled exclusively by the
Promise's status, which does not work correctly with the current chaining
behaviour.

2. During fulfillment, When a Promise is resolved with a thenable, the spec
chains the promises together by invoking the thenable's `then` function with the
original
Promise's resolve and reject methods (per section 25.4.2.2, or
http://www.ecma-international.org/ecma-262/6.0/#sec-promiseresolvethenablejob),
on
the next tick, regardless of whether or not there are pending tasks.

3. Adds a spec compliance fix to ensure that the Promise constructor is only
loaded once when `then()` is called, solving v8:4539 as well. This involves
refactoring PromiseChain to accept a constructor argument.
PromiseChain/PromiseDeferred will hopefully be removed soon, simplifying the
process.

BUG=v8:4162, v8:4539, v8:3237
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org
==========

[caitp (gmail), 2015-11-10 12:44:44]

On 2015/11/10 08:30:10, rossberg wrote:
> On 2015/11/10 08:20:43, rossberg wrote:
> > On 2015/11/10 00:58:21, Dan Ehrenberg wrote:
> > > If PromiseChain is removed, I think it should be in a different CL. Maybe
it
> > > should be driven by a flag, since there are possible web compatibility
> > concerns.
> > > Andreas, do you have any idea how to keep PromiseChain working in the
> presence
> > > of ES6-compatible promises?
> 
> [Oops, wrong button before.]
> 
> No, as discussed in one of the recent meetings, I don't have any idea how to
> salvage it. And given other developments, typing JS properly is a lost cause
> anyway, so I'm ready to give up on this.
> 
> Commenting out the tests for now is fine, I think, and what I had suggested
> earlier. However, a suitable TODO comment would be preferable to DISABLED.

I've added TODOs in place of the DISABLED comment, and updated the description
to include the various fixes in this patch

[caitp (gmail), 2015-11-10 12:47:29]

Description was changed from

==========
[es6] refactor Promise resolution

Several changes are included here:

1. Each resolution callback references shared data indicating whether it has
already been resolved or not. Previously this was handled exclusively by the
Promise's status, which does not work correctly with the current chaining
behaviour.

2. During fulfillment, When a Promise is resolved with a thenable, the spec
chains the promises together by invoking the thenable's `then` function with the
original
Promise's resolve and reject methods (per section 25.4.2.2, or
http://www.ecma-international.org/ecma-262/6.0/#sec-promiseresolvethenablejob),
on
the next tick, regardless of whether or not there are pending tasks.

3. Adds a spec compliance fix to ensure that the Promise constructor is only
loaded once when `then()` is called, solving v8:4539 as well. This involves
refactoring PromiseChain to accept a constructor argument.
PromiseChain/PromiseDeferred will hopefully be removed soon, simplifying the
process.

BUG=v8:4162, v8:4539, v8:3237
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org
==========

to

==========
[es6] refactor Promise resolution

Several changes are included here:

1. Each resolution callback references shared data indicating whether
it has already been resolved or not. Previously this was handled
exclusively by the Promise's status, which does not work correctly
with the current chaining behaviour.

2. During fulfillment, When a Promise is resolved with a thenable, the
spec chains the promises together by invoking the thenable's `then`
function with the original Promise's resolve and reject methods (per
section 25.4.2.2, or http://www.ecma-international.org/ecma-262/6.0
/#sec-promiseresolvethenablejob), on the next tick, regardless of
whether or not there are pending tasks.

3. Adds a spec compliance fix to ensure that the Promise constructor
is only loaded once when `then()` is called, solving v8:4539 as well.
This involves refactoring PromiseChain to accept a constructor
argument. PromiseChain/PromiseDeferred will hopefully be removed soon,
simplifying the process.

BUG=v8:4162, v8:4539, v8:3237
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org
==========

[caitp (gmail), 2015-11-10 12:50:48]

Description was changed from

==========
[es6] refactor Promise resolution

Several changes are included here:

1. Each resolution callback references shared data indicating whether
it has already been resolved or not. Previously this was handled
exclusively by the Promise's status, which does not work correctly
with the current chaining behaviour.

2. During fulfillment, When a Promise is resolved with a thenable, the
spec chains the promises together by invoking the thenable's `then`
function with the original Promise's resolve and reject methods (per
section 25.4.2.2, or http://www.ecma-international.org/ecma-262/6.0
/#sec-promiseresolvethenablejob), on the next tick, regardless of
whether or not there are pending tasks.

3. Adds a spec compliance fix to ensure that the Promise constructor
is only loaded once when `then()` is called, solving v8:4539 as well.
This involves refactoring PromiseChain to accept a constructor
argument. PromiseChain/PromiseDeferred will hopefully be removed soon,
simplifying the process.

BUG=v8:4162, v8:4539, v8:3237
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org
==========

to

==========
[es6] refactor Promise resolution

Several changes are included here:

1. Each resolution callback references shared data indicating whether
it has already been resolved or not. Previously this was handled
exclusively by the Promise's status, which does not work correctly
with the current chaining behaviour.

2. During fulfillment, When a Promise is resolved with a thenable, the
spec chains the promises together by invoking the thenable's `then`
function with the original Promise's resolve and reject methods (per
section 25.4.2.2, or
http://tc39.github.io/ecma262/#sec-promiseresolvethenablejob, on the
next tick, regardless of whether or not there are pending tasks.

3. Adds a spec compliance fix to ensure that the Promise constructor
is only loaded once when `then()` is called, solving v8:4539 as well.
This involves refactoring PromiseChain to accept a constructor
argument. PromiseChain/PromiseDeferred will hopefully be removed soon,
simplifying the process.

BUG=v8:4162, v8:4539, v8:3237
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org
==========

[caitp (gmail), 2015-11-10 12:52:44]

Description was changed from

==========
[es6] refactor Promise resolution

Several changes are included here:

1. Each resolution callback references shared data indicating whether
it has already been resolved or not. Previously this was handled
exclusively by the Promise's status, which does not work correctly
with the current chaining behaviour.

2. During fulfillment, When a Promise is resolved with a thenable, the
spec chains the promises together by invoking the thenable's `then`
function with the original Promise's resolve and reject methods (per
section 25.4.2.2, or
http://tc39.github.io/ecma262/#sec-promiseresolvethenablejob, on the
next tick, regardless of whether or not there are pending tasks.

3. Adds a spec compliance fix to ensure that the Promise constructor
is only loaded once when `then()` is called, solving v8:4539 as well.
This involves refactoring PromiseChain to accept a constructor
argument. PromiseChain/PromiseDeferred will hopefully be removed soon,
simplifying the process.

BUG=v8:4162, v8:4539, v8:3237
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org
==========

to

==========
[es6] refactor Promise resolution

Several changes are included here:

1. Each resolution callback references shared data indicating whether
it has already been resolved or not, as described in 25.4.1.3
http://tc39.github.io/ecma262/#sec-createresolvingfunctions.
Previously this was handled exclusively by the Promise's status,
which does not work correctly with the current chaining behaviour.

2. During fulfillment, When a Promise is resolved with a thenable, the
spec chains the promises together by invoking the thenable's `then`
function with the original Promise's resolve and reject methods (per
section 25.4.2.2, or
http://tc39.github.io/ecma262/#sec-promiseresolvethenablejob, on the
next tick, regardless of whether or not there are pending tasks.

3. Adds a spec compliance fix to ensure that the Promise constructor
is only loaded once when `then()` is called, solving v8:4539 as well.
This involves refactoring PromiseChain to accept a constructor
argument. PromiseChain/PromiseDeferred will hopefully be removed soon,
simplifying the process.

BUG=v8:4162, v8:4539, v8:3237
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org
==========

[Dan Ehrenberg, 2015-11-12 22:08:20]

lgtm

LGTM modulo remaining lack of TODO

https://codereview.chromium.org/1394463003/diff/220001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1394463003/diff/220001/test/cctest/test-api.c...
test/cctest/test-api.cc:20311: // Chain --- DISABLED
TODO?

[caitp (gmail), 2015-11-13 22:00:31]

fixed the nit --- has the v8 4.8 branch already been settled? it's sort of hard
to tell from https://chromium.googlesource.com/v8/v8, I feel like CQing this
should wait for that

https://codereview.chromium.org/1394463003/diff/220001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1394463003/diff/220001/test/cctest/test-api.c...
test/cctest/test-api.cc:20311: // Chain --- DISABLED
On 2015/11/12 22:08:19, Dan Ehrenberg wrote:
> TODO?

Acknowledged.

[rossberg, 2015-11-17 13:33:53]

lgtm

[caitp (gmail), 2015-11-17 13:59:10]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-11-17 13:59:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/1394463003/#ps240001
(title: "TODO nit")

[commit-bot: I haz the power, 2015-11-17 13:59:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394463003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394463003/240001

[commit-bot: I haz the power, 2015-11-17 14:23:50]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-11-17 14:24:27]

Patchset 13 (id:??) landed as
https://crrev.com/24ff30b740236619e911106573ee5cde0cd16dc2
Cr-Commit-Position: refs/heads/master@{#32046}

[Dan Ehrenberg, 2015-11-25 23:54:23]

On 2015/11/17 at 14:24:27, commit-bot wrote:
> Patchset 13 (id:??) landed as
https://crrev.com/24ff30b740236619e911106573ee5cde0cd16dc2
> Cr-Commit-Position: refs/heads/master@{#32046}

With this patch, do we need the coercion logic in PromiseThen? It looks like
this makes PromiseChain already do what PromiseThen should be doing.