[inspector] async stacks for Promise.then calls...

[inspector] async stacks for Promise.then calls...

... which were done after the promise has been resolved.

Goal of this CL - change promise instrumentation to support better callbacks, chained after promise resolution and prepare instrumentation for adding new asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector <-> debugger API transferred by enum instead of string,
- Debug manages async task ids based on promise objects.

More details: https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829, v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org

Review-Url: https://codereview.chromium.org/2578923002
Cr-Commit-Position: refs/heads/master@{#42178}
Committed: https://chromium.googlesource.com/v8/v8/+/754736d26c07454de8eace0b774a1ab13bf2c764

[kozy, 2016-12-15 23:24:08]

Patchset #1 (id:1) has been deleted

[kozy, 2016-12-15 23:36:36]

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

[commit-bot: I haz the power, 2016-12-15 23:36:46]

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

[kozy, 2016-12-15 23:43:01]

Description was changed from

==========
[inspector] better async stacks for later then call

WIP

BUG=chromium:632829,v8:5738
==========

to

==========
[inspector] better async stacks for later then call

Goal of this CL - change promise instrumentation to support better callbacks
chained after promise resolution and prepare instrumentation for adding new
asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that
promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector - debugger API transferred by enum instead of
string,
- Debug manages async task ids based on promise objects.

More details:
https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829,v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org
==========

[kozy, 2016-12-15 23:43:01]

kozyatinskiy@chromium.org changed reviewers:
+ dgozman@chromium.org, gsathya@chromium.org, yangguo@chromium.org

[kozy, 2016-12-15 23:43:59]

Please take a look. You can use codereview two compare test expectations with
patch and without - compare them with first patch set.

[dgozman, 2016-12-15 23:46:07]

Description was changed from

==========
[inspector] better async stacks for later then call

Goal of this CL - change promise instrumentation to support better callbacks
chained after promise resolution and prepare instrumentation for adding new
asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that
promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector - debugger API transferred by enum instead of
string,
- Debug manages async task ids based on promise objects.

More details:
https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829,v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org
==========

to

==========
[inspector] async stacks for Promise.then calls...

... which were done after the promise has been resolved.

Goal of this CL - change promise instrumentation to support better callbacks
chained after promise resolution and prepare instrumentation for adding new
asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that
promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector - debugger API transferred by enum instead of
string,
- Debug manages async task ids based on promise objects.

More details:
https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829,v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org
==========

[dgozman, 2016-12-16 00:02:47]

Tests and expectations looks good!

[kozy, 2016-12-16 00:12:23]

Description was changed from

==========
[inspector] async stacks for Promise.then calls...

... which were done after the promise has been resolved.

Goal of this CL - change promise instrumentation to support better callbacks
chained after promise resolution and prepare instrumentation for adding new
asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that
promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector - debugger API transferred by enum instead of
string,
- Debug manages async task ids based on promise objects.

More details:
https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829,v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org
==========

to

==========
[inspector] async stacks for Promise.then calls...

... which were done after the promise has been resolved.

Goal of this CL - change promise instrumentation to support better callbacks,
chained after promise resolution and prepare instrumentation for adding new
asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that
promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector - debugger API transferred by enum instead of
string,
- Debug manages async task ids based on promise objects.

More details:
https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829,v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org
==========

[kozy, 2016-12-16 00:13:02]

Description was changed from

==========
[inspector] async stacks for Promise.then calls...

... which were done after the promise has been resolved.

Goal of this CL - change promise instrumentation to support better callbacks,
chained after promise resolution and prepare instrumentation for adding new
asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that
promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector - debugger API transferred by enum instead of
string,
- Debug manages async task ids based on promise objects.

More details:
https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829,v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org
==========

to

==========
[inspector] async stacks for Promise.then calls...

... which were done after the promise has been resolved.

Goal of this CL - change promise instrumentation to support better callbacks,
chained after promise resolution and prepare instrumentation for adding new
asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that
promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector <-> debugger API transferred by enum instead of
string,
- Debug manages async task ids based on promise objects.

More details:
https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829,v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org
==========

[commit-bot: I haz the power, 2016-12-16 00:24:56]

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

[commit-bot: I haz the power, 2016-12-16 00:24:57]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17905)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17878)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[kozy, 2016-12-16 01:03:41]

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

[commit-bot: I haz the power, 2016-12-16 01:03:48]

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-12-16 01:32:13]

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

[commit-bot: I haz the power, 2016-12-16 01:32:14]

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

[kozy, 2016-12-16 02:12:28]

Patchset #4 (id:80001) has been deleted

[kozy, 2016-12-16 02:12:41]

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

[commit-bot: I haz the power, 2016-12-16 02:12:51]

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-12-16 02:37:12]

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

[commit-bot: I haz the power, 2016-12-16 02:37:13]

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

[kozy, 2016-12-16 02:42:13]

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

[kozy, 2016-12-16 02:42:21]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-12-16 02:42:23]

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-12-16 03:07:30]

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

[commit-bot: I haz the power, 2016-12-16 03:07:31]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-12-16 13:39:33]

https://codereview.chromium.org/2578923002/diff/120001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2578923002/diff/120001/src/debug/debug.cc#new...
src/debug/debug.cc:1822: Handle<JSWeakMap> map = thread_local_.async_task_ids_;
How about storing this id simply on the promise object itself, as a private
property (using a private symbol)? We use private properties to store a few
things on the promise already.

[kozy, 2016-12-16 15:29:35]

all done, please take another look.

https://codereview.chromium.org/2578923002/diff/120001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2578923002/diff/120001/src/debug/debug.cc#new...
src/debug/debug.cc:1822: Handle<JSWeakMap> map = thread_local_.async_task_ids_;
On 2016/12/16 13:39:33, Yang wrote:
> How about storing this id simply on the promise object itself, as a private
> property (using a private symbol)? We use private properties to store a few
> things on the promise already.

Done.
My idea was: if we store ids in weak map then we can easily cleanup this map on
debugger disabled.

[gsathya, 2016-12-16 15:37:11]

https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc#new...
src/debug/debug.cc:1813: Handle<Object> Debug::NextAsyncTaskId(Handle<JSObject>
promise) {
Can this be a non JSPromise? If not, you can make this private symbol be a flag
in JSPromise

[kozy, 2016-12-16 15:42:15]

On 2016/12/16 15:37:11, gsathya wrote:
> https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc#new...
> src/debug/debug.cc:1813: Handle<Object>
Debug::NextAsyncTaskId(Handle<JSObject>
> promise) {
> Can this be a non JSPromise? If not, you can make this private symbol be a
flag
> in JSPromise

It can, see [1] for example of custom promise.

[1]
https://cs.chromium.org/chromium/src/v8/test/debugger/debug/es6/debug-promise...

[kozy, 2016-12-16 16:26:53]

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

[commit-bot: I haz the power, 2016-12-16 16:27:02]

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

[kozy, 2016-12-16 16:29:26]

https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc#new...
src/debug/debug.cc:1813: Handle<Object> Debug::NextAsyncTaskId(Handle<JSObject>
promise) {
On 2016/12/16 15:37:11, gsathya wrote:
> Can this be a non JSPromise? If not, you can make this private symbol be a
flag
> in JSPromise

I double checked that we always get JSPromise here and replace private symbol
with debug_id on promise object. If it's fine to have additional int per promise
object then updated CL is here. :)

[commit-bot: I haz the power, 2016-12-16 16:33:50]

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

[commit-bot: I haz the power, 2016-12-16 16:33:51]

Dry run: Try jobs failed on following builders:
  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...)

[gsathya, 2016-12-16 16:48:36]

On 2016/12/16 16:29:26, kozyatinskiy wrote:
> https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc#new...
> src/debug/debug.cc:1813: Handle<Object>
Debug::NextAsyncTaskId(Handle<JSObject>
> promise) {
> On 2016/12/16 15:37:11, gsathya wrote:
> > Can this be a non JSPromise? If not, you can make this private symbol be a
> flag
> > in JSPromise
> 
> I double checked that we always get JSPromise here and replace private symbol
> with debug_id on promise object. If it's fine to have additional int per
promise
> object then updated CL is here. :)

Ah sorry I thought this was just a single bit (we have several unused bits in
JSPromise that we could've reused). I was afraid using private symbols would
cause map transitions(maybe not?) but that's probably fine if debugger is on. So
private symbol is better.

lgtm

[kozy, 2016-12-16 17:00:50]

Patchset #6 (id:160001) has been deleted

[kozy, 2016-12-16 17:00:56]

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

[commit-bot: I haz the power, 2016-12-16 17:01:01]

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

[kozy, 2016-12-16 17:02:42]

On 2016/12/16 16:48:36, gsathya wrote:
> Ah sorry I thought this was just a single bit (we have several unused bits in
> JSPromise that we could've reused). I was afraid using private symbols would
> cause map transitions(maybe not?) but that's probably fine if debugger is on.
So
> private symbol is better.
> 
> lgtm

Thanks, I could avoid any changes in JSPromise map if needed by using WeakMap.
Yang, WDYT?

[Yang, 2016-12-16 17:10:26]

I think either is ok. I dont think using a internal field will significantly
grow the memory use. I'm opposed to using a weak map because it's context
dependent and would keep a whole context alive.

lgtm

https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc#new...
src/debug/debug.cc:1800: data->debug->OnAsyncTaskEvent(debug::Cancel,
Can you add a comment explaining why we need a weak callback?

https://codereview.chromium.org/2578923002/diff/140001/test/inspector/debugge...
File test/inspector/debugger/async-instrumentation.js (right):

https://codereview.chromium.org/2578923002/diff/140001/test/inspector/debugge...
test/inspector/debugger/async-instrumentation.js:46: Protocol.Runtime.evaluate({
expression: 'test()//# sourceURL=expr1.js', awaitPromise: true })
80char limit

[Yang, 2016-12-16 17:12:28]

You know, we could actually store the stack directly on the promise instead of
the id, and map the id to the stack in the inspector...

[kozy, 2016-12-16 17:19:31]

On 2016/12/16 17:12:28, Yang wrote:
> You know, we could actually store the stack directly on the promise instead of
> the id, and map the id to the stack in the inspector...

We need a map on inspector side to support other async primitives, e.g.
setTimeout from blink and stack that we store contains previous async chains too
- not only last synchronous stack. It means we need to introduce async stacks
concept in V8 world and still will need to have special handling for blink async
primitives.

[gsathya, 2016-12-16 17:26:28]

On 2016/12/16 17:19:31, kozyatinskiy wrote:
> On 2016/12/16 17:12:28, Yang wrote:
> > You know, we could actually store the stack directly on the promise instead
of
> > the id, and map the id to the stack in the inspector...
> 
> We need a map on inspector side to support other async primitives, e.g.
> setTimeout from blink and stack that we store contains previous async chains
too
> - not only last synchronous stack. It means we need to introduce async stacks
> concept in V8 world and still will need to have special handling for blink
async
> primitives.

I'd prefer private symbol over extra internal field that gets used only if the
debugger is open.

[commit-bot: I haz the power, 2016-12-16 17:29:38]

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

[commit-bot: I haz the power, 2016-12-16 17:29:39]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-12-16 17:47:06]

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

[commit-bot: I haz the power, 2016-12-16 17:47:15]

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

[kozy, 2016-12-16 17:47:15]

all done.

https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2578923002/diff/140001/src/debug/debug.cc#new...
src/debug/debug.cc:1800: data->debug->OnAsyncTaskEvent(debug::Cancel,
On 2016/12/16 17:10:26, Yang wrote:
> Can you add a comment explaining why we need a weak callback?

Done.

https://codereview.chromium.org/2578923002/diff/140001/test/inspector/debugge...
File test/inspector/debugger/async-instrumentation.js (right):

https://codereview.chromium.org/2578923002/diff/140001/test/inspector/debugge...
test/inspector/debugger/async-instrumentation.js:46: Protocol.Runtime.evaluate({
expression: 'test()//# sourceURL=expr1.js', awaitPromise: true })
On 2016/12/16 17:10:26, Yang wrote:
> 80char limit

Done.

[commit-bot: I haz the power, 2016-12-16 18:17:03]

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

[commit-bot: I haz the power, 2016-12-16 18:17:04]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-12-18 17:09:37]

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

[commit-bot: I haz the power, 2016-12-18 17:09:42]

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-12-18 17:38:18]

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

[commit-bot: I haz the power, 2016-12-18 17:38:19]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-12-18 18:10:29]

Dmitry, please take a look.

[kozy, 2016-12-21 07:18:54]

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

[commit-bot: I haz the power, 2016-12-21 07:18:59]

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-12-21 07:49:09]

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

[commit-bot: I haz the power, 2016-12-21 07:49:10]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-01-06 22:12:03]

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

[commit-bot: I haz the power, 2017-01-06 22:12:09]

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, 2017-01-06 22:13:19]

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

[commit-bot: I haz the power, 2017-01-06 22:13:20]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  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_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15125)

[kozy, 2017-01-09 19:01:45]

Rebased and ready for review again.
Dmitry, please take a look.

One minor improvement in comparison with previous version - correct support for
Promise.reject().catch(() => { debugger; }), test added.

[kozy, 2017-01-09 19:19:07]

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

[commit-bot: I haz the power, 2017-01-09 19:19:18]

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

[gsathya, 2017-01-09 19:24:35]

https://codereview.chromium.org/2578923002/diff/230001/src/runtime/runtime-pr...
File src/runtime/runtime-promise.cc (right):

https://codereview.chromium.org/2578923002/diff/230001/src/runtime/runtime-pr...
src/runtime/runtime-promise.cc:113: void SetDebugInfo(Isolate* isolate,
Handle<PromiseReactionJobInfo> info,
info->promise() == promise right? can we just reuse the promise from info

[commit-bot: I haz the power, 2017-01-09 19:41:09]

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

[commit-bot: I haz the power, 2017-01-09 19:41:11]

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

[dgozman, 2017-01-09 22:59:30]

Promises lgtm! Do we have to rebase some blink tests?

Not sure whether async/await stacks changed, was is accidental or not, and
whether they deserve to be in this patch. Up to you though.

https://codereview.chromium.org/2578923002/diff/230001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2578923002/diff/230001/src/debug/debug.cc#new...
src/debug/debug.cc:1784: Object** location;
Does this survive moving GC?

https://codereview.chromium.org/2578923002/diff/230001/src/debug/debug.cc#new...
src/debug/debug.cc:1828: // Since we holding promise when at least one microtask
is scheduled, we can
Who holds the promise for us?

https://codereview.chromium.org/2578923002/diff/230001/src/debug/interface-ty...
File src/debug/interface-types.h (right):

https://codereview.chromium.org/2578923002/diff/230001/src/debug/interface-ty...
src/debug/interface-types.h:63: kDebugEnqueue,
This never happens, right?

https://codereview.chromium.org/2578923002/diff/230001/test/inspector/debugge...
File test/inspector/debugger/async-instrumentation.js (right):

https://codereview.chromium.org/2578923002/diff/230001/test/inspector/debugge...
test/inspector/debugger/async-instrumentation.js:29: if (enableOnPause-- === 0)
Declare this variable somewhere please.

https://codereview.chromium.org/2578923002/diff/230001/test/inspector/debugge...
File test/inspector/debugger/async-stack-await-expected.txt (right):

https://codereview.chromium.org/2578923002/diff/230001/test/inspector/debugge...
test/inspector/debugger/async-stack-await-expected.txt:15: foo2 (test.js:18:8)
So, we don't get "async function" for await calls anymore? Only if there was an
async/await in this function already? Is that how it is now, or do you change
the behavior in this patch?

[kozy, 2017-01-10 00:11:02]

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

[commit-bot: I haz the power, 2017-01-10 00:11:13]

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

[kozy, 2017-01-10 00:11:38]

https://codereview.chromium.org/2578923002/diff/230001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2578923002/diff/230001/src/debug/debug.cc#new...
src/debug/debug.cc:1784: Object** location;
On 2017/01/09 22:59:30, dgozman wrote:
> Does this survive moving GC?

Should survive, it looks like common pattern for GlobalHandles::MakeWeak in V8,
e.g. [1]. And I tested this behavior by calling gc between async task events.

[1] https://cs.chromium.org/chromium/src/v8/src/wasm/wasm-module.cc?rcl=0&l=741

https://codereview.chromium.org/2578923002/diff/230001/src/debug/debug.cc#new...
src/debug/debug.cc:1828: // Since we holding promise when at least one microtask
is scheduled, we can
On 2017/01/09 22:59:30, dgozman wrote:
> Who holds the promise for us?

It is stored inside PromiseReactionJobInfo, tuned comment.

https://codereview.chromium.org/2578923002/diff/230001/src/debug/interface-ty...
File src/debug/interface-types.h (right):

https://codereview.chromium.org/2578923002/diff/230001/src/debug/interface-ty...
src/debug/interface-types.h:63: kDebugEnqueue,
On 2017/01/09 22:59:30, dgozman wrote:
> This never happens, right?

Never, I think we can remove it now and add it later when we need itNeve

https://codereview.chromium.org/2578923002/diff/230001/src/runtime/runtime-pr...
File src/runtime/runtime-promise.cc (right):

https://codereview.chromium.org/2578923002/diff/230001/src/runtime/runtime-pr...
src/runtime/runtime-promise.cc:113: void SetDebugInfo(Isolate* isolate,
Handle<PromiseReactionJobInfo> info,
On 2017/01/09 19:24:35, gsathya wrote:
> info->promise() == promise right? can we just reuse the promise from info

Done.

https://codereview.chromium.org/2578923002/diff/230001/test/inspector/debugge...
File test/inspector/debugger/async-instrumentation.js (right):

https://codereview.chromium.org/2578923002/diff/230001/test/inspector/debugge...
test/inspector/debugger/async-instrumentation.js:29: if (enableOnPause-- === 0)
On 2017/01/09 22:59:30, dgozman wrote:
> Declare this variable somewhere please.

Done.

https://codereview.chromium.org/2578923002/diff/230001/test/inspector/debugge...
File test/inspector/debugger/async-stack-await-expected.txt (right):

https://codereview.chromium.org/2578923002/diff/230001/test/inspector/debugge...
test/inspector/debugger/async-stack-await-expected.txt:15: foo2 (test.js:18:8)
On 2017/01/09 22:59:30, dgozman wrote:
> So, we don't get "async function" for await calls anymore? Only if there was
an
> async/await in this function already? Is that how it is now, or do you change
> the behavior in this patch?

My goal was to save current behavior for async functions and you can compare
results with last changes and without any changes [1] - there is only one small
change related to better support of promise with late chained callback.

[1]
https://codereview.chromium.org/2578923002/diff2/20001:250001/test/inspector/...

[kozy, 2017-01-10 00:12:53]

On 2017/01/09 22:59:30, dgozman wrote:
> Promises lgtm! Do we have to rebase some blink tests?
> 

Yes, we need to rebase at least
inspector/sources/debugger-async/async-callstack-promises.html, will figure out
full list tomorrow.

[commit-bot: I haz the power, 2017-01-10 00:40:31]

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

[commit-bot: I haz the power, 2017-01-10 00:40:32]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-01-10 12:22:18]

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

[commit-bot: I haz the power, 2017-01-10 12:22:28]

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

[kozy, 2017-01-10 12:35:23]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[kozy, 2017-01-10 12:35:31]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-01-10 12:35:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, gsathya@chromium.org, dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2578923002/#ps290001
(title: "avoid calling functions")

[commit-bot: I haz the power, 2017-01-10 12:35:37]

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, 2017-01-10 12:54:13]

CQ is committing da patch.

Bot data: {"patchset_id": 290001, "attempt_start_ts": 1484051731163630,
"parent_rev": "0f159f59736f20cb9fca128fed0ac1b29f6312e5", "commit_rev":
"754736d26c07454de8eace0b774a1ab13bf2c764"}

[commit-bot: I haz the power, 2017-01-10 12:54:17]

Description was changed from

==========
[inspector] async stacks for Promise.then calls...

... which were done after the promise has been resolved.

Goal of this CL - change promise instrumentation to support better callbacks,
chained after promise resolution and prepare instrumentation for adding new
asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that
promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector <-> debugger API transferred by enum instead of
string,
- Debug manages async task ids based on promise objects.

More details:
https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829,v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org
==========

to

==========
[inspector] async stacks for Promise.then calls...

... which were done after the promise has been resolved.

Goal of this CL - change promise instrumentation to support better callbacks,
chained after promise resolution and prepare instrumentation for adding new
asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that
promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector <-> debugger API transferred by enum instead of
string,
- Debug manages async task ids based on promise objects.

More details:
https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829,v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org

Review-Url: https://codereview.chromium.org/2578923002
Cr-Commit-Position: refs/heads/master@{#42178}
Committed:
https://chromium.googlesource.com/v8/v8/+/754736d26c07454de8eace0b774a1ab13bf...
==========

[commit-bot: I haz the power, 2017-01-10 12:54:19]

Committed patchset #12 (id:290001) as
https://chromium.googlesource.com/v8/v8/+/754736d26c07454de8eace0b774a1ab13bf...