[inspector] Add some context scopes to inspector code

[inspector] Add some context scopes to inspector code

Currently, the external API (e.g. v8::Object::Get()) will enter the
context passed to it automatically. This is incorrect and causes some
trouble for Blink, so we want to change that.

It then becomes a potential problem to call the external API without
first entering a context, which the inspector code does in some
places. This patch aims to correct this.

BUG=v8:6307
Review-Url: https://codereview.chromium.org/2841053002
Cr-Commit-Position: refs/heads/master@{#44917}
Committed: https://chromium.googlesource.com/v8/v8/+/c9ab660ebdb2246b6a45c857ae4c875cf6147b53

[Jens Widell, 2017-04-26 12:24:17]

jl@opera.com changed reviewers:
+ dgozman@chromium.org

[Jens Widell, 2017-04-26 12:24:18]

PTAL

I've located these places to add context scopes by running layout tests with
extra checks for calls while no entered context. There may well be better places
to put them, and/or additional places where adding a scope might make sense. I'm
not at all familiar with this code.

[dgozman, 2017-04-26 15:52:48]

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

[dgozman, 2017-04-26 15:52:49]

+kozy FYI

https://codereview.chromium.org/2841053002/diff/1/src/inspector/injected-scri...
File src/inspector/injected-script.cc (right):

https://codereview.chromium.org/2841053002/diff/1/src/inspector/injected-scri...
src/inspector/injected-script.cc:178: v8::Context::Scope contextScope(context);
I don't think this is needed. It calls into wrapValue, which calls into
V8FunctionCall.

Overall, I think it only makes sense to enter context before accessing V8 api.
WDYT?

[Jens Widell, 2017-04-26 15:57:13]

https://codereview.chromium.org/2841053002/diff/1/src/inspector/injected-scri...
File src/inspector/injected-script.cc (right):

https://codereview.chromium.org/2841053002/diff/1/src/inspector/injected-scri...
src/inspector/injected-script.cc:178: v8::Context::Scope contextScope(context);
On 2017/04/26 15:52:49, dgozman wrote:
> I don't think this is needed. It calls into wrapValue, which calls into
> V8FunctionCall.

Indeed. I now wonder how come I ended up adding this one. I'll test without it
to make sure there wasn't a reason, then drop.

> Overall, I think it only makes sense to enter context before accessing V8 api.
> WDYT?

Yes, absolutely. Only reason to cover more code than just V8 API usage would be
if it avoided adding context scopes all over the place, by putting them in
logical central locations. But that's not at all the case here.

[Jens Widell, 2017-04-26 16:05:08]

On 2017/04/26 15:57:13, Jens Widell wrote:
>
https://codereview.chromium.org/2841053002/diff/1/src/inspector/injected-scri...
> File src/inspector/injected-script.cc (right):
> 
>
https://codereview.chromium.org/2841053002/diff/1/src/inspector/injected-scri...
> src/inspector/injected-script.cc:178: v8::Context::Scope
contextScope(context);
> On 2017/04/26 15:52:49, dgozman wrote:
> > I don't think this is needed. It calls into wrapValue, which calls into
> > V8FunctionCall.
> 
> Indeed. I now wonder how come I ended up adding this one. I'll test without it
> to make sure there wasn't a reason, then drop.

Dropped now.

[dgozman, 2017-04-26 19:37:48]

lgtm

[Jens Widell, 2017-04-27 08:25:42]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2017-04-27 08:25:46]

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-04-27 08:48:39]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493281542616910,
"parent_rev": "7b26234c74ae859d7bed7bc00684e337240c6c78", "commit_rev":
"c9ab660ebdb2246b6a45c857ae4c875cf6147b53"}

[commit-bot: I haz the power, 2017-04-27 08:48:45]

Description was changed from

==========
[inspector] Add some context scopes to inspector code

Currently, the external API (e.g. v8::Object::Get()) will enter the
context passed to it automatically. This is incorrect and causes some
trouble for Blink, so we want to change that.

It then becomes a potential problem to call the external API without
first entering a context, which the inspector code does in some
places. This patch aims to correct this.

BUG=v8:6307
==========

to

==========
[inspector] Add some context scopes to inspector code

Currently, the external API (e.g. v8::Object::Get()) will enter the
context passed to it automatically. This is incorrect and causes some
trouble for Blink, so we want to change that.

It then becomes a potential problem to call the external API without
first entering a context, which the inspector code does in some
places. This patch aims to correct this.

BUG=v8:6307

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

[commit-bot: I haz the power, 2017-04-27 08:48:46]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/c9ab660ebdb2246b6a45c857ae4c875cf61...