This is the first part of work in order to split WorkerMessagingProxy into two separate proxies. We need to add WorkerContextCrossThreadProxy and WorkerObjectCrossThreadProxy to do cross-thread messaging in WebKit.
Created attachment 27822 [details] Proposed Patch
Is this patch intended for review? Please mark it as such by clicking Edit button to the right and choosing review? flag.
Small naming comment: looking at this patch, I think WorkerObjectProxy and WorkerContextProxy have some names unnecessarily repeated. WorkerObjectCrossThreadProxy has some methods that are named xxxxxToWorkerObject and WorkerContextCrossThreadProxy has some methods that are named xxxxxToWorkerContext. It seems the 'ToWorkerObject' and 'ToWorkerContext' are unnecessary in method names since this is already implied in the naming of proxies themselves. It probably deserves a separate patch though.
It seems that WorkerContextProxy::create(Worker*) is a bit unclear. It doesn't really create a WorkerContextProxy since the latter is an abstract interface. It could be better to have the following initialization sequence: 1. Worker constructor always first creates a local proxy to itself, WorkerObjectCrossThreadProxy, passing itself as a parameter. 2. Then it can cast WorkerObjectCrossThreadProxy to base type WorkerObjectProxy and pass that into a global function createWorkerContextProxy(WorkerObjectProxy*), which in case of WebKit would create a WorkerContextCrossThreadProxy, and in case of Chromium - an IPC pipe and return something that implements WorkerContextProxy. This also allows to get rid of WorkerContextCrossThreadProxy::setWorkerObjectProxy() sinceit'll be passed to constructor. Also, WorkerContextCrossThreadProxy does not seem to need m_parentScriptExecutionContext, and so does not need this constructor parameter. In Chromium case, it is also impossible to provide it in the worker process. Was it intended for postTaskToLoader() implementation?
Comment on attachment 27822 [details] Proposed Patch I really like the direction here and think this has made a lot of progress in the right direction. I have some comments/questions below -- some of which may be easiest to discuss tomorrow. WebCore/dom/WorkerContextCrossThreadProxy.cpp > PassRefPtr<WorkerContextCrossThreadProxy> WorkerContextCrossThreadProxy::create(ScriptExecutionContext* parentScriptExecutionContext) > { > return adoptRef(new WorkerContextCrossThreadProxy(parentScriptExecutionContext)); > } What do you think about parentContext instead of parentScriptExecutionContext? Also it looks like this is never used, which is just as it should be since WorkerContextCrossThreadProxy could be in a different process from the parent context. Should it be removed? (Or does it serve some purpose like helping xhr in webkit until I get that fixed up :)?) > +WorkerContextCrossThreadProxy::WorkerContextCrossThreadProxy(ScriptExecutionContext* parentScriptExecutionContext) > + : m_parentScriptExecutionContext(parentScriptExecutionContext) > + , m_workerObjectProxy(0) > + , m_askedToTerminate(false) > +{ > + ref(); I don't understand why there is this ref/deref thing. Sure workerObject has to call workerObjectDestroyed, but since it has a pointer to this class, why not have it be a RefPtr<> and get rid of this internal ref/deref. 59 WorkerContextCrossThreadProxy::~WorkerContextCrossThreadProxy() 60 { 61 m_workerThread = 0; 62 } Why is m_workerThread set to zero here? > +void WorkerContextCrossThreadProxy::postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task> task) > +{ ... > + if (m_workerThread) { > + m_workerThread->runLoop().postTask(task); > + } else No {} here (single line statement). > +void WorkerContextCrossThreadProxy::workerObjectDestroyed() > +{ > + terminateWorkerContext(); > + m_workerObjectProxy->terminateWorkerObject(); > + m_workerObjectProxy = 0; It looks very odd that it calls terminateWorkerObject when it is told that the worker object was already destroyed. > Index: WebCore/dom/WorkerContextCrossThreadProxy.h > +#include <wtf/Noncopyable.h> Appears unused. > + class WorkerContextCrossThreadProxy : public WorkerContextProxy { ... > + virtual void startWorkerContext(const KURL& scriptURL, const String& userAgent, const String& sourceCode); > + virtual void terminateWorkerContext(); > + virtual void postMessageToWorkerContext(const String&); > + void postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task>); It would be nice if "[To]WorkerContext" were removed from all of these. > + RefPtr<ScriptExecutionContext> m_parentScriptExecutionContext; As mentioned before, this appears unused. > Index: WebCore/dom/WorkerMessagingWebKit.cpp > =================================================================== > --- WebCore/dom/WorkerMessagingWebKit.cpp (revision 0) > +++ WebCore/dom/WorkerMessagingWebKit.cpp (revision 0) > @@ -0,0 +1,53 @@ > +/* > + * Copyright (C) 2008 Apple Inc. All Rights Reserved. Why is there an Apple copyright here? It would be great if all of this were done in Worker directory off of WebCore, since you are essentially defining a platform specific file now. > +WorkerContextProxy* WorkerContextProxy::create(Worker* worker) > +{ > + RefPtr<WorkerContextCrossThreadProxy> workerContextProxy = WorkerContextCrossThreadProxy::create(worker->scriptExecutionContext()); > + RefPtr<WorkerObjectCrossThreadProxy> workerObjectProxy = WorkerObjectCrossThreadProxy::create(worker); > + workerContextProxy->setWorkerObjectProxy(workerObjectProxy); workerObjectProxy.release() ? > + workerObjectProxy->setWorkerContextProxy(workerContextProxy); workerContextProxy.release() ? > Index: WebCore/dom/WorkerObjectCrossThreadProxy.cpp > > + > +WorkerObjectCrossThreadProxy::WorkerObjectCrossThreadProxy(Worker* workerObject) > + : m_scriptExecutionContext(workerObject->scriptExecutionContext()) > + , m_workerObject(workerObject) > + , m_askedToTerminate(false) > + , m_workerThreadHadPendingActivity(false) > +{ > + ASSERT(m_workerObject); > + ASSERT((m_scriptExecutionContext->isDocument() && isMainThread()) > + || (m_scriptExecutionContext->isWorkerContext() && currentThread() == static_cast<WorkerContext*>(m_scriptExecutionContext.get())->thread()->threadID())); > + > + ref(); I see you manage all the ref counts on this object on the WorkerObjectThread, but this feel fragile. Exposing a ref count on an object that is used cross thread but only one thread is suppose to do ref counting. Is there a way to avoid having this object be ref counted? > +static void postMessageToWorkerObjectTask(ScriptExecutionContext*, WorkerObjectCrossThreadProxy* proxy, const String& message) > +{ > + if (proxy->askedToTerminate()) > + return; How do you know that proxy is still alive at this point? > +void WorkerObjectCrossThreadProxy::postMessageToWorkerObject(const String& message) > +{ > + if (m_askedToTerminate) > + return; I don't think if is the right place to check this, but we should discuss. > Index: WebCore/dom/WorkerObjectCrossThreadProxy.h > + class WorkerObjectCrossThreadProxy : public WorkerObjectProxy { ... > + RefPtr<WorkerContextProxy> m_workerContextProxy; Is this ever used? > + bool m_workerThreadHadPendingActivity; // The latest confirmation from worker thread reported that it was still active. This appears never to be set.
The reason I do double ref-counting between WorkerContextCrossThreadProxy and WorkerObjectCrossThreadProxy is to manage the lifetime correctly. When the worker page is unloaded, the following destruction steps are executed: 1) Worker::terminate() is called and then it calls WorkerContextProxy::terminateWorkerContext(). Then WorkerThread::stop() is called to stop the worker thread. 2) GC kicks in and destructs Worker object. As the result, Worker::~Worker() calls WorkerContextProxy::workerObjectDestroyed(). 3) After WorkerThread leaves its run loop, WorkerContext is destructed. As the result, WorkerContext::~WorkerContext calls WorkerObjectProxy::workerContextDestroyed. Step 2 and 3 might be swapped depending when GC kicks in on different platform. WorkerContextCrossThreadProxy contains WorkerThread object. The WorkerThread object should not be deleted before WorkerThread leaves its run loop. When destructions are done in step 1, 2, and 3. WorkerContextCrossThreadProxy ::workerObjectDestroyed() could delete itself if there is not other reference being held. This will cause WorkerThread being destroyed before step 3 and crash could happen when some code is still executed. So we let WorkerContextCrossThreadProxy and WorkerObjectCrossThreadProxy hold the reference to each time. Only after both Worker (step 2) and WorkerContext (step 3) are destroyed, we then delete both proxies. (In reply to comment #5) > (From update of attachment 27822 [details] [review]) > I really like the direction here and think this has made a lot of progress in > the right direction. > > I have some comments/questions below -- some of which may be easiest to discuss > tomorrow. > > > WebCore/dom/WorkerContextCrossThreadProxy.cpp > > PassRefPtr<WorkerContextCrossThreadProxy> WorkerContextCrossThreadProxy::create(ScriptExecutionContext* parentScriptExecutionContext) > > { > > return adoptRef(new WorkerContextCrossThreadProxy(parentScriptExecutionContext)); > > } > What do you think about parentContext instead of parentScriptExecutionContext? > > Also it looks like this is never used, which is just as it should be since > WorkerContextCrossThreadProxy could be in a different process from the parent > context. > > Should it be removed? (Or does it serve some purpose like helping xhr in webkit > until I get that fixed up :)?) > > > > +WorkerContextCrossThreadProxy::WorkerContextCrossThreadProxy(ScriptExecutionContext* parentScriptExecutionContext) > > + : m_parentScriptExecutionContext(parentScriptExecutionContext) > > + , m_workerObjectProxy(0) > > + , m_askedToTerminate(false) > > +{ > > + ref(); > > I don't understand why there is this ref/deref thing. Sure workerObject has to > call workerObjectDestroyed, but since it has a pointer to this class, why not > have it be a RefPtr<> and get rid of this internal ref/deref. > > > 59 WorkerContextCrossThreadProxy::~WorkerContextCrossThreadProxy() > 60 { > 61 m_workerThread = 0; > 62 } > > Why is m_workerThread set to zero here? > > > > +void WorkerContextCrossThreadProxy::postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task> task) > > +{ > ... > > + if (m_workerThread) { > > + m_workerThread->runLoop().postTask(task); > > + } else > > No {} here (single line statement). > > > > +void WorkerContextCrossThreadProxy::workerObjectDestroyed() > > +{ > > + terminateWorkerContext(); > > + m_workerObjectProxy->terminateWorkerObject(); > > + m_workerObjectProxy = 0; > It looks very odd that it calls terminateWorkerObject when it is told that the > worker object was already destroyed. > > > > Index: WebCore/dom/WorkerContextCrossThreadProxy.h > > > +#include <wtf/Noncopyable.h> > Appears unused. > > > + class WorkerContextCrossThreadProxy : public WorkerContextProxy { > ... > > + virtual void startWorkerContext(const KURL& scriptURL, const String& userAgent, const String& sourceCode); > > + virtual void terminateWorkerContext(); > > + virtual void postMessageToWorkerContext(const String&); > > + void postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task>); > It would be nice if "[To]WorkerContext" were removed from all of these. > > > > + RefPtr<ScriptExecutionContext> m_parentScriptExecutionContext; > As mentioned before, this appears unused. > > > > Index: WebCore/dom/WorkerMessagingWebKit.cpp > > =================================================================== > > --- WebCore/dom/WorkerMessagingWebKit.cpp (revision 0) > > +++ WebCore/dom/WorkerMessagingWebKit.cpp (revision 0) > > @@ -0,0 +1,53 @@ > > +/* > > + * Copyright (C) 2008 Apple Inc. All Rights Reserved. > Why is there an Apple copyright here? > > > It would be great if all of this were done in Worker directory off of WebCore, > since you are essentially defining a platform specific file now. > > > > +WorkerContextProxy* WorkerContextProxy::create(Worker* worker) > > +{ > > + RefPtr<WorkerContextCrossThreadProxy> workerContextProxy = WorkerContextCrossThreadProxy::create(worker->scriptExecutionContext()); > > + RefPtr<WorkerObjectCrossThreadProxy> workerObjectProxy = WorkerObjectCrossThreadProxy::create(worker); > > + workerContextProxy->setWorkerObjectProxy(workerObjectProxy); > workerObjectProxy.release() ? > > > + workerObjectProxy->setWorkerContextProxy(workerContextProxy); > workerContextProxy.release() ? > > > > Index: WebCore/dom/WorkerObjectCrossThreadProxy.cpp > > > > + > > +WorkerObjectCrossThreadProxy::WorkerObjectCrossThreadProxy(Worker* workerObject) > > + : m_scriptExecutionContext(workerObject->scriptExecutionContext()) > > + , m_workerObject(workerObject) > > + , m_askedToTerminate(false) > > + , m_workerThreadHadPendingActivity(false) > > +{ > > + ASSERT(m_workerObject); > > + ASSERT((m_scriptExecutionContext->isDocument() && isMainThread()) > > + || (m_scriptExecutionContext->isWorkerContext() && currentThread() == static_cast<WorkerContext*>(m_scriptExecutionContext.get())->thread()->threadID())); > > + > > + ref(); > I see you manage all the ref counts on this object on the WorkerObjectThread, > but this feel fragile. Exposing a ref count on an object that is used cross > thread but only one thread is suppose to do ref counting. Is there a way to > avoid having this object be ref counted? > > > > +static void postMessageToWorkerObjectTask(ScriptExecutionContext*, WorkerObjectCrossThreadProxy* proxy, const String& message) > > +{ > > + if (proxy->askedToTerminate()) > > + return; > How do you know that proxy is still alive at this point? > > > > +void WorkerObjectCrossThreadProxy::postMessageToWorkerObject(const String& message) > > +{ > > + if (m_askedToTerminate) > > + return; > I don't think if is the right place to check this, but we should discuss. > > > > Index: WebCore/dom/WorkerObjectCrossThreadProxy.h > > > + class WorkerObjectCrossThreadProxy : public WorkerObjectProxy { > ... > > + RefPtr<WorkerContextProxy> m_workerContextProxy; > Is this ever used? > > > > + bool m_workerThreadHadPendingActivity; // The latest confirmation from worker thread reported that it was still active. > > This appears never to be set. >
OK, after staring at this code for a while (and talking to you), I think I understand the lifetime issues and they all seem fine. We all need to spend some time trying to figure out something simpler though after this as discussed. (Lifetime issues are a place where the WorkerThreadableLoader code got complex as well, so it feels like there is a core issue here). For this code, it was especially tricky that WorkerContextCrossThreadProxy is the way to keep the WorkerThread alive but I totally understand why you do this.
Created attachment 27839 [details] Proposed Patch
All fixed except those commented inline. (In reply to comment #5) > (From update of attachment 27822 [details] [review]) > I really like the direction here and think this has made a lot of progress in > the right direction. > > I have some comments/questions below -- some of which may be easiest to discuss > tomorrow. > > > WebCore/dom/WorkerContextCrossThreadProxy.cpp > > PassRefPtr<WorkerContextCrossThreadProxy> WorkerContextCrossThreadProxy::create(ScriptExecutionContext* parentScriptExecutionContext) > > { > > return adoptRef(new WorkerContextCrossThreadProxy(parentScriptExecutionContext)); > > } > What do you think about parentContext instead of parentScriptExecutionContext? > > Also it looks like this is never used, which is just as it should be since > WorkerContextCrossThreadProxy could be in a different process from the parent > context. > > Should it be removed? (Or does it serve some purpose like helping xhr in webkit > until I get that fixed up :)?) > > > > +WorkerContextCrossThreadProxy::WorkerContextCrossThreadProxy(ScriptExecutionContext* parentScriptExecutionContext) > > + : m_parentScriptExecutionContext(parentScriptExecutionContext) > > + , m_workerObjectProxy(0) > > + , m_askedToTerminate(false) > > +{ > > + ref(); > > I don't understand why there is this ref/deref thing. Sure workerObject has to > call workerObjectDestroyed, but since it has a pointer to this class, why not > have it be a RefPtr<> and get rid of this internal ref/deref. > Currently I just keep as it was before. I could change to let Worker object keep a RefPtr for WorkerContextProxy. However, extra ref count for WorkerObjectProxy is still held by itself since WorkerContext could not hold the ref count to WorkerObjectProxy per current design. I keep this in order to make ref/deref logic consistent for both proxy classes. > > 59 WorkerContextCrossThreadProxy::~WorkerContextCrossThreadProxy() > 60 { > 61 m_workerThread = 0; > 62 } > > Why is m_workerThread set to zero here? > > > > +void WorkerContextCrossThreadProxy::postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task> task) > > +{ > ... > > + if (m_workerThread) { > > + m_workerThread->runLoop().postTask(task); > > + } else > > No {} here (single line statement). > > > > +void WorkerContextCrossThreadProxy::workerObjectDestroyed() > > +{ > > + terminateWorkerContext(); > > + m_workerObjectProxy->terminateWorkerObject(); > > + m_workerObjectProxy = 0; > It looks very odd that it calls terminateWorkerObject when it is told that the > worker object was already destroyed. > Yes. I moved the logic to WorkerContextCrossThreadProxy::terminate(). > > > Index: WebCore/dom/WorkerContextCrossThreadProxy.h > > > +#include <wtf/Noncopyable.h> > Appears unused. > > > + class WorkerContextCrossThreadProxy : public WorkerContextProxy { > ... > > + virtual void startWorkerContext(const KURL& scriptURL, const String& userAgent, const String& sourceCode); > > + virtual void terminateWorkerContext(); > > + virtual void postMessageToWorkerContext(const String&); > > + void postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task>); > It would be nice if "[To]WorkerContext" were removed from all of these. > > > > + RefPtr<ScriptExecutionContext> m_parentScriptExecutionContext; > As mentioned before, this appears unused. > > > > Index: WebCore/dom/WorkerMessagingWebKit.cpp > > =================================================================== > > --- WebCore/dom/WorkerMessagingWebKit.cpp (revision 0) > > +++ WebCore/dom/WorkerMessagingWebKit.cpp (revision 0) > > @@ -0,0 +1,53 @@ > > +/* > > + * Copyright (C) 2008 Apple Inc. All Rights Reserved. > Why is there an Apple copyright here? > > > It would be great if all of this were done in Worker directory off of WebCore, > since you are essentially defining a platform specific file now. > Yes. However, I would rather do it in next patch. > > > +WorkerContextProxy* WorkerContextProxy::create(Worker* worker) > > +{ > > + RefPtr<WorkerContextCrossThreadProxy> workerContextProxy = WorkerContextCrossThreadProxy::create(worker->scriptExecutionContext()); > > + RefPtr<WorkerObjectCrossThreadProxy> workerObjectProxy = WorkerObjectCrossThreadProxy::create(worker); > > + workerContextProxy->setWorkerObjectProxy(workerObjectProxy); > workerObjectProxy.release() ? > > > + workerObjectProxy->setWorkerContextProxy(workerContextProxy); > workerContextProxy.release() ? > Can not use release() since it returns pointer. > > > Index: WebCore/dom/WorkerObjectCrossThreadProxy.cpp > > > > + > > +WorkerObjectCrossThreadProxy::WorkerObjectCrossThreadProxy(Worker* workerObject) > > + : m_scriptExecutionContext(workerObject->scriptExecutionContext()) > > + , m_workerObject(workerObject) > > + , m_askedToTerminate(false) > > + , m_workerThreadHadPendingActivity(false) > > +{ > > + ASSERT(m_workerObject); > > + ASSERT((m_scriptExecutionContext->isDocument() && isMainThread()) > > + || (m_scriptExecutionContext->isWorkerContext() && currentThread() == static_cast<WorkerContext*>(m_scriptExecutionContext.get())->thread()->threadID())); > > + > > + ref(); > I see you manage all the ref counts on this object on the WorkerObjectThread, > but this feel fragile. Exposing a ref count on an object that is used cross > thread but only one thread is suppose to do ref counting. Is there a way to > avoid having this object be ref counted? > Yes, as discussed. > > > +static void postMessageToWorkerObjectTask(ScriptExecutionContext*, WorkerObjectCrossThreadProxy* proxy, const String& message) > > +{ > > + if (proxy->askedToTerminate()) > > + return; > How do you know that proxy is still alive at this point? > > > > +void WorkerObjectCrossThreadProxy::postMessageToWorkerObject(const String& message) > > +{ > > + if (m_askedToTerminate) > > + return; > I don't think if is the right place to check this, but we should discuss. > > > > Index: WebCore/dom/WorkerObjectCrossThreadProxy.h > > > + class WorkerObjectCrossThreadProxy : public WorkerObjectProxy { > ... > > + RefPtr<WorkerContextProxy> m_workerContextProxy; > Is this ever used? > > > > + bool m_workerThreadHadPendingActivity; // The latest confirmation from worker thread reported that it was still active. > > This appears never to be set. >
Thx! This looks good to me. Two caveats to be done *after this* (imo): 1. We need to move some (all?) of the worker stuff to its own directory (so we can put some platform specific directories there). 2. We all need to brainstorm some ways to simplify the lifetime model here.
Created attachment 27842 [details] Proposed Patch
Why do we need to split WorkerMessagingProxy? The bug description says it's to do cross-thread messaging in WebKit, but this works already, doesn't it?
(In reply to comment #12) > Why do we need to split WorkerMessagingProxy? The bug description says it's to > do cross-thread messaging in WebKit, but this works already, doesn't it? > This is because we want to reuse what's implemented for cross-thread messaging in WebKit towards other platform, like Chromium. Without splitting WorkerMessagingProxy, we will have to duplicate a lot of existing logic as in WorkerMessagingProxy when implementing WorkerContextProxy and WorkerObjectProxy in Chromium. To share as much code as possible and improve the consistency, we propose splitting WorkerMessagingProxy into two halves. For WebKit (cross-thread): Worker -> WorkerContextCrossThreadProxy -> WorkerContext <- WorkerObjectCrossThreadProxy <- For Chromium (cross-process): Worker -> WorkerContextCrossProcessProxy <...> WorkerObjectCrossProcessProxy <- WorkerContext | | v v <- WorkerObjectCrossThreadProxy WorkerContextCrossThreadProxy ->
I'm not very sympathetic towards the idea of making the threaded implementation so much more complicated. Could you please tell more about what logic will have to be duplicated if we don't do this?
(In reply to comment #14) > I'm not very sympathetic towards the idea of making the threaded implementation > so much more complicated. Could you please tell more about what logic will have > to be duplicated if we don't do this? > The logic is to share as much code as possible for different platforms so that it will ease the pain of the future integration and merge of WebKit code into other platform, like Chromium. Yes, this brings the complexity to the cross-thread implementation for WebKit. So probably it is not a good idea to do it now. We will not do the splitting now. Instead, we will implement both proxies by doing similar things as in WorkerMessagingProxy plus cross-process IPC stuffs.
Comment on attachment 27842 [details] Proposed Patch Clearing review flag to get this out of queue.