monitor console command implemented.

monitor console command implemented.

monitor logs each function call in console. For instance:

function f() {}
monitor(f);
f();
unmonitor(f);

Logs:
function f called

BUG=168776
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152561

[vsevik, 2013-06-10 15:06:00]

Yury, could you please have a look on this monitor implementation based on
conditional breakpoint?
Are you fine with that?

https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/http/t...
File LayoutTests/http/tests/inspector/console-test.js (right):

https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/http/t...
LayoutTests/http/tests/inspector/console-test.js:91:
InspectorTest.waitUntilMessageReceived = function(callback)
Please use InspectorTest.addConsoleSniffer instead

https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/inspec...
File LayoutTests/inspector/debugger/monitor-console-command.html (right):

https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/inspec...
LayoutTests/inspector/debugger/monitor-console-command.html:50:
InspectorTest.evaluateInConsole("unmonitor(simpleTestFunction2)");
This test (testSimpleUnmonitor) looks weird, what are you testing here?

https://chromiumcodereview.appspot.com/16143005/diff/12001/Source/core/inspec...
File Source/core/inspector/InjectedScriptSource.js (right):

https://chromiumcodereview.appspot.com/16143005/diff/12001/Source/core/inspec...
Source/core/inspector/InjectedScriptSource.js:1328: var details =
InjectedScriptHost.functionDetails(fn);
Looking at this piece of code I think that you don't need custom bindings for
both monitor and debug command since functionDetails structure already provides
you with all the data needed.

https://chromiumcodereview.appspot.com/16143005/diff/12001/Source/core/inspec...
Source/core/inspector/InjectedScriptSource.js:1330:
InjectedScriptHost.setMonitorBreakpoint(fn, "console.log('function " + name + "
called' + (arguments.length > 0 ? ' with arguments: ' +
Array.prototype.join.call(arguments, ', ') : '')) && false");
Please don't use single quotes in blink.

[SeRya, 2013-06-11 08:24:12]

https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/http/t...
File LayoutTests/http/tests/inspector/console-test.js (right):

https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/http/t...
LayoutTests/http/tests/inspector/console-test.js:91:
InspectorTest.waitUntilMessageReceived = function(callback)
On 2013/06/10 15:06:01, vsevik wrote:
> Please use InspectorTest.addConsoleSniffer instead

addConsoleSniffer doesn't work because it is also triggered on evaluation
result. I need to handle only console messages from the back end.

https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/inspec...
File LayoutTests/inspector/debugger/monitor-console-command.html (right):

https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/inspec...
LayoutTests/inspector/debugger/monitor-console-command.html:50:
InspectorTest.evaluateInConsole("unmonitor(simpleTestFunction2)");
On 2013/06/10 15:06:01, vsevik wrote:
> This test (testSimpleUnmonitor) looks weird, what are you testing here?

I'm testing that simpleTestFunction3 is monitored but simpleTestFunction2 isn't.

https://chromiumcodereview.appspot.com/16143005/diff/12001/Source/core/inspec...
File Source/core/inspector/InjectedScriptSource.js (right):

https://chromiumcodereview.appspot.com/16143005/diff/12001/Source/core/inspec...
Source/core/inspector/InjectedScriptSource.js:1328: var details =
InjectedScriptHost.functionDetails(fn);
On 2013/06/10 15:06:01, vsevik wrote:
> Looking at this piece of code I think that you don't need custom bindings for
> both monitor and debug command since functionDetails structure already
provides
> you with all the data needed.

Now I use results of InjectedScriptHost.functionDetails everywhere.

https://chromiumcodereview.appspot.com/16143005/diff/12001/Source/core/inspec...
Source/core/inspector/InjectedScriptSource.js:1330:
InjectedScriptHost.setMonitorBreakpoint(fn, "console.log('function " + name + "
called' + (arguments.length > 0 ? ' with arguments: ' +
Array.prototype.join.call(arguments, ', ') : '')) && false");
On 2013/06/10 15:06:01, vsevik wrote:
> Please don't use single quotes in blink.

Replaced by \"

[vsevik, 2013-06-11 09:18:32]

https://chromiumcodereview.appspot.com/16143005/diff/23003/Source/bindings/v8...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://chromiumcodereview.appspot.com/16143005/diff/23003/Source/bindings/v8...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:368: if (args.Length()
< 5) {
Can we avoid having custom bindings at all?

[vsevik, 2013-06-11 09:22:05]

On 2013/06/11 08:24:12, SeRya wrote:
>
https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/http/t...
> File LayoutTests/http/tests/inspector/console-test.js (right):
> 
>
https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/http/t...
> LayoutTests/http/tests/inspector/console-test.js:91:
> InspectorTest.waitUntilMessageReceived = function(callback)
> On 2013/06/10 15:06:01, vsevik wrote:
> > Please use InspectorTest.addConsoleSniffer instead
> 
> addConsoleSniffer doesn't work because it is also triggered on evaluation
> result. I need to handle only console messages from the back end.
Ok, then you should just use callback of evaluateInConsole. The messages are
printed to console anyway (you have two instances of each in test expectations
now)


>
https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/inspec...
> File LayoutTests/inspector/debugger/monitor-console-command.html (right):
> 
>
https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/inspec...
> LayoutTests/inspector/debugger/monitor-console-command.html:50:
> InspectorTest.evaluateInConsole("unmonitor(simpleTestFunction2)");
> On 2013/06/10 15:06:01, vsevik wrote:
> > This test (testSimpleUnmonitor) looks weird, what are you testing here?
> 
> I'm testing that simpleTestFunction3 is monitored but simpleTestFunction2
isn't.
Oh, I see it now.

[SeRya, 2013-06-11 09:55:52]

On 2013/06/11 09:22:05, vsevik wrote:
> On 2013/06/11 08:24:12, SeRya wrote:
> >
>
https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/http/t...
> > File LayoutTests/http/tests/inspector/console-test.js (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/16143005/diff/12001/LayoutTests/http/t...
> > LayoutTests/http/tests/inspector/console-test.js:91:
> > InspectorTest.waitUntilMessageReceived = function(callback)
> > On 2013/06/10 15:06:01, vsevik wrote:
> > > Please use InspectorTest.addConsoleSniffer instead
> > 
> > addConsoleSniffer doesn't work because it is also triggered on evaluation
> > result. I need to handle only console messages from the back end.
> Ok, then you should just use callback of evaluateInConsole. The messages are
> printed to console anyway (you have two instances of each in test expectations
> now)

I don't think so. Tested behavior is generating console message. So the code
should wait for that exact event. Waiting for a related event makes the test
fragile.

[SeRya, 2013-06-13 08:18:38]

Removed custom bindings.

[yurys, 2013-06-13 09:46:51]

Please provide more details in the description about the feature being
implemented.

https://chromiumcodereview.appspot.com/16143005/diff/44001/LayoutTests/inspec...
File LayoutTests/inspector/debugger/monitor-console-command.html (right):

https://chromiumcodereview.appspot.com/16143005/diff/44001/LayoutTests/inspec...
LayoutTests/inspector/debugger/monitor-console-command.html:50:
InspectorTest.evaluateInConsole("unmonitor(simpleTestFunction2)");
Can you add a test that unmonitor works ok on a function which is not monitored?

https://chromiumcodereview.appspot.com/16143005/diff/44001/Source/core/inspec...
File Source/core/inspector/InjectedScriptHost.idl (right):

https://chromiumcodereview.appspot.com/16143005/diff/44001/Source/core/inspec...
Source/core/inspector/InjectedScriptHost.idl:50: void setBreakpoint(DOMString
scriptId, long line, long column, DOMString source, DOMString condition);
I don't think it is a good idea to let inspected page pass some arbitrary JS
code that would be evaluated by the debugger. It is a potential security
problem. Can we avoid constructing this condition in the injected script and do
it somewhere the inspected code doesn't have access to?

https://chromiumcodereview.appspot.com/16143005/diff/44001/Source/core/inspec...
File Source/core/inspector/InjectedScriptSource.js (right):

https://chromiumcodereview.appspot.com/16143005/diff/44001/Source/core/inspec...
Source/core/inspector/InjectedScriptSource.js:1319:
this._setBreakpoint(details.location, "debug", "");
Please use enum for "debug" and "monitor" sources.

[yurys, 2013-06-13 09:57:30]

not lgtm until the security concern is addressed. As discussed offline
alternative solution may be to have InjectedScriptHost.monitor(fn) that would
accept reference to a function and construct the condition in the C++ code.

[SeRya, 2013-06-13 10:45:14]

https://chromiumcodereview.appspot.com/16143005/diff/44001/LayoutTests/inspec...
File LayoutTests/inspector/debugger/monitor-console-command.html (right):

https://chromiumcodereview.appspot.com/16143005/diff/44001/LayoutTests/inspec...
LayoutTests/inspector/debugger/monitor-console-command.html:50:
InspectorTest.evaluateInConsole("unmonitor(simpleTestFunction2)");
On 2013/06/13 09:46:51, Yury Semikhatsky wrote:
> Can you add a test that unmonitor works ok on a function which is not
monitored?

Done.

https://chromiumcodereview.appspot.com/16143005/diff/44001/Source/core/inspec...
File Source/core/inspector/InjectedScriptHost.idl (right):

https://chromiumcodereview.appspot.com/16143005/diff/44001/Source/core/inspec...
Source/core/inspector/InjectedScriptHost.idl:50: void setBreakpoint(DOMString
scriptId, long line, long column, DOMString source, DOMString condition);
On 2013/06/13 09:46:51, Yury Semikhatsky wrote:
> I don't think it is a good idea to let inspected page pass some arbitrary JS
> code that would be evaluated by the debugger. It is a potential security
> problem. Can we avoid constructing this condition in the injected script and
do
> it somewhere the inspected code doesn't have access to?

Moved to C++.

https://chromiumcodereview.appspot.com/16143005/diff/44001/Source/core/inspec...
File Source/core/inspector/InjectedScriptSource.js (right):

https://chromiumcodereview.appspot.com/16143005/diff/44001/Source/core/inspec...
Source/core/inspector/InjectedScriptSource.js:1319:
this._setBreakpoint(details.location, "debug", "");
On 2013/06/13 09:46:51, Yury Semikhatsky wrote:
> Please use enum for "debug" and "monitor" sources.

Not actual now.

[yurys, 2013-06-17 12:06:14]

https://codereview.chromium.org/16143005/diff/56001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/16143005/diff/56001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:416:
v8::Handle<v8::Function> function = v8::Handle<v8::Function>::Cast(args[0]);
Please add a note that we rely on IsFunction check in getFunctionLocation or
even better do the cast there next to the check.

https://codereview.chromium.org/16143005/diff/56001/Source/core/inspector/Inj...
File Source/core/inspector/InjectedScriptHost.cpp (right):

https://codereview.chromium.org/16143005/diff/56001/Source/core/inspector/Inj...
Source/core/inspector/InjectedScriptHost.cpp:165: builder.appendLiteral("
called\" + (arguments.length > 0 ? \" with arguments: \" +
Array.prototype.join.call(arguments, \", \") : \"\")) && false");
This still calls Array.prototype.join that may be overridden by the inspected
code, not sure how bad it is as the code that overrides the method should
already have access to the inspected context. But evaluating it in debugger
context may result in granting extended privileges, can we avoid such call?

[Peter.Rybin, 2013-06-17 14:38:13]

https://codereview.chromium.org/16143005/diff/56001/Source/core/inspector/Inj...
File Source/core/inspector/InjectedScriptHost.cpp (right):

https://codereview.chromium.org/16143005/diff/56001/Source/core/inspector/Inj...
Source/core/inspector/InjectedScriptHost.cpp:165: builder.appendLiteral("
called\" + (arguments.length > 0 ? \" with arguments: \" +
Array.prototype.join.call(arguments, \", \") : \"\")) && false");
On 2013/06/17 12:06:14, Yury Semikhatsky wrote:
> This still calls Array.prototype.join that may be overridden by the inspected
> code, not sure how bad it is as the code that overrides the method should
> already have access to the inspected context. But evaluating it in debugger
> context may result in granting extended privileges, can we avoid such call?

It looks that breakpoint conditions are processed in V8 by
frame.evaluate(condition). This is probably secure enough.

[SeRya, 2013-06-17 15:08:56]

https://codereview.chromium.org/16143005/diff/56001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/16143005/diff/56001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:416:
v8::Handle<v8::Function> function = v8::Handle<v8::Function>::Cast(args[0]);
On 2013/06/17 12:06:14, Yury Semikhatsky wrote:
> Please add a note that we rely on IsFunction check in getFunctionLocation or
> even better do the cast there next to the check.

Done.

https://codereview.chromium.org/16143005/diff/56001/Source/core/inspector/Inj...
File Source/core/inspector/InjectedScriptHost.cpp (right):

https://codereview.chromium.org/16143005/diff/56001/Source/core/inspector/Inj...
Source/core/inspector/InjectedScriptHost.cpp:165: builder.appendLiteral("
called\" + (arguments.length > 0 ? \" with arguments: \" +
Array.prototype.join.call(arguments, \", \") : \"\")) && false");
On 2013/06/17 14:38:14, peter.rybin wrote:
> On 2013/06/17 12:06:14, Yury Semikhatsky wrote:
> > This still calls Array.prototype.join that may be overridden by the
inspected
> > code, not sure how bad it is as the code that overrides the method should
> > already have access to the inspected context. But evaluating it in debugger
> > context may result in granting extended privileges, can we avoid such call?
> 
> It looks that breakpoint conditions are processed in V8 by
> frame.evaluate(condition). This is probably secure enough.

It evaluates in v8/src/debug-debugger.js:
var mirror = exec_state.frame(0).evaluate(this.condition());

then v8/src/mirror-debugger.js:
%DebugEvaluate(this.break_id_,
    this.details_.frameId(),
    this.details_.inlinedFrameIndex(),
    source,
    Boolean(disable_break),
    opt_context_object);

Then executes Runtime_DebugEvaluate in v8/src/runtime.cc.

So I see no way how to debugger context may get to this point.

[yurys, 2013-06-17 15:11:13]

On 2013/06/17 14:38:13, peter.rybin wrote:
>
https://codereview.chromium.org/16143005/diff/56001/Source/core/inspector/Inj...
> File Source/core/inspector/InjectedScriptHost.cpp (right):
> 
>
https://codereview.chromium.org/16143005/diff/56001/Source/core/inspector/Inj...
> Source/core/inspector/InjectedScriptHost.cpp:165: builder.appendLiteral("
> called\" + (arguments.length > 0 ? \" with arguments: \" +
> Array.prototype.join.call(arguments, \", \") : \"\")) && false");
> On 2013/06/17 12:06:14, Yury Semikhatsky wrote:
> > This still calls Array.prototype.join that may be overridden by the
inspected
> > code, not sure how bad it is as the code that overrides the method should
> > already have access to the inspected context. But evaluating it in debugger
> > context may result in granting extended privileges, can we avoid such call?
> 
> It looks that breakpoint conditions are processed in V8 by
> frame.evaluate(condition). This is probably secure enough.

Ok then we just need to make sure that arguments.callee.caller will never return
functions from debug context which seems to be the case.

[yurys, 2013-06-17 15:15:45]

lgtm

[commit-bot: I haz the power, 2013-06-17 15:19:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/serya@chromium.org/16143005/75001

[commit-bot: I haz the power, 2013-06-17 16:48:03]

Change committed as 152561