Bug 58765

Created attachment 90000 [details] Patch

Comment on attachment 90000 [details] Patch LGTM

Comment on attachment 90000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90000&action=review > Source/WebCore/websockets/WebSocketChannel.cpp:222 > + m_context->addMessage(OtherMessageSource, NetworkErrorMessageType, ErrorMessageLevel, message, 0, m_handshake.url(), 0); What if the error happened before handshake (e.g. DNS resolution failed)? What guarantees that m_handshake.url() will be already set (and that we won't break that in the future)?

Comment on attachment 90000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90000&action=review >> Source/WebCore/websockets/WebSocketChannel.cpp:222 >> + m_context->addMessage(OtherMessageSource, NetworkErrorMessageType, ErrorMessageLevel, message, 0, m_handshake.url(), 0); > > What if the error happened before handshake (e.g. DNS resolution failed)? What guarantees that m_handshake.url() will be already set (and that we won't break that in the future)? WebSocketHandshake::m_url is initialized in the constructor, and m_handshake persists for the same duration as WebSocketChannel, so "m_handshake.url()" will always return the right URL. An existing test http/tests/inspector/console-websocket-error.html checks the exact case you mentioned (DNS resolution failure), so I think we can guarantee the validness of this statement as long as the test passes. However, I agree that the usage of "m_handshake.url()" is a bit confusing. Do you think it's okay to add a m_url member variable to WebSocketChannel to make the code clearer? It adds small amount of memory usage for each WS connection, but I assume it is negligible.

I didn't think about it a lot, but the current solution seems the most straightforward to me. Having a failing URL member in SocketStreamError matches what we have in HTTP, so it's easiest to grasp and remember.

I'm also OK with keeping failingURL in SocketStreamError (abondoning this change). Ukai-san, do you have any comments on this?

(In reply to comment #6) > I'm also OK with keeping failingURL in SocketStreamError (abondoning this change). > > Ukai-san, do you have any comments on this? For now, websocket doesn't support redirection, so requesting URL should be the same as failing URL. Is there any plan to add redirection support in WebSocket protocol?

No, redirection will not happen anytime soon. Then, how about adding an ASSERT that checks if m_handshake.url() is equal to error.failingURL(), instead of removing failingURL? This adds another sanity check, and we'll be able to drop it happily when redirection is implemented.

(In reply to comment #8) > No, redirection will not happen anytime soon. > > Then, how about adding an ASSERT that checks if m_handshake.url() is equal to error.failingURL(), instead of removing failingURL? This adds another sanity check, and we'll be able to drop it happily when redirection is implemented. Sounds good to me to adding an ASSERT. When redirection is implemented, I think we'll need failingURL to know on which connection the error is occurred.

OK, changing the bug summary to reflect the discussion.

Created attachment 90188 [details] Patch v2

Comment on attachment 90188 [details] Patch v2 LGTM

The commit-queue encountered the following flaky tests while processing attachment 90188 [details]: http/tests/appcache/credential-url.html bug 58962 (author: jchaffraix@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 90188 [details] Patch v2 Clearing flags on attachment: 90188 Committed r84340: <http://trac.webkit.org/changeset/84340>

All reviewed patches have been landed. Closing bug.