Bug 65248 – WebSocket: Implement "protocol" attribute

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

[Yuta Kitamura]

Current WebSocket API defines "protocol" attribute which, after the WebSocket connection is established, returns the value of Sec-WebSocket-Protocol header in server's response.

<http://dev.w3.org/html5/websockets/>

    interface WebSocket {
      ...
      readonly attribute DOMString protocol;
      ...
    };

[Yuta Kitamura]

Created attachment 103587 [details]
Patch

[Kent Tamura]

Comment on attachment 103587 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        Tests: http/tests/websocket/tests/hybi/no-subprotocol.html (added)
> +               http/tests/websocket/tests/hybi/workers/no-subprotocol.html (added)
> +               http/tests/websocket/tests/hixie76/undefined-attributes.html (updated)
> +               http/tests/websocket/tests/hybi/multiple-subprotocols.html (updated)
> +               http/tests/websocket/tests/hybi/workers/multiple-subprotocols.html (updated)
> +

We need a test to try updating .protocol by a script.

> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:231
> -static void workerContextDidConnect(ScriptExecutionContext* context, RefPtr<ThreadableWebSocketChannelClientWrapper> workerClientWrapper)
> +static void workerContextDidConnect(ScriptExecutionContext* context, PassRefPtr<ThreadableWebSocketChannelClientWrapper> workerClientWrapper, const String& subprotocol)

Why did you change the RefPtr<> to PassRefPtr<>?

[Yuta Kitamura]

Comment on attachment 103587 [details]
Patch

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

>> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:231
>> +static void workerContextDidConnect(ScriptExecutionContext* context, PassRefPtr<ThreadableWebSocketChannelClientWrapper> workerClientWrapper, const String& subprotocol)
> 
> Why did you change the RefPtr<> to PassRefPtr<>?

I updated this line because I was bugged by check-webkit-style about using RefPtr<> as an argument. Using PassRefPtr<> should be safe, because CrossThreadTask and CrossThreadCopier know how to pass a RefPtr across threads.

I think I should update other occurrences of RefPtr<> arguments in this file. I'm not sure if it's fine to do in this patch, though.

[Kent Tamura]

Comment on attachment 103587 [details]
Patch

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

>>> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:231
>>> +static void workerContextDidConnect(ScriptExecutionContext* context, PassRefPtr<ThreadableWebSocketChannelClientWrapper> workerClientWrapper, const String& subprotocol)
>> 
>> Why did you change the RefPtr<> to PassRefPtr<>?
> 
> I updated this line because I was bugged by check-webkit-style about using RefPtr<> as an argument. Using PassRefPtr<> should be safe, because CrossThreadTask and CrossThreadCopier know how to pass a RefPtr across threads.
> 
> I think I should update other occurrences of RefPtr<> arguments in this file. I'm not sure if it's fine to do in this patch, though.

This change is unrelated to this bug, and the behavior will change a little. We had better handle it in a separated patch.

[Yuta Kitamura]

Created attachment 103608 [details]
Patch v2

[Yuta Kitamura]

Patch v1 has a bug that assertion fails if "protocol" attribute is accessed after m_channel is released.

Patch v2 fixes this problem, and the tests in the patch are modified to check this case.

[WebKit Review Bot]

Attachment 103608 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:231:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yuta Kitamura]

Comment on attachment 103587 [details]
Patch

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

>> Source/WebCore/ChangeLog:16
>> +
> 
> We need a test to try updating .protocol by a script.

Patch v2 contains a new test for this.

[Yuta Kitamura]

(In reply to comment #7)
> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:231:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]

This should be fixed in bug 66047.

[Kent Tamura]

Comment on attachment 103608 [details]
Patch v2

ok

[Yuta Kitamura]

Created attachment 103735 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 103735 [details]
Patch for landing

Clearing flags on attachment: 103735

Committed r92946: <http://trac.webkit.org/changeset/92946>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

Note for future reviews: Check for additions of non-threadsafe objects (like String whose destructor is not threadsafe) into ThreadSafeRefCounted classes.

Filed https://bugs.webkit.org/show_bug.cgi?id=67908