Bug 32165 – WebSocket errors should be logged to console

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Currently, WebSocket errors are logged via LOG and LOG_ERROR, which is only helpful to WebKit developers. People who really need to see these errors are Web developers, so we should print WebSocket errors to Web Inspector console.

[Patrick Mueller]

As long as there's a way for me to NOT see them.  I already consider the XHR messages logged to the console to be spammy.  And I'm happy for "seeing them" to be the default.

[Fumitoshi Ukai]

Created attachment 44459 [details]
Log WebSocket error to Web Inspector console.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44459 [details] without any errors.

[Fumitoshi Ukai]

This patch just logs errors to Web Inspector console.  Should we provide a way to not see them?

I think it would be nice to log more details of WebSocket activities, which should have a way to not see them.  I think it would be like InspectorResource.

(In reply to comment #1)
> As long as there's a way for me to NOT see them.  I already consider the XHR
> messages logged to the console to be spammy.  And I'm happy for "seeing them"
> to be the default.

[Alexey Proskuryakov]

         LOG(Network, "Error in sending handshake message.");
+#if ENABLE(INSPECTOR)
+        m_context->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, ErrorMessageLevel, "Error in sending handshake message.", 0, m_handshake.clientOrigin());
+#endif

I don't think it's useful to keep logging the error with LOG - looks like we're not doing this for e.g. XMLHttpRequest.

Someone working on Web Inspector should probably review this patch.

[Fumitoshi Ukai]

Created attachment 44508 [details]
Log WebSocket error to Web Inspector console.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44508 [details] without any errors.

[Pavel Feldman]

Comment on attachment 44508 [details]
Log WebSocket error to Web Inspector console.

> +        m_context->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, ErrorMessageLevel, "Error in sending handshake message.", 0, m_handshake.clientOrigin());

Why do you dump this using InspectorControllerDestinatio (not the ConsoleDestination)? Latter would dump both into the console clients and inspector controller.

[Darin Fisher (:fishd, Google)]

Comment on attachment 44508 [details]
Log WebSocket error to Web Inspector console.

Some feedback on the error strings:


> +++ b/WebCore/websockets/WebSocketChannel.cpp
...
> +        m_context->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, ErrorMessageLevel, "Error in sending handshake message.", 0, m_handshake.clientOrigin());

nit: "Error sending handshake message."


> +    m_context->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, ErrorMessageLevel, String::format("Too long WebSocket frame %d", m_bufferSize + len), 0, 

nit: "WebSocket frame (at %d bytes) is too long."


> +            m_context->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, ErrorMessageLevel, "short server handshake: " + String(header, len), 0, clientOrigin());

nit: Start with an upper case letter?  "Short server handshake:"


> +            m_context->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, ErrorMessageLevel, "no response code found: " + String(header, len), 0, clientOrigin());

nit: Start with an upper case letter?  "No response code found:"


> +        m_context->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, ErrorMessageLevel, "Mismatch origin: " + clientOrigin() + " != " + m_wsOrigin, 0, clientOrigin());
> +        m_context->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, ErrorMessageLevel, "Mismatch location: " + clientLocation() + " != " + m_wsLocation, 0, clientOrigin());
> +        m_context->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, ErrorMessageLevel, "Mismatch protocol: " + m_clientProtocol + " != " + m_wsProtocol, 0, clientOrigin());

nit: "Origin mismatch" or "Mismatched origin"

[Fumitoshi Ukai]

Created attachment 44591 [details]
Log WebSokcet error to Web Inspector console.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44591 [details] without any errors.

[Pavel Feldman]

Comment on attachment 44591 [details]
Log WebSokcet error to Web Inspector console.

Sorry for not pointing it out earlier - now that you use non-inspector destination, it seems to be fine to make this call no matter whether inspector is there or not. I see this check in Console.cpp.

[Fumitoshi Ukai]

Created attachment 44645 [details]
Log WebSocket error to Web Inspector console.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44645 [details] without any errors.

[Pavel Feldman]

Comment on attachment 44645 [details]
Log WebSocket error to Web Inspector console.

nits: "WebSocket-Origin not found" and "WebSocket-Location not found" should use same pattern as "No response code found". Also "origin found" and "location found" might sound like the corresponding urls could not be resolved, while in your case you want to say that they are missing in headers. Consider reporting something more concrete like "Error during WebSocket handshake: 'websocket-origin' header is missing". Same for location.

[Fumitoshi Ukai]

Committed r51979: <http://trac.webkit.org/changeset/51979>