Bug 66925 – [WebSocket] WebSocket::close didn't handle code and reason arguments

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

[Takashi Toyoshima]

This is a sending side of code and reason support for latest hybi spec.
Receiving side is already landed (https://bugs.webkit.org/show_bug.cgi?id=66362).

FYI: Reference specifications
http://dev.w3.org/html5/websockets/#dom-websocket-close
http://www.w3.org/TR/WebIDL/#es-unsigned-short

[Takashi Toyoshima]

Created attachment 105146 [details]
Patch (v8 code missing now)

[Takashi Toyoshima]

Created attachment 105176 [details]
Patch (include pywebsocket update patch)

[Takashi Toyoshima]

Created attachment 105178 [details]
Patch

[Takashi Toyoshima]

Second patch included 66924 change.
Difference between second one and third one is that.

[Kent Tamura]

Comment on attachment 105178 [details]
Patch

Would you post a patch which won't cause conflicts please?

[Takashi Toyoshima]

Created attachment 105338 [details]
Patch

[WebKit Review Bot]

Comment on attachment 105338 [details]
Patch

Attachment 105338 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9508916

[Takashi Toyoshima]

Created attachment 105340 [details]
Patch

[WebKit Review Bot]

Comment on attachment 105340 [details]
Patch

Attachment 105340 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9508927

New failing tests:
http/tests/websocket/tests/hybi/client-close.html
http/tests/websocket/tests/hybi/close.html

[Kent Tamura]

Comment on attachment 105340 [details]
Patch

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

> LayoutTests/http/tests/websocket/tests/hybi/close.html:108
> +    // TODO(toyoshim): unpaired surrogates throw SYNTAX_ERR

TODO(name) is not a WebKit style.

> LayoutTests/http/tests/websocket/tests/hybi/close_wsh.py:56
> +
> +
> +# vi:sts=4 sw=4 et

Should remove such editor-specific annotation.

> LayoutTests/http/tests/websocket/tests/hybi/workers/resources/close.js:120
> +            //shouldBeTrue("exceptionProto === DOMException.prototype");

should remove unused code.

> LayoutTests/http/tests/websocket/tests/hybi/workers/resources/close.js:160
> +            //shouldBeTrue("exceptionProto === DOMException.prototype");

ditto.

> Source/WebCore/bindings/js/JSWebSocketCustom.cpp:93
> +    int argumentCount = static_cast<int>(exec->argumentCount());

Why don't you use unsigned or size_t for argumentCount?

> Source/WebCore/websockets/WebSocketChannel.cpp:737
> +                startClosingHandshake(0, "");

What does "0" mean?

[Yuta Kitamura]

Comment on attachment 105340 [details]
Patch

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

> Source/WebCore/bindings/js/JSWebSocketCustom.cpp:102
> +        code = clampToInteger(x);

Just curious, what happens if isnan(x) is true?

> Source/WebCore/bindings/js/JSWebSocketCustom.cpp:105
> +            reason = ustringToString(exec->argument(1).toString(exec));

toString() may raise an exception. If that happens, you need to exit the function (see constructJSWebSocket() function above).

> Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:121
> +      double x = args[0]->NumberValue();

Lousy indentation here.

> Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:123
> +            x = clampTo(x, 0, 0xffff);

You could use USHORT_MAX, or numeric_limit<uint16_t>::max().

> Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:127
> +            reason = toWebCoreString(args[1]->ToString());

Same here, you need to watch an exception coming from ToString().

> Source/WebCore/websockets/WebSocket.cpp:267
> +        if (!(code == WebSocketChannel::CloseEventCodeNormalClosure || (3000 <= code && code <= 4999))) {

You probably need to give names to bare numbers like 3000 and 4999.

> Source/WebCore/websockets/WebSocket.cpp:272
> +        if (reason.utf8().length() > 123) {

Same here, define a constant for the value 123.

[Takashi Toyoshima]

Comment on attachment 105340 [details]
Patch

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

>> Source/WebCore/bindings/js/JSWebSocketCustom.cpp:102
>> +        code = clampToInteger(x);
> 
> Just curious, what happens if isnan(x) is true?

clampToInteger handle NaN by static cast to int.
I guess its result depends on compilers.

I assign 0.0 explicitly when x is NaN.

>> Source/WebCore/websockets/WebSocketChannel.cpp:737
>> +                startClosingHandshake(0, "");
> 
> What does "0" mean?

Actually, these arguments have no mean because this function is for old protocol hixie76.
Hixie76 doesn't use code and reason.
Anyway, I use CloseEventCodeNotSpecified (-1) for readability.

[Takashi Toyoshima]

Created attachment 105763 [details]
Patch

[Takashi Toyoshima]

Created attachment 105767 [details]
Patch

[Kent Tamura]

Comment on attachment 105767 [details]
Patch

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

r- for some style issues.

> LayoutTests/http/tests/websocket/tests/hybi/close-expected.txt:7
> +invalid code test: 0

nit: Capitalize: invalid -> Invalid

> LayoutTests/http/tests/websocket/tests/hybi/close-expected.txt:8
> +code 999 must cause INVALID_ACCESS_ERR

nit: This is a sentence.  So "code" should be "Code", and should end with '.'.

> LayoutTests/http/tests/websocket/tests/hybi/close.html:48
> +var onopen = function ()

The names onopen, onerror, onclose, onmessage are confusing.
I recommend to rename them like handleOpen, handleError, ....

> LayoutTests/http/tests/websocket/tests/hybi/close.html:68
> +var set_defaults = function (ws)

We usually use the naming rule same as C++.  So this should be "setDefaults".
Also, there is no reason to use "var".

function setDefaultHandlers(ws)
{
  ....

> LayoutTests/http/tests/websocket/tests/hybi/close.html:76
> +var run_code_test = function ()

ditto.

function runCodeTest() ...

> LayoutTests/http/tests/websocket/tests/hybi/close.html:106
> +var run_invalid_string_test = function ()

ditto.

function runInvalidStringTest() ...

> LayoutTests/http/tests/websocket/tests/hybi/close.html:113
> +var run_reason_test = function (next_test)

ditto.

function runReasonTest() ...

> Source/WebCore/bindings/js/JSWebSocketCustom.cpp:109
> +            // in optional DOMString reason

It seems that this comment adds no information to the code.
It should be removed.

> Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:131
> +            // in optional DOMString reason

ditto.

[Takashi Toyoshima]

Created attachment 105909 [details]
Patch

[Kent Tamura]

Comment on attachment 105909 [details]
Patch

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

> LayoutTests/http/tests/websocket/tests/hybi/close.html:108
> +    // TODO: unpaired surrogates throw SYNTAX_ERR

TODO -> FIXME

> Source/WebCore/websockets/WebSocket.idl:67
> -        void close();
> +        [Custom] void close(in [Optional] unsigned short code, in [Optional] DOMString reason) raises(DOMException);

Do we really need custom binding code?
Probably we can avoid [Custom] by providing close(ExceptionCode&), close(unsigned short, ExceptionCode&), and close(unsigned short, const String&, ExceptionCode&) in WebSocket.

[Takashi Toyoshima]

Comment on attachment 105909 [details]
Patch

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

>> Source/WebCore/websockets/WebSocket.idl:67
>> +        [Custom] void close(in [Optional] unsigned short code, in [Optional] DOMString reason) raises(DOMException);
> 
> Do we really need custom binding code?
> Probably we can avoid [Custom] by providing close(ExceptionCode&), close(unsigned short, ExceptionCode&), and close(unsigned short, const String&, ExceptionCode&) in WebSocket.

Main purpose to use [Custom] is to realize [Clamp] attribute behavior exactly.
Autogenerated code will handle casting from Number to unsigned short as modulo.

[Takashi Toyoshima]

Created attachment 105915 [details]
Patch

[Kent Tamura]

Comment on attachment 105909 [details]
Patch

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

>>> Source/WebCore/websockets/WebSocket.idl:67
>>> +        [Custom] void close(in [Optional] unsigned short code, in [Optional] DOMString reason) raises(DOMException);
>> 
>> Do we really need custom binding code?
>> Probably we can avoid [Custom] by providing close(ExceptionCode&), close(unsigned short, ExceptionCode&), and close(unsigned short, const String&, ExceptionCode&) in WebSocket.
> 
> Main purpose to use [Custom] is to realize [Clamp] attribute behavior exactly.
> Autogenerated code will handle casting from Number to unsigned short as modulo.

So the right direction is to implement [Clamp] for our binding code generator.

[Takashi Toyoshima]

Kent-san,

Thank you for review.
I totally agree with you.

But, could you permit me to split that work to another change?
I'd like to try my current implementation here, then apply it to the generator.
To my understanding, this is the first and only use case of [Clamp] in WebKit for now.

[Kent Tamura]

(In reply to comment #21)
> But, could you permit me to split that work to another change?
> I'd like to try my current implementation here, then apply it to the generator.

It's reasonable.
Please file a bug for the generator change now, and add FIXME comments to JSWebSocketCustom.cpp, V8WebSocketCustom.cpp, and WebSocket.idl.

[Takashi Toyoshima]

Created attachment 106092 [details]
Patch

[Kent Tamura]

Comment on attachment 106092 [details]
Patch

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

> Source/WebCore/websockets/WebSocket.idl:67
> -        void close();
> +        [Custom] void close(in [Optional] unsigned short code, in [Optional] DOMString reason) raises(DOMException);

Add a FIXME comment about Clamp please.

[Takashi Toyoshima]

Created attachment 106093 [details]
Patch

[Kent Tamura]

Comment on attachment 106093 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 106093 [details]
Patch

Clearing flags on attachment: 106093

Committed r94387: <http://trac.webkit.org/changeset/94387>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

(In reply to comment #22)
> (In reply to comment #21)
> > But, could you permit me to split that work to another change?
> > I'd like to try my current implementation here, then apply it to the generator.
> 
> It's reasonable.
> Please file a bug for the generator change now, and add FIXME comments to JSWebSocketCustom.cpp, V8WebSocketCustom.cpp, and WebSocket.idl.

Filed Bug 77605.