Bug 25435 – Dedicated workers do not support sending MessagePorts in messages

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Andrew Wilson]

Worker.postMessage() and WorkerContext.postMessage() should take an optional second argument that is a MessagePort (actually, the latest spec has changed this to be an array of MessagePorts, but I'll file a separate bug for that).

Implementing this is the first step towards SharedWorkers as it lets us shake out any threading issues in the MessagePort implementation.

[Andrew Wilson]

Created attachment 29947 [details]
proposed patch

[Maciej Stachowiak]

How does this patch handle cross-thread GC of message ports?

[Eric Seidel (no email)]

Comment on attachment 29947 [details]
proposed patch

Is this safe/correct?  args.size() < 1?
 88     String message = args.at(exec, 0).toString(exec);

Sad that JSWorker and JSWorkerContext can't share more code...

style:
7     if (proxy) {
 58         context = proxy->workerContext();
 59     } else {


We eventually need to write a throwOrReturn(ec, value) helper function to git rid of this idiom:
67     if (ec)
 68         return throwError(ec);
 69 
 70     return v8::Undefined();

return throwOrReturn(ec, v8::Undefined())

Or better yet, a special case for there of:

return throwOrReturnUndefined(ec);

static v8::Handle<Value> throwOrReturn(ExceptionCode ec, v8::Handle<Value> value)
{
   if (ec)
       return throwError(ec);
   return value;
}

that would clean up many of these bindings think.  Even just using it in these two new places might be worth the win :)

We only name arguments when the names provide clarity beyond the typename:
 83         void postMessage(const String& message, MessagePort* port, ExceptionCode&);
 84         void postMessage(const String& message, ExceptionCode& ec);

again:
 56         virtual void postMessageToWorkerContext(const String&, MessagePort*, ExceptionCode& ec) = 0;

Overall this looks sane to me, but I'm not the right reviewer here.  Alexey is your man I think.

r- for the style issues.  Re-post and Alexey can review for content.

[Andrew Wilson]

I'll clean up the style changes that Eric mentioned.

As Maciej points out, this patch does not address the cross-thread GC issues, and it also does not properly copy the MessagePort to the destination heap.

I'll revisit this patch once we've resolved some of the spec issues around MessagePorts and GC - it's possible that this API will be removed from HTML5.

[Andrew Wilson]

Created attachment 31895 [details]
revised patch

[David Levin]

Comment on attachment 31895 [details]
revised patch

Just a few issues to address.



> diff --git a/LayoutTests/fast/workers/worker-cloneport.html-disabled b/LayoutTests/fast/workers/worker-cloneport.html-disabled

> +worker.postMessage("postBack " + numMessages, channel.port1);
> +
> +// Test posting back 50000 messages, make sure ordering is fine

Nice to add "."

> +worker.onmessage = function(evt) {
> +    if (evt.data == "postBackDone") {
> +       stopCloning = true;

indent off.

> +// Keep cloning the passed port until we're told to stop

Nice to add "."


> +        if (numClones < 1000) {
> +            // If we didn't clone at least 1000 times, then there's something amiss

Nice to add "."

This seems timing sensitive.  I guess you're relying on a 1:50 ratio so it shouldn't be too timing sensitive but I don't really see why an implementation that did 50,000 messages of the other messages before 1000 of these would be wrong.


> +        // Make sure the messages arrived in order

Nice to add "."

> +        var timer = setTimeout(function() {
> +            log("FAILURE: Received: " + itemNum + " events - expected: " + numMessages);
> +        }, 1000);

How fast does the test run? (How close do you come to this time out and will slower machines hit this time out?)
 

> +            if (done) {
> +               gc();
> +               setTimeout(reportDone, 100);  // Make sure no unexpected events come in

Nice to add "."

Also these are indented 3 spaces instead of 4.  However, it has been argued that inconsistency in formatting js code helps ensure that the js parser can handle these different formats, so I leave it up to you.




> diff --git a/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp b/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp
>  
>  #include "Document.h"
>  #include "Frame.h"
> +#include "WorkerContext.h"
>  
>  #include "V8Binding.h"
>  #include "V8Proxy.h"
> +#include "WorkerContextExecutionProxy.h"

These headers aren't ordered correctly.  Sort alphabetically.
  



> diff --git a/WebCore/platform/CrossThreadCopier.h b/WebCore/platform/CrossThreadCopier.h
> +    template<typename T> struct CrossThreadCopierBase<false, PassOwnPtr<T> > {
> +        typedef PassOwnPtr<T> Type;
> +        static Type copy(const PassOwnPtr<T>& refPtr)
> +        {
> +            return PassOwnPtr<T>(static_cast<T*>(refPtr.release()));
> +        }

s/refPtr/ownPtr/



> diff --git a/WebCore/workers/Worker.idl b/WebCore/workers/Worker.idl
> +        void postMessage(in DOMString message, in [Optional] MessagePort messagePort)
> +            raises(DOMException);

Same question as WorkerContext.idl about this extra parameter (see below).


> diff --git a/WebCore/workers/WorkerContext.idl b/WebCore/workers/WorkerContext.idl

> +        void postMessage(in DOMString message, in [Optional] MessagePort messagePort)
> +            raises(DOMException);

Should MessagePort be allowed to be passed in if !ENABLE(CHANNEL_MESSAGING) ?

>  
> +#if ENABLE_CHANNEL_MESSAGING

Can this be
  #if ENABLE(CHANNEL_MESSAGING)
?

> +        attribute [JSCCustomGetter] MessageChannelConstructor MessageChannel;
> +#endif


> diff --git a/WebCore/workers/WorkerObjectProxy.h b/WebCore/workers/WorkerObjectProxy.h
> +#include <wtf/PassOwnPtr.h>


The <> come after the "" headers.

>  #include "Console.h"

[Andrew Wilson]

>This seems timing sensitive.  I guess you're relying on a 1:50 ratio so it
>shouldn't be too timing sensitive but I don't really see why an implementation
>that did 50,000 messages of the other messages before 1000 of these would be
>wrong.
The point of this test is to check for race conditions/lost/out-of-order messages when posting messages to a queue while it is in the middle of cloning. I'm just trying to make sure that enough iterations of both loops happen to shake out potential race conditions. I chose a number that was sufficiently conservative that it shouldn't be a problem - it's mainly a test to make sure that someone doesn't accidentally break my test in silent ways.


>How fast does the test run? (How close do you come to this time out and will
>slower machines hit this time out?)

This timeout should never be hit - I could've just set a timeout of 10 msecs. The point is to queue up a callback that happens after the queue has been emptied of messages. I'll see if I can clarify this code.


>Also these are indented 3 spaces instead of 4.  However, it has been argued
>that inconsistency in formatting js code helps ensure that the js parser can
>handle these different formats, so I leave it up to you.

I'll fix this, since it was unintentional.



> diff --git a/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp b/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp
>  
>  #include "Document.h"
>  #include "Frame.h"
> +#include "WorkerContext.h"
>  
>  #include "V8Binding.h"
>  #include "V8Proxy.h"
> +#include "WorkerContextExecutionProxy.h"

These headers aren't ordered correctly.  Sort alphabetically.






>> +        void postMessage(in DOMString message, in [Optional] MessagePort messagePort)
>> +            raises(DOMException);
>
>Should MessagePort be allowed to be passed in if !ENABLE(CHANNEL_MESSAGING) ?

You can't create MessagePorts if !ENABLE(CHANNEL_MESSAGING) so no check is needed.

>  
>> +#if ENABLE_CHANNEL_MESSAGING
>
>Can this be
>  #if ENABLE(CHANNEL_MESSAGING)

Apparently not - the ENABLE macro is not available to IDL files, per a comment in the ChangeLog from ap@webkit

[Andrew Wilson]

Created attachment 32029 [details]
Patch addressing Levin's comments

[Andrew Wilson]

Created attachment 32074 [details]
Patch with merge error fixed.

This fixes the merge error Levin ran into while trying to land this patch.

[David Levin]

Committed as http://trac.webkit.org/changeset/45381