Bug 67908 - ThreadableWebSocketChannelClientWrapper shouldn't have a String in it.
: ThreadableWebSocketChannelClientWrapper shouldn't have a String in it.
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 68005
: 50099
  Show dependency treegraph
 
Reported: 2011-09-11 14:56 PST by
Modified: 2011-09-15 00:40 PST (History)


Attachments
Patch (3.36 KB, patch)
2011-09-12 03:05 PST, Yuta Kitamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (3.90 KB, patch)
2011-09-14 23:38 PST, Yuta Kitamura
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-09-11 14:56:25 PST
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.
------- Comment #1 From 2011-09-11 20:37:08 PST -------
Oops sorry, I'll create a fix. Having Vector<UChar> in ThreadSafeRefCounted should be fine, right?
------- Comment #2 From 2011-09-11 20:44:26 PST -------
(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.
------- Comment #3 From 2011-09-12 03:05:56 PST -------
Created an attachment (id=107034) [details]
Patch
------- Comment #4 From 2011-09-12 08:08:36 PST -------
(From update of attachment 107034 [details])
Thanks!
------- Comment #5 From 2011-09-13 06:24:26 PST -------
(From update of attachment 107034 [details])
Clearing flags on attachment: 107034

Committed r95025: <http://trac.webkit.org/changeset/95025>
------- Comment #6 From 2011-09-13 06:24:30 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #7 From 2011-09-13 07:47:35 PST -------
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.
------- Comment #8 From 2011-09-13 08:10:44 PST -------
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.
------- Comment #9 From 2011-09-13 08:16:21 PST -------
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
------- Comment #10 From 2011-09-14 14:37:52 PST -------
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.)
------- Comment #11 From 2011-09-14 14:40:16 PST -------
(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 :)
------- Comment #12 From 2011-09-14 15:22:08 PST -------
That's better!
------- Comment #13 From 2011-09-14 23:03:07 PST -------
Argh! String(emptyVector) returns a null string, not an empty string! I'm going to fix and re-land soon.
------- Comment #14 From 2011-09-14 23:04:41 PST -------
Sounds like a pretty simple modification
r=me on that change to this patch :).
------- Comment #15 From 2011-09-14 23:19:49 PST -------
Thanks!
------- Comment #16 From 2011-09-14 23:38:57 PST -------
Created an attachment (id=107463) [details]
Patch for landing
------- Comment #17 From 2011-09-15 00:40:10 PST -------
(From update of attachment 107463 [details])
Clearing flags on attachment: 107463

Committed r95176: <http://trac.webkit.org/changeset/95176>
------- Comment #18 From 2011-09-15 00:40:15 PST -------
All reviewed patches have been landed.  Closing bug.