Include a NetworkAgent in ServiceWorkers for network debugging This will include all of the necessary parts on the backend (WebCore) to inform the frontend (Web Inspector window) of Network Requests created by a ServiceWorker. This doesn't include the necessary changes to the Web Inspector frontend to support showing a Network Tab when debugging a ServiceWorker. That will be a follow-up.
<rdar://problem/35456402>
Created attachment 326536 [details] [PATCH] Proposed Fix
Attachment 326536 [details] did not pass style-queue: ERROR: Source/WebCore/workers/WorkerMessagingProxy.h:68: The parameter name "disabled" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/workers/WorkerDebuggerProxy.h:39: The parameter name "disabled" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/inspector/agents/page/PageNetworkAgent.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/inspector/agents/InspectorNetworkAgent.h:121: The parameter name "disabled" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h:66: The parameter name "disabled" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/inspector/agents/worker/WorkerNetworkAgent.h:42: The parameter name "disabled" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/inspector/agents/page/PageNetworkAgent.h:42: The parameter name "disabled" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/loader/WorkerThreadableLoader.cpp:110: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 8 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 326536 [details] [PATCH] Proposed Fix Attachment 326536 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5172800 New failing tests: inspector/codemirror/prettyprinting-javascript.html http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html inspector/codemirror/prettyprinting-css.html http/tests/inspector/network/loadResource-insecure-resource.html
Created attachment 326541 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 326536 [details] [PATCH] Proposed Fix Attachment 326536 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5172927 New failing tests: inspector/codemirror/prettyprinting-javascript.html http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html inspector/codemirror/prettyprinting-css.html http/tests/inspector/network/loadResource-insecure-resource.html
Created attachment 326546 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 326607 [details] [PATCH] Proposed Fix
Attachment 326607 [details] did not pass style-queue: ERROR: Source/WebCore/loader/WorkerThreadableLoader.cpp:110: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 326610 [details] [PATCH] Proposed Fix
Attachment 326610 [details] did not pass style-queue: ERROR: Source/WebCore/loader/WorkerThreadableLoader.cpp:110: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 326536 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=326536&action=review > Source/WebCore/inspector/InspectorInstrumentation.h:181 > + static void willSendRequest(WorkerGlobalScope&, unsigned long identifier, ResourceRequest&); Might want to comment here that these are for SW, not for web workers that have a page. If these are truly only for SW and we don't intend to reroute web workers through this code path, then these new things should be guarded with ENABLE(SERVICE_WORKER) so that's obvious. > Source/WebCore/inspector/WorkerInspectorController.cpp:171 > + auto workerContext = workerAgentContext(); This local can go inside the branch. > Source/WebCore/inspector/WorkerInspectorController.h:75 > + void createLazyAgents(); I find this name a little weird. Wouldn't we normally call this createAgentsIfNeeded or ensureAgents() ? > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:619 > + // FIXME: Worker support. What would the initiator be once this is finished? A new type? > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:721 > + ASSERT(is<WebSocketChannel>(webSocket->channel().get())); This seems more strict about the subtype. Is that okay? > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:-708 > - Document* document = downcast<Document>(webSocket->scriptExecutionContext()); It took me a while to figure out that activeWebSockets was the new point of page vs worker differentiation. > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:853 > + // FIXME: <https://webkit.org/b/168475> Web Inspector: Correctly display iframe's and worker's WebSockets Why would this not work for iframe websockets? an iframe has a document..? > Source/WebCore/inspector/agents/page/PageNetworkAgent.cpp:77 > + if (document->page() != &m_pageAgent->page()) This comment makes more sense given this condition. It is horrifying we don't support this, though. > Source/WebCore/loader/WorkerThreadableLoader.cpp:211 > Vector<char> vector(dataLength); Perhaps this is better named 'buffer', at the least? > Source/WebCore/loader/WorkerThreadableLoader.cpp:216 > + InspectorInstrumentation::didReceiveData(downcast<WorkerGlobalScope>(context), workerRequestIdentifier, vector.data(), vector.size()); ,...because this reads poorly with 'vector' > Source/WebCore/loader/WorkerThreadableLoader.h:120 > + unsigned long m_workerRequestIdentifier; Nit: { 0 } or make it std::optional? > Source/WebCore/loader/WorkerThreadableLoader.h:121 > + NetworkLoadMetrics m_networkLoadMetrics; Ditto.
Comment on attachment 326610 [details] [PATCH] Proposed Fix Attachment 326610 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5180637 New failing tests: http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html
Created attachment 326615 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 326610 [details] [PATCH] Proposed Fix Attachment 326610 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5180626 New failing tests: http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html svg/wicd/test-rightsizing-a.xhtml
Created attachment 326616 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 326610 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=326610&action=review > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:832 > if (IdentifiersFactory::requestId(channel->identifier()) != requestId) Oops this should be `==` now to fix the test.
Comment on attachment 326536 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=326536&action=review >> Source/WebCore/inspector/InspectorInstrumentation.h:181 >> + static void willSendRequest(WorkerGlobalScope&, unsigned long identifier, ResourceRequest&); > > Might want to comment here that these are for SW, not for web workers that have a page. > > If these are truly only for SW and we don't intend to reroute web workers through this code path, then these new things should be guarded with ENABLE(SERVICE_WORKER) so that's obvious. These can be used for a DedicatedWorker and we may transition to that eventually. Right now its harmless for a DedicatedWorker since there will never be an active NetworkAgent in the InstrumentingAgents for a DedicatedWorker, only a ServiceWorker. That said, this would allow us to log console warnings for failed loads in DedicatedWorkers via didFailLoading. >> Source/WebCore/inspector/WorkerInspectorController.h:75 >> + void createLazyAgents(); > > I find this name a little weird. Wouldn't we normally call this createAgentsIfNeeded or ensureAgents() ? Hmm, I'll move in that direction in a follow-up. Right now we share the same names as the Page's InspectorController and I can move them all at once and try to make a few more lazy. >> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:619 >> + // FIXME: Worker support. > > What would the initiator be once this is finished? A new type? We just need to be able to get a JavaScript backtrace from a WorkerThread instead of assuming the MainThread (`JSMainThreadExecState::currentState()` below). >> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:721 >> + ASSERT(is<WebSocketChannel>(webSocket->channel().get())); > > This seems more strict about the subtype. Is that okay? We expect activeWebSockets() to return us a list of WebSockets that have channels. This is just asserting that. >> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:853 >> + // FIXME: <https://webkit.org/b/168475> Web Inspector: Correctly display iframe's and worker's WebSockets > > Why would this not work for iframe websockets? an iframe has a document..? The frontend doesn't properly associate an iframe's WebSocket with the iframe. >> Source/WebCore/inspector/agents/page/PageNetworkAgent.cpp:77 >> + if (document->page() != &m_pageAgent->page()) > > This comment makes more sense given this condition. It is horrifying we don't support this, though. We show WebSockets in iframe, but we don't associate it with the frame. I believe the frontend just thinks its top level. >> Source/WebCore/loader/WorkerThreadableLoader.cpp:216 >> + InspectorInstrumentation::didReceiveData(downcast<WorkerGlobalScope>(context), workerRequestIdentifier, vector.data(), vector.size()); > > ,...because this reads poorly with 'vector' Agreed. >> Source/WebCore/loader/WorkerThreadableLoader.h:120 >> + unsigned long m_workerRequestIdentifier; > > Nit: { 0 } or make it std::optional? This always gets initialized in the constructor, but I can put a 0 here.
Created attachment 326621 [details] [PATCH] Proposed Fix
Attachment 326621 [details] did not pass style-queue: ERROR: Source/WebCore/loader/WorkerThreadableLoader.cpp:110: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 326621 [details] [PATCH] Proposed Fix r=me
LGTM too. Some potential nits below. View in context: https://bugs.webkit.org/attachment.cgi?id=326621&action=review > Source/WebCore/inspector/agents/page/PageNetworkAgent.h:39 > + String loaderIdentifier(DocumentLoader*) override; I believe style says to use final in that case. > Source/WebCore/inspector/agents/worker/WorkerNetworkAgent.h:35 > + WorkerNetworkAgent(WorkerAgentContext&); explicit? > Source/WebCore/inspector/agents/worker/WorkerNetworkAgent.h:43 > + ScriptExecutionContext* scriptExecutionContext(ErrorString&, const String& frameId) override; final as well? > Source/WebCore/loader/WorkerThreadableLoader.cpp:202 > + ASSERT(context.isWorkerGlobalScope()); Probably somehow redundant with the downcast below, maybe put the downcast upfront would be better? Ditto in other places. > Source/WebCore/workers/WorkerGlobalScope.h:67 > + String identifier() const { return m_identifier; } const String&
Comment on attachment 326621 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=326621&action=review Looks good! My only general comment is that I would prefer using std::optional<DocumentLoader&> instead of DocumentLoader* (or even a Ref<DocumentLoader>. The reason for this is based on what was discussed at the WebKit Contributors Meeting, and that we should try to avoid adding more raw pointers and use our own controllers where able. I think this is fine as it is, as we don't save the DocumentLoader, but if we need to in the future it would help avoid potential memory leaks or null-dereferences. > Source/WebCore/ChangeLog:19 > + This patch add InspectorInstrumentation networking hooks for workers inside Typo: adds > Source/WebCore/ChangeLog:97 > + Drive-by use a move. Drive-by: use a move. > Source/WebCore/ChangeLog:142 > + Add some assertions for all Worker agents for clarify. Typo: clarity. > Source/WebCore/ChangeLog:154 > + * workers/WorkerMessagingProxy.h: NIT: I typically move the *.h above the *.cpp so it's easier to read. > Source/WebCore/ChangeLog:157 > + * workers/service/context/ServiceWorkerThreadProxy.h: Ditto (154). > Source/WebCore/inspector/WorkerInspectorController.cpp:129 > + InspectorInstrumentation::frontendDeleted(); Do we also want to stop the m_executionStopwatch, or is that not a problem? > Source/WebCore/inspector/agents/InspectorNetworkAgent.h:117 > + // Subclasses. Is this comment necessary?
> Looks good! My only general comment is that I would prefer using > std::optional<DocumentLoader&> instead of DocumentLoader* (or even a > Ref<DocumentLoader>. The reason for this is based on what was discussed at > the WebKit Contributors Meeting, and that we should try to avoid adding more > raw pointers and use our own controllers where able. I think this is fine > as it is, as we don't save the DocumentLoader, but if we need to in the > future it would help avoid potential memory leaks or null-dereferences. I am not sure of this. There also was some discussion on the mailing list, going in the direction of keeping raw pointers here. To me, references and raw pointers have the same security risks, it is just that one may be null and not the other.
Comment on attachment 326621 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=326621&action=review >> Source/WebCore/ChangeLog:154 >> + * workers/WorkerMessagingProxy.h: > > NIT: I typically move the *.h above the *.cpp so it's easier to read. I do as well, but normally only when I move things around (like I did with pageNetworkAgent WorkerNetworkAgent above). I'll do it here and try to do more in the future. Maybe we should tweak prepare-ChangeLog (or make it an option). >> Source/WebCore/inspector/WorkerInspectorController.cpp:129 >> + InspectorInstrumentation::frontendDeleted(); > > Do we also want to stop the m_executionStopwatch, or is that not a problem? Not a problem. A Stopwatch is just a few numbers so there isn't any active work that we need to stop and we know that the next time we get a frontend we will restart the stopwatch so it'll be good then.
<https://trac.webkit.org/r224788>