Bug 32700 - WebSocket: Invalid url should raise SYNTAX_ERR exception.
Summary: WebSocket: Invalid url should raise SYNTAX_ERR exception.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-17 22:44 PST by Fumitoshi Ukai
Modified: 2009-12-21 20:48 PST (History)
3 users (show)

See Also:


Attachments
WebCore: Invalid url should raise SYNTAX_ERR exception. (8.71 KB, patch)
2009-12-17 22:58 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Invalid url should raise SYNTAX_ERR exception. (9.68 KB, patch)
2009-12-20 00:45 PST, Fumitoshi Ukai
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2009-12-17 22:44:13 PST
If it failed to parse the url argument, thow a SYNTAX_ERR exception, as spec says.
Comment 1 Fumitoshi Ukai 2009-12-17 22:58:34 PST
Created attachment 45125 [details]
WebCore: Invalid url should raise SYNTAX_ERR exception.
Comment 2 WebKit Review Bot 2009-12-17 23:00:31 PST
style-queue ran check-webkit-style on attachment 45125 [details] without any errors.
Comment 3 Alexey Proskuryakov 2009-12-18 11:29:39 PST
Comment on attachment 45125 [details]
WebCore: Invalid url should raise SYNTAX_ERR exception.

r=me

I wish we could get JavaScript line number for the error messages.
Comment 4 Fumitoshi Ukai 2009-12-19 19:33:45 PST
Committed r52399: <http://trac.webkit.org/changeset/52399>
Comment 5 Adam Barth 2009-12-19 23:37:01 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/websocket/tests/bad-sub-protocol-expected.txt
	M	LayoutTests/websocket/tests/script-tests/url-parsing.js
	M	LayoutTests/websocket/tests/url-parsing-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/websockets/WebSocket.cpp
Committed r52402
	M	WebCore/ChangeLog
	M	WebCore/websockets/WebSocket.cpp
	M	LayoutTests/websocket/tests/url-parsing-expected.txt
	M	LayoutTests/websocket/tests/bad-sub-protocol-expected.txt
	M	LayoutTests/websocket/tests/script-tests/url-parsing.js
	M	LayoutTests/ChangeLog
r52402 = e9ae8c0dad923306815a6290589fea519962d3b4 (refs/remotes/trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
Comment 6 Adam Barth 2009-12-19 23:39:00 PST
Comment on attachment 45125 [details]
WebCore: Invalid url should raise SYNTAX_ERR exception.

websocket/tests/bad-sub-protocol.html -> failed
Comment 7 Fumitoshi Ukai 2009-12-20 00:45:15 PST
Created attachment 45258 [details]
Invalid url should raise SYNTAX_ERR exception.
Comment 8 Fumitoshi Ukai 2009-12-20 00:50:13 PST
(In reply to comment #6)
> (From update of attachment 45125 [details])
> websocket/tests/bad-sub-protocol.html -> failed

Seems it was failed because console message containing a non-printable character differs on windows.
http://build.webkit.org/results/Windows%20Release%20(Tests)/r52399%20(7249)/websocket/tests/

protocol string should be ASCII string, so report bad protocol string with escaping.
Comment 9 WebKit Review Bot 2009-12-20 00:50:24 PST
style-queue ran check-webkit-style on attachment 45258 [details] without any errors.
Comment 10 Eric Seidel (no email) 2009-12-20 23:02:44 PST
What's the status of this patch?  Was it rolled out?
Comment 11 Fumitoshi Ukai 2009-12-20 23:10:22 PST
(In reply to comment #10)
> What's the status of this patch?  Was it rolled out?

Yes. patch#1 was landed at r52399, but it caused layout test failure on win (because of difference in console message of non-printable chars), so rolled out at r52402 by Adam Barth.
patch#2 addressed the issue of the layout test failures by escaping non-printable characters in protocol string.
Comment 12 Alexey Proskuryakov 2009-12-21 09:28:44 PST
Comment on attachment 45258 [details]
Invalid url should raise SYNTAX_ERR exception.

> +static WebCore::String encodeProtocolString(const WebCore::String& protocol)

This is inside namespace WebCore, and doesn't need namespace prefixes. Please fix before landing.

> +    const UChar* characters = protocol.characters();
> +    StringBuilder builder;
> +    for (size_t i = 0; i < protocol.length(); i++) {
> +        if (characters[i] < 0x20 || characters[i] > 0x7E)

Why not just use protocol[i]?

r=me
Comment 13 Fumitoshi Ukai 2009-12-21 20:48:00 PST
Committed r52478: <http://trac.webkit.org/changeset/52478>