Bug 66925 - [WebSocket] WebSocket::close didn't handle code and reason arguments
Summary: [WebSocket] WebSocket::close didn't handle code and reason arguments
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: Takashi Toyoshima
URL:
Keywords:
Depends on: 66924
Blocks: 50099
  Show dependency treegraph
 
Reported: 2011-08-25 01:58 PDT by Takashi Toyoshima
Modified: 2012-02-01 22:42 PST (History)
6 users (show)

See Also:


Attachments
Patch (v8 code missing now) (41.92 KB, patch)
2011-08-25 02:15 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (include pywebsocket update patch) (45.44 KB, patch)
2011-08-25 06:56 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (44.44 KB, patch)
2011-08-25 07:14 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (44.25 KB, patch)
2011-08-26 02:42 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (44.25 KB, patch)
2011-08-26 02:58 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (45.79 KB, patch)
2011-08-31 02:54 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (45.82 KB, patch)
2011-08-31 04:22 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (45.59 KB, patch)
2011-09-01 00:14 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (45.59 KB, patch)
2011-09-01 00:43 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (45.81 KB, patch)
2011-09-01 22:00 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff
Patch (45.89 KB, patch)
2011-09-01 22:24 PDT, Takashi Toyoshima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Toyoshima 2011-08-25 01:58:08 PDT
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
Comment 1 Takashi Toyoshima 2011-08-25 02:15:32 PDT
Created attachment 105146 [details]
Patch (v8 code missing now)
Comment 2 Takashi Toyoshima 2011-08-25 06:56:33 PDT
Created attachment 105176 [details]
Patch (include pywebsocket update patch)
Comment 3 Takashi Toyoshima 2011-08-25 07:14:05 PDT
Created attachment 105178 [details]
Patch
Comment 4 Takashi Toyoshima 2011-08-25 07:16:06 PDT
Second patch included 66924 change.
Difference between second one and third one is that.
Comment 5 Kent Tamura 2011-08-25 18:19:27 PDT
Comment on attachment 105178 [details]
Patch

Would you post a patch which won't cause conflicts please?
Comment 6 Takashi Toyoshima 2011-08-26 02:42:11 PDT
Created attachment 105338 [details]
Patch
Comment 7 WebKit Review Bot 2011-08-26 02:49:19 PDT
Comment on attachment 105338 [details]
Patch

Attachment 105338 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9508916
Comment 8 Takashi Toyoshima 2011-08-26 02:58:52 PDT
Created attachment 105340 [details]
Patch
Comment 9 WebKit Review Bot 2011-08-26 03:25:32 PDT
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
Comment 10 Kent Tamura 2011-08-30 01:25:51 PDT
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?
Comment 11 Yuta Kitamura 2011-08-30 02:06:28 PDT
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.
Comment 12 Takashi Toyoshima 2011-08-30 07:31:27 PDT
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.
Comment 13 Takashi Toyoshima 2011-08-31 02:54:39 PDT
Created attachment 105763 [details]
Patch
Comment 14 Takashi Toyoshima 2011-08-31 04:22:43 PDT
Created attachment 105767 [details]
Patch
Comment 15 Kent Tamura 2011-08-31 23:08:37 PDT
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.
Comment 16 Takashi Toyoshima 2011-09-01 00:14:41 PDT
Created attachment 105909 [details]
Patch
Comment 17 Kent Tamura 2011-09-01 00:24:19 PDT
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.
Comment 18 Takashi Toyoshima 2011-09-01 00:40:40 PDT
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.
Comment 19 Takashi Toyoshima 2011-09-01 00:43:56 PDT
Created attachment 105915 [details]
Patch
Comment 20 Kent Tamura 2011-09-01 00:46:37 PDT
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.
Comment 21 Takashi Toyoshima 2011-09-01 07:01:02 PDT
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.
Comment 22 Kent Tamura 2011-09-01 18:28:15 PDT
(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.
Comment 23 Takashi Toyoshima 2011-09-01 22:00:57 PDT
Created attachment 106092 [details]
Patch
Comment 24 Kent Tamura 2011-09-01 22:18:03 PDT
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.
Comment 25 Takashi Toyoshima 2011-09-01 22:24:18 PDT
Created attachment 106093 [details]
Patch
Comment 26 Kent Tamura 2011-09-01 22:29:56 PDT
Comment on attachment 106093 [details]
Patch

ok
Comment 27 WebKit Review Bot 2011-09-01 23:32:58 PDT
Comment on attachment 106093 [details]
Patch

Clearing flags on attachment: 106093

Committed r94387: <http://trac.webkit.org/changeset/94387>
Comment 28 WebKit Review Bot 2011-09-01 23:33:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Kent Tamura 2012-02-01 22:42:19 PST
(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.