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.
Created attachment 311560 [details] Patch
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.
Created attachment 311571 [details] Patch
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.
Created attachment 311605 [details] Patch
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.
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());
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.
Created attachment 311753 [details] Patch
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.
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.
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.
Created attachment 311782 [details] Patch
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.
Created attachment 311794 [details] Patch
Comment on attachment 311794 [details] Patch Clearing flags on attachment: 311794 Committed r217691: <http://trac.webkit.org/changeset/217691>
All reviewed patches have been landed. Closing bug.
(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
Reverted r217691 for reason: This change broke the Windows build. Committed r217694: <http://trac.webkit.org/changeset/217694>
> 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.
Created attachment 311828 [details] Patch [After Rollout]
Comment on attachment 311828 [details] Patch [After Rollout] Clearing flags on attachment: 311828 Committed r217721: <http://trac.webkit.org/changeset/217721>