[Webkit-unassigned] [Bug 50056] WebSocket: Workers threading issues related to String

[bugzilla-daemon at webkit.org]

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





--- Comment #1 from David Levin <levin at chromium.org>  2010-11-25 01:02:47 PST ---
(In reply to comment #0)
> I have found some (potential) errors from WebSocket's worker implementation.
> 
> * ThreadableWebSocketChannelClientWrapper (WebCore/websocket/ThreadableWebSocketChannelClientWrapper.h)
> This class contains String (Vector<String> in fact) even though it is derived from ThreadSafeShared.
>   - IIRC classes derived from ThreadSafeShared should not contain any String.

Yep (or else you have to be extremely careful and so do any subsequent changes which is nearly impossible so don't do it :) ).

In this case, it may be ok, but it is hard to tell.

I think while in the vector it may hold the only reference to the string (because it was just copied due to being passed cross thread).

Then before the string is given to any other place it is removed from the vector, so the underlying StringImpl always has a ref count of 1 while in the vector so it isn't shared... (Now it may happen that it has a ref count of 2 briefly when first put in the vector but when the method returns this ref count goes away... but I don't know the ordering of that ref count-- vs a ref count on the wrapper class).

All this is a way of saying I think it may be ok but it is hard to tell. 

It would be much better if this class was made really minimal and this other stuff was moved out into a class that was only owned on one thread.

For example, right now the wrapper calls out to m_client and that client is cleared at a certain point.  That m_client should be this thing with all of these extra variables. 

Ideally, this class would become a lot simpler like this: http://trac.webkit.org/browser/trunk/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.h?rev=52892  :)

I hope that made sense. If not, ping me and I'll try to explain better.

> * WorkerThreadableWebSocketChannel::Bridge::send (WebCore/websocket/WorkerThreadableWebSocketChannel.{h,cpp})
> A String is passed across threads with WorkerLoaderProxy::postTaskToLoader, but we don't call crossThreadString beforehand.

This looks fine.  It does createCallbackTask which does copies of all arguments for you (unless you pass in a Type*... I should close down that loophole and make it more explicit to avoid possible mistakes).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.