[inspector] implemented blackboxing inside v8

[inspector] implemented blackboxing inside v8

V8 has internal mechanism to ignore steps and breaks inside internal scripts, in this CL it's reused for blackboxing implementation.
Advantages:
- much faster blackboxing implementation (before we at least wrap and collect current call stack for each step),
- get rid of StepFrame action and potential pause in blackboxed code after N StepFrame steps,
- simplification of debugger agent logic.
Disadvtanges:
- currently when user was paused in blackboxed code (e.g. on breakpoint) and then makes step action, debugger ignores blackboxed state of the script and allows to use step actions as usual - this behavior is regressed, we still able to support it on frontend side.

Current state and proposed changes for blackboxing: https://docs.google.com/document/d/1hnzaXPAN8_QC5ENxIgxgMNDbXLraM_OXT73rAyijTF8/edit?usp=sharing

BUG=v8:5842
R=yangguo@chromium.org,dgozman@chromium.org,alph@chromium.org

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

[kozy, 2017-01-18 16:28:32]

Description was changed from

==========
[inspector] framework debugging tests

BUG=v8:5842
==========

to

==========
[inspector] implemented blackboxing inside v8

V8 has internal mechanism to ignore steps and breaks inside internal scripts, in
this CL it's reused for blackboxing implementation.
Advantages:
- much faster blackboxing implementation (before we at least wrap and collect
current call frame for each step),
- get rid of StepFrame action and potential pause in blackboxed code after N
StepFrame steps,
- simplification of debugger agent logic.
Disadvtanges:
- currently when user was paused in blackboxed code (e.g. on breakpoint) and
then makes step action, debugger ignores blackboxed state of the script and
allows to use step actions as usual - this behavior is regressed, we still able
to support it on frontend side.

Current state and proposed changes for blackboxing:
https://docs.google.com/document/d/1hnzaXPAN8_QC5ENxIgxgMNDbXLraM_OXT73rAyijT...

BUG=v8:5842
R=yangguo@chromium.org,dgozman@chromium.org,alph@chromium.org
==========

[kozy, 2017-01-18 16:28:32]

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

[kozy, 2017-01-18 16:29:22]

Please take a look!

[Yang, 2017-01-19 13:55:21]

On 2017/01/18 16:29:22, kozyatinskiy wrote:
> Please take a look!

src/debug part is looking good. Some comments.

[Yang, 2017-01-19 13:55:35]

oh yeah. the actual comments.

https://codereview.chromium.org/2633803002/diff/100001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2633803002/diff/100001/src/compiler.cc#newcod...
src/compiler.cc:1091: isolate->debug()->OnNewSharedFunctionInfo(result);
I wonder whether it is easier to keep track of all the locations, or lazily
compute it, and cache it on the shared function info. We would need two bits on
the shared function info, one to cache, and one to state whether the cache is
up-to-date.

The advantage is that you would not have to keep track of all the places we add
new shared function info, and would not need to eagerly compute the blackboxing
state. We do the same thing for side-effect-free debug evaluate. See
SharedFunctionInfo::HasNoSideEffect.

When you reset the black box state, you would simply need to iterate all shared
function infos and reset the first bit. That's probably a lot less expensive
than computing the black box state for all of them at once.

https://codereview.chromium.org/2633803002/diff/100001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2633803002/diff/100001/src/debug/debug.cc#new...
src/debug/debug.cc:866: if (shared->debug_is_blackboxed()) return;
Do we need to ensure debug info if it's blackboxed?
Also, we definitely need to deopt the function. Otherwise we could assume the
wrong black box state for inlined functions. I'd just put this on the same line
as the IsSubjectToDebugging check above.

https://codereview.chromium.org/2633803002/diff/100001/src/debug/debug.cc#new...
src/debug/debug.cc:1765: if (!it.done() &&
it.frame()->function()->shared()->debug_is_blackboxed())
please add {} around if-statement body if we have a line break. We usually have
this in the v8 code base. Also below.

https://codereview.chromium.org/2633803002/diff/100001/src/debug/debug.cc#new...
src/debug/debug.cc:1768: if (!it.done() && IsMutedAtCurrentLocation(it.frame()))
return;
can we merge these two redundant checks?

[kozy, 2017-01-19 16:08:38]

all done, please take another look!

https://codereview.chromium.org/2633803002/diff/100001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2633803002/diff/100001/src/compiler.cc#newcod...
src/compiler.cc:1091: isolate->debug()->OnNewSharedFunctionInfo(result);
On 2017/01/19 13:55:35, Yang wrote:
> I wonder whether it is easier to keep track of all the locations, or lazily
> compute it, and cache it on the shared function info. We would need two bits
on
> the shared function info, one to cache, and one to state whether the cache is
> up-to-date.
> 
> The advantage is that you would not have to keep track of all the places we
add
> new shared function info, and would not need to eagerly compute the
blackboxing
> state. We do the same thing for side-effect-free debug evaluate. See
> SharedFunctionInfo::HasNoSideEffect.
> 
> When you reset the black box state, you would simply need to iterate all
shared
> function infos and reset the first bit. That's probably a lot less expensive
> than computing the black box state for all of them at once.

Done.

https://codereview.chromium.org/2633803002/diff/100001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2633803002/diff/100001/src/debug/debug.cc#new...
src/debug/debug.cc:866: if (shared->debug_is_blackboxed()) return;
On 2017/01/19 13:55:35, Yang wrote:
> Do we need to ensure debug info if it's blackboxed?
> Also, we definitely need to deopt the function. Otherwise we could assume the
> wrong black box state for inlined functions. I'd just put this on the same
line
> as the IsSubjectToDebugging check above.

Done.

https://codereview.chromium.org/2633803002/diff/100001/src/debug/debug.cc#new...
src/debug/debug.cc:1765: if (!it.done() &&
it.frame()->function()->shared()->debug_is_blackboxed())
On 2017/01/19 13:55:35, Yang wrote:
> please add {} around if-statement body if we have a line break. We usually
have
> this in the v8 code base. Also below.

Done.

https://codereview.chromium.org/2633803002/diff/100001/src/debug/debug.cc#new...
src/debug/debug.cc:1768: if (!it.done() && IsMutedAtCurrentLocation(it.frame()))
return;
On 2017/01/19 13:55:35, Yang wrote:
> can we merge these two redundant checks?

Done.

[kozy, 2017-01-19 16:09:55]

Description was changed from

==========
[inspector] implemented blackboxing inside v8

V8 has internal mechanism to ignore steps and breaks inside internal scripts, in
this CL it's reused for blackboxing implementation.
Advantages:
- much faster blackboxing implementation (before we at least wrap and collect
current call frame for each step),
- get rid of StepFrame action and potential pause in blackboxed code after N
StepFrame steps,
- simplification of debugger agent logic.
Disadvtanges:
- currently when user was paused in blackboxed code (e.g. on breakpoint) and
then makes step action, debugger ignores blackboxed state of the script and
allows to use step actions as usual - this behavior is regressed, we still able
to support it on frontend side.

Current state and proposed changes for blackboxing:
https://docs.google.com/document/d/1hnzaXPAN8_QC5ENxIgxgMNDbXLraM_OXT73rAyijT...

BUG=v8:5842
R=yangguo@chromium.org,dgozman@chromium.org,alph@chromium.org
==========

to

==========
[inspector] implemented blackboxing inside v8

V8 has internal mechanism to ignore steps and breaks inside internal scripts, in
this CL it's reused for blackboxing implementation.
Advantages:
- much faster blackboxing implementation (before we at least wrap and collect
current call stack for each step),
- get rid of StepFrame action and potential pause in blackboxed code after N
StepFrame steps,
- simplification of debugger agent logic.
Disadvtanges:
- currently when user was paused in blackboxed code (e.g. on breakpoint) and
then makes step action, debugger ignores blackboxed state of the script and
allows to use step actions as usual - this behavior is regressed, we still able
to support it on frontend side.

Current state and proposed changes for blackboxing:
https://docs.google.com/document/d/1hnzaXPAN8_QC5ENxIgxgMNDbXLraM_OXT73rAyijT...

BUG=v8:5842
R=yangguo@chromium.org,dgozman@chromium.org,alph@chromium.org
==========

[dgozman, 2017-01-19 21:49:15]

https://codereview.chromium.org/2633803002/diff/140001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2633803002/diff/140001/src/api.cc#newcode9136
src/api.cc:9136: void debug::Script::BlackboxStateChanged() const {
Why doesn't this get a bool parameter |blackboxed|?

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
src/debug/debug-interface.h:127: void BlackboxStateChanged() const;
Does it make sense to push ranges here instead of asking via callback?

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
src/debug/debug-interface.h:171: void SetIsBlackboxedCallback(Isolate* isolate,
IsBlackboxedCallback callback,
Make this a part of DebugEventListener?

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
src/debug/debug-interface.h:174: bool HasUserFrameOnStack(Isolate* isolate);
- Let's move this closer to other functions (e.g.next to PrepareStep).
- HasNonBlackboxedFrameOnStack.

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

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug.cc#new...
src/debug/debug.cc:967: it.frame()->function()->shared()->DebugIsBlackboxed()))
{
Should we make IsSubjectToDebugging() to check for blackboxed? That could be
easier to maintain.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:459: return
std::distance(ranges.begin(), itStartRange) % 2 ||
Let's be safe instead and blackbox only if script is continuously blackboxed
between start and end.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:770:
m_state->setString(DebuggerAgentState::blackboxPattern, pattern);
Why don't we notify scripts about blackboxStateChanged here? Let's move it from
the setBlackboxPattern.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:1060: if (!isOOMBreak && (!topCallFrame
|| m_skipAllPauses)) return false;
- Let's check for m_skipAllPauses before retrieving callframes.
- Is (!topCallFrame) even a case now? Can we stop in "V8 internal scripts"?

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:1151: if
(m_debugger->hasUserFrameOnStack())
You already check for that in breakProgram.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:1182: it.second->blackboxStateChanged();
I don't really like that we are talking to v8::debug directly here. So far, we
only did that in V8Debugger, right?

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
File src/inspector/v8-debugger-agent-impl.h (right):

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.h:132: bool
didPause(v8::Local<v8::Context>, v8::Local<v8::Value> exception,
shouldPause

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

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:485: bool result =
result -> shouldPause

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:601:
debugger->m_inspector->enabledDebuggerAgentForGroup(contextId);
again?

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:603: return
agent->isBlackboxed(String16::fromInteger(script->Id()), start, end);
Converting to string here is unfortunate.

[dgozman, 2017-01-19 22:34:20]

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
src/debug/debug-interface.h:127: void BlackboxStateChanged() const;
- ResetBlackboxedStateCache
- Make it a separate function and pass script to it.

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

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug.cc#new...
src/debug/debug.cc:1914: int line = Script::GetLineNumber(script,
source_position);
GetPositionInfo

[kozy, 2017-01-20 02:23:50]

Patchset #9 (id:160001) has been deleted

[kozy, 2017-01-20 02:30:48]

Patchset #9 (id:180001) has been deleted

[kozy, 2017-01-20 02:32:37]

all done, please take another look.

https://codereview.chromium.org/2633803002/diff/140001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2633803002/diff/140001/src/api.cc#newcode9136
src/api.cc:9136: void debug::Script::BlackboxStateChanged() const {
On 2017/01/19 21:49:14, dgozman wrote:
> Why doesn't this get a bool parameter |blackboxed|?

After offline discussion we decided to try to introduce least possible new
structures and new fields internally in V8.
Advantages:
- API without any JS allocation,
- blackboxed state logic computation is located inside of V8DebuggerAgentImpl.
Disadvantage:
- two methods in API instead of one.

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
src/debug/debug-interface.h:127: void BlackboxStateChanged() const;
On 2017/01/19 21:49:14, dgozman wrote:
> Does it make sense to push ranges here instead of asking via callback?

Acknowledged.

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
src/debug/debug-interface.h:127: void BlackboxStateChanged() const;
On 2017/01/19 22:34:20, dgozman wrote:
> - ResetBlackboxedStateCache
> - Make it a separate function and pass script to it.

Done.

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
src/debug/debug-interface.h:171: void SetIsBlackboxedCallback(Isolate* isolate,
IsBlackboxedCallback callback,
On 2017/01/19 21:49:14, dgozman wrote:
> Make this a part of DebugEventListener?

DebugEventListener -> DebugDelegate and done.

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
src/debug/debug-interface.h:174: bool HasUserFrameOnStack(Isolate* isolate);
On 2017/01/19 21:49:14, dgozman wrote:
> - Let's move this closer to other functions (e.g.next to PrepareStep).
> - HasNonBlackboxedFrameOnStack.

Done.

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

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug.cc#new...
src/debug/debug.cc:967: it.frame()->function()->shared()->DebugIsBlackboxed()))
{
On 2017/01/19 21:49:14, dgozman wrote:
> Should we make IsSubjectToDebugging() to check for blackboxed? That could be
> easier to maintain.

Acknowledged.

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug.cc#new...
src/debug/debug.cc:1914: int line = Script::GetLineNumber(script,
source_position);
On 2017/01/19 22:34:20, dgozman wrote:
> GetPositionInfo

Done.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:459: return
std::distance(ranges.begin(), itStartRange) % 2 ||
On 2017/01/19 21:49:14, dgozman wrote:
> Let's be safe instead and blackbox only if script is continuously blackboxed
> between start and end.

Done.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:770:
m_state->setString(DebuggerAgentState::blackboxPattern, pattern);
On 2017/01/19 21:49:14, dgozman wrote:
> Why don't we notify scripts about blackboxStateChanged here? Let's move it
from
> the setBlackboxPattern.

Done.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:1060: if (!isOOMBreak && (!topCallFrame
|| m_skipAllPauses)) return false;
On 2017/01/19 21:49:14, dgozman wrote:
> - Let's check for m_skipAllPauses before retrieving callframes.
> - Is (!topCallFrame) even a case now? Can we stop in "V8 internal scripts"?

first done, second in follow up.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:1151: if
(m_debugger->hasUserFrameOnStack())
On 2017/01/19 21:49:14, dgozman wrote:
> You already check for that in breakProgram.

Done.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:1182: it.second->blackboxStateChanged();
On 2017/01/19 21:49:14, dgozman wrote:
> I don't really like that we are talking to v8::debug directly here. So far, we
> only did that in V8Debugger, right?

Acknowledged.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
File src/inspector/v8-debugger-agent-impl.h (right):

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.h:132: bool
didPause(v8::Local<v8::Context>, v8::Local<v8::Value> exception,
On 2017/01/19 21:49:14, dgozman wrote:
> shouldPause

but I think main purpose of this method: send notification to frontend about
pause, I prefer to not change this code and just move m_skipAllPauses (as you
proposed) to debug.cc in follow up.

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

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:485: bool result =
On 2017/01/19 21:49:14, dgozman wrote:
> result -> shouldPause

Done.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:601:
debugger->m_inspector->enabledDebuggerAgentForGroup(contextId);
On 2017/01/19 21:49:15, dgozman wrote:
> again?

extracted to not repeat again.

https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:603: return
agent->isBlackboxed(String16::fromInteger(script->Id()), start, end);
On 2017/01/19 21:49:15, dgozman wrote:
> Converting to string here is unfortunate.

Acknowledged.

[Yang, 2017-01-20 08:32:50]

On 2017/01/20 02:32:37, kozyatinskiy wrote:
> all done, please take another look.
> 
> https://codereview.chromium.org/2633803002/diff/140001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/2633803002/diff/140001/src/api.cc#newcode9136
> src/api.cc:9136: void debug::Script::BlackboxStateChanged() const {
> On 2017/01/19 21:49:14, dgozman wrote:
> > Why doesn't this get a bool parameter |blackboxed|?
> 
> After offline discussion we decided to try to introduce least possible new
> structures and new fields internally in V8.
> Advantages:
> - API without any JS allocation,
> - blackboxed state logic computation is located inside of V8DebuggerAgentImpl.
> Disadvantage:
> - two methods in API instead of one.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
> File src/debug/debug-interface.h (right):
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
> src/debug/debug-interface.h:127: void BlackboxStateChanged() const;
> On 2017/01/19 21:49:14, dgozman wrote:
> > Does it make sense to push ranges here instead of asking via callback?
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
> src/debug/debug-interface.h:127: void BlackboxStateChanged() const;
> On 2017/01/19 22:34:20, dgozman wrote:
> > - ResetBlackboxedStateCache
> > - Make it a separate function and pass script to it.
> 
> Done.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
> src/debug/debug-interface.h:171: void SetIsBlackboxedCallback(Isolate*
isolate,
> IsBlackboxedCallback callback,
> On 2017/01/19 21:49:14, dgozman wrote:
> > Make this a part of DebugEventListener?
> 
> DebugEventListener -> DebugDelegate and done.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug-interf...
> src/debug/debug-interface.h:174: bool HasUserFrameOnStack(Isolate* isolate);
> On 2017/01/19 21:49:14, dgozman wrote:
> > - Let's move this closer to other functions (e.g.next to PrepareStep).
> > - HasNonBlackboxedFrameOnStack.
> 
> Done.
> 
> https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug.cc#new...
> src/debug/debug.cc:967:
it.frame()->function()->shared()->DebugIsBlackboxed()))
> {
> On 2017/01/19 21:49:14, dgozman wrote:
> > Should we make IsSubjectToDebugging() to check for blackboxed? That could be
> > easier to maintain.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug.cc#new...
> src/debug/debug.cc:1914: int line = Script::GetLineNumber(script,
> source_position);
> On 2017/01/19 22:34:20, dgozman wrote:
> > GetPositionInfo
> 
> Done.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> File src/inspector/v8-debugger-agent-impl.cc (right):
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> src/inspector/v8-debugger-agent-impl.cc:459: return
> std::distance(ranges.begin(), itStartRange) % 2 ||
> On 2017/01/19 21:49:14, dgozman wrote:
> > Let's be safe instead and blackbox only if script is continuously blackboxed
> > between start and end.
> 
> Done.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> src/inspector/v8-debugger-agent-impl.cc:770:
> m_state->setString(DebuggerAgentState::blackboxPattern, pattern);
> On 2017/01/19 21:49:14, dgozman wrote:
> > Why don't we notify scripts about blackboxStateChanged here? Let's move it
> from
> > the setBlackboxPattern.
> 
> Done.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> src/inspector/v8-debugger-agent-impl.cc:1060: if (!isOOMBreak &&
(!topCallFrame
> || m_skipAllPauses)) return false;
> On 2017/01/19 21:49:14, dgozman wrote:
> > - Let's check for m_skipAllPauses before retrieving callframes.
> > - Is (!topCallFrame) even a case now? Can we stop in "V8 internal scripts"?
> 
> first done, second in follow up.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> src/inspector/v8-debugger-agent-impl.cc:1151: if
> (m_debugger->hasUserFrameOnStack())
> On 2017/01/19 21:49:14, dgozman wrote:
> > You already check for that in breakProgram.
> 
> Done.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> src/inspector/v8-debugger-agent-impl.cc:1182:
it.second->blackboxStateChanged();
> On 2017/01/19 21:49:14, dgozman wrote:
> > I don't really like that we are talking to v8::debug directly here. So far,
we
> > only did that in V8Debugger, right?
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> File src/inspector/v8-debugger-agent-impl.h (right):
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> src/inspector/v8-debugger-agent-impl.h:132: bool
> didPause(v8::Local<v8::Context>, v8::Local<v8::Value> exception,
> On 2017/01/19 21:49:14, dgozman wrote:
> > shouldPause
> 
> but I think main purpose of this method: send notification to frontend about
> pause, I prefer to not change this code and just move m_skipAllPauses (as you
> proposed) to debug.cc in follow up.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> File src/inspector/v8-debugger.cc (right):
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> src/inspector/v8-debugger.cc:485: bool result =
> On 2017/01/19 21:49:14, dgozman wrote:
> > result -> shouldPause
> 
> Done.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> src/inspector/v8-debugger.cc:601:
> debugger->m_inspector->enabledDebuggerAgentForGroup(contextId);
> On 2017/01/19 21:49:15, dgozman wrote:
> > again?
> 
> extracted to not repeat again.
> 
>
https://codereview.chromium.org/2633803002/diff/140001/src/inspector/v8-debug...
> src/inspector/v8-debugger.cc:603: return
> agent->isBlackboxed(String16::fromInteger(script->Id()), start, end);
> On 2017/01/19 21:49:15, dgozman wrote:
> > Converting to string here is unfortunate.
> 
> Acknowledged.

non-inspector part lgtm!

[Yang, 2017-01-20 09:30:48]

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

https://codereview.chromium.org/2633803002/diff/140001/src/debug/debug.cc#new...
src/debug/debug.cc:967: it.frame()->function()->shared()->DebugIsBlackboxed()))
{
On 2017/01/19 21:49:14, dgozman wrote:
> Should we make IsSubjectToDebugging() to check for blackboxed? That could be
> easier to maintain.

I think the reason we don't is because we want blackboxed functions to still
show up in the stack trace.

[kozy, 2017-01-20 17:56:28]

One more test is added, Dmitry please take a look.

[kozy, 2017-01-20 22:10:55]

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

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

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-20 22:13:51]

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

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

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_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19715)
  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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/19615)

[kozy, 2017-01-20 22:37:55]

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

[commit-bot: I haz the power, 2017-01-20 22:37:56]

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-20 23:00:26]

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

[commit-bot: I haz the power, 2017-01-20 23:00:27]

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/21267)
  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...)

[kozy, 2017-01-20 23:22:58]

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

[commit-bot: I haz the power, 2017-01-20 23:23: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-01-20 23:43:50]

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

[commit-bot: I haz the power, 2017-01-20 23:43:51]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/16105)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21352)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[kozy, 2017-01-21 00:12:00]

Yang, I need your help with this, after adding new flags related to no side
effects, there are no enough space for two new bits for blackboxing in
SharedFunctionInfo flags on ia32 architecture :(

What could we do here? Could we introduce additional field for debugger only
flags? Or increase size of field on not x64 architectures?

[Yang, 2017-01-21 05:39:05]

On 2017/01/21 00:12:00, kozyatinskiy wrote:
> Yang, I need your help with this, after adding new flags related to no side
> effects, there are no enough space for two new bits for blackboxing in
> SharedFunctionInfo flags on ia32 architecture :(
> 
> What could we do here? Could we introduce additional field for debugger only
> flags? Or increase size of field on not x64 architectures?

Since this is related to setting breakpoints, how about we use a special value
for the debug info to represent this info? Undefined means we haven't computed
yet. Having a debug info obviously means being able to set breaks. Having a
Smi(kBlackboxed) means being blackboxed. WDYT?

[kozy, 2017-01-21 16:20:47]

On 2017/01/21 05:39:05, Yang wrote:
> On 2017/01/21 00:12:00, kozyatinskiy wrote:
> > Yang, I need your help with this, after adding new flags related to no side
> > effects, there are no enough space for two new bits for blackboxing in
> > SharedFunctionInfo flags on ia32 architecture :(
> > 
> > What could we do here? Could we introduce additional field for debugger only
> > flags? Or increase size of field on not x64 architectures?
> 
> Since this is related to setting breakpoints, how about we use a special value
> for the debug info to represent this info? Undefined means we haven't computed
> yet. Having a debug info obviously means being able to set breaks. Having a
> Smi(kBlackboxed) means being blackboxed. WDYT?

It sounds good to me but we still can set breakpoint in blackboxed script. It's
current behavior, we d like to change it but we need a way to easily revert it
back - I probably can use described technique and add additional flag to
DebugInfo.

[Yang, 2017-01-21 16:22:27]

On 2017/01/21 16:20:47, kozyatinskiy wrote:
> On 2017/01/21 05:39:05, Yang wrote:
> > On 2017/01/21 00:12:00, kozyatinskiy wrote:
> > > Yang, I need your help with this, after adding new flags related to no
side
> > > effects, there are no enough space for two new bits for blackboxing in
> > > SharedFunctionInfo flags on ia32 architecture :(
> > > 
> > > What could we do here? Could we introduce additional field for debugger
only
> > > flags? Or increase size of field on not x64 architectures?
> > 
> > Since this is related to setting breakpoints, how about we use a special
value
> > for the debug info to represent this info? Undefined means we haven't
computed
> > yet. Having a debug info obviously means being able to set breaks. Having a
> > Smi(kBlackboxed) means being blackboxed. WDYT?
> 
> It sounds good to me but we still can set breakpoint in blackboxed script.
It's
> current behavior, we d like to change it but we need a way to easily revert it
> back - I probably can use described technique and add additional flag to
> DebugInfo.

I can also take a look at this on monday to figure out an alternative.

[Yang, 2017-01-21 16:29:59]

On 2017/01/21 16:22:27, Yang wrote:
> On 2017/01/21 16:20:47, kozyatinskiy wrote:
> > On 2017/01/21 05:39:05, Yang wrote:
> > > On 2017/01/21 00:12:00, kozyatinskiy wrote:
> > > > Yang, I need your help with this, after adding new flags related to no
> side
> > > > effects, there are no enough space for two new bits for blackboxing in
> > > > SharedFunctionInfo flags on ia32 architecture :(
> > > > 
> > > > What could we do here? Could we introduce additional field for debugger
> only
> > > > flags? Or increase size of field on not x64 architectures?
> > > 
> > > Since this is related to setting breakpoints, how about we use a special
> value
> > > for the debug info to represent this info? Undefined means we haven't
> computed
> > > yet. Having a debug info obviously means being able to set breaks. Having
a
> > > Smi(kBlackboxed) means being blackboxed. WDYT?
> > 
> > It sounds good to me but we still can set breakpoint in blackboxed script.
> It's
> > current behavior, we d like to change it but we need a way to easily revert
it
> > back - I probably can use described technique and add additional flag to
> > DebugInfo.
> 
> I can also take a look at this on monday to figure out an alternative.

in the compiler hints enum, there is a kUnused. Can we remove that to make
space?

[kozy, 2017-01-21 23:27:23]

On 2017/01/21 16:29:59, Yang wrote:
> On 2017/01/21 16:22:27, Yang wrote:
> > On 2017/01/21 16:20:47, kozyatinskiy wrote:
> > > On 2017/01/21 05:39:05, Yang wrote:
> > > > On 2017/01/21 00:12:00, kozyatinskiy wrote:
> > > > > Yang, I need your help with this, after adding new flags related to no
> > side
> > > > > effects, there are no enough space for two new bits for blackboxing in
> > > > > SharedFunctionInfo flags on ia32 architecture :(
> > > > > 
> > > > > What could we do here? Could we introduce additional field for
debugger
> > only
> > > > > flags? Or increase size of field on not x64 architectures?
> > > > 
> > > > Since this is related to setting breakpoints, how about we use a special
> > value
> > > > for the debug info to represent this info? Undefined means we haven't
> > computed
> > > > yet. Having a debug info obviously means being able to set breaks.
Having
> a
> > > > Smi(kBlackboxed) means being blackboxed. WDYT?
> > > 
> > > It sounds good to me but we still can set breakpoint in blackboxed script.
> > It's
> > > current behavior, we d like to change it but we need a way to easily
revert
> it
> > > back - I probably can use described technique and add additional flag to
> > > DebugInfo.
> > 
> > I can also take a look at this on monday to figure out an alternative.
> 
> in the compiler hints enum, there is a kUnused. Can we remove that to make
> space?

In last patchset I removed it: STATIC_ASSERT doesn't complain but "Check failed:
Smi::IsValid(value)." complains and crashes.

[kozy, 2017-01-21 23:37:24]

On 2017/01/21 16:22:27, Yang wrote:
> I can also take a look at this on monday to figure out an alternative.

I found that SharedFunctionInfo contains start_position_and_type int, which uses
two bits for type. We probably can use two bits from end_position for
blackboxing in similar way. WDYT?

[kozy, 2017-01-22 00:04:36]

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

[commit-bot: I haz the power, 2017-01-22 00:04:43]

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-22 00: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-22 00:19:10]

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-22 00:19:14]

Patchset #13 (id:280001) has been deleted

[kozy, 2017-01-22 00:21:50]

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

[commit-bot: I haz the power, 2017-01-22 00:22:00]

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-22 00:55:31]

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

[commit-bot: I haz the power, 2017-01-22 00:55:32]

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

[kozy, 2017-01-22 05:29:30]

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

[commit-bot: I haz the power, 2017-01-22 05:29:35]

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-22 06:07:33]

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

[commit-bot: I haz the power, 2017-01-22 06:07:34]

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

[Yang, 2017-01-22 06:09:04]

On 2017/01/21 23:37:24, kozyatinskiy wrote:
> On 2017/01/21 16:22:27, Yang wrote:
> > I can also take a look at this on monday to figure out an alternative.
> 
> I found that SharedFunctionInfo contains start_position_and_type int, which
uses
> two bits for type. We probably can use two bits from end_position for
> blackboxing in similar way. WDYT?

you have a 31bit unsigned int to store, but a smi is a 31bit signed int. I think
with a reinterpret cast we could make it work

[Yang, 2017-01-22 07:01:51]

On 2017/01/22 06:09:04, Yang wrote:
> On 2017/01/21 23:37:24, kozyatinskiy wrote:
> > On 2017/01/21 16:22:27, Yang wrote:
> > > I can also take a look at this on monday to figure out an alternative.
> > 
> > I found that SharedFunctionInfo contains start_position_and_type int, which
> uses
> > two bits for type. We probably can use two bits from end_position for
> > blackboxing in similar way. WDYT?
> 
> you have a 31bit unsigned int to store, but a smi is a 31bit signed int. I
think
> with a reinterpret cast we could make it work

I thought a bit about this. to be future proof, let's add a smi field to debug
info for flags. In case the debug info is not allocated yet, the smi is stored
directly on the shared function info in place of the debug info. wdyt?

I'll work on a CL for this on Monday.

[kozy, 2017-01-22 19:19:48]

On 2017/01/22 07:01:51, Yang wrote:
> On 2017/01/22 06:09:04, Yang wrote:
> 
> I thought a bit about this. to be future proof, let's add a smi field to debug
> info for flags. In case the debug info is not allocated yet, the smi is stored
> directly on the shared function info in place of the debug info. wdyt?
> 
> I'll work on a CL for this on Monday.

Ok, sounds good to me. Then I just will wait with this CL.

[Yang, 2017-01-23 08:48:29]

This should make enough room for the bits:
https://codereview.chromium.org/2649873002/#

Also consider comments below.

https://codereview.chromium.org/2633803002/diff/340001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2633803002/diff/340001/src/objects.cc#newcode...
src/objects.cc:13456: set_computed_debug_is_blackboxed(true);
Oh yeah, this is not GC-safe. set_debug_is_blackboxed refers to 'this'. however,
since debug()->IsBlackboxed() can trigger GC, 'this' can become invalid if GC
moves the shared function info.

Can you make this method static with this signature?
static bool SharedFunctionInfo::DebugIsBlackboxed(Handle<SharedFunctionInfo>)

Then again, if we did that, why not just call Debug::IsBlackboxed from where we
need it and only store the bits on the shared function info?

[kozy, 2017-01-23 20:26:35]

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

[commit-bot: I haz the power, 2017-01-23 20:26:45]

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-23 20:28:01]

rebased on top of "flags" CL, thank you for your help.

Dmitry, please take a look.

https://codereview.chromium.org/2633803002/diff/340001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2633803002/diff/340001/src/objects.cc#newcode...
src/objects.cc:13456: set_computed_debug_is_blackboxed(true);
On 2017/01/23 08:48:29, Yang wrote:
> Oh yeah, this is not GC-safe. set_debug_is_blackboxed refers to 'this'.
however,
> since debug()->IsBlackboxed() can trigger GC, 'this' can become invalid if GC
> moves the shared function info.
> 
> Can you make this method static with this signature?
> static bool SharedFunctionInfo::DebugIsBlackboxed(Handle<SharedFunctionInfo>)
> 
> Then again, if we did that, why not just call Debug::IsBlackboxed from where
we
> need it and only store the bits on the shared function info?

Thank you! Done. I'll try to be more accurate about possible gc next time.

[commit-bot: I haz the power, 2017-01-23 21:06:07]

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

[commit-bot: I haz the power, 2017-01-23 21:06:08]

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

[kozy, 2017-01-23 21:52:35]

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

[commit-bot: I haz the power, 2017-01-23 21:52: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, 2017-01-23 22:24:50]

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

[commit-bot: I haz the power, 2017-01-23 22:24:52]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-01-24 00:12:08]

lgtm

https://codereview.chromium.org/2633803002/diff/360001/src/debug/debug-interf...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2633803002/diff/360001/src/debug/debug-interf...
src/debug/debug-interface.h:164: virtual bool
IsBlackboxed(v8::Local<debug::Script> script,
IsFunctionBlackboxed

https://codereview.chromium.org/2633803002/diff/360001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2633803002/diff/360001/src/debug/debug.cc#new...
src/debug/debug.cc:1025: if (step_action == StepNext && IsBlackboxed(shared)) {
nit: fits one line

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:457: return std::distance(itStartRange,
itEndRange) == 0 &&
itStartRange == itEndRange

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:1000:
scriptRef->resetBlackboxedStateCache();
This is a new script - do we reset to account for script reuse? I think we
disable script reuse when debugging, so maybe we don't need this? If we really
need it, let's add a comment why.

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
File src/inspector/v8-debugger-agent-impl.h (right):

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.h:33: enum SkipPauseRequest {
RequestNoSkip, RequestContinue };
Remove.

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.h:140: bool isBlackboxed(const String16&
scriptId, const v8::debug::Location& start,
isFunctionBlackboxed

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

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:509: namespace {
nit: put all helpers in the namespace at the top.

[kozy, 2017-01-24 01:11:52]

thank you, all done!

https://codereview.chromium.org/2633803002/diff/360001/src/debug/debug-interf...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2633803002/diff/360001/src/debug/debug-interf...
src/debug/debug-interface.h:164: virtual bool
IsBlackboxed(v8::Local<debug::Script> script,
On 2017/01/24 00:12:08, dgozman wrote:
> IsFunctionBlackboxed

Done.

https://codereview.chromium.org/2633803002/diff/360001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2633803002/diff/360001/src/debug/debug.cc#new...
src/debug/debug.cc:1025: if (step_action == StepNext && IsBlackboxed(shared)) {
On 2017/01/24 00:12:08, dgozman wrote:
> nit: fits one line

Done.

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:457: return std::distance(itStartRange,
itEndRange) == 0 &&
On 2017/01/24 00:12:08, dgozman wrote:
> itStartRange == itEndRange

Done.

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:1000:
scriptRef->resetBlackboxedStateCache();
On 2017/01/24 00:12:08, dgozman wrote:
> This is a new script - do we reset to account for script reuse? I think we
> disable script reuse when debugging, so maybe we don't need this? If we really
> need it, let's add a comment why.

Done.

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
File src/inspector/v8-debugger-agent-impl.h (right):

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.h:33: enum SkipPauseRequest {
RequestNoSkip, RequestContinue };
On 2017/01/24 00:12:08, dgozman wrote:
> Remove.

Done.

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.h:140: bool isBlackboxed(const String16&
scriptId, const v8::debug::Location& start,
On 2017/01/24 00:12:08, dgozman wrote:
> isFunctionBlackboxed

Done.

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

https://codereview.chromium.org/2633803002/diff/360001/src/inspector/v8-debug...
src/inspector/v8-debugger.cc:509: namespace {
On 2017/01/24 00:12:08, dgozman wrote:
> nit: put all helpers in the namespace at the top.

Done.

[kozy, 2017-01-24 01:15:05]

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

[commit-bot: I haz the power, 2017-01-24 01:15:12]

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-24 01:42:09]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[kozy, 2017-01-24 01:42:15]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-01-24 01:42:15]

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

[commit-bot: I haz the power, 2017-01-24 01:42:26]

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 01:50:26]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1485222135257920,
"parent_rev": "d90e6e12e68c93230c18454eafa981e5566b2128", "commit_rev":
"ac50c79a3e07cbe3c420ce55c1a3e0e3d5bd6454"}

[commit-bot: I haz the power, 2017-01-24 01:50:32]

Description was changed from

==========
[inspector] implemented blackboxing inside v8

V8 has internal mechanism to ignore steps and breaks inside internal scripts, in
this CL it's reused for blackboxing implementation.
Advantages:
- much faster blackboxing implementation (before we at least wrap and collect
current call stack for each step),
- get rid of StepFrame action and potential pause in blackboxed code after N
StepFrame steps,
- simplification of debugger agent logic.
Disadvtanges:
- currently when user was paused in blackboxed code (e.g. on breakpoint) and
then makes step action, debugger ignores blackboxed state of the script and
allows to use step actions as usual - this behavior is regressed, we still able
to support it on frontend side.

Current state and proposed changes for blackboxing:
https://docs.google.com/document/d/1hnzaXPAN8_QC5ENxIgxgMNDbXLraM_OXT73rAyijT...

BUG=v8:5842
R=yangguo@chromium.org,dgozman@chromium.org,alph@chromium.org
==========

to

==========
[inspector] implemented blackboxing inside v8

V8 has internal mechanism to ignore steps and breaks inside internal scripts, in
this CL it's reused for blackboxing implementation.
Advantages:
- much faster blackboxing implementation (before we at least wrap and collect
current call stack for each step),
- get rid of StepFrame action and potential pause in blackboxed code after N
StepFrame steps,
- simplification of debugger agent logic.
Disadvtanges:
- currently when user was paused in blackboxed code (e.g. on breakpoint) and
then makes step action, debugger ignores blackboxed state of the script and
allows to use step actions as usual - this behavior is regressed, we still able
to support it on frontend side.

Current state and proposed changes for blackboxing:
https://docs.google.com/document/d/1hnzaXPAN8_QC5ENxIgxgMNDbXLraM_OXT73rAyijT...

BUG=v8:5842
R=yangguo@chromium.org,dgozman@chromium.org,alph@chromium.org

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

[commit-bot: I haz the power, 2017-01-24 01:50:34]

Committed patchset #18 (id:400001) as
https://chromium.googlesource.com/v8/v8/+/ac50c79a3e07cbe3c420ce55c1a3e0e3d5b...