[inspector] don't make v8::debug::Call for breakProgram.

[inspector] don't make v8::debug::Call for breakProgram.

We emulate break by callling breakProgramCallback function in debugger context, we can just use HandleDebugBreak.
It allows us to move all stepping logic to debug.cc later and remove one usage of debugger context.
+ two minor issues fixed, see tests.

BUG=v8:5510
R=yangguo@chromium.org

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

[kozy, 2017-03-08 03:09:48]

Description was changed from

==========
[inspector] don't make v8::debug::Call for breakProgram.

We emulate break by callling breakProgramCallback function in debugger context,
we can just use HandleDebugBreak.
It allows us to move all stepping logic to debug.cc later and remove one usage
of debugger context.

BUG=v8:5510
R=yangguo@chromium.org
==========

to

==========
[inspector] don't make v8::debug::Call for breakProgram.

We emulate break by callling breakProgramCallback function in debugger context,
we can just use HandleDebugBreak.
It allows us to move all stepping logic to debug.cc later and remove one usage
of debugger context.
+ two minor issues fixed, see tests.

BUG=v8:5510
R=yangguo@chromium.org
==========

[kozy, 2017-03-08 03:11:20]

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

[kozy, 2017-03-08 03:11:20]

Yang and Dmitry, please take a look!

It's preparation CL before we can get rid of willExecuteScript and
didExecuteScript instrumentation and implement similar logic internally.

[kozy, 2017-03-08 03:11:26]

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

[commit-bot: I haz the power, 2017-03-08 03:11:32]

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-03-08 03:28:42]

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

[commit-bot: I haz the power, 2017-03-08 03:28:42]

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...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[kozy, 2017-03-08 19:27:45]

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

[commit-bot: I haz the power, 2017-03-08 19:27:56]

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

[dgozman, 2017-03-08 19:59:37]

Could you please better explain what are we trying to achieve?

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug-interfa...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug-interfa...
src/debug/debug-interface.h:106: void HandleDebugBreak(Isolate* isolate);
Not sure what this means. RequestBreakAtSomePointLater? BreakRightNow?

Should we instead request executionState and handle the break in inspector.
Right now it seems like we call debug::HandleDebugBreak just so that it calls us
back through DebugDelegate::BreakProgramRequested, which is not particularly
clear.

[commit-bot: I haz the power, 2017-03-08 20:12:37]

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

[commit-bot: I haz the power, 2017-03-08 20:12:40]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-03-08 20:49:44]

On 2017/03/08 19:59:37, dgozman wrote:
> Could you please better explain what are we trying to achieve?
> 
>
https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug-interfa...
> File src/debug/debug-interface.h (right):
> 
>
https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug-interfa...
> src/debug/debug-interface.h:106: void HandleDebugBreak(Isolate* isolate);
> Not sure what this means. RequestBreakAtSomePointLater? BreakRightNow?
> 
> Should we instead request executionState and handle the break in inspector.
> Right now it seems like we call debug::HandleDebugBreak just so that it calls
us
> back through DebugDelegate::BreakProgramRequested, which is not particularly
> clear.

There are two ways to call BreakProgramRequested from debug.cc:
- ::HandleDebugBreak - used for debugger statement and when break is requested
with RequestDebugBreak (we use it for pause).
- ::Break - used for stepping and breakpoints - debugger patches code to put
this callback in regular JavaScript.

In these methods debugger gets information about hit breakpoints, ignore break
if current locations is muted (contains conditional breakpoint which returns
false), correctly clear stepping, uses blackboxing information to ignore breaks
in blackboxed code and gets execution state and paused context to report back
with BreakProgramRequested callback.

breakProgram on our side pretends to be debugger statement but actually only
gets execution state and paused context to report back and ignore everything
else related to normal break it produces some issues, at least:
- stepping are not cleared after breakProgram,
- hit breakpoints array is empty for Debugger.paused notification,
- no way to mute breakProgram with special conditional breakpoints.
We can fix all of them on inspector side but would we like to do it and maintain
the same logic twice?

Can we move all or part of this logic to inspector?
- stepping and related patching code - should be on V8 side to not expose so
internal details like how to patch code with break,
- blackboxing - prefer to be as closest to execution as possible to reduce
checks time, so V8 side looks like better place,
- managing breakpoints and calculating conditional breakpoints - I think could
be on both sides, maybe inspector even a little better - inspector will just ask
V8 to patch code by this location and will get BreakProgramRequested later, but
we couldn't go forward with this solution as long as we should not completely
break debugger context / mirrors API, this CL just merge related logic in one
place and makes further refactoring easier.

Resume: this CL fixes some issues, makes further big breakpoints refactoring
easier, reduced code complexity by reducing logic duplication, makes other small
step to never intervene stepping related business from inspector side (we
wouldn't need to call ClearStepping after breakProgram), and, yes,  -18 lines of
code without tests.

[Yang, 2017-03-09 16:40:15]

On 2017/03/08 20:49:44, kozy wrote:
> On 2017/03/08 19:59:37, dgozman wrote:
> > Could you please better explain what are we trying to achieve?
> > 
> >
>
https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug-interfa...
> > File src/debug/debug-interface.h (right):
> > 
> >
>
https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug-interfa...
> > src/debug/debug-interface.h:106: void HandleDebugBreak(Isolate* isolate);
> > Not sure what this means. RequestBreakAtSomePointLater? BreakRightNow?
> > 
> > Should we instead request executionState and handle the break in inspector.
> > Right now it seems like we call debug::HandleDebugBreak just so that it
calls
> us
> > back through DebugDelegate::BreakProgramRequested, which is not particularly
> > clear.
> 
> There are two ways to call BreakProgramRequested from debug.cc:
> - ::HandleDebugBreak - used for debugger statement and when break is requested
> with RequestDebugBreak (we use it for pause).
> - ::Break - used for stepping and breakpoints - debugger patches code to put
> this callback in regular JavaScript.
> 
> In these methods debugger gets information about hit breakpoints, ignore break
> if current locations is muted (contains conditional breakpoint which returns
> false), correctly clear stepping, uses blackboxing information to ignore
breaks
> in blackboxed code and gets execution state and paused context to report back
> with BreakProgramRequested callback.
> 
> breakProgram on our side pretends to be debugger statement but actually only
> gets execution state and paused context to report back and ignore everything
> else related to normal break it produces some issues, at least:
> - stepping are not cleared after breakProgram,
> - hit breakpoints array is empty for Debugger.paused notification,
> - no way to mute breakProgram with special conditional breakpoints.
> We can fix all of them on inspector side but would we like to do it and
maintain
> the same logic twice?
> 
> Can we move all or part of this logic to inspector?
> - stepping and related patching code - should be on V8 side to not expose so
> internal details like how to patch code with break,
> - blackboxing - prefer to be as closest to execution as possible to reduce
> checks time, so V8 side looks like better place,
> - managing breakpoints and calculating conditional breakpoints - I think could
> be on both sides, maybe inspector even a little better - inspector will just
ask
> V8 to patch code by this location and will get BreakProgramRequested later,
but
> we couldn't go forward with this solution as long as we should not completely
> break debugger context / mirrors API, this CL just merge related logic in one
> place and makes further refactoring easier.
> 
> Resume: this CL fixes some issues, makes further big breakpoints refactoring
> easier, reduced code complexity by reducing logic duplication, makes other
small
> step to never intervene stepping related business from inspector side (we
> wouldn't need to call ClearStepping after breakProgram), and, yes,  -18 lines
of
> code without tests.

I had a cursory look, but haven't had time to finish due to perf. Will continue
tomorrow. Sorry!

[dgozman, 2017-03-09 23:04:01]

The change from description lgtm modulo some nits. Please separate patches
though.

https://codereview.chromium.org/2738503006/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2738503006/diff/80001/src/api.cc#newcode9109
src/api.cc:9109: void debug::HandleDebugBreak(Isolate* v8_isolate) {
BreakRightNow

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (left):

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug.cc#oldc...
src/debug/debug.cc:963: if (!info->IsSubjectToDebugging() || IsBlackboxed(info))
continue;
Let's do a separate patch for this.

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug.cc#oldc...
src/debug/debug.cc:1746: if (!frame->HasInlinedFrames()) {
Together with this.

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug.h#newco...
src/debug/debug.h:68: enum IgnoreBreakMode { kAllFramesBlackboxed,
kTopFrameBlackboxed };
kIgnoreIfAllFramesBlackboxed, kIgnoreIfTopFrameBlackboxed

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

https://codereview.chromium.org/2738503006/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1312: if (!m_breakReason.empty()) {
Separate patch!

[kozy, 2017-03-10 03:14:55]

all done!
Yang, please take a look.

https://codereview.chromium.org/2738503006/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2738503006/diff/80001/src/api.cc#newcode9109
src/api.cc:9109: void debug::HandleDebugBreak(Isolate* v8_isolate) {
On 2017/03/09 23:04:00, dgozman wrote:
> BreakRightNow

Done.

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug-interfa...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug-interfa...
src/debug/debug-interface.h:106: void HandleDebugBreak(Isolate* isolate);
On 2017/03/08 19:59:36, dgozman wrote:
> Not sure what this means. RequestBreakAtSomePointLater? BreakRightNow?
> 
> Should we instead request executionState and handle the break in inspector.
> Right now it seems like we call debug::HandleDebugBreak just so that it calls
us
> back through DebugDelegate::BreakProgramRequested, which is not particularly
> clear.

Done.

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (left):

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug.cc#oldc...
src/debug/debug.cc:963: if (!info->IsSubjectToDebugging() || IsBlackboxed(info))
continue;
On 2017/03/09 23:04:01, dgozman wrote:
> Let's do a separate patch for this.

Done.

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug.cc#oldc...
src/debug/debug.cc:1746: if (!frame->HasInlinedFrames()) {
On 2017/03/09 23:04:00, dgozman wrote:
> Together with this.

Done.

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2738503006/diff/80001/src/debug/debug.h#newco...
src/debug/debug.h:68: enum IgnoreBreakMode { kAllFramesBlackboxed,
kTopFrameBlackboxed };
On 2017/03/09 23:04:01, dgozman wrote:
> kIgnoreIfAllFramesBlackboxed, kIgnoreIfTopFrameBlackboxed

Done.

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

https://codereview.chromium.org/2738503006/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1312: if (!m_breakReason.empty()) {
On 2017/03/09 23:04:01, dgozman wrote:
> Separate patch!

It could not be separated. We need other part of the CL to make it reproducible.
We never cancel scheduled break currently and breakProgram doesn't affect it.

[kozy, 2017-03-10 03:15:06]

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

[commit-bot: I haz the power, 2017-03-10 03:15:10]

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-03-10 03:45:30]

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

[commit-bot: I haz the power, 2017-03-10 03:45:32]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-03-10 17:23:35]

Yang, I still need your review for this CL :)

[Yang, 2017-03-13 13:02:45]

On 2017/03/10 17:23:35, kozy wrote:
> Yang, I still need your review for this CL :)

Sorry for the delay. LGTM!

[kozy, 2017-03-13 14:27:55]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-03-13 14:27:55]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2738503006/#ps100001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-03-13 14:28:06]

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-13 14:59:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489415275616540,
"parent_rev": "01cc4f9fbbd901e51e03b09089d2371191196946", "commit_rev":
"c418902be4c0d345ec4c562f51819b146cb85be3"}

[commit-bot: I haz the power, 2017-03-13 14:59:49]

Description was changed from

==========
[inspector] don't make v8::debug::Call for breakProgram.

We emulate break by callling breakProgramCallback function in debugger context,
we can just use HandleDebugBreak.
It allows us to move all stepping logic to debug.cc later and remove one usage
of debugger context.
+ two minor issues fixed, see tests.

BUG=v8:5510
R=yangguo@chromium.org
==========

to

==========
[inspector] don't make v8::debug::Call for breakProgram.

We emulate break by callling breakProgramCallback function in debugger context,
we can just use HandleDebugBreak.
It allows us to move all stepping logic to debug.cc later and remove one usage
of debugger context.
+ two minor issues fixed, see tests.

BUG=v8:5510
R=yangguo@chromium.org

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

[commit-bot: I haz the power, 2017-03-13 14:59:50]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/c418902be4c0d345ec4c562f51819b146cb...