[Webkit-unassigned] [Bug 24054] Implement two separate proxy classes in order to split WorkerMessagingProxy.

[bugzilla-daemon at webkit.org]

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





------- Comment #6 from jianli at chromium.org  2009-02-20 11:26 PDT -------
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 [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.
> 


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