Bug 67908 – ThreadableWebSocketChannelClientWrapper shouldn't have a String in it.

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

[David Levin]

A String was introduced here: http://trac.webkit.org/changeset/92946/trunk/Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.h

This is a mistake because String's contain StringImpl which are RefCounted (not threadsafe) but ThreadableWebSocketChannelClientWrapper is threadsafe refcounted --i.e. it may destroyed on different threads (which will affect the String's refcouting).

Please find a way to remove this as it may cause memory corruption, etc.

[Yuta Kitamura]

Oops sorry, I'll create a fix. Having Vector<UChar> in ThreadSafeRefCounted should be fine, right?

[David Levin]

(In reply to comment #1)
> Oops sorry, I'll create a fix. 

No worries. 

> Having Vector<UChar> in ThreadSafeRefCounted should be fine, right?

Yep, since there is no ref counting on that. It is simply owned by the class.

Thanks!

PS I'm fixing/investigating another issue and I just happened to come across this.

[Yuta Kitamura]

Created attachment 107034 [details]
Patch

[David Levin]

Comment on attachment 107034 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 107034 [details]
Patch

Clearing flags on attachment: 107034

Committed r95025: <http://trac.webkit.org/changeset/95025>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This unusual idiom needs a comment. I don’t think it will be obvious to people reading this code in the future why a Vector<UChar> is used here.

[Yuta Kitamura]

There seem unexpected failures on SL bots; I'm reverting this change to investigate the regression.

Darin: Sure -- I'm going to add comments when I re-land this patch.

[Yuta Kitamura]

Actual results:

--- /Volumes/Big/slave/snowleopard-intel-debug-tests/build/layout-test-results/http/tests/websocket/tests/hybi/workers/no-subprotocol-expected.txt 
+++ /Volumes/Big/slave/snowleopard-intel-debug-tests/build/layout-test-results/http/tests/websocket/tests/hybi/workers/no-subprotocol-actual.txt 
@@ -4,12 +4,12 @@
 
 PASS PASS: ws.protocol is equal to ""
 INFO: Connected
-PASS PASS: ws.protocol is equal to ""
+FAIL FAIL: ws.protocol should be "" but was "undefined"
 INFO: Closed
-PASS PASS: ws.protocol is equal to ""
+FAIL FAIL: ws.protocol should be "" but was "undefined"
 PASS PASS: closeEvent.wasClean is true
 INFO: Exited onclose handler
-PASS PASS: ws.protocol is equal to ""
+FAIL FAIL: ws.protocol should be "" but was "undefined"
 DONE
 PASS successfullyParsed is true

[Joe Mason]

Please mark this as blocking bug 50099 so that it's visible in the dependency graph.  (I'd do it but I don't seem to have access.)

[David Levin]

(In reply to comment #10)
> Please mark this as blocking bug 50099 so that it's visible in the dependency graph.  (I'd do it but I don't seem to have access.)

Try again :)

[Joe Mason]

That's better!

[Yuta Kitamura]

Argh! String(emptyVector) returns a null string, not an empty string! I'm going to fix and re-land soon.

[David Levin]

Sounds like a pretty simple modification
r=me on that change to this patch :).

[Yuta Kitamura]

Thanks!

[Yuta Kitamura]

Created attachment 107463 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 107463 [details]
Patch for landing

Clearing flags on attachment: 107463

Committed r95176: <http://trac.webkit.org/changeset/95176>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.