Reland "Clean up promises and fix an edge case bug (patchset #4 id:60001 of https://codereview.chro…

Reland "Clean up promises and fix an edge case bug (patchset #4 id:60001 of https://codereview.chromium.org/1488783002/ )"

This patch relands a change to ES2015 Promises which brings us closer to
spec compliance. In this new version, a bug which would lose async callstack
data was fixed.

R=adamk
CC=rossberg,caitp
LOG=Y
BUG=v8:3641
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel

Committed: https://crrev.com/797d1090aea9eb5bda85ca39f06f3f0009b0d476
Cr-Commit-Position: refs/heads/master@{#33065}

[Dan Ehrenberg, 2015-12-17 21:21:10]

Description was changed from

==========
Reland "Clean up promises and fix an edge case bug (patchset #4 id:60001 of
https://codereview.chromium.org/1488783002/ )"

This patch relands a change to ES2015 Promises which brings us closer to
spec compliance. In this new version, a bug which would lose async callstack
data was fixed.

R=adamk
CC=rossberg,caitp
LOG=Y
BUG=v8:3641
==========

to

==========
Reland "Clean up promises and fix an edge case bug (patchset #4 id:60001 of
https://codereview.chromium.org/1488783002/ )"

This patch relands a change to ES2015 Promises which brings us closer to
spec compliance. In this new version, a bug which would lose async callstack
data was fixed.

R=adamk
CC=rossberg,caitp
LOG=Y
BUG=v8:3641
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
==========

[Dan Ehrenberg, 2015-12-17 21:21:19]

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

[commit-bot: I haz the power, 2015-12-17 21:21:27]

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

[commit-bot: I haz the power, 2015-12-17 21:24:45]

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

[commit-bot: I haz the power, 2015-12-17 21:24:48]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/13409)

[Dan Ehrenberg, 2015-12-17 21:41:55]

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

[commit-bot: I haz the power, 2015-12-17 21:42:09]

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

[commit-bot: I haz the power, 2015-12-17 22:52:25]

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

[commit-bot: I haz the power, 2015-12-17 22:52:26]

Dry run: This issue passed the CQ dry run.

[adamk, 2015-12-18 06:34:37]

adamk@chromium.org changed reviewers:
+ rossberg@chromium.org

[adamk, 2015-12-18 06:34:38]

I could rubber-stamp this, but I think rossberg's the right reviewer, since he
reviewed the original patch. Might also want to get an eye from someone on
devtools for the async callstack stuff.

[rossberg, 2015-12-18 08:04:59]

On 2015/12/18 06:34:38, adamk (ooo until 2016) wrote:
> I could rubber-stamp this, but I think rossberg's the right reviewer, since he
> reviewed the original patch. Might also want to get an eye from someone on
> devtools for the async callstack stuff.

It would help to have the change relative to the original version in a separate
patch set. ;)

[Dan Ehrenberg, 2015-12-18 19:11:46]

littledan@chromium.org changed reviewers:
- rossberg@chromium.org

[Dan Ehrenberg, 2015-12-18 22:03:05]

On 2015/12/18 at 08:04:59, rossberg wrote:
> On 2015/12/18 06:34:38, adamk (ooo until 2016) wrote:
> > I could rubber-stamp this, but I think rossberg's the right reviewer, since
he
> > reviewed the original patch. Might also want to get an eye from someone on
> > devtools for the async callstack stuff.
> 
> It would help to have the change relative to the original version in a
separate patch set. ;)

The only thing that changed here is in src/js/promise.js, what's now lines
179-199, there was more code written to call out to the debugger.

[Dan Ehrenberg, 2015-12-22 16:25:08]

littledan@chromium.org changed reviewers:
+ cbruni@chromium.org, rossberg@chromium.org

[Dan Ehrenberg, 2015-12-28 06:11:30]

On 2015/12/18 at 22:03:05, Dan Ehrenberg wrote:
> On 2015/12/18 at 08:04:59, rossberg wrote:
> > On 2015/12/18 06:34:38, adamk (ooo until 2016) wrote:
> > > I could rubber-stamp this, but I think rossberg's the right reviewer,
since he
> > > reviewed the original patch. Might also want to get an eye from someone on
> > > devtools for the async callstack stuff.
> > 
> > It would help to have the change relative to the original version in a
separate patch set. ;)
> 
> The only thing that changed here is in src/js/promise.js, what's now lines
179-199, there was more code written to call out to the debugger.

If we can't get this in in time for the next release, I think we should roll
back https://codereview.chromium.org/1394463003 in the branch because it leaves
things in a funny intermediate state.

[Camillo Bruni, 2015-12-28 12:22:35]

Hi Dan,
I looked at the CL and I added some questions inline + comments mostly nits, due
to function names not corresponding to the spec / missing references to the
spec.

I really like having a comment on each function that corresponds to the a spec
function to help navigating (if you know the code this probably doesn't matter
;).
Additionally I like to have the code structured as close as possible to the spec
(unless speed...).

I think the CL looks okay... but I'd like to have a second look after your
feedback.

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

https://codereview.chromium.org/1538663002/diff/20001/src/js/promise.js#newco...
src/js/promise.js:34: // Status values: 0 = pending, +1 = resolved, -1 =
rejected
Has adding named constants for the status values a performance impact? If not
that would help code readability...

https://codereview.chromium.org/1538663002/diff/20001/src/js/promise.js#newco...
src/js/promise.js:66: throw MakeTypeError(kResolverNotAFunction, resolver);
please add braces here.

https://codereview.chromium.org/1538663002/diff/20001/src/js/promise.js#newco...
src/js/promise.js:111: if (tasks.length) PromiseEnqueue(value, tasks, status);
According to the spec that's step 8, probably irrelevant, but unless critical I
would move it to a position that matches the spec (makes it easier for future
comparison).

https://codereview.chromium.org/1538663002/diff/20001/src/js/promise.js#newco...
src/js/promise.js:112: PromiseSet(promise, status, value);
I'd make it explicit that you set the [[PromiseFulfillReactions]] and
[[PromiseRejectReactions]] to undefined here.

https://codereview.chromium.org/1538663002/diff/20001/src/js/promise.js#newco...
src/js/promise.js:170: if (IS_RECEIVER(x)) {
Early return would make this more readable, and easier to compare with the spec.

https://codereview.chromium.org/1538663002/diff/20001/src/js/promise.js#newco...
src/js/promise.js:177: if (IS_CALLABLE(then)) {
I'd say the same here ;) (yes, yes nit-picking :P)

https://codereview.chromium.org/1538663002/diff/20001/src/js/promise.js#newco...
src/js/promise.js:249: function PromiseRejected(r) {
Dangerous name if you ask me :) PromiseReject vs. PromiseRejected

https://codereview.chromium.org/1538663002/diff/20001/src/js/promise.js#newco...
src/js/promise.js:300: SET_PRIVATE(this, promiseHasHandlerSymbol, true);
Shouldn't this happen only in the rejected case? (spec step 25.4.5.3.1 9.e)

https://codereview.chromium.org/1538663002/diff/20001/src/js/promise.js#newco...
src/js/promise.js:318: function PromiseCast(x) {
I know this is probably not very important, but as a new reader PromiseCast is
not a nice name :) Probably we would have to rename PromiseResolve to
PromisePrototypeResovle... and so forth, to rename PromiseCast to
PromiseResolve.

As a minimal aid I would put the spec section. (Yes it's work to maintain these
references ;), but if the spec changes we have to recheck our implementation)

[Camillo Bruni, 2015-12-30 08:56:18]

LGTM
as discussed offline the suggested cleanups should probably happen at a later
point

[Dan Ehrenberg, 2015-12-30 18:40:38]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-12-30 18:40:39]

Thanks for the leniency. I do want to fix these issues, but I think it'll be
cleaner to do in a follow-on patch.

[commit-bot: I haz the power, 2015-12-30 18:40:49]

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

[commit-bot: I haz the power, 2015-12-30 18:53:55]

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

[commit-bot: I haz the power, 2015-12-30 18:53:56]

Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[Dan Ehrenberg, 2015-12-30 19:48:15]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-12-30 19:48:16]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/1538663002/#ps40001
(title: "Fixes two test262 tests!")

[commit-bot: I haz the power, 2015-12-30 19:48:30]

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

[commit-bot: I haz the power, 2015-12-30 20:47:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-30 20:47:41]

Description was changed from

==========
Reland "Clean up promises and fix an edge case bug (patchset #4 id:60001 of
https://codereview.chromium.org/1488783002/ )"

This patch relands a change to ES2015 Promises which brings us closer to
spec compliance. In this new version, a bug which would lose async callstack
data was fixed.

R=adamk
CC=rossberg,caitp
LOG=Y
BUG=v8:3641
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
==========

to

==========
Reland "Clean up promises and fix an edge case bug (patchset #4 id:60001 of
https://codereview.chromium.org/1488783002/ )"

This patch relands a change to ES2015 Promises which brings us closer to
spec compliance. In this new version, a bug which would lose async callstack
data was fixed.

R=adamk
CC=rossberg,caitp
LOG=Y
BUG=v8:3641
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel

Committed: https://crrev.com/797d1090aea9eb5bda85ca39f06f3f0009b0d476
Cr-Commit-Position: refs/heads/master@{#33065}
==========

[commit-bot: I haz the power, 2015-12-30 20:47:42]

Patchset 3 (id:??) landed as
https://crrev.com/797d1090aea9eb5bda85ca39f06f3f0009b0d476
Cr-Commit-Position: refs/heads/master@{#33065}