Bug 163844 - Web Inspector: Include ConsoleAgent in Workers - real console.log support
Summary: Web Inspector: Include ConsoleAgent in Workers - real console.log support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
: 154213 (view as bug list)
Depends on: 163835
Blocks: 127634 164073
  Show dependency treegraph
 
Reported: 2016-10-22 01:41 PDT by Joseph Pecoraro
Modified: 2016-10-27 15:23 PDT (History)
9 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (79.31 KB, patch)
2016-10-22 03:38 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (79.50 KB, patch)
2016-10-22 03:39 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff
[PATCH] For Bots - All Parts (495.06 KB, patch)
2016-10-22 03:40 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (971.86 KB, application/zip)
2016-10-22 04:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.14 MB, application/zip)
2016-10-22 04:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.64 MB, application/zip)
2016-10-22 04:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (12.60 MB, application/zip)
2016-10-22 04:56 PDT, Build Bot
no flags Details
[PATCH] For Bots - All Parts (519.84 KB, patch)
2016-10-26 10:44 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-10-22 01:41:42 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-10-22 01:42:14 PDT
<rdar://problem/28903328>
Comment 2 Joseph Pecoraro 2016-10-22 03:38:55 PDT
Created attachment 292491 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2016-10-22 03:39:33 PDT
Created attachment 292492 [details]
[PATCH] Proposed Fix
Comment 4 Joseph Pecoraro 2016-10-22 03:40:14 PDT
Created attachment 292493 [details]
[PATCH] For Bots - All Parts
Comment 5 Joseph Pecoraro 2016-10-22 03:40:52 PDT
*** Bug 154213 has been marked as a duplicate of this bug. ***
Comment 6 WebKit Commit Bot 2016-10-22 03:43:21 PDT
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.
Comment 7 Build Bot 2016-10-22 04:40:10 PDT
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
Comment 8 Build Bot 2016-10-22 04:40:13 PDT
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
Comment 9 Build Bot 2016-10-22 04:42:17 PDT
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
Comment 10 Build Bot 2016-10-22 04:42:20 PDT
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
Comment 11 Build Bot 2016-10-22 04:51:41 PDT
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
Comment 12 Build Bot 2016-10-22 04:51:44 PDT
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
Comment 13 Build Bot 2016-10-22 04:56:11 PDT
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
Comment 14 Build Bot 2016-10-22 04:56:15 PDT
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
Comment 15 Joseph Pecoraro 2016-10-23 12:30:06 PDT
> 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.
Comment 16 Joseph Pecoraro 2016-10-26 10:44:09 PDT
Created attachment 292943 [details]
[PATCH] For Bots - All Parts
Comment 17 WebKit Commit Bot 2016-10-26 15:47:10 PDT
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.
Comment 18 Brian Burg 2016-10-26 16:27:35 PDT
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
Comment 19 Joseph Pecoraro 2016-10-27 12:53:47 PDT
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).
Comment 20 Joseph Pecoraro 2016-10-27 15:23:43 PDT
<https://trac.webkit.org/changeset/208010>