[inspector] added creation frame for async call chains for promises

[inspector] added creation frame for async call chains for promises

With creation frame we can show additional information with description of each async stack trace, which could help user to understand where promises were chained.
At least in case of Promise.resolve().then(foo1).then(foo2) we would be able to show following stack trace for break in foo2 callback:

foo2 (test.js:14:2)
-- Promise.resolve (test.js:29:14)--
-- Promise.resolve (test.js:28:14)--
promiseThen (test.js:30:2)

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

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

Review-Url: https://codereview.chromium.org/2648873002
Cr-Commit-Position: refs/heads/master@{#42682}
Committed: https://chromium.googlesource.com/v8/v8/+/b98dd0af92656df4889350e63081552aced61ad7

[kozy, 2017-01-22 22:51:13]

Patchset #2 (id:20001) has been deleted

[kozy, 2017-01-22 22:51:18]

Patchset #1 (id:1) has been deleted

[kozy, 2017-01-22 22:57:09]

Description was changed from

==========
async-task-created

BUG=
==========

to

==========
[inspector] added creation frame for async call chains for promises

With creation frame we can show additional information with description of each
async stack trace, which could help user to understand where promises were
chained.
At least in case of Promise.resolve().then(foo1).then(foo2) we would be able to
show following stack trace for break in foo2 callback:

foo2 (test.js:14:2)
-- Promise.resolve -- <- without this CL this async hope was just omitted since
it doesn't contain any synchronous frames.
--- created in ---
promiseThen (test.js:28:14)
-- Promise.resolve --
promiseThen (test.js:30:2)
(anonymous) (expr.js:0:0)
--- created in ---
promiseThen (test.js:27:11)

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

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

[kozy, 2017-01-22 22:57:09]

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

[kozy, 2017-01-22 22:58:40]

Dmitry, please take a look on inspector part.
Sathya, please take a look on promise related part.

[gsathya, 2017-01-23 07:42:09]

On 2017/01/22 22:58:40, kozyatinskiy wrote:
> Dmitry, please take a look on inspector part.
> Sathya, please take a look on promise related part.

lgtm for promises

[Yang, 2017-01-23 14:49:21]

yangguo@chromium.org changed reviewers:
+ yangguo@chromium.org

[Yang, 2017-01-23 14:49:22]

https://codereview.chromium.org/2648873002/diff/40001/src/builtins/builtins-p...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2648873002/diff/40001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:45: GotoUnless(Word32Or(IsPromiseHookEnabled(),
IsDebugActive()), &out);
If this becomes a performance bottleneck, we could precompute this value when
they are set, and perform a single check, skipping the Word32Or.

This is what we do when calling a function: we need to check whether we are
stepping, and whether we need to perform a side-effect check. Both are
debugger-related, and we have Debug::hook_on_function_call_ field that we expose
via external reference to generated code.

[Yang, 2017-01-23 14:49:41]

On 2017/01/23 14:49:22, Yang wrote:
>
https://codereview.chromium.org/2648873002/diff/40001/src/builtins/builtins-p...
> File src/builtins/builtins-promise.cc (right):
> 
>
https://codereview.chromium.org/2648873002/diff/40001/src/builtins/builtins-p...
> src/builtins/builtins-promise.cc:45:
GotoUnless(Word32Or(IsPromiseHookEnabled(),
> IsDebugActive()), &out);
> If this becomes a performance bottleneck, we could precompute this value when
> they are set, and perform a single check, skipping the Word32Or.
> 
> This is what we do when calling a function: we need to check whether we are
> stepping, and whether we need to perform a side-effect check. Both are
> debugger-related, and we have Debug::hook_on_function_call_ field that we
expose
> via external reference to generated code.

lgtm. but please also wait for Dmitry's review.

[kozy, 2017-01-23 22:28:07]

Patchset #2 (id:60001) has been deleted

[kozy, 2017-01-23 22:29:01]

all done.

Dmitry, please take a look!

https://codereview.chromium.org/2648873002/diff/40001/src/builtins/builtins-p...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2648873002/diff/40001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:45: GotoUnless(Word32Or(IsPromiseHookEnabled(),
IsDebugActive()), &out);
On 2017/01/23 14:49:22, Yang wrote:
> If this becomes a performance bottleneck, we could precompute this value when
> they are set, and perform a single check, skipping the Word32Or.
> 
> This is what we do when calling a function: we need to check whether we are
> stepping, and whether we need to perform a side-effect check. Both are
> debugger-related, and we have Debug::hook_on_function_call_ field that we
expose
> via external reference to generated code.

Done.

[kozy, 2017-01-23 22:29:42]

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

[commit-bot: I haz the power, 2017-01-23 22:29:49]

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-23 22:34:17]

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

[commit-bot: I haz the power, 2017-01-23 22:34:18]

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

[kozy, 2017-01-23 23:18:31]

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

[commit-bot: I haz the power, 2017-01-23 23:18:38]

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-24 00:04:29]

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

[commit-bot: I haz the power, 2017-01-24 00:04:30]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-01-24 01:02:41]

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/js_proto...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/js_proto...
src/inspector/js_protocol.json:205: { "name": "creationFrame", "$ref":
"CallFrame", "optional": true, "experimental": true, "description": "Frame where
asynchronous task was created, if available." }
- promiseCreationFrame
- Creation frame of the Promise which produced the next synchronous trace when
resolved, if available.

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

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:880: V8StackTraceImpl::capture(this, 0, 1,
String16());
Comment that we don't need contextgroupid.

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:882: String16 s =
chain->buildInspectorObjectImpl()->serialize();
Remove.

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:883: m_asyncTaskCreatedStacks[task] =
std::move(chain);
Should we do this after registering?

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:954: m_asyncTaskCreatedStacks.erase(task);
Let's call asyncTaskCanceled here?

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

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger.h:169: AsyncTaskToStackTrace m_asyncTaskCreatedStacks;
m_asyncTaskCreationStacks

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-stack...
File src/inspector/v8-stack-trace-impl.cc (right):

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.cc:131: V8StackTraceImpl* creationCallChain =
nullptr;
Remove.

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.cc:146: // Only the top stack in the chain may
be empty, so ensure that second stack
Amend the comment.

https://codereview.chromium.org/2648873002/diff/100001/test/inspector/debugge...
File test/inspector/debugger/async-stack-await.js (right):

https://codereview.chromium.org/2648873002/diff/100001/test/inspector/debugge...
test/inspector/debugger/async-stack-await.js:37:
InspectorTest.logCallFrames([asyncStackTrace.creationFrame]);
Let's dump it on the same line as description.

[kozy, 2017-01-24 21:33:59]

Description was changed from

==========
[inspector] added creation frame for async call chains for promises

With creation frame we can show additional information with description of each
async stack trace, which could help user to understand where promises were
chained.
At least in case of Promise.resolve().then(foo1).then(foo2) we would be able to
show following stack trace for break in foo2 callback:

foo2 (test.js:14:2)
-- Promise.resolve -- <- without this CL this async hope was just omitted since
it doesn't contain any synchronous frames.
--- created in ---
promiseThen (test.js:28:14)
-- Promise.resolve --
promiseThen (test.js:30:2)
(anonymous) (expr.js:0:0)
--- created in ---
promiseThen (test.js:27:11)

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

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

to

==========
[inspector] added creation frame for async call chains for promises

With creation frame we can show additional information with description of each
async stack trace, which could help user to understand where promises were
chained.
At least in case of Promise.resolve().then(foo1).then(foo2) we would be able to
show following stack trace for break in foo2 callback:

foo2 (test.js:14:2)
-- Promise.resolve (test.js:29:14)--
-- Promise.resolve (test.js:28:14)--
promiseThen (test.js:30:2)

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

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

[kozy, 2017-01-24 21:43:39]

Rebased, Dmitry pease take a look!

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/js_proto...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/js_proto...
src/inspector/js_protocol.json:205: { "name": "creationFrame", "$ref":
"CallFrame", "optional": true, "experimental": true, "description": "Frame where
asynchronous task was created, if available." }
On 2017/01/24 01:02:41, dgozman wrote:
> - promiseCreationFrame
> - Creation frame of the Promise which produced the next synchronous trace when
> resolved, if available.

Done.

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

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:880: V8StackTraceImpl::capture(this, 0, 1,
String16());
On 2017/01/24 01:02:41, dgozman wrote:
> Comment that we don't need contextgroupid.

Done.

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:882: String16 s =
chain->buildInspectorObjectImpl()->serialize();
On 2017/01/24 01:02:41, dgozman wrote:
> Remove.

Done.

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:883: m_asyncTaskCreatedStacks[task] =
std::move(chain);
On 2017/01/24 01:02:41, dgozman wrote:
> Should we do this after registering?

it should be called after, otherwise we could cancel task and then remove stack
for it.

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:954: m_asyncTaskCreatedStacks.erase(task);
On 2017/01/24 01:02:41, dgozman wrote:
> Let's call asyncTaskCanceled here?

Done.

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

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger.h:169: AsyncTaskToStackTrace m_asyncTaskCreatedStacks;
On 2017/01/24 01:02:41, dgozman wrote:
> m_asyncTaskCreationStacks

Done.

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-stack...
File src/inspector/v8-stack-trace-impl.cc (right):

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.cc:131: V8StackTraceImpl* creationCallChain =
nullptr;
On 2017/01/24 01:02:41, dgozman wrote:
> Remove.

Done.

https://codereview.chromium.org/2648873002/diff/100001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.cc:146: // Only the top stack in the chain may
be empty, so ensure that second stack
On 2017/01/24 01:02:41, dgozman wrote:
> Amend the comment.

Done.

https://codereview.chromium.org/2648873002/diff/100001/test/inspector/debugge...
File test/inspector/debugger/async-stack-await.js (right):

https://codereview.chromium.org/2648873002/diff/100001/test/inspector/debugge...
test/inspector/debugger/async-stack-await.js:37:
InspectorTest.logCallFrames([asyncStackTrace.creationFrame]);
On 2017/01/24 01:02:41, dgozman wrote:
> Let's dump it on the same line as description.

Done.

[kozy, 2017-01-24 22:53:55]

Dmitry, please take another look.

[dgozman, 2017-01-24 23:57:44]

lgtm

https://codereview.chromium.org/2648873002/diff/140001/src/inspector/v8-stack...
File src/inspector/v8-stack-trace-impl.h (right):

https://codereview.chromium.org/2648873002/diff/140001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.h:84: void
setCreation(std::unique_ptr<V8StackTraceImpl> creation) {
setPromiseCreationStack

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

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:879: std::unique_ptr<V8StackTraceImpl> chain =
chain -> creationStack

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:880: V8StackTraceImpl::capture(this, 0, 1,
String16());
Let's comment that passing 1 here force no async chain for the new stack, which
allows us to not grow exponentially.

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-stack...
File src/inspector/v8-stack-trace-impl.cc (right):

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.cc:156: if (asyncCallChain &&
asyncCallChain->isEmpty() && asyncCallChain->m_parent) {
Let's move this logic to setCreationStack to avoid modifying parent.

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-stack...
File src/inspector/v8-stack-trace-impl.h (right):

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.h:84: void
setCreation(std::unique_ptr<V8StackTraceImpl> creation) {
Move definition to implementation file.

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.h:92: std::unique_ptr<V8StackTraceImpl>
creation);
If you have a setter, don't pass in constructor.

https://codereview.chromium.org/2648873002/diff/160001/test/inspector/debugge...
File test/inspector/debugger/async-stack-await.js (right):

https://codereview.chromium.org/2648873002/diff/160001/test/inspector/debugge...
test/inspector/debugger/async-stack-await.js:30:
InspectorTest.logCallFrames(message.params.callFrames);
Do you want to extract this whole method into InspectorTest.logCallChain() ?

https://codereview.chromium.org/2648873002/diff/160001/test/inspector/debugge...
File test/inspector/debugger/async-stacks-limit-expected.txt (left):

https://codereview.chromium.org/2648873002/diff/160001/test/inspector/debugge...
test/inspector/debugger/async-stacks-limit-expected.txt:22: -- Promise.resolve
--
Can we adjust constants in this test for it to be more meaningful now?

[kozy, 2017-01-25 00:45:43]

thank you!, all done.

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

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:879: std::unique_ptr<V8StackTraceImpl> chain =
On 2017/01/24 23:57:44, dgozman wrote:
> chain -> creationStack

Done.

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:880: V8StackTraceImpl::capture(this, 0, 1,
String16());
On 2017/01/24 23:57:44, dgozman wrote:
> Let's comment that passing 1 here force no async chain for the new stack,
which
> allows us to not grow exponentially.

Done.

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-stack...
File src/inspector/v8-stack-trace-impl.cc (right):

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.cc:156: if (asyncCallChain &&
asyncCallChain->isEmpty() && asyncCallChain->m_parent) {
On 2017/01/24 23:57:44, dgozman wrote:
> Let's move this logic to setCreationStack to avoid modifying parent.

Done.

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-stack...
File src/inspector/v8-stack-trace-impl.h (right):

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.h:84: void
setCreation(std::unique_ptr<V8StackTraceImpl> creation) {
On 2017/01/24 23:57:44, dgozman wrote:
> Move definition to implementation file.

Done.

https://codereview.chromium.org/2648873002/diff/160001/src/inspector/v8-stack...
src/inspector/v8-stack-trace-impl.h:92: std::unique_ptr<V8StackTraceImpl>
creation);
On 2017/01/24 23:57:44, dgozman wrote:
> If you have a setter, don't pass in constructor.

Done.

https://codereview.chromium.org/2648873002/diff/160001/test/inspector/debugge...
File test/inspector/debugger/async-stack-await.js (right):

https://codereview.chromium.org/2648873002/diff/160001/test/inspector/debugge...
test/inspector/debugger/async-stack-await.js:30:
InspectorTest.logCallFrames(message.params.callFrames);
On 2017/01/24 23:57:44, dgozman wrote:
> Do you want to extract this whole method into InspectorTest.logCallChain() ?

Done.

https://codereview.chromium.org/2648873002/diff/160001/test/inspector/debugge...
File test/inspector/debugger/async-stacks-limit-expected.txt (left):

https://codereview.chromium.org/2648873002/diff/160001/test/inspector/debugge...
test/inspector/debugger/async-stacks-limit-expected.txt:22: -- Promise.resolve
--
On 2017/01/24 23:57:44, dgozman wrote:
> Can we adjust constants in this test for it to be more meaningful now?

Done.

[kozy, 2017-01-25 07:06:59]

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

[commit-bot: I haz the power, 2017-01-25 07:07:04]

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-25 07:41:58]

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

[commit-bot: I haz the power, 2017-01-25 07:42:01]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-01-26 08:56:01]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-01-26 08:56:01]

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/2648873002/#ps220001
(title: "add test for setTimeout")

[commit-bot: I haz the power, 2017-01-26 08:56:07]

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-26 09:32:38]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1485420961544590,
"parent_rev": "25bfdf1b46f21435343cd94b31add664395db146", "commit_rev":
"b98dd0af92656df4889350e63081552aced61ad7"}

[commit-bot: I haz the power, 2017-01-26 09:32:43]

Description was changed from

==========
[inspector] added creation frame for async call chains for promises

With creation frame we can show additional information with description of each
async stack trace, which could help user to understand where promises were
chained.
At least in case of Promise.resolve().then(foo1).then(foo2) we would be able to
show following stack trace for break in foo2 callback:

foo2 (test.js:14:2)
-- Promise.resolve (test.js:29:14)--
-- Promise.resolve (test.js:28:14)--
promiseThen (test.js:30:2)

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

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

to

==========
[inspector] added creation frame for async call chains for promises

With creation frame we can show additional information with description of each
async stack trace, which could help user to understand where promises were
chained.
At least in case of Promise.resolve().then(foo1).then(foo2) we would be able to
show following stack trace for break in foo2 callback:

foo2 (test.js:14:2)
-- Promise.resolve (test.js:29:14)--
-- Promise.resolve (test.js:28:14)--
promiseThen (test.js:30:2)

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

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

Review-Url: https://codereview.chromium.org/2648873002
Cr-Commit-Position: refs/heads/master@{#42682}
Committed:
https://chromium.googlesource.com/v8/v8/+/b98dd0af92656df4889350e63081552aced...
==========

[commit-bot: I haz the power, 2017-01-26 09:32:44]

Committed patchset #9 (id:220001) as
https://chromium.googlesource.com/v8/v8/+/b98dd0af92656df4889350e63081552aced...