Bug 163844 – Web Inspector: Include ConsoleAgent in Workers - real console.log support

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Summary:
Include ConsoleAgent in Workers - real console.log support

Current console.log support in the Worker just proxied through the Page and dropped JSValues on the floor. This meant that you could only log a single argument and it would be stringified instead of a real intractable object.

Now we can just implement ConsoleAgent in the Worker's InspectorController and get real ConsoleMessage support. When an inspector frontend connects it can receive the real ConsoleMessages with RemoteObjects in the WorkerGlobalScope as expected in the console.

There is one challenge here. We must ensure we've enabled all our agents before the Worker evaluates its initial script. So we are not racing against any `console.logs` in the script and ConsoleAgent.enable.

Notes:
- This will complete bug 154213 as well.

[Radar WebKit Bug Importer]

<rdar://problem/28903328>

[Joseph Pecoraro]

Created attachment 292491 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 292492 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 292493 [details]
[PATCH] For Bots - All Parts

[Joseph Pecoraro]

*** Bug 154213 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Attachment 292493 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/WorkerInspectorController.cpp:88:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/inspector/WorkerInspectorController.cpp:96:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 88 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 292493 [details]
[PATCH] For Bots - All Parts

Attachment 292493 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2344326

New failing tests:
imported/w3c/web-platform-tests/fetch/api/policies/csp-blocked-worker.html
imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection.https.html
http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp.html
http/tests/security/contentSecurityPolicy/worker-multiple-csp-headers.html
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp-importScripts-block-aborts-all-subsequent-imports.html
http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html
http/tests/security/contentSecurityPolicy/worker-set-timeout-blocked.html
http/tests/security/contentSecurityPolicy/worker-importscripts-blocked.html
http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple.html
security/contentSecurityPolicy/worker-inherits-blocks-xhr.html

[Build Bot]

Created attachment 292495 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292493 [details]
[PATCH] For Bots - All Parts

Attachment 292493 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2344332

New failing tests:
imported/w3c/web-platform-tests/fetch/api/policies/csp-blocked-worker.html
imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection.https.html
http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp.html
http/tests/security/contentSecurityPolicy/worker-multiple-csp-headers.html
security/contentSecurityPolicy/worker-inherits-blocks-xhr.html
http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html
http/tests/security/contentSecurityPolicy/worker-set-timeout-blocked.html
http/tests/security/contentSecurityPolicy/worker-importscripts-blocked.html
http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple.html
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp-importScripts-block-aborts-all-subsequent-imports.html

[Build Bot]

Created attachment 292496 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292493 [details]
[PATCH] For Bots - All Parts

Attachment 292493 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2344327

New failing tests:
imported/w3c/web-platform-tests/fetch/api/policies/csp-blocked-worker.html
imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection.https.html
http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp.html
http/tests/security/contentSecurityPolicy/worker-multiple-csp-headers.html
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp-importScripts-block-aborts-all-subsequent-imports.html
http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html
http/tests/security/contentSecurityPolicy/worker-set-timeout-blocked.html
http/tests/security/contentSecurityPolicy/worker-importscripts-blocked.html
http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple.html
security/contentSecurityPolicy/worker-inherits-blocks-xhr.html

[Build Bot]

Created attachment 292497 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292493 [details]
[PATCH] For Bots - All Parts

Attachment 292493 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2344346

New failing tests:
imported/w3c/web-platform-tests/fetch/api/policies/csp-blocked-worker.html
imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection.https.html
http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp.html
http/tests/security/contentSecurityPolicy/worker-multiple-csp-headers.html
http/tests/websocket/tests/hybi/workers/close.html
http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html
http/tests/security/contentSecurityPolicy/worker-set-timeout-blocked.html
http/tests/security/contentSecurityPolicy/worker-importscripts-blocked.html
http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple.html
security/contentSecurityPolicy/worker-inherits-blocks-xhr.html
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp-importScripts-block-aborts-all-subsequent-imports.html

[Build Bot]

Created attachment 292498 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

> New failing tests:
> http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html
> http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp.html
> http/tests/security/contentSecurityPolicy/worker-multiple-csp-headers.html
> ...

These are failing because I eliminated the Worker console messages going to the Page and then to the WebKit Client that outputs with CONSOLE MESSAGE. If we want to keep that we should discuss how it should work. The current implementation is limited to a stringification of the first argument of console.log, we could preserve that.

[Joseph Pecoraro]

Created attachment 292943 [details]
[PATCH] For Bots - All Parts

[WebKit Commit Bot]

Attachment 292943 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/WorkerInspectorController.cpp:88:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/inspector/WorkerInspectorController.cpp:96:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: LayoutTests/imported/w3c/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 3 in 96 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Comment on attachment 292492 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=292492&action=review

r=me

> LayoutTests/inspector/worker/console-basic.html:18
> +    function remoteObjectJSONFilter(key, value) {

Should this be reused?

> LayoutTests/inspector/worker/console-basic.html:74
> +        preprocess(message) {

I think we should name this 'inspect' or 'validate' since the message itself isn't altered.

> Source/JavaScriptCore/inspector/protocol/Worker.json:16
> +            "description": "Sent by the frontend after all initialization messages have been sent for this worker.",

"after the frontend has initialized and is ready to receive messages from this worker."

> Source/WebCore/workers/WorkerInspectorProxy.cpp:35
>  #include <inspector/InspectorAgentBase.h>

Joe thinks: "If the inspector closes before WorkerAgent.initialized, the worker will hang." Disconnect should make sure to resume. Maybe we should have a test for this case.

> Source/WebCore/workers/WorkerInspectorProxy.cpp:61
> +    bool pauseOnStart = InspectorInstrumentation::shouldWaitForDebuggerOnStart(scriptExecutionContext);

I am slightly surprised we need to go through instrumentation to ask WorkerAgent. Does WorkerInspectorProxy have access to WorkerInspectorController via WorkerGlobalScope?

> Source/WebCore/workers/WorkerThread.cpp:206
> +void WorkerThread::startRunningDebuggerTasks()

I would prefer if this were named in terms of enter/exit nested run loops.

enterNestedRunLoopForDebugger()
exitNestedRunLoopForDebugger()

and m_shouldProcessDebuggerTasks

[Joseph Pecoraro]

Comment on attachment 292492 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=292492&action=review

>> LayoutTests/inspector/worker/console-basic.html:18
>> +    function remoteObjectJSONFilter(key, value) {
> 
> Should this be reused?

sanitizeURL is used in far more tests. This however is getting popular! So yes, maybe eventually we should share it. I tend to make this different in different tests, since the RemoteObject tests might actually want to include things like the _hasChildren properties.

>> LayoutTests/inspector/worker/console-basic.html:74
>> +        preprocess(message) {
> 
> I think we should name this 'inspect' or 'validate' since the message itself isn't altered.

I like that!

>> Source/JavaScriptCore/inspector/protocol/Worker.json:16
>> +            "description": "Sent by the frontend after all initialization messages have been sent for this worker.",
> 
> "after the frontend has initialized and is ready to receive messages from this worker."

I worded this as: "Sent after the frontend has sent all initialization messages and can resume this worker. This command is required to allow execution in the worker.". 

The fronted can already receive messages. The important thing about sending initialized, is that the frontend has sent all its initialization messages (Foo.enable, Debugger.setBreakpoint, ...) before anything has executed in the Worker. This will ensure that if the first statement in the worker script has a breakpoint, or is a console.log, that we get expected behavior, instead of a race between the breakpoint getting set, or the  console getting enabled.

>> Source/WebCore/workers/WorkerInspectorProxy.cpp:35
>>  #include <inspector/InspectorAgentBase.h>
> 
> Joe thinks: "If the inspector closes before WorkerAgent.initialized, the worker will hang." Disconnect should make sure to resume. Maybe we should have a test for this case.

I should be able to write a test for this. I'll have a follow-up fix for this this specific case since it should be nice and small.

>> Source/WebCore/workers/WorkerInspectorProxy.cpp:61
>> +    bool pauseOnStart = InspectorInstrumentation::shouldWaitForDebuggerOnStart(scriptExecutionContext);
> 
> I am slightly surprised we need to go through instrumentation to ask WorkerAgent. Does WorkerInspectorProxy have access to WorkerInspectorController via WorkerGlobalScope?

This is before the WorkerGlobalScope has constructed. This is right after we have successfully loaded the new Worker("foo.js") script, and are about to start the worker.

>> Source/WebCore/workers/WorkerThread.cpp:206
>> +void WorkerThread::startRunningDebuggerTasks()
> 
> I would prefer if this were named in terms of enter/exit nested run loops.
> 
> enterNestedRunLoopForDebugger()
> exitNestedRunLoopForDebugger()
> 
> and m_shouldProcessDebuggerTasks

This is not actually a nested run loop case. During thread startup, we run the WorkerRunLoop in this mode to process debugger commands, and then afterwards we run the run loop in the default mode. Nested run loops will happen later in WorkerScriptDebugServer. And it turns out I didn't use these methods because it needs to wait on a different boolean (one held not by the thread but by WorkerScriptDebugServer).

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/208010>