Make ExecutionContext aware about how it can return CoreProbeSink (issue 2847703002 by kinuko@chromium.org)

kin...@chromium.org

unread,

Apr 27, 2017, 5:07:23 AM4/27/17

to dgo...@chromium.org, nhi...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

Reviewers: dgozman, nhiroki
CL: https://codereview.chromium.org/2847703002/

Message:
I'm not sure if there was a strong preference not to do this, but what do you
think about this change?

Description:
Make ExecutionContext aware about how it can return CoreProbeSink

Currently ToCoreProbe code checks the given ExecutionContext's type
and calls different code to get its CoreProbeSink, but in this way
we need to keep adding new ToCoreProbe code every time we add
new ExecutionContext, which sounds a bit cumbersome.

This CL adds ExecutionContext::GetInstrumentingAgents() virtual method
to mitigate that.

BUG=538751

Affected files (+21, -18 lines):
M third_party/WebKit/Source/core/dom/ExecutionContext.h
M third_party/WebKit/Source/core/probe/CoreProbes.cpp
M third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.h
M third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.cpp
M third_party/WebKit/Source/core/workers/WorkerGlobalScope.h
M third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp

Index: third_party/WebKit/Source/core/dom/ExecutionContext.h
diff --git a/third_party/WebKit/Source/core/dom/ExecutionContext.h b/third_party/WebKit/Source/core/dom/ExecutionContext.h
index f4c1aedbda219adf3835150714039930425fbb94..a93d37c27275b56119989325714aa655e65ac403 100644
--- a/third_party/WebKit/Source/core/dom/ExecutionContext.h
+++ b/third_party/WebKit/Source/core/dom/ExecutionContext.h
@@ -43,14 +43,15 @@

namespace blink {

-class SuspendableObject;
class ConsoleMessage;
+class CoreProbeSink;
class DOMTimerCoordinator;
class ErrorEvent;
class EventQueue;
class EventTarget;
class ExecutionContextTask;
class LocalDOMWindow;
+class SuspendableObject;
class PublicURLManager;
class SecurityOrigin;
class ScriptState;
@@ -192,6 +193,8 @@ class CORE_EXPORT ExecutionContext : public ContextLifecycleNotifier,
void SetReferrerPolicy(ReferrerPolicy);
virtual ReferrerPolicy GetReferrerPolicy() const { return referrer_policy_; }

+ virtual CoreProbeSink* GetInstrumentingAgents() { return nullptr; }
+
protected:
ExecutionContext();
virtual ~ExecutionContext();
Index: third_party/WebKit/Source/core/probe/CoreProbes.cpp
diff --git a/third_party/WebKit/Source/core/probe/CoreProbes.cpp b/third_party/WebKit/Source/core/probe/CoreProbes.cpp
index 40fcaf9810929ba6c938d1fdb361897778c40d47..366d1086cc7001337faca078ae93ab66d78f35a4 100644
--- a/third_party/WebKit/Source/core/probe/CoreProbes.cpp
+++ b/third_party/WebKit/Source/core/probe/CoreProbes.cpp
@@ -41,11 +41,8 @@
#include "core/inspector/InspectorTraceEvents.h"
#include "core/inspector/MainThreadDebugger.h"
#include "core/inspector/ThreadDebugger.h"
-#include "core/inspector/WorkerInspectorController.h"
#include "core/page/Page.h"
-#include "core/workers/MainThreadWorkletGlobalScope.h"
#include "core/workers/WorkerGlobalScope.h"
-#include "core/workers/WorkerThread.h"
#include "platform/instrumentation/tracing/TraceEvent.h"
#include "platform/loader/fetch/FetchInitiatorInfo.h"

@@ -139,21 +136,8 @@ void ContinueWithPolicyIgnore(LocalFrame* frame,
DidReceiveResourceResponseButCanceled(frame, loader, identifier, r, resource);
}

-CoreProbeSink* ToCoreProbeSink(WorkerGlobalScope* worker_global_scope) {
- if (!worker_global_scope)
- return nullptr;
- if (WorkerInspectorController* controller =
- worker_global_scope->GetThread()->GetWorkerInspectorController())
- return controller->InstrumentingAgents();
- return nullptr;
-}
-
CoreProbeSink* ToCoreProbeSinkForNonDocumentContext(ExecutionContext* context) {
- if (context->IsWorkerGlobalScope())
- return ToCoreProbeSink(ToWorkerGlobalScope(context));
- if (context->IsMainThreadWorkletGlobalScope())
- return ToCoreProbeSink(ToMainThreadWorkletGlobalScope(context)->GetFrame());
- return nullptr;
+ return context->GetInstrumentingAgents();
}

} // namespace probe
Index: third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.cpp
diff --git a/third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.cpp b/third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.cpp
index 3b11b5281e04b9b97bc76c6e122bb67ee3624147..8c314ab25603ad1d045787082967c73dff310adc 100644
--- a/third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.cpp
+++ b/third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.cpp
@@ -11,6 +11,7 @@
#include "core/frame/FrameConsole.h"
#include "core/frame/LocalFrame.h"
#include "core/inspector/MainThreadDebugger.h"
+#include "core/probe/CoreProbes.h"

namespace blink {

@@ -94,6 +95,10 @@ void MainThreadWorkletGlobalScope::ExceptionThrown(ErrorEvent* event) {
MainThreadDebugger::Instance()->ExceptionThrown(this, event);
}

+CoreProbeSink* MainThreadWorkletGlobalScope::GetInstrumentingAgents() {
+ return probe::ToCoreProbeSink(GetFrame());
+}
+
DEFINE_TRACE(MainThreadWorkletGlobalScope) {
visitor->Trace(loader_set_);
visitor->Trace(object_proxy_);
Index: third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.h
diff --git a/third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.h b/third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.h
index 2496c399f7d641664b4dfab27c9bac34f8c67fd4..2fa237eff030394103a714ace4acc276670e674a 100644
--- a/third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.h
+++ b/third_party/WebKit/Source/core/workers/MainThreadWorkletGlobalScope.h
@@ -53,6 +53,7 @@ class CORE_EXPORT MainThreadWorkletGlobalScope
// ExecutionContext
void AddConsoleMessage(ConsoleMessage*) final;
void ExceptionThrown(ErrorEvent*) final;
+ CoreProbeSink* GetInstrumentingAgents() final;

DECLARE_VIRTUAL_TRACE();

Index: third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
diff --git a/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp b/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
index a4ee138eff77f7102a53cd46587bbe0e57ed0cf6..f9fe4565f5926d18adb71fdbd5d494f74ab68cb0 100644
--- a/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
+++ b/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
@@ -39,6 +39,7 @@
#include "core/frame/DOMTimerCoordinator.h"
#include "core/inspector/ConsoleMessage.h"
#include "core/inspector/ConsoleMessageStorage.h"
+#include "core/inspector/WorkerInspectorController.h"
#include "core/inspector/WorkerThreadDebugger.h"
#include "core/loader/WorkerFetchContext.h"
#include "core/loader/WorkerThreadableLoader.h"
@@ -283,6 +284,13 @@ WorkerEventQueue* WorkerGlobalScope::GetEventQueue() const {
return event_queue_.Get();
}

+CoreProbeSink* WorkerGlobalScope::GetInstrumentingAgents() {
+ if (WorkerInspectorController* controller =
+ GetThread()->GetWorkerInspectorController())
+ return controller->InstrumentingAgents();
+ return nullptr;
+}
+
bool WorkerGlobalScope::IsSecureContext(
String& error_message,
const SecureContextCheck privilege_context_check) const {
Index: third_party/WebKit/Source/core/workers/WorkerGlobalScope.h
diff --git a/third_party/WebKit/Source/core/workers/WorkerGlobalScope.h b/third_party/WebKit/Source/core/workers/WorkerGlobalScope.h
index ec165161ef75c46579b4154b31888734fad4cb16..2fea004d7a34f33412838313e77a9d2aeb86ab3e 100644
--- a/third_party/WebKit/Source/core/workers/WorkerGlobalScope.h
+++ b/third_party/WebKit/Source/core/workers/WorkerGlobalScope.h
@@ -137,6 +137,8 @@ class CORE_EXPORT WorkerGlobalScope
String& error_message,
const SecureContextCheck = kStandardSecureContextCheck) const override;

+ CoreProbeSink* GetInstrumentingAgents() final;
+
// EventTarget
ExecutionContext* GetExecutionContext() const final;

kin...@chromium.org

unread,

Apr 27, 2017, 5:45:51 AM4/27/17

to dgo...@chromium.org, nhi...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

On 2017/04/27 09:07:23, kinuko wrote:
> I'm not sure if there was a strong preference not to do this, but what do you
> think about this change?

Context: I'm trying to decouple resource loading code from Frame and Document,
and thinking about creating a small ExecutionContext class that can be used only
for resource loading and inspector plumbing for the cases we don't have real
documents. (Suggestions to this approach itself is appreciated too)

https://codereview.chromium.org/2847703002/

dgo...@chromium.org

unread,

Apr 27, 2017, 11:33:09 AM4/27/17

to kin...@chromium.org, nhi...@chromium.org, al...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

Note that resolving the sink must be very performant, as we try to impede the
minimum overhead when instrumentation is off.

@alph: what do you think?

https://codereview.chromium.org/2847703002/

alph@chromium.org via codereview.chromium.org

unread,

Apr 27, 2017, 7:40:10 PM4/27/17

to kin...@chromium.org, dgo...@chromium.org, nhi...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

On 2017/04/27 15:33:08, dgozman wrote:
> Note that resolving the sink must be very performant, as we try to impede the
> minimum overhead when instrumentation is off.
>
> @alph: what do you think?

I'm ok with it. I don't think a virtual call is much slower than a check for
context->IsWorkerGlobalScope()
So I'd lgtm it.

https://codereview.chromium.org/2847703002/

dgo...@chromium.org

unread,

Apr 27, 2017, 8:06:39 PM4/27/17

to kin...@chromium.org, nhi...@chromium.org, al...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

kin...@chromium.org

unread,

Apr 27, 2017, 11:16:26 PM4/27/17

to dgo...@chromium.org, nhi...@chromium.org, al...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

Thanks, updated.

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Source/core/dom/ExecutionContext.h
File third_party/WebKit/Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Source/core/dom/ExecutionContext.h#newcode196
third_party/WebKit/Source/core/dom/ExecutionContext.h:196: virtual
CoreProbeSink* GetInstrumentingAgents() { return nullptr; }

On 2017/04/28 00:06:38, dgozman wrote:
> Let's call this GetProbeSink. @alph: we should rename from
instrumenting agents
> elsewhere.

Done.

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Source/core/probe/CoreProbes.cpp
File third_party/WebKit/Source/core/probe/CoreProbes.cpp (right):

https://codereview.chromium.org/2847703002/diff/20001/third_party/WebKit/Source/core/probe/CoreProbes.cpp#newcode139
third_party/WebKit/Source/core/probe/CoreProbes.cpp:139: CoreProbeSink*
ToCoreProbeSinkForNonDocumentContext(ExecutionContext* context) {

On 2017/04/28 00:06:38, dgozman wrote:
> Why not replace ToCoreProbeSink in CoreProbes.h as well?
>
>
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/probe/CoreProbes.h?rcl=8ba4e2e2f7cab6455607be4902f72379c9e49060&l=88

Done.

https://codereview.chromium.org/2847703002/

nhi...@chromium.org

unread,

Apr 28, 2017, 1:25:47 AM4/28/17

to kin...@chromium.org, dgo...@chromium.org, al...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

lgtm

https://codereview.chromium.org/2847703002/

dgo...@chromium.org

unread,

Apr 28, 2017, 2:38:41 PM4/28/17

to kin...@chromium.org, nhi...@chromium.org, al...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

lgtm

https://codereview.chromium.org/2847703002/

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

unread,

Apr 29, 2017, 2:07:13 AM4/29/17

to kin...@chromium.org, dgo...@chromium.org, nhi...@chromium.org, al...@chromium.org, commi...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2847703002/60001

https://codereview.chromium.org/2847703002/

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

unread,

Apr 29, 2017, 2:10:24 AM4/29/17

to kin...@chromium.org, dgo...@chromium.org, nhi...@chromium.org, al...@chromium.org, commi...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

Try jobs failed on following builders:
cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/357821)
chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/423992)
linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/415697)

https://codereview.chromium.org/2847703002/

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

unread,

Apr 29, 2017, 7:05:03 AM4/29/17

to kin...@chromium.org, dgo...@chromium.org, nhi...@chromium.org, al...@chromium.org, commi...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

CQ is trying da patch.

Follow status at:

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2847703002/80001

https://codereview.chromium.org/2847703002/

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

unread,

Apr 29, 2017, 11:19:29 AM4/29/17

to kin...@chromium.org, dgo...@chromium.org, nhi...@chromium.org, al...@chromium.org, commi...@chromium.org, chromium...@chromium.org, kinuko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, shimazu...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, horo+...@chromium.org, falken...@chromium.org, blink-work...@chromium.org, rob....@samsung.com

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b66ac187d7971914c0cbff0bf3733feb5b27f006

https://codereview.chromium.org/2847703002/

Reply all

Reply to author

Forward