Bug 215928 – Enable inspection of WebSocket when opening Web Inspector in the middle of the connection like done for the legacy WebSocket implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Enable inspection of WebSocket when opening Web Inspector in the middle of the connection

[youenn fablet]

Created attachment 407469 [details]
Patch

[youenn fablet]

Created attachment 407476 [details]
Patch

[Devin Rousso]

Comment on attachment 407476 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Enable inspection of WebSocket when opening Web Inspector in the middle of the connection

This should already be possible.  Did this functionality regress, or is there something else that this patch enables?

> Source/WebCore/ChangeLog:8
> +        Retrofit API used by Inspector from WebCore::WebSocketChannel  to ThreadableWebSocketChannel.

NIT: double space

> Source/WebCore/ChangeLog:12
> +        Covered by http/tests/websocket/tests/hybi/inspector with NSURLSession WebSocket code path enabled.

Is there any new functionality for this bug?  The title makes me think that there is.  If so, can we add tests to make sure?

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:163
> +    // Dummy implementation of inspector related APIs.

FYI, if you wanted to do worker `WebSocket` inspection I think you'd only need to implement `WorkerNetworkAgent::activeWebSockets` and adjust the logic in `InspectorNetworkAgent::enable` for non-`document`.  See <https://webkit.org/b/168475>.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:817
> +                continue;

Style: I'd add a newline after this.

> Source/WebCore/inspector/agents/page/PageNetworkAgent.cpp:73
> +        auto* channel = webSocket->channel().get();

NIT: do we actually want/need the `.get()`?

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:208
> +    WebSocketFrame closingFrame(WebSocketFrame::OpCodeClose, true, false, true, nullptr, 0);
> +    m_inspector.didSendWebSocketFrame(m_document.get(), closingFrame);

Why do we want/need this fake websocket frame?

(for my own knowledge) Why should this frame be marked as `masked`?

NIT: you should be able to drop the `nullptr` and `0` at the end.

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:328
> +    WebSocketFrame closingFrame(WebSocketFrame::OpCodeClose, true, false, false, nullptr, 0);
> +    m_inspector.didReceiveWebSocketFrame(m_document.get(), closingFrame);

ditto (:207)

> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:423
> +ResourceRequest WebSocketChannel::clientHandshakeRequest(Function<String(const URL&)>&&) const
> +{
> +    return m_handshakeRequest;
> +}
> +
> +const ResourceResponse& WebSocketChannel::serverHandshakeResponse() const
> +{
> +    return m_handshakeResponse;
>  }

NIT: any reason to not inline these in the header?

[youenn fablet]

I’ll change the title to mention this is for the new WebSocket code path.

[youenn fablet]

Comment on attachment 407476 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Enable inspection of WebSocket when opening Web Inspector in the middle of the connection
> 
> This should already be possible.  Did this functionality regress, or is there something else that this patch enables?

This is working for the legacy WebSocket implementation but not the new one based on NSURLSession WebSocket.

>> Source/WebCore/ChangeLog:12
>> +        Covered by http/tests/websocket/tests/hybi/inspector with NSURLSession WebSocket code path enabled.
> 
> Is there any new functionality for this bug?  The title makes me think that there is.  If so, can we add tests to make sure?

Plan is to enable the NSURLSession code path soon. These changes will then be covered by the above tests on BigSur bots.

>> Source/WebCore/inspector/agents/page/PageNetworkAgent.cpp:73
>> +        auto* channel = webSocket->channel().get();
> 
> NIT: do we actually want/need the `.get()`?

Right, will change. Not exactly sure why channel is returning a RefPtr.

>> Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:208
>> +    m_inspector.didSendWebSocketFrame(m_document.get(), closingFrame);
> 
> Why do we want/need this fake websocket frame?
> 
> (for my own knowledge) Why should this frame be marked as `masked`?
> 
> NIT: you should be able to drop the `nullptr` and `0` at the end.

Servers do not mask their frames, only clients do.
Web Inspector is using the masked boolean to decide whether this is a request or response frame.

In legacy code path, WebKit is generating the closing frame and giving it to Web Inspector just before sending it.
NSURLSession is generating the frame for us in NetworkProcess, so we generate it to be on par with legacy code path.

[youenn fablet]

Created attachment 407592 [details]
Patch

[Devin Rousso]

Comment on attachment 407592 [details]
Patch

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

r=me

> Source/WebCore/Modules/websockets/ThreadableWebSocketChannel.h:86
> +    virtual ResourceRequest clientHandshakeRequest(Function<String(const URL&)>&&) const = 0;

NIT: it'd be nice to create a `using` for this so that it's clear what it's for (or include a parameter name):
```
    using RequestHeaderCookieExtractor = Function<String(const URL&)>;
    virtual ResourceRequest clientHandshakeRequest(RequestHeaderCookieExtractor&&) const = 0;
```
(better names welcome 😅)

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:163
> +    // Dummy implementation of inspector related APIs.

please include a comment for the bug for this
```
    // FIXME: <https://webkit.org/b/168475> Web Inspector: Correctly display iframe's and worker's WebSockets
```

> Source/WebKit/WebProcess/Network/WebSocketChannel.h:106
> +    WebCore::ResourceRequest clientHandshakeRequest(Function<String(const URL&)>&&) const final { return m_handshakeRequest; }

Will this have the right `Set-Cookie` header in it?

[youenn fablet]

Thanks for the review. Will fix the nits.

> > Source/WebKit/WebProcess/Network/WebSocketChannel.h:106
> > +    WebCore::ResourceRequest clientHandshakeRequest(Function<String(const URL&)>&&) const final { return m_handshakeRequest; }
> 
> Will this have the right `Set-Cookie` header in it?

It should and we should probably filter this header if inspector is not enabled like we do for other requests/responses.

[youenn fablet]

Created attachment 407673 [details]
Patch for landing

[EWS]

Committed r266391: <https://trac.webkit.org/changeset/266391>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407673 [details].

[Radar WebKit Bug Importer]

<rdar://problem/68138598>

[youenn fablet]

Ah, I forgot to commit the changes to rename the Function.
Changes included in https://bugs.webkit.org/show_bug.cgi?id=216034