Bug 67908 - ThreadableWebSocketChannelClientWrapper shouldn't have a String in it.
: ThreadableWebSocketChannelClientWrapper shouldn't have a String in it.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Yuta Kitamura
:
Depends on: 68005
Blocks: 50099
  Show dependency treegraph
 
Reported: 2011-09-11 14:56 PDT by David Levin
Modified: 2011-09-15 00:40 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2011-09-11 14:56:25 PDT
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 Yuta Kitamura 2011-09-11 20:37:08 PDT
Oops sorry, I'll create a fix. Having Vector<UChar> in ThreadSafeRefCounted should be fine, right?
Comment 2 David Levin 2011-09-11 20:44:26 PDT
(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 Yuta Kitamura 2011-09-12 03:05:56 PDT
Created attachment 107034 [details]
Patch
Comment 4 David Levin 2011-09-12 08:08:36 PDT
Comment on attachment 107034 [details]
Patch

Thanks!
Comment 5 WebKit Review Bot 2011-09-13 06:24:26 PDT
Comment on attachment 107034 [details]
Patch

Clearing flags on attachment: 107034

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

Clearing flags on attachment: 107463

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