Remove completeURL() call from WebSocket construction code (issue 236013006)

[tyos...@chromium.org]

Reviewers: Adam Rice,

Description:
Remove completeURL() call from WebSocket construction code

The WebSocket constructor takes only absolute URLs. See this spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/network.html#parse-a-websocket-url's-components

This completeURL() call is not just unnecessary but converting given
data into invalid data. Also, implicit KURL to String conversion
happened on WebCore::WebSocket::connect() call caused spec violation.
That is it accepted URLs with double colons i.e. "ws:://example.com" by
mistake.

BUG=322818

Please review this at https://codereview.chromium.org/236013006/

SVN Base: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+7, -4 lines):
M LayoutTests/http/tests/websocket/url-parsing.html
M LayoutTests/http/tests/websocket/url-parsing-expected.txt
M Source/modules/websockets/WebSocket.cpp


Index: LayoutTests/http/tests/websocket/url-parsing-expected.txt
diff --git a/LayoutTests/http/tests/websocket/url-parsing-expected.txt
b/LayoutTests/http/tests/websocket/url-parsing-expected.txt
index
a90af82354a65407e03a28cb7105d8cbe3efdcdd..4ce558d3e442d9d5606621cac539daff78e2e469
100644
--- a/LayoutTests/http/tests/websocket/url-parsing-expected.txt
+++ b/LayoutTests/http/tests/websocket/url-parsing-expected.txt
@@ -3,9 +3,9 @@ Test WebSocket URL parsing.
On success, you will see a series of "PASS" messages, followed by "TEST
COMPLETE".

PASS new WebSocket() threw exception TypeError: Failed to
construct 'WebSocket': 1 argument required, but only 0 present..
-PASS new WebSocket(null) threw exception SyntaxError: Failed to
construct 'WebSocket': The URL's scheme must be either 'ws'
or 'wss'. 'http' is not allowed..
-PASS new WebSocket("ws://javascript:a") threw exception SyntaxError:
Failed to construct 'WebSocket': The URL 'ws://javascript:a/' is invalid..
-PASS new WebSocket("/applet") threw exception SyntaxError: Failed to
construct 'WebSocket': The URL's scheme must be either 'ws'
or 'wss'. 'http' is not allowed..
+PASS new WebSocket(null) threw exception SyntaxError: Failed to
construct 'WebSocket': The URL 'null' is invalid..
+PASS new WebSocket("ws://javascript:a") threw exception SyntaxError:
Failed to construct 'WebSocket': The URL 'ws://javascript:a' is invalid..
+PASS new WebSocket("/applet") threw exception SyntaxError: Failed to
construct 'WebSocket': The URL '/applet' is invalid..
PASS new WebSocket("javascript:a") threw exception SyntaxError: Failed to
construct 'WebSocket': The URL's scheme must be either 'ws'
or 'wss'. 'javascript' is not allowed..
PASS new WebSocket("ws://127.0.0.1:25/") threw exception SecurityError:
Failed to construct 'WebSocket': The port 25 is not allowed..
PASS (new WebSocket("ws://127.0.0.1:8880/a/../simple")).URL
is "ws://127.0.0.1:8880/simple"
@@ -13,6 +13,7 @@ PASS (new WebSocket("ws://127.0.0.1:8880/simple?")).URL
is "ws://127.0.0.1:8880/
PASS (new WebSocket("ws://127.0.0.1:8880/simple?k=v")).URL
is "ws://127.0.0.1:8880/simple?k=v"
PASS new WebSocket("ws://127.0.0.1/path#") threw exception SyntaxError:
Failed to construct 'WebSocket': The URL contains a fragment identifier
(''). Fragment identifiers are not allowed in WebSocket URLs..
PASS new WebSocket("ws://127.0.0.1/path#fragment") threw exception
SyntaxError: Failed to construct 'WebSocket': The URL contains a fragment
identifier ('fragment'). Fragment identifiers are not allowed in WebSocket
URLs..
+PASS new WebSocket("ws:://127.0.0.1/") threw exception SyntaxError: Failed
to construct 'WebSocket': The URL 'ws:://127.0.0.1/' is invalid..
PASS successfullyParsed is true

TEST COMPLETE
Index: LayoutTests/http/tests/websocket/url-parsing.html
diff --git a/LayoutTests/http/tests/websocket/url-parsing.html
b/LayoutTests/http/tests/websocket/url-parsing.html
index
b30a61b4ea2419bd02a98ae09125a15a1e93dc84..9b66b83cebef5f69f2766422cee90423a6170b80
100644
--- a/LayoutTests/http/tests/websocket/url-parsing.html
+++ b/LayoutTests/http/tests/websocket/url-parsing.html
@@ -39,6 +39,8 @@ shouldBe('(new
WebSocket("ws://127.0.0.1:8880/simple?k=v")).URL', '"ws://127.0.0
shouldThrow('new WebSocket("ws://127.0.0.1/path#")');
shouldThrow('new WebSocket("ws://127.0.0.1/path#fragment")');

+shouldThrow('new WebSocket("ws:://127.0.0.1/")');
+
</script>
</body>
</html>
Index: Source/modules/websockets/WebSocket.cpp
diff --git a/Source/modules/websockets/WebSocket.cpp
b/Source/modules/websockets/WebSocket.cpp
index
9c9e541e6788b2da6745d79838496872059ea404..c9097653eb3e188a5cd2c76a82667fc4fa1b8278
100644
--- a/Source/modules/websockets/WebSocket.cpp
+++ b/Source/modules/websockets/WebSocket.cpp
@@ -258,7 +258,7 @@ PassRefPtr<WebSocket>
WebSocket::create(ExecutionContext* context, const String&
RefPtr<WebSocket> webSocket(adoptRef(new WebSocket(context)));
webSocket->suspendIfNeeded();

- webSocket->connect(context->completeURL(url), protocols,
exceptionState);
+ webSocket->connect(url, protocols, exceptionState);
if (exceptionState.hadException())
return nullptr;

[ri...@chromium.org]

lgtm

It looks like the error message in the SyntaxError we return if a relative
URL
is passed to the constructor ("Failed to construct 'WebSocket': The URL's
scheme
must be either 'ws' or 'wss'. 'http' is not allowed.") gives away the fact
that
we aren't following
http://www.whatwg.org/specs/web-apps/current-work/multipage/network.html#parsing-websocket-urls
exactly, but that is a pre-existing problem.

https://codereview.chromium.org/236013006/

[ri...@chromium.org]

Oh, I see that you fixed that too. Never mind.

https://codereview.chromium.org/236013006/

[tyos...@chromium.org]

Exactly!

https://codereview.chromium.org/236013006/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyos...@chromium.org/236013006/1


https://codereview.chromium.org/236013006/

[commi...@chromium.org]

Try jobs failed on following builders:
tryserver.blink on linux_blink_rel

https://codereview.chromium.org/236013006/

[tyos...@chromium.org]

Updated the virtual/stable expectation.

https://codereview.chromium.org/236013006/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/tyos...@chromium.org/236013006/20001


https://codereview.chromium.org/236013006/

[commi...@chromium.org]

Try jobs failed on following builders:

tryserver.blink on mac_blink_rel

https://codereview.chromium.org/236013006/

[commi...@chromium.org]

[commi...@chromium.org]

Try jobs failed on following builders:

tryserver.blink on linux_blink_dbg

https://codereview.chromium.org/236013006/

[commi...@chromium.org]

[commi...@chromium.org]

Try jobs failed on following builders:

tryserver.blink on win_blink_rel

https://codereview.chromium.org/236013006/

[commi...@chromium.org]

[commi...@chromium.org]

Change committed as 171524

https://codereview.chromium.org/236013006/