Bug 57138 – WebSocket urls should always be encoded as UTF-8

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Joe Mason]

If a document includes a URL whose query includes non-ASCII chars, we normally encode it using the document's encoding.  But the websocket spec specifies that websocket URLs should always be encoded with UTF-8 (see http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-06#section-3 - hasn't changed since -00).

[Joe Mason]

Created attachment 87005 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 87005 [details]
patch

What about GKURL?

[Adam Barth]

Comment on attachment 87005 [details]
patch

I don't think this change is correct.  The requirement you cite is w.r.t. the WebSockets protocol, but you've changed how ws and wss behave for all protocols.  Instead, you should make the change in the implementation of the WebSocket protocol.

[Joe Mason]

Created attachment 87180 [details]
updated patch

How's this?

Quite a few ways to implement Document::completeURLWithUTF8 - since from the comments completeURL seems to be performance critical, I chose to make both completeURL and completeURLWithUTF8 into inlines wrapper to a new func which takes a bool param to force UTF-8 or not.  The alternative is to not touch completeURL, and copy the code to a new function which differs only in how it uses m_decoder.

[Adam Barth]

Comment on attachment 87180 [details]
updated patch

Thanks!  Looks good, but I'd like to look at it more carefully before marking r+.  Other reviewers should feel free to finish the review if they like,

[Adam Barth]

Comment on attachment 87180 [details]
updated patch

Looks great!

[WebKit Commit Bot]

Comment on attachment 87180 [details]
updated patch

Rejecting attachment 87180 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build'..." exit_code: 2

Last 500 characters of output:
 XCODE_APP_SUPPORT_DIR /Developer/Library/Xcode
    setenv XCODE_VERSION_ACTUAL 0322
    setenv XCODE_VERSION_MAJOR 0300
    setenv XCODE_VERSION_MINOR 0320
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /mnt/git/webkit-commit-queue/WebKitBuild/WebKit.build/Debug/WebKit.build/Script-5DE6D18C0FCF231B002DE28C.sh

** BUILD FAILED **


The following build commands failed:
WebKit:
	Ld /mnt/git/webkit-commit-queue/WebKitBuild/Debug/WebKit.framework/Versions/A/WebKit normal x86_64
(1 failure)


Full output: http://queues.webkit.org/results/8507818

[Joe Mason]

Created attachment 91574 [details]
fix compile error

Looks like on some platforms completeURL is called across translation units, so it can't be inlined.  This is the same patch, but without inlining completeURL.

[Alexey Proskuryakov]

Comment on attachment 91574 [details]
fix compile error

Why do we even call completeURL on these if they must be absolute in the first place? I think that you should simply change parseURL() call to KURL(KURL(), url, utf8Encoding());

[Joe Mason]

Comment on attachment 91574 [details]
fix compile error

Because completeURL does more than check the encoding: it also checks the document's m_baseURL, and only falls back to using KURL() if it is not set.

[Alexey Proskuryakov]

Comment on attachment 91574 [details]
fix compile error

As I already said, URLs passed to WebSocket constructor need to be absolute, so looking at document base URL is not necessary.

[Joe Mason]

Created attachment 92543 [details]
simplified fix

[Alexey Proskuryakov]

Comment on attachment 92543 [details]
simplified fix

+        // Use a String instead of a KURL for the url to ensure that WebSocket
+        // parses the url itself using WebSocket rules (must be absolute and
+        // UTF-8 encoded).

I don't see how the comment can be useful to someone reading this code. There is no option to use KURL, so explaining why String is better than KURL is pointless.

Otherwise, looks fine to me. Adam?

[Adam Barth]

Comment on attachment 92543 [details]
simplified fix

View in context: https://bugs.webkit.org/attachment.cgi?id=92543&action=review

> Source/WebCore/websockets/WebSocket.h:66
> +        // Use a String instead of a KURL for the url to ensure that WebSocket
> +        // parses the url itself using WebSocket rules (must be absolute and
> +        // UTF-8 encoded).

Yeah, I agree.  Having a test for this behavior is more valuable than having a comment.

[Joe Mason]

(In reply to comment #13)
> (From update of attachment 92543 [details])
> +        // Use a String instead of a KURL for the url to ensure that WebSocket
> +        // parses the url itself using WebSocket rules (must be absolute and
> +        // UTF-8 encoded).
> 
> I don't see how the comment can be useful to someone reading this code. There is no option to use KURL, so explaining why String is better than KURL is pointless.
> 
> Otherwise, looks fine to me. Adam?

My thought is that people looking at it will wonder, "If it's a url, why isn't a KURL?" so an explanation is valuable.

[WebKit Review Bot]

Created attachment 92548 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

> My thought is that people looking at it will wonder, "If it's a url, why isn't a KURL?" so an explanation is valuable.

We do that a lot.  If they try to change it, the test will fail.

[Joe Mason]

Created attachment 92550 [details]
removed useless comment

[WebKit Commit Bot]

Comment on attachment 92550 [details]
removed useless comment

Clearing flags on attachment: 92550

Committed r85939: <http://trac.webkit.org/changeset/85939>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.