[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Also ensures that changes to the list accumulator in Promise.all() is not observable
via accessors installed in the Array prototype chain, using the same mechanism used
in several Array methods.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.js
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js
- built-ins/Promise/all/does-not-invoke-array-setters.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org

Committed: https://crrev.com/7459d8cecb2dbfc219c7a5eb528f907a17f9e34c
Cr-Commit-Position: refs/heads/master@{#33163}

[caitp (gmail), 2015-12-18 17:05:38]

another promise compliance fix

[caitp (gmail), 2015-12-18 17:10:01]

Patchset #1 (id:1) has been deleted

[Dan Ehrenberg, 2016-01-04 02:47:28]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-04 02:47:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534813005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534813005/20001

[commit-bot: I haz the power, 2016-01-04 02:48:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-04 02:48:28]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/11955)
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/12551)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/9372)

[caitp (gmail), 2016-01-04 02:49:06]

On 2016/01/04 02:48:28, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
>   v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
>   v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
>   v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/11955)
>   v8_linux_dbg on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/12551)
>   v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
>   v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
>   v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
>   v8_presubmit on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/9372)

these need some test expectations fixes, I'll take a look at fixing it up on
Monday, unless you get to it first

[Dan Ehrenberg, 2016-01-04 02:49:15]

One comment; haven't yet verified that this matches the spec but looks good at a
first glance.

https://codereview.chromium.org/1534813005/diff/20001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1534813005/diff/20001/src/js/promise.js#newco...
src/js/promise.js:78: callbacks.reject(e);
I think this needs to be %_Call(callbacks.reject, UNDEFINED, e)

https://codereview.chromium.org/1534813005/diff/20001/src/js/promise.js#newco...
src/js/promise.js:211: callbacks.reject(promise, e);
Here too

[caitp (gmail), 2016-01-04 13:49:03]

Description was changed from

==========
[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
==========

to

==========
[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.js
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js
- built-ins/Promise/all/does-not-invoke-array-setters.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
==========

[caitp (gmail), 2016-01-04 14:00:56]

Description was changed from

==========
[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.js
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js
- built-ins/Promise/all/does-not-invoke-array-setters.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
==========

to

==========
[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Also ensures that changes to the list accumulator in Promise.all() is not
observable via accessors installed in the Array prototype chain, using the same
mechanism used in several Array methods.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.js
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js
- built-ins/Promise/all/does-not-invoke-array-setters.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
==========

[caitp (gmail), 2016-01-04 14:05:09]

On 2016/01/04 02:49:15, Dan Ehrenberg wrote:
> One comment; haven't yet verified that this matches the spec but looks good at
a
> first glance.
> 
> https://codereview.chromium.org/1534813005/diff/20001/src/js/promise.js
> File src/js/promise.js (right):
> 
>
https://codereview.chromium.org/1534813005/diff/20001/src/js/promise.js#newco...
> src/js/promise.js:78: callbacks.reject(e);
> I think this needs to be %_Call(callbacks.reject, UNDEFINED, e)
> 
>
https://codereview.chromium.org/1534813005/diff/20001/src/js/promise.js#newco...
> src/js/promise.js:211: callbacks.reject(promise, e);
> Here too

Addressed these, and added an additional fix for
built-ins/Promise/all/does-not-invoke-array-setters.js

[caitp (gmail), 2016-01-04 14:05:44]

Description was changed from

==========
[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Also ensures that changes to the list accumulator in Promise.all() is not
observable via accessors installed in the Array prototype chain, using the same
mechanism used in several Array methods.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.js
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js
- built-ins/Promise/all/does-not-invoke-array-setters.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
==========

to

==========
[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Also ensures that changes to the list accumulator in Promise.all() is not
observable
via accessors installed in the Array prototype chain, using the same mechanism
used
in several Array methods.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.js
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js
- built-ins/Promise/all/does-not-invoke-array-setters.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
==========

[Dan Ehrenberg, 2016-01-04 18:18:39]

https://codereview.chromium.org/1534813005/diff/40001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1534813005/diff/40001/src/js/promise.js#newco...
src/js/promise.js:362: function CreateResolveElementFunction(index, values,
promiseCapability) {
Does this need to be a nested function? Our js code tends to define top-level
functions, and this doesn't seem to close over anything.

[caitp (gmail), 2016-01-04 18:20:08]

https://codereview.chromium.org/1534813005/diff/40001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1534813005/diff/40001/src/js/promise.js#newco...
src/js/promise.js:362: function CreateResolveElementFunction(index, values,
promiseCapability) {
On 2016/01/04 18:18:39, Dan Ehrenberg wrote:
> Does this need to be a nested function? Our js code tends to define top-level
> functions, and this doesn't seem to close over anything.

It doesn't, but the function isn't really needed by anything else. No problem
moving it out, though

[caitp (gmail), 2016-01-04 19:26:52]

https://codereview.chromium.org/1534813005/diff/40001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1534813005/diff/40001/src/js/promise.js#newco...
src/js/promise.js:362: function CreateResolveElementFunction(index, values,
promiseCapability) {
On 2016/01/04 18:20:08, caitp wrote:
> On 2016/01/04 18:18:39, Dan Ehrenberg wrote:
> > Does this need to be a nested function? Our js code tends to define
top-level
> > functions, and this doesn't seem to close over anything.
> 
> It doesn't, but the function isn't really needed by anything else. No problem
> moving it out, though

Actually, I'm wrong --- `count` is captured, so it would need to be refactored
more in order to keep working

[caitp (gmail), 2016-01-04 20:51:49]

On 2016/01/04 19:26:52, caitp wrote:
> https://codereview.chromium.org/1534813005/diff/40001/src/js/promise.js
> File src/js/promise.js (right):
> 
>
https://codereview.chromium.org/1534813005/diff/40001/src/js/promise.js#newco...
> src/js/promise.js:362: function CreateResolveElementFunction(index, values,
> promiseCapability) {
> On 2016/01/04 18:20:08, caitp wrote:
> > On 2016/01/04 18:18:39, Dan Ehrenberg wrote:
> > > Does this need to be a nested function? Our js code tends to define
> top-level
> > > functions, and this doesn't seem to close over anything.
> > 
> > It doesn't, but the function isn't really needed by anything else. No
problem
> > moving it out, though
> 
> Actually, I'm wrong --- `count` is captured, so it would need to be refactored
> more in order to keep working

So, it can be moved out, but I don't think anything is gained in doing so. Is it
alright to leave it as-is?

[Dan Ehrenberg, 2016-01-04 21:01:03]

lgtm

Yes, once you need to box something, it seems fine like this. One thing that I'd
prefer is if you could move the definition of the function before the usage.
Your code is fine in terms of ES2015 function hoisting semantics, but given how
finicky it is, I'd kinda prefer definitions within a function to come before
usages. But that's just an optional stylistic suggestion.

[caitp (gmail), 2016-01-05 14:23:36]

Patchset #4 (id:80001) has been deleted

[caitp (gmail), 2016-01-05 14:24:49]

On 2016/01/04 21:01:03, Dan Ehrenberg wrote:
> lgtm
> 
> Yes, once you need to box something, it seems fine like this. One thing that
I'd
> prefer is if you could move the definition of the function before the usage.
> Your code is fine in terms of ES2015 function hoisting semantics, but given
how
> finicky it is, I'd kinda prefer definitions within a function to come before
> usages. But that's just an optional stylistic suggestion.

it caused a compilation error, as the scoping rules for native scopes seem to
require variables to be declared before they're used rather than relying on
hoisting. To work around that, it was necessary to move the declaration of
`count`, which made it harder to understand. I'm sticking with the original
style choice

[caitp (gmail), 2016-01-05 14:24:56]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-01-05 14:24:59]

The CQ bit was unchecked by caitpotter88@gmail.com

[Dan Ehrenberg, 2016-01-05 16:43:00]

On 2016/01/05 at 14:24:49, caitpotter88 wrote:
> On 2016/01/04 21:01:03, Dan Ehrenberg wrote:
> > lgtm
> > 
> > Yes, once you need to box something, it seems fine like this. One thing that
I'd
> > prefer is if you could move the definition of the function before the usage.
> > Your code is fine in terms of ES2015 function hoisting semantics, but given
how
> > finicky it is, I'd kinda prefer definitions within a function to come before
> > usages. But that's just an optional stylistic suggestion.
> 
> it caused a compilation error, as the scoping rules for native scopes seem to
require variables to be declared before they're used rather than relying on
hoisting. To work around that, it was necessary to move the declaration of
`count`, which made it harder to understand. I'm sticking with the original
style choice

What if you declare count, then declare the function, then initialize count and
have the rest of the function? That doesn't sound like a hard-to-understand
resulting function to me.

Either way, this seems to work, so we can leave it as is.

[caitp (gmail), 2016-01-06 16:17:10]

Patchset #4 (id:100001) has been deleted

[Dan Ehrenberg, 2016-01-06 17:20:56]

lgtm

[caitp (gmail), 2016-01-07 16:00:58]

caitpotter88@gmail.com changed reviewers:
+ adamk@chromium.org

[caitp (gmail), 2016-01-07 16:01:22]

On 2016/01/06 17:20:56, Dan Ehrenberg wrote:
> lgtm

+adamk for src/js/OWNERS

[caitp (gmail), 2016-01-07 16:11:12]

Patchset #4 (id:120001) has been deleted

[adamk, 2016-01-07 19:12:10]

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js#newc...
src/js/promise.js:351: resolutions[i] = UNDEFINED;
Why is this needed?

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js#newc...
src/js/promise.js:373: function CreateResolveElementFunction(index, values,
promiseCapability) {
I think I agree with Dan that this would read better if it were declared above
its use (it's especially odd-looking to me to see code after a return).

[caitp (gmail), 2016-01-07 19:20:36]

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js#newc...
src/js/promise.js:351: resolutions[i] = UNDEFINED;
On 2016/01/07 19:12:10, adamk wrote:
> Why is this needed?

I _think_ the reason is to make sure `hasOwnProperty(someIndex)` returns true,
even if the value is undefined.

It's in the spec as "Append undefined to values", and IIRC is asserted by
test262 (can't recall which test though)

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js#newc...
src/js/promise.js:373: function CreateResolveElementFunction(index, values,
promiseCapability) {
On 2016/01/07 19:12:10, adamk wrote:
> I think I agree with Dan that this would read better if it were declared above
> its use (it's especially odd-looking to me to see code after a return).

ok

[adamk, 2016-01-07 19:29:30]

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js#newc...
src/js/promise.js:351: resolutions[i] = UNDEFINED;
On 2016/01/07 19:20:35, caitp wrote:
> On 2016/01/07 19:12:10, adamk wrote:
> > Why is this needed?
> 
> I _think_ the reason is to make sure `hasOwnProperty(someIndex)` returns true,
> even if the value is undefined.
> 
> It's in the spec as "Append undefined to values", and IIRC is asserted by
> test262 (can't recall which test though)

Ah, ok. Can you just use .push()?

[caitp (gmail), 2016-01-07 19:37:59]

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js#newc...
src/js/promise.js:351: resolutions[i] = UNDEFINED;
On 2016/01/07 19:29:30, adamk wrote:
> On 2016/01/07 19:20:35, caitp wrote:
> > On 2016/01/07 19:12:10, adamk wrote:
> > > Why is this needed?
> > 
> > I _think_ the reason is to make sure `hasOwnProperty(someIndex)` returns
true,
> > even if the value is undefined.
> > 
> > It's in the spec as "Append undefined to values", and IIRC is asserted by
> > test262 (can't recall which test though)
> 
> Ah, ok. Can you just use .push()?

it seems to pass everything without it, presumably because of how
%MoveArrayContents() works. I've just removed the line

https://codereview.chromium.org/1534813005/diff/160001/src/js/promise.js#newc...
src/js/promise.js:373: function CreateResolveElementFunction(index, values,
promiseCapability) {
On 2016/01/07 19:20:36, caitp wrote:
> On 2016/01/07 19:12:10, adamk wrote:
> > I think I agree with Dan that this would read better if it were declared
above
> > its use (it's especially odd-looking to me to see code after a return).
> 
> ok

Done.

[adamk, 2016-01-07 19:47:23]

lgtm

[caitp (gmail), 2016-01-07 19:53:45]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-01-07 19:53:45]

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/1534813005/#ps180001
(title: "Remove "append NULL" since it doesn't seem to be needed, rearrange
code")

[commit-bot: I haz the power, 2016-01-07 19:54:00]

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

[commit-bot: I haz the power, 2016-01-07 20:23:51]

Description was changed from

==========
[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Also ensures that changes to the list accumulator in Promise.all() is not
observable
via accessors installed in the Array prototype chain, using the same mechanism
used
in several Array methods.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.js
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js
- built-ins/Promise/all/does-not-invoke-array-setters.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
==========

to

==========
[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Also ensures that changes to the list accumulator in Promise.all() is not
observable
via accessors installed in the Array prototype chain, using the same mechanism
used
in several Array methods.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.js
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js
- built-ins/Promise/all/does-not-invoke-array-setters.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
==========

[commit-bot: I haz the power, 2016-01-07 20:23:51]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-01-07 20:24:35]

Description was changed from

==========
[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Also ensures that changes to the list accumulator in Promise.all() is not
observable
via accessors installed in the Array prototype chain, using the same mechanism
used
in several Array methods.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.js
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js
- built-ins/Promise/all/does-not-invoke-array-setters.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
==========

to

==========
[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

Testing the promise status is not enough to ensure that resolve functions are
called only once.

This change adds a similar version of the [[AlreadyResolved]] slot to the
Promise.all resolve element function, and also ensures that [[AlreadyResolved]]
is respected in the Promise executor, and when resolving thenables. This means
replacing PromiseReject() shortcuts with promiseCapability.reject(), which has
an [[AlreadyResolved]] record in a context slot.

Also ensures that changes to the list accumulator in Promise.all() is not
observable
via accessors installed in the Array prototype chain, using the same mechanism
used
in several Array methods.

Fixes the following Test262 tests:
- built-ins/Promise/all/call-resolve-element-items.js
- built-ins/Promise/all/call-resolve-element.js
- built-ins/Promise/all/call-resolve-element-after-return.js
- built-ins/Promise/all/same-reject-function.js
- built-ins/Promise/all/resolve-from-same-thenable.js
- built-ins/Promise/all/resolve-before-loop-exit.js
- built-ins/Promise/all/resolve-before-loop-exit-from-same.js
- built-ins/Promise/exception-after-resolve-in-executor.js
- built-ins/Promise/exception-after-resolve-in-thenable-job.js
- built-ins/Promise/all/does-not-invoke-array-setters.js

BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org

Committed: https://crrev.com/7459d8cecb2dbfc219c7a5eb528f907a17f9e34c
Cr-Commit-Position: refs/heads/master@{#33163}
==========

[commit-bot: I haz the power, 2016-01-07 20:24:36]

Patchset 6 (id:??) landed as
https://crrev.com/7459d8cecb2dbfc219c7a5eb528f907a17f9e34c
Cr-Commit-Position: refs/heads/master@{#33163}