| Summary: | Web Inspector: Include ConsoleAgent in Workers - real console.log support | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||||||||
| Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | bburg, buildbot, commit-queue, joepeck, mattbaker, nvasilyev, rniwa, timothy, webkit-bug-importer | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Bug Depends on: | 163835 | ||||||||||||||||||||
| Bug Blocks: | 127634, 164073 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Joseph Pecoraro
2016-10-22 01:41:42 PDT
Created attachment 292491 [details]
[PATCH] Proposed Fix
Created attachment 292492 [details]
[PATCH] Proposed Fix
Created attachment 292493 [details]
[PATCH] For Bots - All Parts
*** Bug 154213 has been marked as a duplicate of this bug. *** 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 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 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 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 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 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 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 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 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
> 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.
Created attachment 292943 [details]
[PATCH] For Bots - All Parts
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 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 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). |