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. * WorkerThreadableWebSocketChannel::Bridge::send (WebCore/websocket/WorkerThreadableWebSocketChannel.{h,cpp}) A String is passed across threads with WorkerLoaderProxy::postTaskToLoader, but we don't call crossThreadString beforehand. Maybe related to bug 48996. David: Could you check out the issues above and give some comments? I'm not very good at WebKit threading stuff. I would be happy to make actual fixes. Thanks,
(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).
Likely also related to bug 44137