Bug 65247 – WebSocket: Accept multiple subprotocols

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yuta Kitamura]

According to current WebSocket API specification, WebSocket constructor can accept an array of subprotocols:

<http://dev.w3.org/html5/websockets/>
    [Constructor(in DOMString url, in optional DOMString protocols),
     Constructor(in DOMString url, in optional DOMString[] protocols)]
    interface WebSocket {
        ...
    }

    

When WebSocket receives multiple subprotocols, it must send Sec-WebSocket-Protocol header field in the handshake request as in hybi protocol specification:

<http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-10>
    Sec-WebSocket-Protocol: chat, superchat

[Yuta Kitamura]

Created attachment 103327 [details]
Patch

[Kent Tamura]

Comment on attachment 103327 [details]
Patch

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

> Source/WebCore/websockets/WebSocket.cpp:59
> +static bool isSeparator(UChar character)

This should be 'inline'

> Source/WebCore/websockets/WebSocket.cpp:66
> +    if (character == '(' || character == ')' || character == '<' || character == '>' || character == '@'
> +        || character == ',' || character == ';' || character == ':' || character == '\\' || character == '"'
> +        || character == '/' || character == '[' || character == ']' || character == '?' || character == '='
> +        || character == '{' || character == '}' || character == ' ' || character == '\t')
> +        return true;
> +    return false;

You can write just
  return character == '(' ....

Test coverage for this code is very low.

Let's improve the performance of this code by checking "character is smaller than the minimum separator character" and "character is larger than the minimum separator character".

> Source/WebCore/websockets/WebSocket.cpp:75
> +    for (size_t i = 0; i < protocol.length(); ++i)
> +        if (protocol[i] < 0x21 || protocol[i] > 0x7E || isSeparator(protocol[i]))
> +            return false;

* You have to enclose the content of 'for' with {} because the content has two physical lines.

* What are 0x21 and 0x7E?

> Source/WebCore/websockets/WebSocket.cpp:182
> +            // Emulate JavaScript's Array.toString() behavior.
> +            StringBuilder builder;

I have a question.  Should we support protocols-in-array in Hixie76?

> Source/WebCore/websockets/WebSocket.cpp:206
> +        for (size_t i = 0; i < protocols.size(); ++i)
> +            if (!isValidProtocolString(protocols[i])) {
> +                scriptExecutionContext()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Wrong protocol for WebSocket '" + encodeProtocolString(protocols[i]) + "'", 0, scriptExecutionContext()->securityOrigin()->toString(), 0);

You have to enclose the 'for' content with {}.

> Source/WebCore/websockets/WebSocket.cpp:218
> +        for (size_t i = 0; i < protocols.size(); ++i)
> +            for (size_t j = i + 1; j < protocols.size(); ++j)
> +                if (protocols[i] == protocols[j]) {
> +                    scriptExecutionContext()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "WebSocket protocols contain duplicates: '" + encodeProtocolString(protocols[i]) + "'", 0, scriptExecutionContext()->securityOrigin()->toString(), 0);
> +                    m_state = CLOSED;
> +                    ec = SYNTAX_ERR;
> +                    return;
> +                }

O(N^2) algorithm is not acceptable.

> Source/WebCore/websockets/WebSocket.cpp:228
> +        if (!protocols.isEmpty()) {
> +            StringBuilder builder;
> +            for (size_t i = 0; i < protocols.size(); ++i) {
> +                if (i)
> +                    builder.append(", ");
> +                builder.append(protocols[i]);
> +            }
> +            protocolString = builder.toString();
> +        }

This phrase appears twice in this function.

[Yuta Kitamura]

Comment on attachment 103327 [details]
Patch

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

>> Source/WebCore/websockets/WebSocket.cpp:182
>> +            StringBuilder builder;
> 
> I have a question.  Should we support protocols-in-array in Hixie76?

If you pass a non-string object, the value will be automatically converted to a string by calling object.toString(). Auto-generated IDL stubs follow this rule, and Web IDL spec seems to require this behavior.

I admit passing an array is weird. I just tried hard to not lose the compatibility.

[Yuta Kitamura]

Created attachment 103445 [details]
Patch v2

[Yuta Kitamura]

Comment on attachment 103327 [details]
Patch

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

>> Source/WebCore/websockets/WebSocket.cpp:59
>> +static bool isSeparator(UChar character)
> 
> This should be 'inline'

Fixed.

>> Source/WebCore/websockets/WebSocket.cpp:66
>> +    return false;
> 
> You can write just
>   return character == '(' ....
> 
> Test coverage for this code is very low.
> 
> Let's improve the performance of this code by checking "character is smaller than the minimum separator character" and "character is larger than the minimum separator character".

This code was rewritten. Added a new test to check this function.

>> Source/WebCore/websockets/WebSocket.cpp:75
>> +            return false;
> 
> * You have to enclose the content of 'for' with {} because the content has two physical lines.
> 
> * What are 0x21 and 0x7E?

Added a brace. Added new constants to represent these values.

>> Source/WebCore/websockets/WebSocket.cpp:206
>> +                scriptExecutionContext()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Wrong protocol for WebSocket '" + encodeProtocolString(protocols[i]) + "'", 0, scriptExecutionContext()->securityOrigin()->toString(), 0);
> 
> You have to enclose the 'for' content with {}.

Fixed.

>> Source/WebCore/websockets/WebSocket.cpp:218
>> +                }
> 
> O(N^2) algorithm is not acceptable.

Fixed using HashSet<String>.

>> Source/WebCore/websockets/WebSocket.cpp:228
>> +        }
> 
> This phrase appears twice in this function.

Added a new function "joinStrings".

[Kent Tamura]

Comment on attachment 103445 [details]
Patch v2

looks ok

[WebKit Review Bot]

Comment on attachment 103445 [details]
Patch v2

Clearing flags on attachment: 103445

Committed r92757: <http://trac.webkit.org/changeset/92757>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.