Bug 32165

Summary: WebSocket errors should be logged to console
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: pfeldman, pmuellr, ukai, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Log WebSocket error to Web Inspector console.
none
Log WebSocket error to Web Inspector console.
none
Log WebSokcet error to Web Inspector console.
none
Log WebSocket error to Web Inspector console. pfeldman: review+

Description Alexey Proskuryakov 2009-12-04 11:22:33 PST
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.
Comment 1 Patrick Mueller 2009-12-04 12:50:41 PST
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.
Comment 2 Fumitoshi Ukai 2009-12-07 23:29:47 PST
Created attachment 44459 [details]
Log WebSocket error to Web Inspector console.
Comment 3 WebKit Review Bot 2009-12-07 23:34:39 PST
style-queue ran check-webkit-style on attachment 44459 [details] without any errors.
Comment 4 Fumitoshi Ukai 2009-12-07 23:35:32 PST
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.
Comment 5 Alexey Proskuryakov 2009-12-08 14:11:27 PST
         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.
Comment 6 Fumitoshi Ukai 2009-12-08 19:56:45 PST
Created attachment 44508 [details]
Log WebSocket error to Web Inspector console.
Comment 7 WebKit Review Bot 2009-12-08 19:59:59 PST
style-queue ran check-webkit-style on attachment 44508 [details] without any errors.
Comment 8 Pavel Feldman 2009-12-09 08:44:33 PST
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.
Comment 9 Darin Fisher (:fishd, Google) 2009-12-09 08:45:11 PST
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"
Comment 10 Fumitoshi Ukai 2009-12-09 21:08:13 PST
Created attachment 44591 [details]
Log WebSokcet error to Web Inspector console.
Comment 11 WebKit Review Bot 2009-12-09 21:12:36 PST
style-queue ran check-webkit-style on attachment 44591 [details] without any errors.
Comment 12 Pavel Feldman 2009-12-10 05:51:59 PST
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.
Comment 13 Fumitoshi Ukai 2009-12-10 16:09:06 PST
Created attachment 44645 [details]
Log WebSocket error to Web Inspector console.
Comment 14 WebKit Review Bot 2009-12-10 16:12:38 PST
style-queue ran check-webkit-style on attachment 44645 [details] without any errors.
Comment 15 Pavel Feldman 2009-12-10 22:18:31 PST
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.
Comment 16 Fumitoshi Ukai 2009-12-11 02:45:04 PST
Committed r51979: <http://trac.webkit.org/changeset/51979>