[Devtools] Add stacktrace to protocol for scriptParsed event

[Devtools] Add stacktrace to protocol for scriptParsed event

Stacktrace data is now passed when scriptParsed event is triggered.

R=kozyatinskiy@chromium.org,dgozman
BUG=chromium:646849
Review-Url: https://codereview.chromium.org/2755863002
Cr-Commit-Position: refs/heads/master@{#43879}
Committed: https://chromium.googlesource.com/v8/v8/+/3e1e90dec273a508785c109c6dce816a9aeb3372

[allada, 2017-03-16 01:59:03]

PTL

[allada, 2017-03-16 01:59:07]

Patchset #1 (id:1) has been deleted

[allada, 2017-03-16 02:05:33]

Description was changed from

==========
[Devtools] Add stacktrace to protocol for scriptParsed event

Stacktrace data is now passed when scriptParsed event is triggered.

R=kozyatinskiy@chromium.org,dgozman
BUG=chromium:646849
==========

to

==========
[Devtools] Add stacktrace to protocol for scriptParsed event

Stacktrace data is now passed when scriptParsed event is triggered.

R=kozyatinskiy@chromium.org,dgozman
BUG=chromium:646849
==========

[allada, 2017-03-16 02:05:33]

allada@chromium.org changed reviewers:
+ luoe@chromium.org

[kozy, 2017-03-16 18:35:13]

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1150: isModuleParam,
scriptRef->source().length(), std::move(stackTrace));
let's do the same for scriptFailedToParse.

[dgozman, 2017-03-16 18:39:04]

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/js_protoc...
src/inspector/js_protocol.json:717: { "name": "stackTrace", "$ref":
"Runtime.StackTrace", "optional": true, "description": "JavaScript stack trace
if available.", "experimental": true }
- creationStackTrace? evaluationStackTrace?
- please add a meaningful description.

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1140:
V8StackTraceImpl::capture(m_inspector->debugger(), contextGroupId,
Do we need the whole stack? Maybe just a top frame? I'm worried this could be
slow. Let's measure the performance impact.

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1157: scriptRef->source().length());
It makes sense to send it for syntax errors as well.

[allada, 2017-03-16 20:59:07]

Patchset #2 (id:40001) has been deleted

[kozy, 2017-03-16 21:00:13]

lgtm

https://codereview.chromium.org/2755863002/diff/60001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2755863002/diff/60001/src/inspector/js_protoc...
src/inspector/js_protocol.json:717: { "name": "stackTrace", "$ref":
"Runtime.StackTrace", "optional": true, "description": "JavaScript stack trace
of where the script parsed event was triggered if available.", "experimental":
true }
let's mention that we collect only evaluation frame - not whole stack.

[allada, 2017-03-16 21:02:13]

done.

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/js_protoc...
src/inspector/js_protocol.json:717: { "name": "stackTrace", "$ref":
"Runtime.StackTrace", "optional": true, "description": "JavaScript stack trace
if available.", "experimental": true }
On 2017/03/16 18:39:03, dgozman wrote:
> - creationStackTrace? evaluationStackTrace?
> - please add a meaningful description.

Per offline comments, I updated description.

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1140:
V8StackTraceImpl::capture(m_inspector->debugger(), contextGroupId,
On 2017/03/16 18:39:03, dgozman wrote:
> Do we need the whole stack? Maybe just a top frame? I'm worried this could be
> slow. Let's measure the performance impact.

Per our offline discussion, capturing 1 frame was apx 5% slower capturing full
stack trace was 10-15% slower

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1150: isModuleParam,
scriptRef->source().length(), std::move(stackTrace));
On 2017/03/16 18:35:12, kozy wrote:
> let's do the same for scriptFailedToParse.

Done.

https://codereview.chromium.org/2755863002/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1157: scriptRef->source().length());
On 2017/03/16 18:39:04, dgozman wrote:
> It makes sense to send it for syntax errors as well.

Done.

https://codereview.chromium.org/2755863002/diff/60001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2755863002/diff/60001/src/inspector/js_protoc...
src/inspector/js_protocol.json:717: { "name": "stackTrace", "$ref":
"Runtime.StackTrace", "optional": true, "description": "JavaScript stack trace
of where the script parsed event was triggered if available.", "experimental":
true }
On 2017/03/16 21:00:13, kozy wrote:
> let's mention that we collect only evaluation frame - not whole stack.

Done.

[allada, 2017-03-16 21:02:20]

The CQ bit was checked by allada@chromium.org

[allada, 2017-03-16 21:02:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org
Link to the patchset: https://codereview.chromium.org/2755863002/#ps80001
(title: "better stack trace for fails and test for fail")

[commit-bot: I haz the power, 2017-03-16 21:02:28]

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-03-16 21:05:31]

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

[commit-bot: I haz the power, 2017-03-16 21:05:31]

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/36913)

[dgozman, 2017-03-16 21:09:21]

lgtm with comments

https://codereview.chromium.org/2755863002/diff/80001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2755863002/diff/80001/src/inspector/js_protoc...
src/inspector/js_protocol.json:717: { "name": "stackTrace", "$ref":
"Runtime.StackTrace", "optional": true, "description": "JavaScript top stack
frame of where the script parsed event was triggered if available.",
"experimental": true }
"Stack trace at the point of script evaluation"

https://codereview.chromium.org/2755863002/diff/80001/src/inspector/js_protoc...
src/inspector/js_protocol.json:737: { "name": "stackTrace", "$ref":
"Runtime.StackTrace", "optional": true, "description": "JavaScript top stack
frame of where the script parsed event was triggered if available.",
"experimental": true }
"Stack trace at the point of failed script evaluation"

https://codereview.chromium.org/2755863002/diff/80001/test/inspector/debugger...
File test/inspector/debugger/script-on-after-compile-expected.txt (right):

https://codereview.chromium.org/2755863002/diff/80001/test/inspector/debugger...
test/inspector/debugger/script-on-after-compile-expected.txt:650: scriptId : 61
You should sanitize this scriptId.

[kozy, 2017-03-16 21:26:01]

https://codereview.chromium.org/2755863002/diff/80001/test/inspector/debugger...
File test/inspector/debugger/script-on-after-compile-expected.txt (right):

https://codereview.chromium.org/2755863002/diff/80001/test/inspector/debugger...
test/inspector/debugger/script-on-after-compile-expected.txt:650: scriptId : 61
On 2017/03/16 21:09:21, dgozman wrote:
> You should sanitize this scriptId.

it means that we need to fix our InspectorTest.logMessage method to support
property by indexes..

[allada, 2017-03-16 22:23:58]

done.

https://codereview.chromium.org/2755863002/diff/80001/test/inspector/debugger...
File test/inspector/debugger/script-on-after-compile-expected.txt (right):

https://codereview.chromium.org/2755863002/diff/80001/test/inspector/debugger...
test/inspector/debugger/script-on-after-compile-expected.txt:650: scriptId : 61
On 2017/03/16 21:26:01, kozy wrote:
> On 2017/03/16 21:09:21, dgozman wrote:
> > You should sanitize this scriptId.
> 
> it means that we need to fix our InspectorTest.logMessage method to support
> property by indexes..

Done.

[allada, 2017-03-16 22:24:02]

The CQ bit was checked by allada@chromium.org

[allada, 2017-03-16 22:24:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org, dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2755863002/#ps100001
(title: "fix tests")

[commit-bot: I haz the power, 2017-03-16 22:24:15]

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-03-16 23:34:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489703042714230,
"parent_rev": "82b5c8c9440781976ce23cf8fcabd250ef2bee02", "commit_rev":
"3e1e90dec273a508785c109c6dce816a9aeb3372"}

[commit-bot: I haz the power, 2017-03-16 23:34:18]

Description was changed from

==========
[Devtools] Add stacktrace to protocol for scriptParsed event

Stacktrace data is now passed when scriptParsed event is triggered.

R=kozyatinskiy@chromium.org,dgozman
BUG=chromium:646849
==========

to

==========
[Devtools] Add stacktrace to protocol for scriptParsed event

Stacktrace data is now passed when scriptParsed event is triggered.

R=kozyatinskiy@chromium.org,dgozman
BUG=chromium:646849

Review-Url: https://codereview.chromium.org/2755863002
Cr-Commit-Position: refs/heads/master@{#43879}
Committed:
https://chromium.googlesource.com/v8/v8/+/3e1e90dec273a508785c109c6dce816a9ae...
==========

[commit-bot: I haz the power, 2017-03-16 23:34:19]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/3e1e90dec273a508785c109c6dce816a9ae...