Bug 23859 – Change worker code to use different proxy class pointers.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jian Li]

Need to change worker code to use different proxy class pointers in order to draw  a boundary between worker and worker context.

[Jian Li]

Created attachment 27513 [details]
Proposed Patch

This patch only changes the worker code to use different proxy class pointers. I will move necessary files to work directory in next patch.

[David Levin]

Here's my comments on this patch.  There's a lot here but it is a big patch.


*WebCore/bindings/js/WorkerScriptController.cpp
1. extra blank lines added to the end.  Please remove them.


*WebCore/dom/Worker.cpp
>  , m_contextProxy(NULL)
1. Use 0 instead of NULL.

2. Previously, m_messagingProxy would never be 0.  Now m_contextProxy can be.
It doesn't look like all of this code has been fixed to handle that.  
One quick example, Worker::~Worker calls terminate() and terminate() does
this "m_contextProxy->terminateWorkerContext();"

> // The context is protected by context proxy....
3. This statement is confusing as is.  The statement would be clearer if it said "worker context proxy" instead of "context proxy".


*WebCore/dom/Worker.h
>  void dispatchMessage(const String& message);
1. Omit param name "message" (as explained below for "postDebugMessageToWorkerObject").

2. Extra blank lines added at end.  Pls remove.


*WebCore/dom/WorkerContext.cpp
> #include "GenericWorkerTask.h"
1. I don't think this include is needed anymore.


*WebCore/dom/WorkerContext.h
> void dispatchMessage(const String& message);
1. remove "message"
2. Extra blank lines added at end.  Pls remove.


*WebCore/dom/WorkerContextProxy.h
> static WorkerContextProxy* create(const String&, Worker*);
1. Unlike other cases, the param name associated with const String& would add information here so it should be included.


*WebCore/dom/WorkerMessagingProxy.cpp 
> WorkerDebugMessageTask
1. Why was this added instead of just doing what it did before with "postTaskToWorkerObject(createCallbackTask"?


> // The worker context does not exist while loading, so we much ensure that the worker object is not collected, as well as its event listeners.
2. I know you didn't add the comment, but it looks like "much" should be "must".


> ASSERT((m_scriptExecutionContext->isDocument() && isMainThread())
        || (m_scriptExecutionContext->isWorkerContext() && ...
3. Why did this assert get removed?

> WorkerMessagingProxy::notifyFinished
4. The position of "m_unconfirmedMessageCount--;" changes where hasPendingActivity may become false compared to the previous code.  Is this on purpose?
  
> postDebugMessageToWorkerObject
5. "debug" sounds like debug code to me.  Can you come up with a better name here?

> postTaskToWorkerObject(loaderTask);
6. Should be "postTaskToWorkerObject(loaderTask.release());" since the RefPtr is no longer used after this.



*WebCore/dom/WorkerMessagingProxy.h

> virtual void postDebugMessageToWorkerObject(MessageDestination destination, MessageSource source, MessageLevel level, const String& errorMessage, int lineNumber, const String& sourceURL) = 0;

1. Don't add parameter names in declarations when they don't add information.  In this case, leave out destination, source, level, so the declaration would look like this:

    virtual void postDebugMessageToWorkerObject(MessageDestination, MessageSource, MessageLevel, const String& errorMessage, int lineNumber, const String& sourceURL) = 0;


>  virtual void postTaskToLoader(PassRefPtr<ScriptExecutionContext::Task> task);
2. Omit "task"



*WebCore/dom/WorkerObjectProxy.h
> PostDebugMessageToWorkerObject
1. same parameter name comment.


*WebCore/dom/WorkerThread.cpp
1. Extra blank lines were added to the end.  Pls remove them.


*WebCore/dom/WorkerThread.h
> WorkerObjectProxy* m_objectProxy
1. The abbreviation "m_objectProxy" is confusing to me when I read it quickly it in the code.  Perhaps stay with m_workerObjectProxy;

2. Extra blank lines were added to the end.  Pls remove them.


*WebCore/loader/WorkerThreadableLoader.cpp
  >   if (thisPtr->m_messagingProxy.askedToTerminate())	
  >        return;	
1. Why was this removed?



*WebCore/loader/WorkerThreadableLoader.h
   >  WorkerObjectProxy* m_objectProxy;
1. Same comment about the name m_objectProxy.
2. This was previously a "T&".  Why did it get changed to a "T*"?

[David Levin]

One more comment in Worker.cpp

 "m_messagingProxy->workerObjectDestroyed()" changed to this "Worker::terminate()".

It doesn't look like these calls do the same thing.

[Jian Li]

Created attachment 27532 [details]
Proposed Patch

[Jian Li]

All are fixed except those commented inline. Thanks.

(In reply to comment #2)
> Here's my comments on this patch.  There's a lot here but it is a big patch.
> 
> 
> *WebCore/bindings/js/WorkerScriptController.cpp
> 1. extra blank lines added to the end.  Please remove them.
> 
> 
> *WebCore/dom/Worker.cpp
> >  , m_contextProxy(NULL)
> 1. Use 0 instead of NULL.
> 
> 2. Previously, m_messagingProxy would never be 0.  Now m_contextProxy can be.
> It doesn't look like all of this code has been fixed to handle that.  
> One quick example, Worker::~Worker calls terminate() and terminate() does
> this "m_contextProxy->terminateWorkerContext();"
> 
> > // The context is protected by context proxy....
> 3. This statement is confusing as is.  The statement would be clearer if it
> said "worker context proxy" instead of "context proxy".
> 
> 
> *WebCore/dom/Worker.h
> >  void dispatchMessage(const String& message);
> 1. Omit param name "message" (as explained below for
> "postDebugMessageToWorkerObject").
> 
> 2. Extra blank lines added at end.  Pls remove.
> 
> 
> *WebCore/dom/WorkerContext.cpp
> > #include "GenericWorkerTask.h"
> 1. I don't think this include is needed anymore.
> 
> 
> *WebCore/dom/WorkerContext.h
> > void dispatchMessage(const String& message);
> 1. remove "message"
> 2. Extra blank lines added at end.  Pls remove.
> 
> 
> *WebCore/dom/WorkerContextProxy.h
> > static WorkerContextProxy* create(const String&, Worker*);
> 1. Unlike other cases, the param name associated with const String& would add
> information here so it should be included.
> 
> 
> *WebCore/dom/WorkerMessagingProxy.cpp 
> > WorkerDebugMessageTask
> 1. Why was this added instead of just doing what it did before with
> "postTaskToWorkerObject(createCallbackTask"?
> 
> 
> > // The worker context does not exist while loading, so we much ensure that the worker object is not collected, as well as its event listeners.
> 2. I know you didn't add the comment, but it looks like "much" should be
> "must".
> 
> 
> > ASSERT((m_scriptExecutionContext->isDocument() && isMainThread())
>         || (m_scriptExecutionContext->isWorkerContext() && ...
> 3. Why did this assert get removed?
> 
> > WorkerMessagingProxy::notifyFinished
> 4. The position of "m_unconfirmedMessageCount--;" changes where
> hasPendingActivity may become false compared to the previous code.  Is this on
> purpose?
> 
Yes. I added the comment for this.

> > postDebugMessageToWorkerObject
> 5. "debug" sounds like debug code to me.  Can you come up with a better name
> here?

Changed to postConsoleMessageToWorkerObject, as discussed.
> 
> > postTaskToWorkerObject(loaderTask);
> 6. Should be "postTaskToWorkerObject(loaderTask.release());" since the RefPtr
> is no longer used after this.
> 
> 
> 
> *WebCore/dom/WorkerMessagingProxy.h
> 
> > virtual void postDebugMessageToWorkerObject(MessageDestination destination, MessageSource source, MessageLevel level, const String& errorMessage, int lineNumber, const String& sourceURL) = 0;
> 
> 1. Don't add parameter names in declarations when they don't add information. 
> In this case, leave out destination, source, level, so the declaration would
> look like this:
> 
>     virtual void postDebugMessageToWorkerObject(MessageDestination,
> MessageSource, MessageLevel, const String& errorMessage, int lineNumber, const
> String& sourceURL) = 0;
> 
> 
> >  virtual void postTaskToLoader(PassRefPtr<ScriptExecutionContext::Task> task);
> 2. Omit "task"
> 
> 
> 
> *WebCore/dom/WorkerObjectProxy.h
> > PostDebugMessageToWorkerObject
> 1. same parameter name comment.
> 
> 
> *WebCore/dom/WorkerThread.cpp
> 1. Extra blank lines were added to the end.  Pls remove them.
> 
> 
> *WebCore/dom/WorkerThread.h
> > WorkerObjectProxy* m_objectProxy
> 1. The abbreviation "m_objectProxy" is confusing to me when I read it quickly
> it in the code.  Perhaps stay with m_workerObjectProxy;
> 
> 2. Extra blank lines were added to the end.  Pls remove them.
> 
> 
> *WebCore/loader/WorkerThreadableLoader.cpp
>   >   if (thisPtr->m_messagingProxy.askedToTerminate()) 
>   >        return;      
> 1. Why was this removed?
> 
This is because I do not want to expose another method in the proxy interface. The logic is moved into the LoaderTask::performTask.
> 
> 
> *WebCore/loader/WorkerThreadableLoader.h
>    >  WorkerObjectProxy* m_objectProxy;
> 1. Same comment about the name m_objectProxy.
> 2. This was previously a "T&".  Why did it get changed to a "T*"?
> 

[David Levin]

Comment on attachment 27513 [details]
Proposed Patch

replaced

[Jian Li]

I added it back. Thanks.

(In reply to comment #3)
> One more comment in Worker.cpp
> 
>  "m_messagingProxy->workerObjectDestroyed()" changed to this
> "Worker::terminate()".
> 
> It doesn't look like these calls do the same thing.
> 

[David Levin]

*WebCore/dom/Worker.cpp
> Worker::hasPendingActivity() 

1. Can this method be called when m_contextProxy is 0 (after the Worker constructor)?
Have all usages of m_contextProxy been checked to make sure that they won't be used when it is 0.


*WebCore/dom/WorkerMessagingProxy.cpp
> m_unconfirmedMessageCount--; // workerThreadCreated checks if this count is 0.
Now, I understand why you moved it, but I still don't understand if this change in what WorkerMessagingProxy::hasPendingActivity() returns is ok.  

It looks like there is a variable "m_workerThreadHadPendingActivity" to indicate that the worker has pending activity.  Perhaps it should be used instead of the message count in this case.


*WebCore/dom/WorkerContext.h
1. Extra blank lines added to the end.

*WebCore/dom/WorkerContextProxy.h
>   static WorkerContextProxy* create(const String& scriptURL, Worker* worker);
1. remove "worker". It doesn't add any info.


*WebCore/dom/WorkerThread.h
1. Extra blank lines added to the end.

[Jian Li]

Created attachment 27536 [details]
Proposed Patch

[Jian Li]

All fixed excepted the commented ones. Thanks.

(In reply to comment #8)
> *WebCore/dom/Worker.cpp
> > Worker::hasPendingActivity() 
> 
> 1. Can this method be called when m_contextProxy is 0 (after the Worker
> constructor)?
> Have all usages of m_contextProxy been checked to make sure that they won't be
> used when it is 0.
> 
Added asserts for checking it.
> 
> *WebCore/dom/WorkerMessagingProxy.cpp
> > m_unconfirmedMessageCount--; // workerThreadCreated checks if this count is 0.
> Now, I understand why you moved it, but I still don't understand if this change
> in what WorkerMessagingProxy::hasPendingActivity() returns is ok.  
> 
> It looks like there is a variable "m_workerThreadHadPendingActivity" to
> indicate that the worker has pending activity.  Perhaps it should be used
> instead of the message count in this case.
> 
m_workerThreadHadPendingActivity can not be used in this case. So I had to use m_unconfirmedMessageCount at our discussion.
> 
> *WebCore/dom/WorkerContext.h
> 1. Extra blank lines added to the end.
> 
> *WebCore/dom/WorkerContextProxy.h
> >   static WorkerContextProxy* create(const String& scriptURL, Worker* worker);
> 1. remove "worker". It doesn't add any info.
> 
> 
> *WebCore/dom/WorkerThread.h
> 1. Extra blank lines added to the end.
> 

[Jian Li]

Created attachment 27537 [details]
Proposed Patch

Changed from using asserts to if check because worker object can still be accessed even after the exception is thrown.

[Jian Li]

Created attachment 27540 [details]
Proposed Patch

Fixed wrong use of release per discussion with levin.

[Jian Li]

Created attachment 27542 [details]
Proposed Patch

Removed one comment that does not apply in WorkerThread.cpp, per feedback from levin. Thanks.

[David Levin]

This looks good to me.

[Alexey Proskuryakov]

Comment on attachment 27542 [details]
Proposed Patch

-    , m_messagingProxy(new WorkerMessagingProxy(doc, this))
+    , m_contextProxy(0)
...
-    ASSERT(scriptExecutionContext()); // The context is protected by messaging proxy, so it cannot be destroyed while a Worker exists.
-    m_messagingProxy->workerObjectDestroyed();
+    ASSERT(scriptExecutionContext()); // The context is protected by worker context proxy, so it cannot be destroyed while a Worker exists.
+    if (m_contextProxy)
+        m_contextProxy->workerObjectDestroyed();

What's the reason for these changes? The proxy used to be non-null, as long as the worker object existed. Adding a null check in destructor makes this design less obvious, complicating the life of anyone trying to understand this code.

If the only reason is to pass scriptURL to the proxy, I suggest doing that after construction as a separate setScriptURL() call.

-    m_cachedScript = doc->docLoader()->requestScript(m_scriptURL, document()->charset());
-    if (!m_cachedScript) {
-        dispatchErrorEvent();
-        return;
-    }

What's the reason for moving this into proxy class? I don't think that fetching the script has anything to do with messaging proxy responsibilities.

+    m_unconfirmedMessageCount++;  // The worker context does not exist while loading, so we must ensure that the worker object is not collected, as well as its event listeners.

I think that this is an abuse of this variable. It counts unconfirmed messages, which has nothing to do with keeping the object alive while loading script source. Even though this technically works, it is misleading and will cause mistakes later.

-        context->thread()->messagingProxy()->confirmWorkerThreadMessage(context->hasPendingActivity());
+        m_messagingProxy->confirmWorkerThreadMessage(context->hasPendingActivity());

Why do you need to store m_messagingProxy? Knowing that we have a threaded implementation, I think you can always upcast context->thread()->workerObjectProxy(). But also, I expect that you'll ultimately need confirmWorkerThreadMessage() in multi-process design, too.

[Jian Li]

(In reply to comment #15)
> (From update of attachment 27542 [details] [review])
> -    , m_messagingProxy(new WorkerMessagingProxy(doc, this))
> +    , m_contextProxy(0)
> ...
> -    ASSERT(scriptExecutionContext()); // The context is protected by messaging
> proxy, so it cannot be destroyed while a Worker exists.
> -    m_messagingProxy->workerObjectDestroyed();
> +    ASSERT(scriptExecutionContext()); // The context is protected by worker
> context proxy, so it cannot be destroyed while a Worker exists.
> +    if (m_contextProxy)
> +        m_contextProxy->workerObjectDestroyed();
> 
> What's the reason for these changes? The proxy used to be non-null, as long as
> the worker object existed. Adding a null check in destructor makes this design
> less obvious, complicating the life of anyone trying to understand this code.
> 
> If the only reason is to pass scriptURL to the proxy, I suggest doing that
> after construction as a separate setScriptURL() call.

I will change to do as you suggest.
> 
> -    m_cachedScript = doc->docLoader()->requestScript(m_scriptURL,
> document()->charset());
> -    if (!m_cachedScript) {
> -        dispatchErrorEvent();
> -        return;
> -    }
> 
> What's the reason for moving this into proxy class? I don't think that fetching
> the script has anything to do with messaging proxy responsibilities.

Yes, we can choose to still load the script in main thread. However, for multi-process design, I think it would be better to load the script directly in the worker process, instead of passing the big chunk of data via IPC from one process to anther. How do you think?
> 
> +    m_unconfirmedMessageCount++;  // The worker context does not exist while
> loading, so we must ensure that the worker object is not collected, as well as
> its event listeners.
> 
> I think that this is an abuse of this variable. It counts unconfirmed messages,
> which has nothing to do with keeping the object alive while loading script
> source. Even though this technically works, it is misleading and will cause
> mistakes later.
> 
I can change to use another boolean variable. How do you think?
> -       
> context->thread()->messagingProxy()->confirmWorkerThreadMessage(context->hasPendingActivity());
> +       
> m_messagingProxy->confirmWorkerThreadMessage(context->hasPendingActivity());
> 
> Why do you need to store m_messagingProxy? Knowing that we have a threaded
> implementation, I think you can always upcast
> context->thread()->workerObjectProxy(). But also, I expect that you'll
> ultimately need confirmWorkerThreadMessage() in multi-process design, too.
> 
Yes, we can do upcast. I will check if we need to add method to confirm message. Thanks.

[Alexey Proskuryakov]

> Yes, we can choose to still load the script in main thread. However, for
> multi-process design, I think it would be better to load the script directly in
> the worker process, instead of passing the big chunk of data via IPC from one
> process to anther.

I don't think that the overhead of IPC will be significant. For all intents and purposes (e.g. Web Inspector display, assignment of Referer header and other loader options), the script is loaded by Worker object, not by Worker context.

[Jian Li]

Created attachment 27564 [details]
Proposed Patch

[Jian Li]

All fixed. Changed to load script in worker object. Also changed to use upcast. I think we do not need to add confirmMessage method into WorkerObjectProxy interface since though it is needed for multi-process model, it is part of the platform implementation detail and we do not need to expose it.

(In reply to comment #17)
> > Yes, we can choose to still load the script in main thread. However, for
> > multi-process design, I think it would be better to load the script directly in
> > the worker process, instead of passing the big chunk of data via IPC from one
> > process to anther.
> 
> I don't think that the overhead of IPC will be significant. For all intents and
> purposes (e.g. Web Inspector display, assignment of Referer header and other
> loader options), the script is loaded by Worker object, not by Worker context.
> 

[David Levin]

Sorry for this late comment, but I just realized this morning that this patch will break the destruction of MainThreadBridge when worker.Terminate is called.

At one time, I did something similar to the following code where I automatically called askedToTerminate before doing performTask:

 188     virtual void performTask(ScriptExecutionContext* context)
 189     {
 190         if (!m_messagingProxy->askedToTerminate())
 191             m_task->performTask(context);
 192     }
 
It is usually useful but not always, so I removed it.

For example, this call
        m_workerObjectProxy.postTaskToLoader(createCallbackTask(&MainThreadBridge::mainThreadDestroy, this));

should run no matter what.  ~MainThreadBridge cannot run on the worker context thread because it releases a ThreadableLoader which was created on the "loader thread".

[Jian Li]

Created attachment 27572 [details]
Proposed Patch

Removed LoaderTask wrapper since it is not longer needed per levin's latest feedback.

[David Levin]

In WebCore/loader/WorkerThreadableLoader.cpp, this deleted code needs to come back in some form
90      if (thisPtr->m_messagingProxy.askedToTerminate())
91          return;

[Jian Li]

Created attachment 27580 [details]
Proposed Patch

Changed to casting object proxy pointer to messaging proxy pointer in order to access loader functionalites from WorkerThreadableLoader for now until we get this fixed, after talking with levin.

[David Levin]

All my concerns have been addressed.

[Jian Li]

Created attachment 27584 [details]
Proposed Patch

Rename the newly added method in WorkerContextProxy to be consistent with the naming.

[Alexey Proskuryakov]

Comment on attachment 27584 [details]
Proposed Patch

+#include "Console.h"
+#include "ScriptExecutionContext.h"
+#include <wtf/PassRefPtr.h>

Most of these includes do not look necessary in WorkerObjectProxy.h.

+// FIXME: For now, we just cast object proxy pointer in order to access the functionalities needed for loader.

A FIXME should say what's wrong with the current situation, not just describe it. To anyone actively working on this code, it's pretty obvious what the problem is, but it may not be so obvious to others.

A good ChangeLog should say more about the changes - in this case, explaining that this is a step towards splitting proxies for multi-process design would suffice.

r=me - this patch looks really good!

[Alexey Proskuryakov]

Committed revision 40890.