[inspector] introduced debug::SetCompileEventListener

[inspector] introduced debug::SetCompileEventListener

Listener is called instead of event listener for v8::AfterCompile and v8::CompileError events if installed.
- removed v8::debug::Script::Wrap.

BUG=v8:5510
R=yangguo@chromium.org,jgruber@chromium.org,dgozman@chromium.org,clemensh@chromium.org, alph@chromium.org,

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

[kozy, 2017-01-13 17:19:28]

Description was changed from

==========
[inspector] introduced debug::SetCompileEventListener

With new listener we can avoid any redundant JS allocations during compile event
processing.
Additionally: v8::debug::Script::Wrap removed.

BUG=
R=yangguo@chromium.org
==========

to

==========
[inspector] introduced debug::SetCompileEventListener

Listener is called instead of event listener for v8::AfterCompile and
v8::CompileError events if installed.
- removed v8::debug::Script::Wrap.

BUG=v8:5510
R=yangguo@chromium.org,jgruber@chromium.org,dgozman@chromium.org,clemensh@chr...
==========

[kozy, 2017-01-13 17:19:28]

kozyatinskiy@chromium.org changed reviewers:
+ clemensh@chromium.org, dgozman@chromium.org, jgruber@chromium.org

[kozy, 2017-01-13 17:22:24]

Dmitry, please take a look on inspector part (at least on using
script->context_data for getting enabled debugger agent).
Jacob, please take a look on src/debug related changes.
Clemens, please take a look on src/wasm/wasm-module.cc, it looks like this
marking was missing for wasm scripts. We mark TYPE_NORMAL scripts here [1].

[1] https://cs.chromium.org/chromium/src/v8/src/compiler.cc?rcl=0&l=1062

[Clemens Hammacher, 2017-01-16 09:06:09]

src/wasm lgtm. thanks!

[jgruber, 2017-01-16 19:26:32]

lgtm

https://codereview.chromium.org/2626283002/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2626283002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1982: message_handler_ != nullptr ||
Nit: Maybe it'd make sense to extract this to a method at some point.

[kozy, 2017-01-16 22:22:11]

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

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

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-16 22:22:39]

Dmitry, please take a look.

https://codereview.chromium.org/2626283002/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2626283002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1982: message_handler_ != nullptr ||
On 2017/01/16 19:26:32, jgruber wrote:
> Nit: Maybe it'd make sense to extract this to a method at some point.

Done.

[commit-bot: I haz the power, 2017-01-16 22:42:34]

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

[commit-bot: I haz the power, 2017-01-16 22:42:35]

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

[Yang, 2017-01-17 12:48:34]

https://codereview.chromium.org/2626283002/diff/40001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2626283002/diff/40001/src/debug/debug.h#newco...
src/debug/debug.h:609: (event_listener_.is_null() &&
!event_listener_->IsForeign());
shouldn't this be

!event_listener.is_null()

instead?

[kozy, 2017-01-17 16:11:54]

Fixed condition, but I'm still investigating into
test/debugger/debug/debug-step-stub-callfunction.js failure on try bots.

https://codereview.chromium.org/2626283002/diff/40001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2626283002/diff/40001/src/debug/debug.h#newco...
src/debug/debug.h:609: (event_listener_.is_null() &&
!event_listener_->IsForeign());
On 2017/01/17 12:48:34, Yang wrote:
> shouldn't this be
> 
> !event_listener.is_null()
> 
> instead?

oops, thanks.

[kozy, 2017-01-17 17:14:09]

Fixed tests and left one comment for Jacob, please take a look.

https://codereview.chromium.org/2626283002/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2626283002/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1969: SuppressDebug while_processing(this);
test/debug relies on this (e.g. debug-step-stub-callfunction.js): inspector
doesn't really need this suppression but if we move this line after calling
callback then we will pause inside of test-api.js file and will ignore this
pause because of [1], I think we should be better inside test-api.js and if
stepping action was scheduled then repeat last scheduled action instead of
silent ignorance. Jacob, WDYT?

[1]
https://cs.chromium.org/chromium/src/v8/test/debugger/test-api.js?rcl=0&l=830

[kozy, 2017-01-17 17:14:15]

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

[commit-bot: I haz the power, 2017-01-17 17:14:18]

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-17 17:42:31]

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

[commit-bot: I haz the power, 2017-01-17 17:42:34]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-01-17 21:30:25]

Description was changed from

==========
[inspector] introduced debug::SetCompileEventListener

Listener is called instead of event listener for v8::AfterCompile and
v8::CompileError events if installed.
- removed v8::debug::Script::Wrap.

BUG=v8:5510
R=yangguo@chromium.org,jgruber@chromium.org,dgozman@chromium.org,clemensh@chr...
==========

to

==========
[inspector] introduced debug::SetCompileEventListener

Listener is called instead of event listener for v8::AfterCompile and
v8::CompileError events if installed.
- removed v8::debug::Script::Wrap.

BUG=v8:5510
R=yangguo@chromium.org,jgruber@chromium.org,dgozman@chromium.org,clemensh@chr...,
alph@chromium.org,
==========

[kozy, 2017-01-17 21:30:25]

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

[kozy, 2017-01-17 21:30:47]

Alexei, please take a look on src/inspector changes.

[Yang, 2017-01-18 09:27:24]

On 2017/01/17 21:30:47, kozyatinskiy wrote:
> Alexei, please take a look on src/inspector changes.

lgtm.

[jgruber, 2017-01-18 10:25:35]

https://codereview.chromium.org/2626283002/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2626283002/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1969: SuppressDebug while_processing(this);
On 2017/01/17 17:14:09, kozyatinskiy wrote:
> test/debug relies on this (e.g. debug-step-stub-callfunction.js): inspector
> doesn't really need this suppression but if we move this line after calling
> callback then we will pause inside of test-api.js file and will ignore this
> pause because of [1], I think we should be better inside test-api.js and if
> stepping action was scheduled then repeat last scheduled action instead of
> silent ignorance. Jacob, WDYT?
> 
> [1]
> https://cs.chromium.org/chromium/src/v8/test/debugger/test-api.js?rcl=0&l=830 

Good point, the current solution in test-api.js is suboptimal. What we really
need is blackboxing of everything in test-api.js (and probably the registered JS
listener). Created https://bugs.chromium.org/p/v8/issues/detail?id=5859 to track
this.

[jgruber, 2017-01-18 10:26:55]

By the way, inspector tests on the blink side seem to be failing.

[Yang, 2017-01-18 11:41:08]

https://codereview.chromium.org/2626283002/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2626283002/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1980: DebugScope debug_scope(this);
I think we need to move the DebugScope above the compile listener call. Inside a
debug scope, we do not perform stepping.

[dgozman, 2017-01-18 18:18:29]

inspector lgtm modulo comment

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

https://codereview.chromium.org/2626283002/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:585:
debugger->m_inspector->enabledDebuggerAgentForGroup(contextId);
contextId is not a group id.

[kozy, 2017-01-18 19:46:22]

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

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

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-18 20:00:03]

all done, issue with blink tests was related to using contextId instead of
contextGroupId and I checked - blink tests are passed with this CL locally.

https://codereview.chromium.org/2626283002/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2626283002/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1980: DebugScope debug_scope(this);
On 2017/01/18 11:41:08, Yang wrote:
> I think we need to move the DebugScope above the compile listener call. Inside
a
> debug scope, we do not perform stepping.

Done.

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

https://codereview.chromium.org/2626283002/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:585:
debugger->m_inspector->enabledDebuggerAgentForGroup(contextId);
On 2017/01/18 18:18:29, dgozman wrote:
> contextId is not a group id.

Done.

[kozy, 2017-01-18 20:00:08]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[kozy, 2017-01-18 20:00:24]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-01-18 20:00:25]

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

[commit-bot: I haz the power, 2017-01-18 20:00:37]

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

[commit-bot: I haz the power, 2017-01-18 20:16:28]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484769624647150,
"parent_rev": "80234ee31f7387036f91b72cce8893300ada5657", "commit_rev":
"b0577a3d3d0f64380b15643552e3ebfb9ed94a07"}

[commit-bot: I haz the power, 2017-01-18 20:16:37]

Description was changed from

==========
[inspector] introduced debug::SetCompileEventListener

Listener is called instead of event listener for v8::AfterCompile and
v8::CompileError events if installed.
- removed v8::debug::Script::Wrap.

BUG=v8:5510
R=yangguo@chromium.org,jgruber@chromium.org,dgozman@chromium.org,clemensh@chr...,
alph@chromium.org,
==========

to

==========
[inspector] introduced debug::SetCompileEventListener

Listener is called instead of event listener for v8::AfterCompile and
v8::CompileError events if installed.
- removed v8::debug::Script::Wrap.

BUG=v8:5510
R=yangguo@chromium.org,jgruber@chromium.org,dgozman@chromium.org,clemensh@chr...,
alph@chromium.org,

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

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

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