Bug 58765

Summary: WebSocket: Add assertion for SocketStreamError::failingURL.
Product: WebKit Reporter: Yuta Kitamura <yutak>
Component: WebCore Misc.Assignee: Yuta Kitamura <yutak>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, ukai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch v2 none

Description Yuta Kitamura 2011-04-18 00:44:33 PDT
Currently SocketStreamError holds "failingURL" but it is not necessary; SocketStreamHandleClient (== WebSocketChannel) should be aware of it.
Comment 1 Yuta Kitamura 2011-04-18 01:00:08 PDT
Created attachment 90000 [details]
Patch
Comment 2 Fumitoshi Ukai 2011-04-18 03:28:05 PDT
Comment on attachment 90000 [details]
Patch

LGTM
Comment 3 Alexey Proskuryakov 2011-04-18 08:52:04 PDT
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 4 Yuta Kitamura 2011-04-18 23:16:21 PDT
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.
Comment 5 Alexey Proskuryakov 2011-04-18 23:23:53 PDT
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.
Comment 6 Yuta Kitamura 2011-04-18 23:34:48 PDT
I'm also OK with keeping failingURL in SocketStreamError (abondoning this change).

Ukai-san, do you have any comments on this?
Comment 7 Fumitoshi Ukai 2011-04-18 23:37:23 PDT
(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?
Comment 8 Yuta Kitamura 2011-04-18 23:57:08 PDT
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.
Comment 9 Fumitoshi Ukai 2011-04-19 00:08:33 PDT
(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.
Comment 10 Yuta Kitamura 2011-04-19 05:14:29 PDT
OK, changing the bug summary to reflect the discussion.
Comment 11 Yuta Kitamura 2011-04-19 06:25:36 PDT
Created attachment 90188 [details]
Patch v2
Comment 12 Fumitoshi Ukai 2011-04-19 19:36:41 PDT
Comment on attachment 90188 [details]
Patch v2

LGTM
Comment 13 WebKit Commit Bot 2011-04-19 21:49:17 PDT
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 14 WebKit Commit Bot 2011-04-19 21:51:28 PDT
Comment on attachment 90188 [details]
Patch v2

Clearing flags on attachment: 90188

Committed r84340: <http://trac.webkit.org/changeset/84340>
Comment 15 WebKit Commit Bot 2011-04-19 21:51:33 PDT
All reviewed patches have been landed.  Closing bug.