Bug 172312 – Web Inspector: Should see active Web Sockets when opening Web Inspector

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Summary:
Should see active Web Sockets when opening Web Inspector.

Steps to Reproduce:
1. Open https://www.websocket.org/echo.html
2. Click Connect to create a web socket
3. Inspect the page
  => Expected to see the open Web Socket connect, but it did not show up

Notes:
- Workaround is to reload the page.

[Devin Rousso]

Created attachment 311560 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311560 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:195
> +HashSet<WebSocket*>& WebSocket::allActiveWebSockets()
> +{
> +    static NeverDestroyed<HashSet<WebSocket*>> activeWebSockets;
> +    return activeWebSockets;
> +}

I think this may need to be threadsafe, since Worker contexts may create these.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:837
> +unsigned WebSocketChannel::identifier() const
> +{
> +    return m_identifier;
> +}

Lets inline this in the header.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:664
> +        Document& document = downcast<Document>(*webSocket->scriptExecutionContext());
> +        if (document.page() != &m_pageAgent->page())
> +            continue;

You cannot assume the scriptExecutionContext here is a Document. It might be a WorkerGlobalScope since a Worker can create a WebSocket. You will need to is<Document> check first.

[Devin Rousso]

Created attachment 311571 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311571 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:165
> +    LockHolder lock(allActiveWebSocketsMutex());
> +
> +    allActiveWebSockets().remove(this);

Its best to scope the lock as finely as possible:

    {
        LockHolder lock(allActiveWebSocketsMutex());
        allActiveWebSockets().remove(this);
    }

Otherwise the lock is held for longer than it needs to be held (e.g. in the other operations in the method like the disconnect).

> Source/WebCore/Modules/websockets/WebSocket.cpp:200
> +HashSet<WebSocket*>& WebSocket::allActiveWebSockets()
> +{
> +    static NeverDestroyed<HashSet<WebSocket*>> activeWebSockets;
> +    return activeWebSockets;
> +}

Since this requires the lock to be held when called, a common strategy is to make this function take a LockHolder reference parameter. This ensures, at compile time, that the person calling this function knows they need to be holding a lock, and do call this holding a lock. For example:

   static void buildBaseTextCodecMaps(const std::lock_guard<StaticLock>&)

I'm actually surprised this isn't done more widely in WebCore. Or maybe I'm missing the pattern used there.

> Source/WebCore/Modules/websockets/WebSocket.cpp:495
> -EventTargetInterface WebSocket::eventTargetInterface() const
> +ScriptExecutionContext* WebSocket::scriptExecutionContext() const
>  {
> -    return WebSocketEventTargetInterfaceType;
> +    return ActiveDOMObject::scriptExecutionContext();
>  }
>  
> -ScriptExecutionContext* WebSocket::scriptExecutionContext() const
> +EventTargetInterface WebSocket::eventTargetInterface() const
>  {
> -    return ActiveDOMObject::scriptExecutionContext();
> +    return WebSocketEventTargetInterfaceType;
>  }

Lets avoid the churn here.

[Devin Rousso]

Created attachment 311605 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311605 [details]
Patch

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

r-, we should be able to write a test for this. Ask me or NVI if you have questions.

> Source/WebCore/Modules/websockets/WebSocketChannel.h:37
> +#include "ResourceRequest.h"
> +#include "ResourceResponse.h"

Avoid adding these includes to the header and use forward declarations.

> Source/WebCore/Modules/websockets/WebSocketChannel.h:118
> +    ResourceRequest clientHandshakeRequest() const;

You should be able to make this a `const ResourceRequest&`. I think that will avoid a copy when using this getter. Note the one below this is constructing an handing us a request. The inspector code only expects a const reference as well.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:679
> +        unsigned identifier = channel->identifier();
> +        didCreateWebSocket(identifier, webSocket->url());
> +        willSendWebSocketHandshakeRequest(identifier, channel->clientHandshakeRequest());
> +        didReceiveWebSocketHandshakeResponse(identifier, channel->serverHandshakeResponse());

What if this WebSocket has only been initialized but hasn't received a Handshake response yet?

If the WebSocketHandshake's Mode is Incomplete, it looks like we haven't yet received a response. So the serverHandshakeResponse would be completely empty and would eventually come in later, moving the Handshake to either Failed or Connected. I think you can check the WebSocket's readyState to determine whether or not to call this.

Likewise, since WebSockets are removed from this list in ~WebSocket it looks like we can include a closed WebSocket in this list. In which case we should send a didCloseWebSocket.

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:522
> +        for (let pendingResourceData of this._pendingFunctionCalls)
> +            pendingResourceData();

This has the potential to be time-consuming and UI breaking if its use expands beyond web sockets.

    1. It may handle a lot of messages at the same time
        - this is something that is normally chunked when the frontend processes incoming messages

    2. Any uncaught exception handling these arbitrary messages may break whatever actual message is being handled
        - a try/catch might reduce exposure

However given its very limited use right now, I'm okay with it. I also couldn't think of reasonably better solution.

[Joseph Pecoraro]

Comment on attachment 311605 [details]
Patch

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

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:673
> +        WebSocketChannel* channel = downcast<WebSocketChannel>(webSocket->channel().get());
> +        if (!channel)

This is a real debug build failure:

    WebCore/inspector/InspectorNetworkAgent.cpp:672:37: note: in instantiation of function template specialization 'WTF::downcast<WebCore::WebSocketChannel, WebCore::ThreadableWebSocketChannel>' requested here
        WebSocketChannel* channel = downcast<WebSocketChannel>(webSocket->channel().get());

[Joseph Pecoraro]

Comment on attachment 311605 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:213
> +        if (!WebInspector.frameResourceManager.mainFrame) {

Shouldn't this just be `this._mainFrame`.

>> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:522
>> +            pendingResourceData();
> 
> This has the potential to be time-consuming and UI breaking if its use expands beyond web sockets.
> 
>     1. It may handle a lot of messages at the same time
>         - this is something that is normally chunked when the frontend processes incoming messages
> 
>     2. Any uncaught exception handling these arbitrary messages may break whatever actual message is being handled
>         - a try/catch might reduce exposure
> 
> However given its very limited use right now, I'm okay with it. I also couldn't think of reasonably better solution.

I think we have a better solution.

Just call PageAgent.getResourceTree earlier (e.g. right after PageAgent.enable) instead of waiting till later.

[Devin Rousso]

Created attachment 311753 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311753 [details]
Patch

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

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:681
> +        if (channel->handshakeMode() != WebSocketHandshake::Connected)
> +            didReceiveWebSocketHandshakeResponse(identifier, channel->serverHandshakeResponse());

This condition doesn't make sense. If the mode is Incomplete won't this be empty?

It seems like this should be:

    if (webSocket->readyState() != CONNECTING)

In which case you wouldn't need to expose the handshakeMode. But I didn't look deeply, maybe the handshake mode is more accurate.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:684
> +        if (webSocket->readyState() == WebSocket::CLOSED)
> +            didCloseWebSocket(identifier);

We should extend your test below to cover this.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:11
> +const url = "ws://127.0.0.1:8880/websocket/tests/hybi/inspector/before-load";
> +let webSocket = new WebSocket(url);
> +webSocket.addEventListener("open", (event) => {
> +    // Only run the tests once the websocket has connected.
> +    runTest();
> +});

Lets add a test for two web sockets.

    (1) one that is open and connected (window.webSocketConnected)
    (2) one that is closed but still alive (window.webSocketClosed)

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:21
> +            InspectorTest.evaluateInPage("webSocket.close()");

Why are we doing this?

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:25
> +                InspectorTest.pass(`WebSocket resource exists for url "${url}"`);

You should confirm that this is a WebInspector.WebSocket and that its request/response data has headers etc.

[Devin Rousso]

Comment on attachment 311753 [details]
Patch

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

>> Source/WebCore/inspector/InspectorNetworkAgent.cpp:681
>> +            didReceiveWebSocketHandshakeResponse(identifier, channel->serverHandshakeResponse());
> 
> This condition doesn't make sense. If the mode is Incomplete won't this be empty?
> 
> It seems like this should be:
> 
>     if (webSocket->readyState() != CONNECTING)
> 
> In which case you wouldn't need to expose the handshakeMode. But I didn't look deeply, maybe the handshake mode is more accurate.

This was a typo.  It is supposed to be:

if (webSocket->readyState() == CONNECTING)

The reasoning for this is that `InspectorInstrumentation::didReceiveWebSocketHandshakeResponse` is only called if the WebSocketChannel's `m_handshake->mode() == WebSocketHandshake::Connected`.

>> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:21
>> +            InspectorTest.evaluateInPage("webSocket.close()");
> 
> Why are we doing this?

I figured that it would be a good idea to close the websocket.  I guess it isn't necessary.

[Devin Rousso]

Comment on attachment 311753 [details]
Patch

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

>> Source/WebCore/inspector/InspectorNetworkAgent.cpp:684
>> +            didCloseWebSocket(identifier);
> 
> We should extend your test below to cover this.

So I did some digging, and I think that this event will rarely (if ever) be fired.  When the WebSocket closes (or is stopped, although I'm not sure when that happens), it sets its WebSocketChannel to `nullptr`, meaning that even though it will still be in `allActiveWebSockets`, it won't be able to fire any events as the WebSocketChannel is what holds all the data needed for the events (namely the `requestIdentifier`).  I don't think we need this event (although it might be a good idea to keep it if there are other scenarios I didn't discover) or the test for it.

[Devin Rousso]

Created attachment 311782 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311782 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        New tests:
> +         - http/tests/websocket/tests/hybi/inspector/before-load.html

The isn't the right syntax for a ChangeLog. It should be:

    Test: http/tests/websocket/tests/hybi/inspector/before-load.html

> Source/WebCore/Modules/websockets/WebSocket.cpp:155
> +

Style: remove this unnecessary empty line.

> Source/WebCore/Modules/websockets/WebSocket.h:88
> +    const RefPtr<ThreadableWebSocketChannel> channel() const;

I don't think we normally use `const RefPtr`, that doesn't really add any value over `RefPtr`.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:684
> +        if (webSocket->readyState() == WebSocket::CLOSED)
> +            didCloseWebSocket(identifier);

Based on your last comments it sounds like you may be able to ASSERT that this is not the case, or just leave this in.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:41
> +        test(resolve, reject) {

Can we also assert that the number of WebSocketResource instances is 1:

    let webSocketResources = WebInspector.frameResourceManager.mainFrame.resourceCollection.resourceCollectionForType(WebInspector.Resource.Type.WebSocket);
    InspectorTest.expectEqual(webSocketResources.items.size, 1, "Main frame should have 1 WebSocketResource.");

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:53
> +            logReadyState(webSocketResource.readyState);

Style: Just log String(webSocketResource.readyState).
Style: Lets shorten the Symbol descriptions from "web-socket-ready-state-foo" to just "foo".
Style: We normally phrase expectation messages as "Foo should be bar." sentences. See other tests for examples.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:63
> +<body onload="testRunner.waitUntilDone();">

Style: I'd prefer this be broken out into a function above. Its super easy to miss here.

[Devin Rousso]

Created attachment 311794 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 311794 [details]
Patch

Clearing flags on attachment: 311794

Committed r217691: <http://trac.webkit.org/changeset/217691>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #16)
> Comment on attachment 311794 [details]
> Patch
> 
> Clearing flags on attachment: 311794
> 
> Committed r217691: <http://trac.webkit.org/changeset/217691>

This change broke the Windows build:
https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/1934

[Ryan Haddad]

Reverted r217691 for reason:

This change broke the Windows build.

Committed r217694: <http://trac.webkit.org/changeset/217694>

[Joseph Pecoraro]

> Committed r217694: <http://trac.webkit.org/changeset/217694>

I landed a Windows build fix r217693..., but since this got rolled out, just reland it tomorrow (put a patch up to make sure Windows EWS is okay with it this time). Always check a Red EWS, even if no comment was added to the bugzilla.

[Devin Rousso]

Created attachment 311828 [details]
Patch [After Rollout]

[WebKit Commit Bot]

Comment on attachment 311828 [details]
Patch [After Rollout]

Clearing flags on attachment: 311828

Committed r217721: <http://trac.webkit.org/changeset/217721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.