Re: [DevTools] Remove [V8] from InspectorInstrumentation. (issue 2112593003 by dgozman@chromium.org)

0 views
Skip to first unread message

dgo...@chromium.org

unread,
Jun 30, 2016, 8:24:07 PM6/30/16
to kozyat...@chromium.org, har...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org
Reviewers: kozyatinskiy, haraken
CL: https://codereview.chromium.org/2112593003/

Message:
Could you please take a look?

haraken - at bindings, core.
kozyatinskiy - at devtools, inspector.

Thanks,
Dmitry

Description:
[DevTools] Remove [V8] from InspectorInstrumentation.

We issue a fine-grained runtimeEnabled/runtimeDiabled notification.
Introduced a new setting forceMainWorldInitialization, which inspector
uses to force contexts in all frames to be able to evaluate there.

BUG=580337

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+74, -117 lines):
M third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
M third_party/WebKit/Source/core/dom/Document.h
M third_party/WebKit/Source/core/dom/Document.cpp
M third_party/WebKit/Source/core/dom/ExecutionContext.h
M third_party/WebKit/Source/core/frame/Settings.in
M third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
M third_party/WebKit/Source/core/inspector/CodeGeneratorInstrumentation.py
M third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h
M third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
M third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl
M third_party/WebKit/Source/core/inspector/InspectorSession.h
M third_party/WebKit/Source/core/inspector/InspectorSession.cpp
M third_party/WebKit/Source/core/loader/FrameLoader.cpp
M third_party/WebKit/Source/core/testing/NullExecutionContext.h
M third_party/WebKit/Source/core/workers/WorkerGlobalScope.h
M third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
M third_party/WebKit/Source/core/workers/WorkletGlobalScope.h
M third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp
M third_party/WebKit/Source/devtools/front_end/common/Settings.js
M third_party/WebKit/Source/devtools/front_end/sources/EventListenerBreakpointsSidebarPane.js
M third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
M third_party/WebKit/Source/platform/v8_inspector/V8InspectorSessionImpl.h
M third_party/WebKit/Source/platform/v8_inspector/V8InspectorSessionImpl.cpp
M third_party/WebKit/Source/platform/v8_inspector/V8ProfilerAgentImpl.cpp
M third_party/WebKit/Source/platform/v8_inspector/V8RuntimeAgentImpl.cpp
M third_party/WebKit/Source/platform/v8_inspector/public/V8InspectorSessionClient.h
M third_party/WebKit/Source/web/FrameLoaderClientImpl.h
M third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp
M third_party/WebKit/Source/web/WebDevToolsAgentImpl.h
M third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp


har...@chromium.org

unread,
Jun 30, 2016, 10:28:04 PM6/30/16
to dgo...@chromium.org, kozyat...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
(right):

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp#newcode287
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:287: if
(!forceMainWorldInitialization &&
!m_windowProxyManager->mainWorldProxy()->isGlobalInitialized())

Hmm, it looks a bit nasty to force the main world initialization in
updateDocument(). Don't we have any explicit timing where the inspector
can force the initialization?

https://codereview.chromium.org/2112593003/

dgo...@chromium.org

unread,
Jul 6, 2016, 1:02:20 PM7/6/16
to kozyat...@chromium.org, har...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
(right):

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp#newcode287
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:287: if
(!forceMainWorldInitialization &&
!m_windowProxyManager->mainWorldProxy()->isGlobalInitialized())
On 2016/07/01 02:28:03, haraken wrote:
>
> Hmm, it looks a bit nasty to force the main world initialization in
> updateDocument(). Don't we have any explicit timing where the
inspector can
> force the initialization?

Alternatives are LocalDOMWindow::installNewDocument,
DocumentLoader::createWriterFor and DocumentLoader::ensureWriter. I
don't think any of them is better.

As for forcing this from inspector, we try to minimize the side-effects
of instrumentation, and instead rely on direct manipulation through
settings. This makes it clear to anyone working on the specific area
that such a functionality exist. Otherwise, it's easy to miss
inspector-only side-effect and break things. WDYT?

https://codereview.chromium.org/2112593003/

kozyat...@chromium.org

unread,
Jul 6, 2016, 3:38:22 PM7/6/16
to dgo...@chromium.org, har...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
File
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
(right):

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp#newcode641
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:641:
m_v8Session->breakProgramOnException(protocol::Debugger::Paused::ReasonEnum::CSPViolation,
std::move(directive));
Let's move it to "DOMDebugger way" instead of breakProgramOnException.

https://codereview.chromium.org/2112593003/

dgo...@chromium.org

unread,
Jul 6, 2016, 6:52:33 PM7/6/16
to kozyat...@chromium.org, har...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org
Please take another look.



https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
File
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
(right):

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp#newcode641
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:641:
m_v8Session->breakProgramOnException(protocol::Debugger::Paused::ReasonEnum::CSPViolation,
std::move(directive));
On 2016/07/06 19:38:22, kozyatinskiy wrote:
> Let's move it to "DOMDebugger way" instead of breakProgramOnException.

har...@chromium.org

unread,
Jul 6, 2016, 8:43:33 PM7/6/16
to dgo...@chromium.org, kozyat...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org
Hmm, InspectorSession is already explicitly calling
frame->script().initializeMainWorld() in a couple of places. Can we take the
same approach here? i.e., add some InspectorInstrumentation callback and call
initializeMainWorld() there.


https://codereview.chromium.org/2112593003/

dgo...@chromium.org

unread,
Jul 6, 2016, 8:47:23 PM7/6/16
to kozyat...@chromium.org, har...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org
This is exactly what I'm trying to remove to avoid side-effects of
InspectorInstrumentation.

Ideally, we'd also force contexts in all frames when the setting changes, and
not call initializeMainWorld() from InspectorSession at all. I can do that if
you think it's better.


https://codereview.chromium.org/2112593003/

kozyat...@chromium.org

unread,
Jul 6, 2016, 8:48:06 PM7/6/16
to dgo...@chromium.org, har...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org

har...@chromium.org

unread,
Jul 6, 2016, 8:51:43 PM7/6/16
to dgo...@chromium.org, kozyat...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org
Yeah, it makes more sense. It's not nice that we have two ways to force
initializing contexts. With that change, looks good.


https://codereview.chromium.org/2112593003/

dgo...@chromium.org

unread,
Jul 7, 2016, 8:45:26 PM7/7/16
to kozyat...@chromium.org, har...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org
@haraken: please take another look.

https://codereview.chromium.org/2112593003/

har...@chromium.org

unread,
Jul 7, 2016, 8:47:44 PM7/7/16
to dgo...@chromium.org, kozyat...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 8, 2016, 8:22:27 PM7/8/16
to dgo...@chromium.org, kozyat...@chromium.org, har...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 10, 2016, 1:09:22 AM7/10/16
to dgo...@chromium.org, kozyat...@chromium.org, har...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 10, 2016, 9:16:52 PM7/10/16
to dgo...@chromium.org, kozyat...@chromium.org, har...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org
Committed patchset #7 (id:120001)

https://codereview.chromium.org/2112593003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 10, 2016, 9:16:56 PM7/10/16
to dgo...@chromium.org, kozyat...@chromium.org, har...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 10, 2016, 9:19:31 PM7/10/16
to dgo...@chromium.org, kozyat...@chromium.org, har...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, kinuko...@chromium.org, tyoshin...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, gavinp...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, sergey...@chromium.org, jap...@chromium.org, loading...@chromium.org, kozyatins...@chromium.org
Patchset 7 (id:??) landed as
https://crrev.com/c1e6c531c70657b519121ab6b626471a75448a9e
Cr-Commit-Position: refs/heads/master@{#404588}

https://codereview.chromium.org/2112593003/
Reply all
Reply to author
Forward
0 new messages