Bug 65248

Created attachment 103587 [details] Patch

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<>?

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.

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.

Created attachment 103608 [details] Patch v2

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.

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.

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.

(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.

Comment on attachment 103608 [details] Patch v2 ok

Created attachment 103735 [details] Patch for landing

Comment on attachment 103735 [details] Patch for landing Clearing flags on attachment: 103735 Committed r92946: <http://trac.webkit.org/changeset/92946>

All reviewed patches have been landed. Closing bug.

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