[debugger] expose side-effect free evaluate to inspector.

[debugger] expose side-effect free evaluate to inspector.

R=jgruber@chromium.org, kozyatinskiy@chromium.org
BUG=v8:5821
Review-Url: https://codereview.chromium.org/2685483002
Cr-Original-Commit-Position: refs/heads/master@{#43049}
Committed: https://chromium.googlesource.com/v8/v8/+/1a989bdeefdc679745215ae547007773edb3d29e
Review-Url: https://codereview.chromium.org/2685483002
Cr-Commit-Position: refs/heads/master@{#43114}
Committed: https://chromium.googlesource.com/v8/v8/+/588ed4b3c465cff59197436a375aeb8cbe8c39c4

[jgruber, 2017-02-07 08:12:19]

lgtm, nice!

[Yang, 2017-02-07 08:14:29]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2017-02-07 08:14: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-02-07 08:17:26]

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

[commit-bot: I haz the power, 2017-02-07 08:17:27]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/34125)

[Yang, 2017-02-07 14:26:20]

On 2017/02/07 08:17:27, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_presubmit on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/34125)

Alexey, can you tell me why the LayoutTests fail? How come the default value
(true) for the new argument (allowSideEffect) is not being used?

[kozy, 2017-02-07 19:17:41]

On 2017/02/07 14:26:20, Yang wrote:
> On 2017/02/07 08:17:27, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   v8_presubmit on master.tryserver.v8 (JOB_FAILED,
> > http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/34125)
> 
> Alexey, can you tell me why the LayoutTests fail? How come the default value
> (true) for the new argument (allowSideEffect) is not being used?

On FE side we call protocol method as regular function like
evaluateOnCallFrame(callFrameId, ...) then to generate message FE check types
and store each argument to next field from method signature - it means that all
new arguments should be added to the end of the current arguments list. So you
can just move new argument after generatePreview and tests will pass.

And please add 'experimental': true flag.

Since this CL introduces protocol change - please add an inspector-test for new
argument.

And are you going to add same flag for regular Runtime.evaluate?

[kozy, 2017-02-07 19:18:46]

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

[kozy, 2017-02-07 19:18:46]

+@pfeldman.
Pavel, please take a look at js_protocol.json change.

[Yang, 2017-02-07 19:21:53]

On 2017/02/07 19:18:46, kozy wrote:
> +@pfeldman.
> Pavel, please take a look at js_protocol.json change.

[pfeldman, 2017-02-07 23:11:26]

https://codereview.chromium.org/2685483002/diff/1/src/inspector/js_protocol.json
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2685483002/diff/1/src/inspector/js_protocol.j...
src/inspector/js_protocol.json:636: { "name": "allowSideEffect", "type":
"boolean", "optional": true, "description": "If side effect is not allowed,
throw an exception if side effect cannot be ruled out during evaluation." },
fromMaybe(true) is not good, please reverse the condition. throwOnSideEffect?

[Yang, 2017-02-08 07:01:45]

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

[commit-bot: I haz the power, 2017-02-08 07:01:55]

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

[Yang, 2017-02-08 08:27:55]

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

[commit-bot: I haz the power, 2017-02-08 08:28:05]

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

[Yang, 2017-02-08 08:28:40]

On 2017/02/07 23:11:26, pfeldman wrote:
>
https://codereview.chromium.org/2685483002/diff/1/src/inspector/js_protocol.json
> File src/inspector/js_protocol.json (right):
> 
>
https://codereview.chromium.org/2685483002/diff/1/src/inspector/js_protocol.j...
> src/inspector/js_protocol.json:636: { "name": "allowSideEffect", "type":
> "boolean", "optional": true, "description": "If side effect is not allowed,
> throw an exception if side effect cannot be ruled out during evaluation." },
> fromMaybe(true) is not good, please reverse the condition. throwOnSideEffect?

Addressed comments. Layout tests pass now thanks to Alexey's tip. Please take a
look.

[commit-bot: I haz the power, 2017-02-08 08:50:45]

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

[commit-bot: I haz the power, 2017-02-08 08:50:46]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-02-08 18:31:45]

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

https://codereview.chromium.org/2685483002/diff/40001/src/inspector/js_protoc...
src/inspector/js_protocol.json:639: { "name": "throwOnSideEffect", "type":
"boolean", "optional": true, "experimental": true, "description": "Whether to
throw an exception if side effect cannot be ruled out during evaluation." }
Could you elaborate on what side-effect is?

[Yang, 2017-02-08 19:03:55]

On 2017/02/08 18:31:45, pfeldman wrote:
>
https://codereview.chromium.org/2685483002/diff/40001/src/inspector/js_protoc...
> File src/inspector/js_protocol.json (right):
> 
>
https://codereview.chromium.org/2685483002/diff/40001/src/inspector/js_protoc...
> src/inspector/js_protocol.json:639: { "name": "throwOnSideEffect", "type":
> "boolean", "optional": true, "experimental": true, "description": "Whether to
> throw an exception if side effect cannot be ruled out during evaluation." }
> Could you elaborate on what side-effect is?

Side-effect is anything that can be observed, i.e. changes to any global state.
The design doc is here:
https://docs.google.com/document/d/1l_JzDbOZ6Cn1k0c5vnyEXHe7B2Xxm7lROs1vYR3nR...

TL;DR: before entering a function, we check its bytecode and runtime intrinsics
against a whitelist. For example, storing a property to an object is not in the
whitelist, but loading a property from an object is. The load could trigger a
getter, but before entering the getter we would check it against the whitelist
too. This mechanism will have false positives. For example, stores to objects
that do not escape the function are still not whitelisted.

[Yang, 2017-02-08 19:04:57]

On 2017/02/08 19:03:55, Yang wrote:
> On 2017/02/08 18:31:45, pfeldman wrote:
> >
>
https://codereview.chromium.org/2685483002/diff/40001/src/inspector/js_protoc...
> > File src/inspector/js_protocol.json (right):
> > 
> >
>
https://codereview.chromium.org/2685483002/diff/40001/src/inspector/js_protoc...
> > src/inspector/js_protocol.json:639: { "name": "throwOnSideEffect", "type":
> > "boolean", "optional": true, "experimental": true, "description": "Whether
to
> > throw an exception if side effect cannot be ruled out during evaluation." }
> > Could you elaborate on what side-effect is?
> 
> Side-effect is anything that can be observed, i.e. changes to any global
state.
> The design doc is here:
>
https://docs.google.com/document/d/1l_JzDbOZ6Cn1k0c5vnyEXHe7B2Xxm7lROs1vYR3nR...
> 
> TL;DR: before entering a function, we check its bytecode and runtime
intrinsics
> against a whitelist. For example, storing a property to an object is not in
the
> whitelist, but loading a property from an object is. The load could trigger a
> getter, but before entering the getter we would check it against the whitelist
> too. This mechanism will have false positives. For example, stores to objects
> that do not escape the function are still not whitelisted.

Oh and this feature is still under active development. I am still adding more
test cases and items to the whitelist.

[pfeldman, 2017-02-08 19:13:10]

> > Side-effect is anything that can be observed, i.e. changes to any global
> state.
> > The design doc is here:
> >
>
https://docs.google.com/document/d/1l_JzDbOZ6Cn1k0c5vnyEXHe7B2Xxm7lROs1vYR3nR...

Yes, I've seen this, but it seems to be too subtle to expose using remote
debugging protocol. What's the actual use case? I'm looking for a cleaner way to
expose it.

[Yang, 2017-02-08 19:24:37]

On 2017/02/08 19:13:10, pfeldman wrote:
> > > Side-effect is anything that can be observed, i.e. changes to any global
> > state.
> > > The design doc is here:
> > >
> >
>
https://docs.google.com/document/d/1l_JzDbOZ6Cn1k0c5vnyEXHe7B2Xxm7lROs1vYR3nR...
> 
> Yes, I've seen this, but it seems to be too subtle to expose using remote
> debugging protocol. What's the actual use case? I'm looking for a cleaner way
to
> expose it.

The cloud debugger needs a way to sample expression values at runtime without
affecting execution. They are also migrating over to the inspector protocol.
Alexey also said that this is something potentially interesting for  DevTools.
Aside from the fact that it throws on any potential side effect, the semantic is
the same as evaluateOnCallFrame.

[pfeldman, 2017-02-08 19:31:22]

> The cloud debugger needs a way to sample expression values at runtime without
> affecting execution. They are also migrating over to the inspector protocol.
> Alexey also said that this is something potentially interesting for  DevTools.
> Aside from the fact that it throws on any potential side effect, the semantic
is
> the same as evaluateOnCallFrame.

This is something other vendors are going to look into implementing, so it needs
a clear definition.

To make sure I understand, cloud debugger is interested in sampling expression
at given location (i.e. they would stop in debugger to evaluate), but they want
to make sure there are no side-effects to the running program whatsoever. But
throwing seems to be a huge side-effect that would show up on all the
dashboards, etc.

[pfeldman, 2017-02-08 19:34:13]

> To make sure I understand, cloud debugger is interested in sampling expression
> at given location (i.e. they would stop in debugger to evaluate), but they
want
> to make sure there are no side-effects to the running program whatsoever. But
> throwing seems to be a huge side-effect that would show up on all the
> dashboards, etc.

Or do you mean that wasThrown is returned to me as I evaluate :) I guess the
latter, ignore me.

[Yang, 2017-02-08 19:35:32]

On 2017/02/08 19:34:13, pfeldman wrote:
> > To make sure I understand, cloud debugger is interested in sampling
expression
> > at given location (i.e. they would stop in debugger to evaluate), but they
> want
> > to make sure there are no side-effects to the running program whatsoever.
But
> > throwing seems to be a huge side-effect that would show up on all the
> > dashboards, etc.
> 
> Or do you mean that wasThrown is returned to me as I evaluate :) I guess the
> latter, ignore me.

Yeah. The thrown exception would not propagate out of debug-evaluate, but caught
by the inspector backend.

[pfeldman, 2017-02-08 19:44:51]

protocol lgtm.

Longer term questions: 1) could we get this for simple evaluate? 2) if yes,
should this be a mode where cloud debugger enters it via
Runtime.setThrowOnSideEffects(true) and no matter what they do (evaluate,
callFunctionOn, evaluateOnCallFrame) would result in throwing.

rationale: if that is supported for all evaluate-alike commands, we would stop
polluting our commands with extra parameters...

[Yang, 2017-02-08 19:58:12]

On 2017/02/08 19:44:51, pfeldman wrote:
> protocol lgtm.
> 
> Longer term questions: 1) could we get this for simple evaluate? 2) if yes,
> should this be a mode where cloud debugger enters it via
> Runtime.setThrowOnSideEffects(true) and no matter what they do (evaluate,
> callFunctionOn, evaluateOnCallFrame) would result in throwing.
> 
> rationale: if that is supported for all evaluate-alike commands, we would stop
> polluting our commands with extra parameters...

1) by simple evaluate, do you mean Runtime.evaluate? That one is more similar to
the user-accessible eval rather than debug-evaluate. We do have a global variant
of debug-evaluate [0], exposed through the debug-context [1], but the debugger
agent does not seem to use it. Adding throw on side-effect to global
debug-evaluate is very easy. Adding it to the current Runtime.evaluate is
possible, but requires some more work. Note though that stores to global
variables are considered side effects, since they are properties on the global
object.

2) interesting idea. At this point probably not worth considering, since we
would have to cover the whole inspector protocol. Side-effect checks do have a
slight overhead, so I would like to not expand it this just yet.

[0]
https://cs.chromium.org/chromium/src/v8/src/debug/debug-evaluate.cc?q=debug-e...
[1]
https://cs.chromium.org/chromium/src/v8/src/debug/debug.js?q=evaluateglobal+p...

[pfeldman, 2017-02-08 21:30:43]

> 1) by simple evaluate, do you mean Runtime.evaluate?

Yes.

> Adding it to the current Runtime.evaluate is
> possible, but requires some more work. Note though that stores to global
> variables are considered side effects, since they are properties on the global
> object.

That is expected.

> 2) interesting idea. At this point probably not worth considering, since we
> would have to cover the whole inspector protocol. Side-effect checks do have a
> slight overhead, so I would like to not expand it this just yet.

Sure. I guess we can always retrofit these convenience modes.

[Yang, 2017-02-09 06:41:53]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-02-09 06:41:54]

The patchset sent to the CQ was uploaded after l-g-t-m from jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2685483002/#ps40001
(title: "tag as experimental")

[commit-bot: I haz the power, 2017-02-09 06:41:56]

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-02-09 06:43:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486622513540430,
"parent_rev": "fa7d1f8f75e340d917831ef74f401154b0a90cc7", "commit_rev":
"1a989bdeefdc679745215ae547007773edb3d29e"}

[commit-bot: I haz the power, 2017-02-09 06:43:41]

Description was changed from

==========
[debugger] expose side-effect free evaluate to inspector.

R=jgruber@chromium.org, kozyatinskiy@chromium.org
BUG=v8:5821
==========

to

==========
[debugger] expose side-effect free evaluate to inspector.

R=jgruber@chromium.org, kozyatinskiy@chromium.org
BUG=v8:5821

Review-Url: https://codereview.chromium.org/2685483002
Cr-Commit-Position: refs/heads/master@{#43049}
Committed:
https://chromium.googlesource.com/v8/v8/+/1a989bdeefdc679745215ae547007773edb...
==========

[commit-bot: I haz the power, 2017-02-09 06:43:42]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/1a989bdeefdc679745215ae547007773edb...

[Michael Achenbach, 2017-02-09 11:49:16]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2687013003/ by machenbach@chromium.org.

The reason for reverting is: Speculative revert. Seems to block the roll:
https://codereview.chromium.org/2685933005/.

[Yang, 2017-02-09 12:26:29]

Description was changed from

==========
[debugger] expose side-effect free evaluate to inspector.

R=jgruber@chromium.org, kozyatinskiy@chromium.org
BUG=v8:5821

Review-Url: https://codereview.chromium.org/2685483002
Cr-Commit-Position: refs/heads/master@{#43049}
Committed:
https://chromium.googlesource.com/v8/v8/+/1a989bdeefdc679745215ae547007773edb...
==========

to

==========
[debugger] expose side-effect free evaluate to inspector.

R=jgruber@chromium.org, kozyatinskiy@chromium.org
BUG=v8:5821

Review-Url: https://codereview.chromium.org/2685483002
Cr-Commit-Position: refs/heads/master@{#43049}
Committed:
https://chromium.googlesource.com/v8/v8/+/1a989bdeefdc679745215ae547007773edb...
==========

[Yang, 2017-02-09 12:27:38]

On 2017/02/09 11:49:16, Michael Achenbach wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/2687013003/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is: Speculative revert. Seems to block the roll:
> https://codereview.chromium.org/2685933005/.

Seems like the change to js_protocol.json makes it incompatible with
DebuggerModel.js.
Alexey or Pavel, is there anyway I can avoid this issue? Or do I have to change
DebuggerModel.js at the same time as the roll?

[pfeldman, 2017-02-09 16:23:40]

> Seems like the change to js_protocol.json makes it incompatible with
> DebuggerModel.js.
> Alexey or Pavel, is there anyway I can avoid this issue? Or do I have to
change
> DebuggerModel.js at the same time as the roll?

It seems like compiler needs fixing. We support not passing optional parameters
to the generated agent methods at runtime, we need to allow it during
compilation as well.

[pfeldman, 2017-02-10 01:01:19]

What you can do in the meanwhile is replace this:

this.debuggerModel._agent.evaluateOnCallFrame(
        this._payload.callFrameId, code, objectGroup, includeCommandLineAPI,
silent, returnByValue, generatePreview,
        didEvaluateOnCallFrame)

with this:

this.debuggerModel._agent.invoke_evaluateOnCallFrame(
    {
        callFrameId: this._payload.callFrameId,
        expression: code,
        objectGroup,
        includeCommandLineAPI,
        silent,
        returnByValue,
        generatePreview
    },
    didEvaluateOnCallFrame);

downstream first.

[pfeldman, 2017-02-10 01:02:29]

pfeldman@chromium.org changed reviewers:
+ chenwilliam@chromium.org

[Yang, 2017-02-10 09:18:20]

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

[commit-bot: I haz the power, 2017-02-10 09:18:29]

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-02-10 09:44:54]

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

[commit-bot: I haz the power, 2017-02-10 09:44:56]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-02-10 17:04:12]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-02-10 17:04:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org, jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2685483002/#ps60001
(title: "add inspector test")

[commit-bot: I haz the power, 2017-02-10 17:04:24]

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-02-10 17:06:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486746252842790,
"parent_rev": "cdf73ea484aa638c6e1ffb4852f682d65cf27d2d", "commit_rev":
"588ed4b3c465cff59197436a375aeb8cbe8c39c4"}

[commit-bot: I haz the power, 2017-02-10 17:06:28]

Description was changed from

==========
[debugger] expose side-effect free evaluate to inspector.

R=jgruber@chromium.org, kozyatinskiy@chromium.org
BUG=v8:5821

Review-Url: https://codereview.chromium.org/2685483002
Cr-Commit-Position: refs/heads/master@{#43049}
Committed:
https://chromium.googlesource.com/v8/v8/+/1a989bdeefdc679745215ae547007773edb...
==========

to

==========
[debugger] expose side-effect free evaluate to inspector.

R=jgruber@chromium.org, kozyatinskiy@chromium.org
BUG=v8:5821

Review-Url: https://codereview.chromium.org/2685483002
Cr-Original-Commit-Position: refs/heads/master@{#43049}
Committed:
https://chromium.googlesource.com/v8/v8/+/1a989bdeefdc679745215ae547007773edb...
Review-Url: https://codereview.chromium.org/2685483002
Cr-Commit-Position: refs/heads/master@{#43114}
Committed:
https://chromium.googlesource.com/v8/v8/+/588ed4b3c465cff59197436a375aeb8cbe8...
==========

[commit-bot: I haz the power, 2017-02-10 17:06:29]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/588ed4b3c465cff59197436a375aeb8cbe8...