Bug 58765 – WebSocket: Add assertion for SocketStreamError::failingURL.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yuta Kitamura]

Currently SocketStreamError holds "failingURL" but it is not necessary; SocketStreamHandleClient (== WebSocketChannel) should be aware of it.

[Yuta Kitamura]

Created attachment 90000 [details]
Patch

[Fumitoshi Ukai]

Comment on attachment 90000 [details]
Patch

LGTM

[Alexey Proskuryakov]

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)?

[Yuta Kitamura]

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.

[Alexey Proskuryakov]

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.

[Yuta Kitamura]

I'm also OK with keeping failingURL in SocketStreamError (abondoning this change).

Ukai-san, do you have any comments on this?

[Fumitoshi Ukai]

(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?

[Yuta Kitamura]

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.

[Fumitoshi Ukai]

(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.

[Yuta Kitamura]

OK, changing the bug summary to reflect the discussion.

[Yuta Kitamura]

Created attachment 90188 [details]
Patch v2

[Fumitoshi Ukai]

Comment on attachment 90188 [details]
Patch v2

LGTM

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 90188 [details]:

http/tests/appcache/credential-url.html bug 58962 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 90188 [details]
Patch v2

Clearing flags on attachment: 90188

Committed r84340: <http://trac.webkit.org/changeset/84340>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.