Bug 179513 – Web Inspector: Include a NetworkAgent in ServiceWorkers for network debugging

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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.

[Radar WebKit Bug Importer]

<rdar://problem/35456402>

[Joseph Pecoraro]

Created attachment 326536 [details]
[PATCH] Proposed Fix

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

Created attachment 326607 [details]
[PATCH] Proposed Fix

[Build Bot]

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.

[Joseph Pecoraro]

Created attachment 326610 [details]
[PATCH] Proposed Fix

[Build Bot]

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.

[Blaze Burg]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 326621 [details]
[PATCH] Proposed Fix

[Build Bot]

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.

[Blaze Burg]

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

r=me

[youenn fablet]

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&

[Devin Rousso]

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?

[youenn fablet]

> 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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

<https://trac.webkit.org/r224788>