Improve stack traces for async functions

Improve stack traces for async functions

This patch tracks the stack of async functions differently from other
Promise async stack tracking. With this patch, the stack trace of a
callstack of async functions should look similarly to the call stack
if all of the functions were synchronous. An example can be found in
the updated test expectations: https://codereview.chromium.org/2362923002 .

The new stack traces are implemented using existing mechanisms in the
inspector. The inspector has two ways to save async stack traces: recurring
and non-recurring stacks. An example of a non-recurring stack is setTimeout,
and a recurring one is saved for setInterval. Recurring stacks are deleted
only when a special "cancel" function is called, rather than being deleted
after being used the first time. Previous Promise async stack tracking always
used non-recurring stacks.

For async functions, this patch saves a recurring stack. The top frame of
the stack is duplicated, as the resuming function contains a similar frame;
the devtools frontend is responsible for removing or marking this frame,
which it can do based on seeing the [async function] line which follows it.
The second frame will instead be provided by the resuming execution
context. The recurring stack is saved when the async function is entered, and
it is deleted from a finally block. The id of the stack is saved in the outer
Promise being constructed by the async function. When an intermediate
throwaway Promise will be triggered as a reaction, it will be identified as
such based on its debugging metadata, and the corresponding async function's
recurring stack will be used.

BUG=v8:4483
Committed: https://crrev.com/f296dad962e452ced1e5396b9ced7203747f7209
Cr-Commit-Position: refs/heads/master@{#39695}

[Dan Ehrenberg, 2016-09-22 21:57:13]

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

[commit-bot: I haz the power, 2016-09-22 21:57:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 21:59:10]

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

[commit-bot: I haz the power, 2016-09-22 21:59:11]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/13298)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/13298)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/13266)

[Dan Ehrenberg, 2016-09-22 22:19:17]

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

[commit-bot: I haz the power, 2016-09-22 22:19:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-09-22 22:25:03]

Description was changed from

==========
Attempted better stack traces for async functions
==========

to

==========
Improve stack traces for async functions

This patch tracks the stack of async functions differently from other
Promise async stack tracking. With this patch, the stack trace of a
callstack of async functions should look similarly to the call stack
if all of the functions were synchronous. An example can be found in
the updated test expectations: https://codereview.chromium.org/2362923002 .

The new stack traces are implemented using existing mechanisms in the
inspector. The inspector has two concepts for
==========

[Dan Ehrenberg, 2016-09-22 22:25:03]

littledan@chromium.org changed reviewers:
+ gsathya@chromium.org, kozyatinskiy@chromium.org

[Dan Ehrenberg, 2016-09-22 22:30:25]

Description was changed from

==========
Improve stack traces for async functions

This patch tracks the stack of async functions differently from other
Promise async stack tracking. With this patch, the stack trace of a
callstack of async functions should look similarly to the call stack
if all of the functions were synchronous. An example can be found in
the updated test expectations: https://codereview.chromium.org/2362923002 .

The new stack traces are implemented using existing mechanisms in the
inspector. The inspector has two concepts for
==========

to

==========
Improve stack traces for async functions

This patch tracks the stack of async functions differently from other
Promise async stack tracking. With this patch, the stack trace of a
callstack of async functions should look similarly to the call stack
if all of the functions were synchronous. An example can be found in
the updated test expectations: https://codereview.chromium.org/2362923002 .

The new stack traces are implemented using existing mechanisms in the
inspector. The inspector has two ways to save async stack traces: recurring
and non-recurring stacks. An example of a non-recurring stack is setTimeout,
and a recurring one is saved for setInterval. Recurring stacks are deleted
only when a special "cancel" function is called, rather than being deleted
after being used the first time. Previous Promise async stack tracking always
used non-recurring stacks.

For async functions, this patch saves a recurring stack, which has the top
frame cut off. The frame will instead be provided by the resuming execution
context. The recurring stack is saved when the async function is entered, and
it is deleted from a finally block. The id of the stack is saved in the outer
Promise being constructed by the async function. When an intermediate
throwaway Promise will be triggered as a reaction, it will be identified as
such based on its debugging metadata, and the corresponding async function's
recurring stack will be used.

BUG=v8:4483
==========

[commit-bot: I haz the power, 2016-09-22 23:06:13]

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

[commit-bot: I haz the power, 2016-09-22 23:06:14]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-09-23 01:27:36]

https://codereview.chromium.org/2357423002/diff/120001/src/inspector/v8-debug...
File src/inspector/v8-debugger.h (right):

https://codereview.chromium.org/2357423002/diff/120001/src/inspector/v8-debug...
src/inspector/v8-debugger.h:93: bool recurring, int skipInitialFrames = 0);
Please not expose this argument in public API. You can mark frames somehow and
then filter them on frontend side or don't report it from V8 api.

[Dan Ehrenberg, 2016-09-23 04:11:31]

https://codereview.chromium.org/2357423002/diff/120001/src/inspector/v8-debug...
File src/inspector/v8-debugger.h (right):

https://codereview.chromium.org/2357423002/diff/120001/src/inspector/v8-debug...
src/inspector/v8-debugger.h:93: bool recurring, int skipInitialFrames = 0);
On 2016/09/23 at 01:27:36, kozyatinskiy wrote:
> Please not expose this argument in public API. You can mark frames somehow and
then filter them on frontend side or don't report it from V8 api.

As we discussed off-line, there's nothing special about these stack frames to
mark. I changed the debugger API to not expose skipping initial frames (except
through V8's debug events), and restricted the code to be stricter and only skip
one frame.

[Dan Ehrenberg, 2016-09-23 04:11:41]

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

[commit-bot: I haz the power, 2016-09-23 04:11:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 04:16:00]

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

[commit-bot: I haz the power, 2016-09-23 04:16:01]

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

[Dan Ehrenberg, 2016-09-23 04:28:36]

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

[commit-bot: I haz the power, 2016-09-23 04:28:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 04:53:48]

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

[commit-bot: I haz the power, 2016-09-23 04:53:49]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-09-23 06:06:20]

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

https://codereview.chromium.org/2357423002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:37: var ObjectHasOwnProperty;
Is this used anywhere?

[Dan Ehrenberg, 2016-09-23 07:05:56]

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

https://codereview.chromium.org/2357423002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:37: var ObjectHasOwnProperty;
On 2016/09/23 at 06:06:20, gsathya wrote:
> Is this used anywhere?

Yes, inside of HAS_PRIVATE.

[gsathya, 2016-09-23 16:06:25]

lgtm

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

https://codereview.chromium.org/2357423002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:240: SET_PRIVATE(promise, promiseDeferredReactionsSymbol,
UNDEFINED);
Extra symbol lookup + update here in non debug case, but I guess it's fine since
1) code is in a cleaner state, 2) should go away in rewrite

[Dan Ehrenberg, 2016-09-23 18:11:44]

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

https://codereview.chromium.org/2357423002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:240: SET_PRIVATE(promise, promiseDeferredReactionsSymbol,
UNDEFINED);
On 2016/09/23 at 16:06:25, gsathya wrote:
> Extra symbol lookup + update here in non debug case, but I guess it's fine
since 1) code is in a cleaner state, 2) should go away in rewrite

If this actually made things slower, I'd argue 2 shouldn't apply, but I think 1
is a good reason. A third reason: this is in the multiple reactions case, which
is both much less common and slower.

[Dan Ehrenberg, 2016-09-23 19:59:49]

Description was changed from

==========
Improve stack traces for async functions

This patch tracks the stack of async functions differently from other
Promise async stack tracking. With this patch, the stack trace of a
callstack of async functions should look similarly to the call stack
if all of the functions were synchronous. An example can be found in
the updated test expectations: https://codereview.chromium.org/2362923002 .

The new stack traces are implemented using existing mechanisms in the
inspector. The inspector has two ways to save async stack traces: recurring
and non-recurring stacks. An example of a non-recurring stack is setTimeout,
and a recurring one is saved for setInterval. Recurring stacks are deleted
only when a special "cancel" function is called, rather than being deleted
after being used the first time. Previous Promise async stack tracking always
used non-recurring stacks.

For async functions, this patch saves a recurring stack, which has the top
frame cut off. The frame will instead be provided by the resuming execution
context. The recurring stack is saved when the async function is entered, and
it is deleted from a finally block. The id of the stack is saved in the outer
Promise being constructed by the async function. When an intermediate
throwaway Promise will be triggered as a reaction, it will be identified as
such based on its debugging metadata, and the corresponding async function's
recurring stack will be used.

BUG=v8:4483
==========

to

==========
Improve stack traces for async functions

This patch tracks the stack of async functions differently from other
Promise async stack tracking. With this patch, the stack trace of a
callstack of async functions should look similarly to the call stack
if all of the functions were synchronous. An example can be found in
the updated test expectations: https://codereview.chromium.org/2362923002 .

The new stack traces are implemented using existing mechanisms in the
inspector. The inspector has two ways to save async stack traces: recurring
and non-recurring stacks. An example of a non-recurring stack is setTimeout,
and a recurring one is saved for setInterval. Recurring stacks are deleted
only when a special "cancel" function is called, rather than being deleted
after being used the first time. Previous Promise async stack tracking always
used non-recurring stacks.

For async functions, this patch saves a recurring stack. The top frame of
the stack is duplicated, as the resuming function contains a similar frame;
the devtools frontend is responsible for removing or marking this frame,
which it can do based on seeing the [async function] line which follows it.
The second frame will instead be provided by the resuming execution
context. The recurring stack is saved when the async function is entered, and
it is deleted from a finally block. The id of the stack is saved in the outer
Promise being constructed by the async function. When an intermediate
throwaway Promise will be triggered as a reaction, it will be identified as
such based on its debugging metadata, and the corresponding async function's
recurring stack will be used.

BUG=v8:4483
==========

[kozy, 2016-09-23 21:49:45]

lgtm for inspector part.

https://codereview.chromium.org/2357423002/diff/160001/src/inspector/v8-debug...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2357423002/diff/160001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:659: asyncTaskScheduled(name, ptr, true, true);
Based on offline discussion on DevTools side:
Let's remove skipping frames at all for now and ship it without any UX change
and then find good representation of async-await stacks in design doc.

[Dan Ehrenberg, 2016-09-23 21:51:25]

https://codereview.chromium.org/2357423002/diff/160001/src/inspector/v8-debug...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2357423002/diff/160001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:659: asyncTaskScheduled(name, ptr, true, true);
On 2016/09/23 at 21:49:45, kozyatinskiy wrote:
> Based on offline discussion on DevTools side:
> Let's remove skipping frames at all for now and ship it without any UX change
and then find good representation of async-await stacks in design doc.

If you look at patchset 12, you will find skipping frames has been removed.

[kozy, 2016-09-23 21:54:11]

On 2016/09/23 21:51:25, Dan Ehrenberg wrote:
> If you look at patchset 12, you will find skipping frames has been removed.

Sorry, I added it early and didn't publish and forgot to remove it prior
publishing lgtm. Please just ignore this comment.

[Dan Ehrenberg, 2016-09-23 21:55:13]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2016-09-23 21:55:13]

The patchset sent to the CQ was uploaded after l-g-t-m from gsathya@chromium.org
Link to the patchset: https://codereview.chromium.org/2357423002/#ps220001
(title: "REbase")

[commit-bot: I haz the power, 2016-09-23 21:55:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 22:23:56]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-09-23 22:24:11]

Description was changed from

==========
Improve stack traces for async functions

This patch tracks the stack of async functions differently from other
Promise async stack tracking. With this patch, the stack trace of a
callstack of async functions should look similarly to the call stack
if all of the functions were synchronous. An example can be found in
the updated test expectations: https://codereview.chromium.org/2362923002 .

The new stack traces are implemented using existing mechanisms in the
inspector. The inspector has two ways to save async stack traces: recurring
and non-recurring stacks. An example of a non-recurring stack is setTimeout,
and a recurring one is saved for setInterval. Recurring stacks are deleted
only when a special "cancel" function is called, rather than being deleted
after being used the first time. Previous Promise async stack tracking always
used non-recurring stacks.

For async functions, this patch saves a recurring stack. The top frame of
the stack is duplicated, as the resuming function contains a similar frame;
the devtools frontend is responsible for removing or marking this frame,
which it can do based on seeing the [async function] line which follows it.
The second frame will instead be provided by the resuming execution
context. The recurring stack is saved when the async function is entered, and
it is deleted from a finally block. The id of the stack is saved in the outer
Promise being constructed by the async function. When an intermediate
throwaway Promise will be triggered as a reaction, it will be identified as
such based on its debugging metadata, and the corresponding async function's
recurring stack will be used.

BUG=v8:4483
==========

to

==========
Improve stack traces for async functions

This patch tracks the stack of async functions differently from other
Promise async stack tracking. With this patch, the stack trace of a
callstack of async functions should look similarly to the call stack
if all of the functions were synchronous. An example can be found in
the updated test expectations: https://codereview.chromium.org/2362923002 .

The new stack traces are implemented using existing mechanisms in the
inspector. The inspector has two ways to save async stack traces: recurring
and non-recurring stacks. An example of a non-recurring stack is setTimeout,
and a recurring one is saved for setInterval. Recurring stacks are deleted
only when a special "cancel" function is called, rather than being deleted
after being used the first time. Previous Promise async stack tracking always
used non-recurring stacks.

For async functions, this patch saves a recurring stack. The top frame of
the stack is duplicated, as the resuming function contains a similar frame;
the devtools frontend is responsible for removing or marking this frame,
which it can do based on seeing the [async function] line which follows it.
The second frame will instead be provided by the resuming execution
context. The recurring stack is saved when the async function is entered, and
it is deleted from a finally block. The id of the stack is saved in the outer
Promise being constructed by the async function. When an intermediate
throwaway Promise will be triggered as a reaction, it will be identified as
such based on its debugging metadata, and the corresponding async function's
recurring stack will be used.

BUG=v8:4483

Committed: https://crrev.com/f296dad962e452ced1e5396b9ced7203747f7209
Cr-Commit-Position: refs/heads/master@{#39695}
==========

[commit-bot: I haz the power, 2016-09-23 22:24:13]

Patchset 12 (id:??) landed as
https://crrev.com/f296dad962e452ced1e5396b9ced7203747f7209
Cr-Commit-Position: refs/heads/master@{#39695}

[Dan Ehrenberg, 2016-09-23 22:28:16]

On 2016/09/23 at 22:24:13, commit-bot wrote:
> Patchset 12 (id:??) landed as
https://crrev.com/f296dad962e452ced1e5396b9ced7203747f7209
> Cr-Commit-Position: refs/heads/master@{#39695}

This patch may cause the blink bot to temporarily break. A fix is being landed
at https://codereview.chromium.org/2362923002 . Sorry if there are any ephemeral
infrastructure issues due to the timing.