Implement transfer of MessagePorts in webkitPostMessage.

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Zarko]

Attached is a dump of the state of my tree integrating changes to support Transferable objects. Comments are welcome!

[Luke Zarko]

Created attachment 102062 [details]
Dump of tree state.

[Luke Zarko]

Created attachment 102407 [details]
First proposed patch.

[David Levin]

+Darin for any comments about modifications/additions in Source/WebKit/chromium/public/

+Oliver for any concerns about changes in Source/WebCore/bindings/js/ specifically Source/WebCore/bindings/js/SerializedScriptValue.cpp (I'll review this, but in case he'd like to look it over, I'm adding him).

[Oliver Hunt]

Comment on attachment 102407 [details]
First proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=102407&action=review

I am still concerned that this API is badly thought out.  It seems very complex for very little gain, and generally seems to be a solution for a problem that has not yet been demonstrated to occur in the real world.

In terms of webkit code when you modify the serialization data you need to bump the serialization version.  JSValueRefArray seems like a bad idea, you'd be much better off simply using Vector<JSValue, ...  rather than JSValueRef.  JSValueRef is really intended only for API usage.

I'm also concerned that it is not clear how this is expected to work for cross-process messages.

> LayoutTests/platform/chromium-mac/fast/dom/Window/window-postmessage-args-expected.txt:5
> -PASS: Posting message ('1', 1): threw exception TypeError: MessagePortArray argument must be an object
> -PASS: Posting message ('2', ): threw exception TypeError: MessagePortArray argument must be an object
> -PASS: Posting message ('3', [object Object]): threw exception TypeError: MessagePortArray argument has no length attribute
> +PASS: Posting message ('1', 1): threw exception TypeError: TransferableArray argument must be an object
> +PASS: Posting message ('2', ): threw exception TypeError: TransferableArray argument must be an object
> +PASS: Posting message ('3', [object Object]): threw exception TypeError: TransferableArray argument has no length attribute

This error message change is concerning it implies that the new API behavior conflicts with existing behavior

[David Levin]

(In reply to comment #4)
> (From update of attachment 102407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102407&action=review
> 
> I am still concerned that this API is badly thought out.  It seems very complex for very little gain, and generally seems to be a solution for a problem that has not yet been demonstrated to occur in the real world.

Which api? The js api or the C++ one.

The js api has been discussed a lot here: http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/0304.html

As far as usefulness, one potential example is in web gl implementations which may create 1m ArrayBuffers in workers and send them to the Document for rendering. Here's a demo https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/demos/google/nvidia-vertex-buffer-object/index.html While this isn't a real world example, it is something which is representative of a the types of things real world apps of this sort would like to do.

[Oliver Hunt]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 102407 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=102407&action=review
> > 
> > I am still concerned that this API is badly thought out.  It seems very complex for very little gain, and generally seems to be a solution for a problem that has not yet been demonstrated to occur in the real world.
> 
> Which api? The js api or the C++ one.
> 
> The js api has been discussed a lot here: http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/0304.html
> 

The DOM API.  My problem is it seems to have added a new and exciting form of message passing, and its API conflicts with the existing API as well (you have an array parameters that can seemingly be either an array of transferable values, and array of message ports to send the message to, or an arbitrary combination of the two)

> As far as usefulness, one potential example is in web gl implementations which may create 1m ArrayBuffers in workers and send them to the Document for rendering. Here's a demo https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/demos/google/nvidia-vertex-buffer-object/index.html While this isn't a real world example, it is something which is representative of a the types of things real world apps of this sort would like to do.

On the other hand use cases that people have come up with in the past (drawing on a worker thread to avoid blocking the main thread) were thrown out because they added complexity, and there weren't obvious signs of people wanting the ability prior to that.

I think at minimum any implementation of this API should be prefixed.

[Kenneth Russell]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 102407 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=102407&action=review
> > > 
> > > I am still concerned that this API is badly thought out.  It seems very complex for very little gain, and generally seems to be a solution for a problem that has not yet been demonstrated to occur in the real world.
> > 
> > Which api? The js api or the C++ one.
> > 
> > The js api has been discussed a lot here: http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/0304.html
> > 
> 
> The DOM API.  My problem is it seems to have added a new and exciting form of message passing, and its API conflicts with the existing API as well (you have an array parameters that can seemingly be either an array of transferable values, and array of message ports to send the message to, or an arbitrary combination of the two)

The new semantics do not conflict at all with the existing API. They were carefully designed as a generalization of the existing support for transferring MessagePort objects, and are completely backward compatible.

There is a very long discussion on public-webapps about the new functionality -- see http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/thread.html and search for "What changes to Web Messaging spec are proposed". It took quite some time, but all of the parties on the thread (including, among others, representatives from Mozilla, Microsoft, and Google) are in support of the new semantics.

There are many use cases in the area of web workers where the transfer of ownership semantics are desired -- essentially any computation that one wants to farm out to workers and which produces a lot of data. In the subdomain of 3D graphics applications, there are already WebGL apps and libraries which would immediately benefit from the new Transferable support in conjunction with typed arrays.

> 
> > As far as usefulness, one potential example is in web gl implementations which may create 1m ArrayBuffers in workers and send them to the Document for rendering. Here's a demo https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/demos/google/nvidia-vertex-buffer-object/index.html While this isn't a real world example, it is something which is representative of a the types of things real world apps of this sort would like to do.
> 
> On the other hand use cases that people have come up with in the past (drawing on a worker thread to avoid blocking the main thread) were thrown out because they added complexity, and there weren't obvious signs of people wanting the ability prior to that.
> 
> I think at minimum any implementation of this API should be prefixed.

This will be difficult. In this spec change, the semantics of postMessage were updated; a new API wasn't added.

[Luke Zarko]

> In terms of webkit code when you modify the serialization data you need to bump the serialization version.

Noted-- but the only time that this change should affect serialization is when a transfer list is passed in. Is there any point where SerializedScriptValues read from disk would be paired with transfer lists?

>JSValueRefArray seems like a bad idea, you'd be much better off simply using Vector<JSValue, ...  rather than JSValueRef.  JSValueRef is really intended only for API usage.

Thanks; I'm quite new to JSC and was sure I made at least one of these kinds of mistakes.

> I'm also concerned that it is not clear how this is expected to work for cross-process messages.

When the message is being serialized for IPC, objects without special transfer semantics (like MessagePorts, which need to link up IPC channels-- compare these with ArrayBuffers, which are just hunks of data) must be flattened. The transfer of control still happens because the end-user shouldn't know that IPC actually happened; it's just this last step that changes. For Chromium IPC there's a (stub, for now) method (SerializedScriptValue::flattenReceiptList) that deals with this case.

> This error message change is concerning it implies that the new API behavior conflicts with existing behavior

As others have stated, the new behavior subsumes the old behavior. (One kind of weird side effect of this is that the .ports property of a MessageEvent needs to filter out only the MessagePort objects that were given to postMessage).

[David Levin]

Oliver has a good point. This api has just changed and doesn't have a lot of implementation experience.

We should expose a webkitPostMessage and give it the desired semantics and leave postMessage alone. If it stays solid, we could graduate the semantics over to postMessage.

As Oliver sagely stated, "We don't want another websockets-esque fiasco, new and exciting api's get prefixes".

[Luke Zarko]

(In reply to comment #9)
> Oliver has a good point. This api has just changed and doesn't have a lot of implementation experience.
> 
> We should expose a webkitPostMessage and give it the desired semantics and leave postMessage alone. If it stays solid, we could graduate the semantics over to postMessage.

OK-- I will amend the patch to behave in this manner.

[David Levin]

Comment on attachment 102407 [details]
First proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=102407&action=review

A few other comments.

> Source/WebCore/ChangeLog:9
> +        The Structured Clone algorithm describes an interface for Transferable objects (http://www.whatwg.org/specs/web-apps/current-work/complete/common-dom-interfaces.html#transferable-objects). Currently the only official Transferable object is the MessagePort, but ArrayBuffers are expected to be sanctioned as such soon.

Please wrap these lines to somewhere like 80-120 columns.

> Source/WebCore/bindings/js/JSMessageEventCustom.cpp:52
> +    for (size_t i = 0; i < transferables->size(); ++i)

Needs braces.

> Source/WebCore/bindings/js/JSMessageEventCustom.cpp:68
> +        fillTransferableArray(exec, exec->argument(7), *transferables, *valueRefArray);

Doesn't this set argument 7 to all of the transferables when it should only be the MessagePorts? (Isn't this the thing that gets passed on to script.)


Also are we affecting the behavior of the js method MessageEvent::initMessageEvent?

> Source/WebCore/bindings/js/JSMessagePortCustom.cpp:107
> +    // FIXME: Once other Transferable types are added, check for them here.

How long does this fixme stay in the code?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1236
> +    if (m_transferList)

needs braces.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1409
> +    RefPtr<SerializedScriptValue> serializedValue = SerializedScriptValue::create(exec, value, transferList);

You should change the return line to
    return serializedValue.release();

> Source/WebCore/bindings/js/SerializedScriptValue.h:30
> +#include "Transferable.h"

Add blank line here.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1026
> +    if (m_transferList) {

Better to return quickly.

if (!m_transferList)
    return true;

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1915
> +        deserializeAndSetProperty(object, propertyName, attribute, value, reinterpret_cast<ValueHandleArray*>(0));

Consider having only one call to deserializeAndSetProperty.


    ValueHandleArray* valueHandleArray = 0;
    ValueHandleArray localArray;
    localArray.reserveCapacity(transferList->size());
    if (transferList) {
       for (size_t i = 0; i < transferList->size(); ++i)
           localArray.append(toV8Transferable((*transferList)[i].get()));
      valueHandleArray = &localArray;
    }
    deserializeAndSetProperty(object, propertyName, attribute, value, valueHandleArray);

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2039
> +    return data.crossThreadString();

Why crossThreadString? Are we creating a string for another thread?

> Source/WebCore/bindings/v8/custom/V8MessageEventCustom.cpp:97
> +    event->initMessageEvent(typeArg, canBubbleArg, cancelableArg, dataArg.release(), originArg, lastEventIdArg, sourceArg, transferableArray.release());

Doesn't this push all transferables into "event . ports"? (same comment as the js section).

Ditto for initMessageEvent behavior as well.

> Source/WebCore/dom/MessagePort.cpp:87
> +                if (dataPort == this || m_entangledChannel->isConnectedTo(dataPort)) {

Consider combining this with the previous if clause to avoid more nesting.

> Source/WebCore/dom/MessagePort.cpp:-218
> -        if (!port || port->isCloned() || portSet.contains(port)) {

It looks like we are loosing this check for isCloned. Why is that ok? (Or did we retain it and I missed that?)

Ditto with respect to checking for duplicates (or is that handled in SerializeScriptValue now)?

> Source/WebCore/dom/MessagePortChannel.cpp:49
> +PassRefPtr<Transferable> MessagePortChannel::transferIn(PassOwnPtr<TransferableReceipt> ownThis, ScriptExecutionContext& context)

s/ownThis/transferableReceipt/

> Source/WebCore/dom/MessagePortChannel.cpp:52
> +    OwnPtr<MessagePortChannel> ownThisCast = adoptPtr(ownThis.leakPtr()->asMessagePortChannel());

s/ownThisCast/channel/

> Source/WebCore/dom/Transferable.h:51
> +class Transferable : public RefCounted<Transferable> {

It would be nice to have a *short* explanation for when/how this class is support to be used. We probably should do this more often in WebKit.

> Source/WebCore/dom/Transferable.h:53
> +    virtual ~Transferable() { }

Why not make this protected and make RefCounted<Transferable> a friend?

> Source/WebCore/dom/Transferable.h:66
> +class TransferableReceipt {

It would be nice if this had its own file (and same thing about the explanation).

> Source/WebCore/workers/SharedWorkerContext.cpp:51
>      return event;

Not your change but return event.release() would be better here.

> Source/WebKit/chromium/public/WebTransferable.h:64
> +    static PassOwnPtr<WebCore::TransferableReceiptArray> fromWebAll(const WebTransferableReceiptArray*);

What does fromWebAll mean?

> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:99
> +            : adoptPtr(reinterpret_cast<WebCore::TransferableReceipt*>(0));

Does nullptr work here?

> Source/WebKit/chromium/src/WebTransferable.cpp:51
> +    return adoptPtr(reinterpret_cast<TransferableReceipt*>(0));

Ditto re nullptr.

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:37
> +#include "Transferable.h"

We should fwd declare as opposed to including a header file if possible.

[Luke Zarko]

Created attachment 103151 [details]
New patch with prefixed webkitPostMessage, initMessageEvent

(In reply to comment #11)

I've added the prefixes to postMessage and initMessageEvent. See below for an explanation for the latter; the former does the following:
- postMessage allows only MessagePorts in the transfer list. On JSC, if a MessagePort appears in the message to be serialized, it turns into an Object (the legacy behavior). On V8, we throw a JavaScript exception.
- webkitPostMessage allows any Transferable in the transfer list. On both JSC and V8, if a Transferable appears both in the message to be serialized and in the transfer list, these objects are reference-equal in the delivered message.
- For both methods, the .ports member on a MessageEvent returns only MessagePorts (found by dropping all non-MessagePort Transferables from the transfer list).

> (From update of attachment 102407 [details])
> > Source/WebCore/bindings/js/JSMessageEventCustom.cpp:68
> > +        fillTransferableArray(exec, exec->argument(7), *transferables, *valueRefArray);
> 
> Doesn't this set argument 7 to all of the transferables when it should only be the MessagePorts? (Isn't this the thing that gets passed on to script.)

Argument 7 is an in argument (it's the list of transferables). initMessageEvent() mutates the MessageEvent on which it is called; this is the thing that JavaScript sees. Since we filter out MessagePorts using a custom callback, script can only see MessagePorts in the MessageEvent's .ports member.

> Also are we affecting the behavior of the js method MessageEvent::initMessageEvent?

There's a little bit of weirdness here and I'm not sure what the standard intends. I am assuming that initMessageEvent exists to permit JavaScript to build MessageEvent objects without doing something overly complicated (eg hooking Window's onMessage handler). If this is the case, then the IDL given at (http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html) is too restrictive (in that portsArg is still a sequence<MessagePort> and not a sequence<Transferable>).

For this reason, the non-prefixed implementation has the same semantics as postMessage (only MessagePorts in portsArg, no reference equality between the ports list and the message); the prefixed implementation permits all Transferables in portsArg, meaning it's more liberal than the IDL version. If this is not the right thing to do it is easy enough to change.

> > Source/WebCore/bindings/js/JSMessagePortCustom.cpp:107
> > +    // FIXME: Once other Transferable types are added, check for them here.
> 
> How long does this fixme stay in the code?

I'm removing it and adding a comment to Transferable.h about updating these special case conversion functions.

> > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2039
> > +    return data.crossThreadString();
> 
> Why crossThreadString? Are we creating a string for another thread?

Yes, potentially-- flattenReceiptList is analogous to serialization in that the string that is produced is destined for another context (new thread or new process).

> > Source/WebCore/bindings/v8/custom/V8MessageEventCustom.cpp:97
> > +    event->initMessageEvent(typeArg, canBubbleArg, cancelableArg, dataArg.release(), originArg, lastEventIdArg, sourceArg, transferableArray.release());
> 
> Doesn't this push all transferables into "event . ports"? (same comment as the js section).
> 
> Ditto for initMessageEvent behavior as well.

Same reply as in the JS section (it's nice to have consistent semantics!), modulo that the order of operations in initMessageEvent has been changed to be in line with all other serialization routines (and with the standard)-- we transfer first, then run structured clone.

> > Source/WebCore/dom/MessagePort.cpp:87
> > +                if (dataPort == this || m_entangledChannel->isConnectedTo(dataPort)) {
> 
> Consider combining this with the previous if clause to avoid more nesting.

The syntax won't let me!

> > Source/WebCore/dom/MessagePort.cpp:-218
> > -        if (!port || port->isCloned() || portSet.contains(port)) {
> 
> It looks like we are loosing this check for isCloned. Why is that ok? (Or did we retain it and I missed that?)
> Ditto with respect to checking for duplicates (or is that handled in SerializeScriptValue now)?

disentanglePorts became Transferable::transferAllOut. transferAllOut's version of the check looks like this:

  if (!transferable || transferable->isTransferClosed() || transferableSet.contains(transferable))

From my reading of the standard, it is possible to close a MessagePort independently of transferring it. That is, you can close a MessagePort and then send it to someone else-- but then they get control of that closed thing.

isTransferClosed() on MessagePort returns m_transferClosed. m_transferClosed is initially false; it is set to true once the MessagePort is transferred (in MessagePort::transferOut). transferOut then calls disentangle(), which in turn calls m_entangledChannel.release(). isCloned() returns !m_entangledChannel. Hence isTransferClosed() will return the same thing as isCloned() *unless* someone is making direct calls to disentangle() and entangle().

Note that attempting to transfer a MessagePort without an entangled channel (not one that is closed) will result in an INVALID_STATE_ERR-- so if someone has done something to the native MessagePort that we didn't expect, we won't do anything too strange.

> > Source/WebCore/dom/Transferable.h:53
> > +    virtual ~Transferable() { }
> 
> Why not make this protected and make RefCounted<Transferable> a friend?

This makes Visual C++ upset. (Note that MessagePort's destructor is public.)

--

One thing that has me slightly worried is an apparent race condition in MessageWorkerTask (WorkerMessagingProxy.cpp). A MessageWorkerTask is constructed in one thread (MessageWorkerTask::MessageWorkerTask) and used in another (MessageWorkerTask::performTask). Part of this process involves the use of a RefPtr<SerializedScriptValue>. If you instrument the code with helpful printfs as such:

    MessageWorkerTask(PassRefPtr<SerializedScriptValue> message, PassOwnPtr<TransferableReceiptArray> receipts, WorkerMessagingProxy* messagingProxy)
        : m_message(message)
        , m_receipts(receipts)
        , m_messagingProxy(messagingProxy)
    {
        printf("CTOR *refcount is %d\n", *(m_message->addressOfCount()));
        printf("CTOR  refcount is %d\n", m_message->refCount());
    }

    virtual void performTask(ScriptExecutionContext* scriptContext)
    {
        Worker* workerObject = m_messagingProxy->workerObject();
        if (!workerObject || m_messagingProxy->askedToTerminate())
            return;
        printf("THRD *refcount is %d\n", *(m_message->addressOfCount()));
        printf("THRD  refcount is %d\n", m_message->refCount());
        OwnPtr<TransferableArray> transferables = TransferableReceipt::transferAllIn(*scriptContext, m_receipts.release());
        workerObject->dispatchEvent(MessageEvent::create(transferables.release(), m_message));
    }

you ordinarily see behavior like this:

CTOR *refcount is 2
CTOR  refcount is 2
THRD *refcount is 1
THRD  refcount is 1

but occasionally the scheduler will give you this:

CTOR *refcount is 2
CTOR  refcount is 2
THRD *refcount is 2
ASSERTION FAILED: m_verifier.isSafeToUse()

where the assertion has failed because the refcount > 1 on a thread that is different than the RefPtr's owning thread. Since SerializedScriptValue only grants read-only access to its single buffer member, it appears to be safe to reparent it to ThreadSafeRefCounted-- but I'd like input from someone with more WebKit threading experience as to whether this is the proper thing to do (or if I've misdiagnosed the problem).

[WebKit Review Bot]

Attachment 103151 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/dom/Transferable.cpp:28:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebCore/dom/TransferableReceipt.cpp:28:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebCore/dom/TransferableReceipt.cpp:29:  Found header this file implements after a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 85 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Luke Zarko]

Created attachment 103152 [details]
New patch with prefixed webkitPostMessage, initMessageEvent, *and* three style nits fixed

[David Levin]

(In reply to comment #12)

> 
> One thing that has me slightly worried is an apparent race condition in MessageWorkerTask (WorkerMessagingProxy.cpp). 

Good concern. I think the real problem is that m_message has a ref count of two in the constructor and we should seek to eliminate that.

I suspect we have a call like this:
  RefPtr<SerializedScriptValue> message;
  func(message);
which should be
  RefPtr<SerializedScriptValue> message;
  func(message.release());
in the call chain which results in MessageWorkerTask::MessageWorkerTask.  All PassRefPtr automatically do this.

>  Since SerializedScriptValue only grants read-only access to its single buffer member, it appears to be safe to reparent it to ThreadSafeRefCounted

We strongly prefer not to use ThreadSafeRefCounted.

[David Levin]

Comment on attachment 103152 [details]
New patch with prefixed webkitPostMessage, initMessageEvent, *and* three style nits fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=103152&action=review

A few quick notes.

> LayoutTests/fast/dom/Window/window-postmessage-args-expected.txt:20
> +FAIL: Posting message ('[object MessagePort]', [object MessagePort]) did not throw an exception

FAIL?

> LayoutTests/platform/chromium/fast/dom/prototype-inheritance-expected.txt:832
> +FAIL inner.webkitPostMessage.isInner should be true. Was false.

Why do we fail? Might be good in changelog.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:715
> +    window->impl()->postMessage(message, &transferables, targetOrigin, activeDOMWindow(exec), ec);

You need a message.release() here, since you (correctly) change the PassRefPtr to a RefPtr above. This may be the cause of the assert that you are seeing.
(PassRefPtr automatically does this.)

> Source/WebCore/bindings/js/JSMessagePortCustom.h:63
> +        impl->postMessage(message, &transferableArray, ec);

message.release()

> Source/WebCore/bindings/js/SerializedScriptValue.h:58
> +class SerializedScriptValue : public ThreadSafeRefCounted<SerializedScriptValue> {

Don't do this as mentioned in my note in the bug.

[David Levin]

Comment on attachment 103152 [details]
New patch with prefixed webkitPostMessage, initMessageEvent, *and* three style nits fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=103152&action=review

Just a few minor comments below. Other than that it looks fine to me.

>> LayoutTests/fast/dom/Window/window-postmessage-args-expected.txt:20
>> +FAIL: Posting message ('[object MessagePort]', [object MessagePort]) did not throw an exception
> 
> FAIL?

I think I understand now but a note in the ChangeLog would be good (and make sure that we have filed a bug).

> Source/WebCore/ChangeLog:14
> +        - Add Transferable.h/cpp and TransferableReceipt.h/cpp

Not needed since this is already listed in the files below as new.

> Source/WebCore/ChangeLog:33
> +        - Reparented JSC's SerializedScriptValue to ThreadSafeRefCounted.

Remove this comment along with actually doing it :)

btw, these types of comments are best at the function file level below.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:4836
> +		B8E0E8FF13EC7E5500C13720 /* TransferableReceipt.cpp in Sources */ = {isa = PBXBuildFile; fileRef = B8E0E8FC13EC7E0C00C13720 /* TransferableReceipt.cpp */; };

out of order.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:697
> +static JSValue metaPostMessage(ExecState* exec, JSDOMWindow* window, bool useLegacyBehavior)

I don't like the "meta" prefix on these function as I can't tell what it means by reading the function alone.

I would just drop it and let the function name be "postMessage"

Also the "bool" parameter should be an enum so that the calling site is more readable (instead of just saying "true/false").

(These two comments apply in a few places.)

> Source/WebCore/bindings/js/SerializedScriptValue.h:81
> +    JSC::JSValue deserialize(JSC::ExecState*, JSC::JSGlobalObject*, TransferableArray* transferList = 0, SerializationErrorMode = Throwing);      

Does the param name "transferList" add any info? If not, please remove it.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2059
> +    for (size_t i = 0; i < receipts.size(); ++i)

I would ifdef out this for loop just to be sure it does nothing in release (and add UNUSED_PARAM(receipts);).

> Source/WebCore/bindings/v8/SerializedScriptValue.h:89
> +    static String flattenReceiptList(const String& data, const TransferableReceiptArray& receipts);

Don't include param name "receipts" unless you think it adds info (I don't).

> Source/WebCore/dom/MessagePort.cpp:82
> +    // Make sure we aren't connected to any of the passed-in transferables.

connected doesn't make sense in the general context of transferables. I think this should still say "ports".

> Source/WebCore/dom/Transferable.h:30
> +#include "ExceptionCode.h"

Typically folks just typedef ExceptionCode (if that is the only thing needed) instead of including this header.

> Source/WebCore/dom/Transferable.h:32
> +#include <wtf/Forward.h>

Is this header needed?

> Source/WebCore/dom/Transferable.h:35
> +#include <wtf/PassRefPtr.h>

Needed?

> Source/WebCore/dom/Transferable.h:38
> +#include <wtf/text/AtomicStringHash.h>

Needed?

> Source/WebCore/dom/TransferableReceipt.h:32
> +#include <wtf/Forward.h>

Needed?

> Source/WebCore/dom/TransferableReceipt.h:38
> +#include <wtf/text/AtomicStringHash.h>

Needed?

> Source/WebCore/page/DOMWindow.idl:222
> +        [DoNotCheckDomainSecurity, Custom] void postMessage(in SerializedScriptValue message, in [Optional] MessagePort transfer, in DOMString targetOrigin)

Why is this called transfer when the type is MessagePort?

> Source/WebKit/chromium/public/WebMessagePortChannel.h:54
> +    virtual WebMessagePortChannel* asWebMessagePortChannel() { return this; }

It looks like this used to be an interface until these two need methods. I wonder if they should really be here.

> Source/WebKit/chromium/public/WebSerializedScriptValue.h:76
> +                                                      const WebTransferableReceiptArray& receipts,

receipts param name not needed.

> Source/WebKit/chromium/public/WebTransferableReceipt.h:70
> +#endif

Add a blank line here.

> Source/WebKit/chromium/src/WebWorkerBase.cpp:370
> +    for (size_t i = 0; i < webReceipts.size(); ++i) {

No {} for single line statement.

> Source/WebKit/chromium/src/WebWorkerBase.h:127
> +        PassOwnPtr<WebCore::TransferableReceiptArray> receipts);

Param name receipts not needed. (Ditto for context above.)

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:76
> +        PassOwnPtr<WebCore::TransferableReceiptArray> receipts);

Param name receipts not needed.

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:134
> +                                               PassOwnPtr<WebCore::TransferableReceiptArray> receipts);

Param name receipts not needed. (Ditto for context above.)

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:144
> +                                              PassOwnPtr<WebCore::TransferableReceiptArray> receipts);

Param name receipts not needed. (Ditto for context above.)

[David Levin]

(In reply to comment #12)
> > Also are we affecting the behavior of the js method MessageEvent::initMessageEvent?
> 
> There's a little bit of weirdness here and I'm not sure what the standard intends. I am assuming that initMessageEvent exists to permit JavaScript to build MessageEvent objects without doing something overly complicated (eg hooking Window's onMessage handler). If this is the case, then the IDL given at (http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html) is too restrictive (in that portsArg is still a sequence<MessagePort> and not a sequence<Transferable>).
> 
> For this reason, the non-prefixed implementation has the same semantics as postMessage (only MessagePorts in portsArg, no reference equality between the ports list and the message); the prefixed implementation permits all Transferables in portsArg, meaning it's more liberal than the IDL version. If this is not the right thing to do it is easy enough to change.


Please bring this up on whatwg (regarding if the spec should change here for initMessageEvent). Dmitry Titov can give you some pointer here if you haven't sent such an email before.

[Kenneth Russell]

(In reply to comment #18)
> (In reply to comment #12)
> > > Also are we affecting the behavior of the js method MessageEvent::initMessageEvent?
> > 
> > There's a little bit of weirdness here and I'm not sure what the standard intends. I am assuming that initMessageEvent exists to permit JavaScript to build MessageEvent objects without doing something overly complicated (eg hooking Window's onMessage handler). If this is the case, then the IDL given at (http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html) is too restrictive (in that portsArg is still a sequence<MessagePort> and not a sequence<Transferable>).
> > 
> > For this reason, the non-prefixed implementation has the same semantics as postMessage (only MessagePorts in portsArg, no reference equality between the ports list and the message); the prefixed implementation permits all Transferables in portsArg, meaning it's more liberal than the IDL version. If this is not the right thing to do it is easy enough to change.
> 
> 
> Please bring this up on whatwg (regarding if the spec should change here for initMessageEvent). Dmitry Titov can give you some pointer here if you haven't sent such an email before.

Definitely feel free to bring this up on one of the mailing lists (I would suggest public-webapps, where the Transferable discussion occurred), but I am pretty sure the intent was not to change the spec for MessageEvent's initMessageEvent method. The "ports" array was intended to be only a legacy construct. Transferable objects were intended to be passed in the main object graph, not as a side array as in the previous MessagePort support.

[Luke Zarko]

Created attachment 103302 [details]
Address comments.

In this revision I changed some conditions which would raise INVALID_STATE_ERR to instead raise DATA_CLONE_ERR to be in line with the spec. These were in particular:

--- a/Source/WebCore/bindings/js/JSMessagePortCustom.cpp
+++ b/Source/WebCore/bindings/js/JSMessagePortCustom.cpp
@@ -88,7 +88,7 @@ void fillTransferableArray(JSC::ExecState* exec, JSC::JSValue value, Transferabl
             return;
         // Validation of non-null objects, per HTML5 spec 8.3.3.
         if (value.isUndefinedOrNull()) {
-            setDOMException(exec, INVALID_STATE_ERR);
+            setDOMException(exec, DATA_CLONE_ERR);
             return;
         }
 
diff --git a/Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp b/Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp
index e8b63b3..f087236 100644
--- a/Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp
+++ b/Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp
@@ -106,7 +106,7 @@ bool getTransferableArray(v8::Local<v8::Value> value, TransferableArray& transfe
         v8::Local<v8::Value> transferable = ports->Get(i);
         // Validation of non-null objects, per HTML5 spec 8.3.3.
         if (isUndefinedOrNull(transferable)) {
-            throwError(INVALID_STATE_ERR);
+            throwError(DATA_CLONE_ERR);
             return false;
         }
         // Validation of Objects implementing an interface, per WebIDL spec 4.1.15.
diff --git a/Source/WebCore/dom/MessagePort.cpp b/Source/WebCore/dom/MessagePort.cpp
index 59e5a0a..3d2eb4f 100644
--- a/Source/WebCore/dom/MessagePort.cpp
+++ b/Source/WebCore/dom/MessagePort.cpp
@@ -85,7 +85,7 @@ void MessagePort::postMessage(PassRefPtr<SerializedScriptValue> message, Transfe
             Transferable* transferable = (*transferables)[i].get();
             if (MessagePort* dataPort = transferable->asMessagePort()) {
                 if (dataPort == this || m_entangledChannel->isConnectedTo(dataPort)) {
-                    ec = INVALID_STATE_ERR;
+                    ec = DATA_CLONE_ERR;
                     return;
                 }
             }
diff --git a/Source/WebCore/dom/Transferable.cpp b/Source/WebCore/dom/Transferable.cpp
index 892e77e..972ee74 100644
--- a/Source/WebCore/dom/Transferable.cpp
+++ b/Source/WebCore/dom/Transferable.cpp
@@ -46,7 +46,7 @@ PassOwnPtr<TransferableReceiptArray> Transferable::transferAllOut(TransferableAr
     for (unsigned int i = 0; i < transferables->size(); ++i) {
         Transferable* transferable = (*transferables)[i].get();
         if (!transferable || transferable->isTransferClosed() || transferableSet.contains(transferable)) {
-            ec = INVALID_STATE_ERR;
+            ec = DATA_CLONE_ERR;
             return nullptr;
         }
         transferableSet.add(transferable);

The no-release() bug in the JSC postMessage path appears to exist in the code prior to this patch; I've looked around the code for similar problems, but it's still worth watching for. What's the motivation for the implicit RefPtr<T> -> PassRefPtr<T> conversion?

[Luke Zarko]

(In reply to comment #16)
> (From update of attachment 103152 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103152&action=review
>
> > LayoutTests/platform/chromium/fast/dom/prototype-inheritance-expected.txt:832
> > +FAIL inner.webkitPostMessage.isInner should be true. Was false.
> 
> Why do we fail? Might be good in changelog.

This appears to be some artifact of the way the V8 bindings work (note that previously postMessage had a similar line, as do some other properties, particularly those with special annotations). I've added a note to the changelog saying as much.

(In reply to comment #17)
> (From update of attachment 103152 [details])
> >> LayoutTests/fast/dom/Window/window-postmessage-args-expected.txt:20
> >> +FAIL: Posting message ('[object MessagePort]', [object MessagePort]) did not throw an exception
> > 
> > FAIL?
> 
> I think I understand now but a note in the ChangeLog would be good (and make sure that we have filed a bug).

Added a note; I will also amend the existing bug at https://bugs.webkit.org/show_bug.cgi?id=65292 .

> > Source/WebKit/chromium/public/WebMessagePortChannel.h:54
> > +    virtual WebMessagePortChannel* asWebMessagePortChannel() { return this; }
> 
> It looks like this used to be an interface until these two need methods. I wonder if they should really be here.

Are there some implementations of WebMessagePortChannel where these methods' behavior would be different?

[Dmitry Lomov]

Comment on attachment 103302 [details]
Address comments.

LGTM. I do not really have any comments.

[David Levin]

Comment on attachment 103302 [details]
Address comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=103302&action=review

At this point, I have only some trivial nits. Other than those very minor things, it looks good to me.

Hopefully Oliver will weight in with any concerns about the changes to WebCore/bindings/js.

And Darin with any concerns about WebKit/chromium/public

> LayoutTests/ChangeLog:13
> +        Note that the default window-postmessage-args-expected contains a FAIL: line. The check that generates that line attempts to verify that non-prefixed postMessage disallows MessagePorts (or any other Transferables) from appearing in message bodies as mandated by the structured clone algorithm (modulo sending Transferables as part of the prefix). A bug relevant to this behavior is at https://bugs.webkit.org/show_bug.cgi?id=65292.

Please wrap this and put it as a comment after the line below that says "fast/dom/Window/window-postmessage-args-expected.txt"

> LayoutTests/ChangeLog:15
> +        prototype-inheritance-expected contains an extra FAIL: line for Chromium with regard to webkitPostMessage; this appears to be an artifact of the way bindings are generated, as a similar FAIL: line previously existed for plain postMessage.

Wrap the test and put it as a comment after the line below that says " platform/chromium/fast/dom/prototype-inheritance-expected.txt"

> Source/WebCore/bindings/js/SerializedScriptValue.h:43
> +typedef Vector<RefPtr<Transferable>, 1> TransferableArray;

I would put classes and then typedef's.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2056
> +                                                 const TransferableReceiptArray& receipts) {

This brace is in the wrong place. It should be on the next line.

> Source/WebCore/dom/MessageEvent.cpp:31
> +#include "Transferable.h"

These two headers are in the wrong place. They should be sorted with the other headers below.

> Source/WebCore/page/DOMWindow.h:77
> +    class Transferable;

Put the fwd class declarations by the other ones at line 68 (but leave the typedef's down here.)

> Source/WebCore/storage/IDBCursor.h:69
> +    TransferableArray* valueDependencies() const { return 0; }

Why does this need to be defined? (Who calls it? I couldn't find it. If it is unused, then just get rid of the changes to this file.)

> Source/WebKit/chromium/public/WebWorker.h:40
> +typedef WebVector<class WebTransferableReceipt*> WebTransferableReceiptArray;

I would put the typedef after the class fwd declarations.

> Source/WebKit/chromium/public/WebWorkerClient.h:42
> +typedef WebVector<class WebTransferableReceipt*> WebTransferableReceiptArray;

I would put typedef's after classes.

> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:86
> +                                                       size_t* unflattenableCount) {

Brace needs to be at the beginning of the next line for function definitions.

> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:98
> +    for (size_t i = 0; i < receipts.size(); ++i)

This for loop should have braces since the statement has more than one physical line.

[Luke Zarko]

Created attachment 103532 [details]
Address comments and rebase.

(In reply to comment #23)
> > Source/WebCore/storage/IDBCursor.h:69
> > +    TransferableArray* valueDependencies() const { return 0; }
> 
> Why does this need to be defined? (Who calls it? I couldn't find it. If it is unused, then just get rid of the changes to this file.)

This is called by autogenerated binding code (when encountering a SerializedScriptValue).

I've also reviewed some constness changes that dimich pointed out offline and rebased.

[Michael Nordman]

View in context: https://bugs.webkit.org/attachment.cgi?id=103302&action=review

> Source/WebCore/bindings/js/JSMessageEventCustom.cpp:55
> +    }

Is it possible for 'list' to be empty at this point, and if so would it make sense to return jsNull().

> Source/WebCore/dom/Transferable.h:39
> +class ScriptExecutionContext;

are the forward decls of MessagePortChannel and ScriptExecutionContext needed?

> Source/WebCore/dom/Transferable.h:61
> +    virtual bool isTransferClosed() const = 0;

maybe wasTransferredOut()

> Source/WebCore/dom/TransferableReceipt.cpp:41
> +        (*transferableArray)[i] = (*receipts)[i]->transferIn((*receipts)[i].release(), context);

What if one fails to transferIn, is it ok to have NULL entries in the return value?

> Source/WebCore/dom/TransferableReceipt.h:53
> +class TransferableReceipt {

Naming nit: The term 'receipt' feels like an awkward fit for this? A receipt is a record of a something having exchanged hands, not so much an in-transit representation. Maybe TransferToken or TransferableToken?

> Source/WebKit/chromium/public/WebTransferableReceipt.h:43
> +namespace WebCore {

can the forward decl also be behind the WEBKIT_IMPLEMETATION guard?

#if WEBKIT_IMPLEMENTATION
#include <wtf/PassOwnPtr.h>
#include <wtf/RefPtr.h>
#include <wtf/Vector.h>

namespace WebCore {
class TransferableReceipt;
typedef Vector<OwnPtr<TransferableReceipt>, 1> TransferableReceiptArray;
}
#endif

[Michael Nordman]

Comment on attachment 103532 [details]
Address comments and rebase.

View in context: https://bugs.webkit.org/attachment.cgi?id=103532&action=review

> Source/WebCore/dom/MessagePort.cpp:86
> +            if (MessagePort* dataPort = transferable->asMessagePort()) {

We're leaking the underlying 'type', can this be abstraced in the transferable interface?

transferable->CanBeTransferedOnPort(this);

> Source/WebCore/dom/TransferableReceipt.h:53
> +class TransferableReceipt {

Converting between Transferables and an in-transit representation (receipt) is handled by the new interfaces, but how is marshalling a 'receipt' from one thread to another handled? Is it assumed that a 'receipt' generated on one thread can be converted to a Transferable on another? If so comments to that affect would help.

> Source/WebCore/dom/TransferableReceipt.h:58
> +    virtual bool isFlattenable() const = 0;

Seems odd to have an isFlattenable() method with no corresponding flatten() or static unflatten() methods.

> Source/WebCore/dom/TransferableReceipt.h:60
> +    virtual PassRefPtr<Transferable> transferIn(PassOwnPtr<TransferableReceipt>, ScriptExecutionContext&) = 0;

I don't understand why the transferIn takes a 'receipt' as its first param, feels like transferIn should operate on 'this' receipt?

> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:90
> +        if (!receipts[i]->isFlattenable())

How are messagePorts sent if their not flattened?

[Luke Zarko]

(In reply to comment #25)
> View in context: https://bugs.webkit.org/attachment.cgi?id=103302&action=review
> 
> > Source/WebCore/bindings/js/JSMessageEventCustom.cpp:55
> > +    }
> 
> Is it possible for 'list' to be empty at this point,
Yes-- if there were Transferables passed but none were MessagePorts.

> and if so would it make sense to return jsNull().
Maybe (see http://www.w3.org/Bugs/Public/show_bug.cgi?id=13675). Either we should return null to mean empty always or we should return an empty array to mean empty always. What do people think? (If we should return an empty array, what would be the proper call to make for JSC?)

> > Source/WebCore/dom/Transferable.h:61
> > +    virtual bool isTransferClosed() const = 0;
> 
> maybe wasTransferredOut()
OK

> > Source/WebCore/dom/TransferableReceipt.cpp:41
> > +        (*transferableArray)[i] = (*receipts)[i]->transferIn((*receipts)[i].release(), context);
> 
> What if one fails to transferIn, is it ok to have NULL entries in the return value?
You shouldn't fail to transferIn; I will add an assert.
 
> > Source/WebCore/dom/TransferableReceipt.h:53
> > +class TransferableReceipt {
> 
> Naming nit: The term 'receipt' feels like an awkward fit for this? A receipt is a record of a something having exchanged hands, not so much an in-transit representation. Maybe TransferToken or TransferableToken?
I justify this as follows: when you transfer control of your bag at the airport you get a baggage claim receipt. At a more meticulous airline, you exchange the receipt for your bag once you arrive at your destination. Tokens and tickets make me think of security; 'record' could be anything; 'promise' is overloaded in the domain of concurrency concepts.

> > Source/WebKit/chromium/public/WebTransferableReceipt.h:43
> > +namespace WebCore {
> 
> can the forward decl also be behind the WEBKIT_IMPLEMETATION guard?
OK

(In reply to comment #26)
> (From update of attachment 103532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103532&action=review
> 
> > Source/WebCore/dom/MessagePort.cpp:86
> > +            if (MessagePort* dataPort = transferable->asMessagePort()) {
> 
> We're leaking the underlying 'type', can this be abstraced in the transferable interface?
> 
> transferable->CanBeTransferedOnPort(this);
This is tempting, but Transferable::asMessagePort() is also used in the implementation of MessageEvent.ports. Likewise, the ::as* methods in TransferableReceipt are needed for flattening; those methods in WebTransferableReceipt are needed for unflattenable objects. Since these objects represent parts of messages, I found it to be useful to keep most control with the mechanisms for message passing (eg, by allowing those mechanisms to pattern match over Transferables). In other words, my goal was to keep details about the message passing system hidden from the messages themselves.
> 
> > Source/WebCore/dom/TransferableReceipt.h:53
> > +class TransferableReceipt {
> 
> Converting between Transferables and an in-transit representation (receipt) is handled by the new interfaces, but how is marshalling a 'receipt' from one thread to another handled? Is it assumed that a 'receipt' generated on one thread can be converted to a Transferable on another? If so comments to that affect would help.

Yes, it is assumed that a TransferableReceipt can be passed to another thread (but not to another process). I will amend the expository comment in TransferableReceipt.h.

> > Source/WebCore/dom/TransferableReceipt.h:58
> > +    virtual bool isFlattenable() const = 0;
> 
> Seems odd to have an isFlattenable() method with no corresponding flatten() or static unflatten() methods.

This happens as part of the implementation of SerializedScriptValue's ::flattenReceiptList static method (as an analog to ArrayBuffer's lack of a ::serialize or ::deserialize method). On further review it seems like isFlattenable() should become a static member of SerializedScriptValue.

> > Source/WebCore/dom/TransferableReceipt.h:60
> > +    virtual PassRefPtr<Transferable> transferIn(PassOwnPtr<TransferableReceipt>, ScriptExecutionContext&) = 0;
> 
> I don't understand why the transferIn takes a 'receipt' as its first param, feels like transferIn should operate on 'this' receipt?

transferIn adopts control of the TransferableReceipt (which is why it accepts it as a PassOwnPtr). This is not a static method so that it can dispatch on the TransferableReceipt type to choose the correct transferIn.

> 
> > Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:90
> > +        if (!receipts[i]->isFlattenable())
> 
> How are messagePorts sent if their not flattened?

A Transferable object not being flattenable implies that it needs special handling when being sent across process boundaries. See http://codereview.chromium.org/7477027/patch/18001/19004 for what happens in Chromium for this case.

[Michael Nordman]

Comment on attachment 103532 [details]
Address comments and rebase.

View in context: https://bugs.webkit.org/attachment.cgi?id=103532&action=review

>> Source/WebCore/bindings/js/JSMessageEventCustom.cpp:55
>> +    }
> 
> Yes-- if there were Transferables passed but none were MessagePorts.

line 50 returns jsNull() in a similar case

>>> Source/WebCore/dom/MessagePort.cpp:86
>>> +            if (MessagePort* dataPort = transferable->asMessagePort()) {
>> 
>> We're leaking the underlying 'type', can this be abstraced in the transferable interface?
>> 
>> transferable->CanBeTransferedOnPort(this);
> 
> This is tempting, but Transferable::asMessagePort() is also used in the implementation of MessageEvent.ports. Likewise, the ::as* methods in TransferableReceipt are needed for flattening; those methods in WebTransferableReceipt are needed for unflattenable objects. Since these objects represent parts of messages, I found it to be useful to keep most control with the mechanisms for message passing (eg, by allowing those mechanisms to pattern match over Transferables). In other words, my goal was to keep details about the message passing system hidden from the messages themselves.

I see, so from webcore's point of view, a TransferableReceipt created on one thread has to be redeemable on another thread, and that's all that is needed in the absence of IPC.

>>> Source/WebCore/dom/TransferableReceipt.h:58
>>> +    virtual bool isFlattenable() const = 0;
>> 
>> Seems odd to have an isFlattenable() method with no corresponding flatten() or static unflatten() methods.
> 
> This happens as part of the implementation of SerializedScriptValue's ::flattenReceiptList static method (as an analog to ArrayBuffer's lack of a ::serialize or ::deserialize method). On further review it seems like isFlattenable() should become a static member of SerializedScriptValue.

That seems more consistent with your goal of keeping message passing details out of this interface.

>>> Source/WebCore/dom/TransferableReceipt.h:60
>>> +    virtual PassRefPtr<Transferable> transferIn(PassOwnPtr<TransferableReceipt>, ScriptExecutionContext&) = 0;
>> 
>> I don't understand why the transferIn takes a 'receipt' as its first param, feels like transferIn should operate on 'this' receipt?
> 
> transferIn adopts control of the TransferableReceipt (which is why it accepts it as a PassOwnPtr). This is not a static method so that it can dispatch on the TransferableReceipt type to choose the correct transferIn.

I see, because transferring in conceptually involves destroying the receipt, but in the MessagePort case you don't actually want to destroy the channel since it gets wrapped up (or entangled if you prefer the technical term:) with the resulting MessagePort.

This is an odd interface for an instance method from a callers point of view.

   OwnPtr<> receipt;
   receipt->transferIn(receipt.release());

Maybe static PRP<Transferable> TransferableReceipt::transferIn(PassOwnPtr<> receipt) which calls a private virtual instance method that has the semantics you want for this?

Or instead of having the 'receipt' type being a MessagePortChannel, have a MessagePortTransferrableReceipt which holds the channel up until transferIn time. After transferIn time, it could be deleted without breaking the resulting MessagePort. I think that might allow a cleaner API for these base classes.

   OwnPtr<> receipt;
   receipt->transferIn(context);

[Luke Zarko]

(In reply to comment #28)
> (From update of attachment 103532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103532&action=review
> 
> >> Source/WebCore/bindings/js/JSMessageEventCustom.cpp:55
> >> +    }
> > 
> > Yes-- if there were Transferables passed but none were MessagePorts.
> 
> line 50 returns jsNull() in a similar case

I've gone ahead and changed both the JSC and V8 sides to return empty arrays instead of null here.

Something interesting came up when I posted about initMessageEvent to public-webapps: what was the rationale for using SerializedScriptValue in initMessageEvent() in the first place? When I first read through the code I thought this came as a result of

"The initMessageEvent() method must initialize the event in a manner analogous to the similarly-named method in the DOM Events interfaces. [DOMEVENTS]"

-- but when I went back just now to verify this, I was unable to find any mention of MessageEvent in the [DOMEVENTS] citation.

> Or instead of having the 'receipt' type being a MessagePortChannel, have a MessagePortTransferrableReceipt which holds the channel up until transferIn time. After transferIn time, it could be deleted without breaking the resulting MessagePort. I think that might allow a cleaner API for these base classes.
> 
>    OwnPtr<> receipt;
>    receipt->transferIn(context);

This makes sense. In the patch I was attempting to change as little of the semantics/rules of ownership as possible, hence the strangeness here. What are your feelings about WebMessagePortChannel/WebTransferableReceipt?

[David Levin]

Comment on attachment 103532 [details]
Address comments and rebase.

btw, the style for the Chromium files is to arrange the methods in the cpp file in the same order as they are declared in the header file. The files under the Source/WebKit/chromium directory should follow this convention.

[Luke Zarko]

Created attachment 104212 [details]
Address comments; add a ScriptExecutionContext* argument to transferOut.

[Michael Nordman]

Comment on attachment 104212 [details]
Address comments; add a ScriptExecutionContext* argument to transferOut.

Thanx for putting this stuff together.

View in context: https://bugs.webkit.org/attachment.cgi?id=104212&action=review

> Source/WebCore/ChangeLog:19
> +            - Transferring out a MessagePort yields a MessagePortChannelTransferableReceipt.

The thing that's being transferred out is a 'port'. Would it make more sense to yield a 'portReceipt', 
MessagePortReceipt or MessagePortTransferableReceipt?

> Source/WebCore/dom/MessagePortChannel.cpp:51
> +{

maybe ASSERT(m_messagePortChannel)

> Source/WebCore/dom/MessagePortChannel.h:120
> +        MessagePortChannel* messagePortChannel() const { return m_messagePortChannel.get(); }

Is this actually needed in the public interface? I don't see a caller for it. If not consider removing it.

> Source/WebKit/chromium/public/WebMessagePortChannel.h:45
> +class WebMessagePortChannel : public WebTransferableReceipt {

Having PortChannel derived from Receipt seems like a shortcut. Should we introduce a WebMessagePortReceipt in the webkit api too?

I don't know about others, but to me it's pretty challenging to follow the logic of the transferable/receipt handling stuff from end-to-end. Seeing that the channel ISA receipt makes it more confusing since sometimes its being treated as a channel and other times its being treated as a receipt, but its not actually usable as both at the same time (afaik). But the interfaces may lead you to believe something else. (Or maybe i'm mistaken about why this derivation is here).

> Source/WebKit/chromium/public/WebTransferableReceipt.h:57
> +    virtual WebMessagePortChannel* asWebMessagePortChannel() { return 0; }

If you buy that previous comment, this should probably be something like asMessagePortReceipt()

[Darin Fisher (:fishd, Google)]

Comment on attachment 104212 [details]
Address comments; add a ScriptExecutionContext* argument to transferOut.

View in context: https://bugs.webkit.org/attachment.cgi?id=104212&action=review

> Source/WebKit/chromium/public/WebMessagePortChannel.h:52
> +    virtual void postMessage(const WebString&, WebTransferableReceiptArray*) = 0;

why is the second parameter non-const?  it seems like it should be passed as |const Foo&| since you don't expect postMessage to mutate the array.

> Source/WebKit/chromium/public/WebMessagePortChannel.h:54
> +    virtual WebMessagePortChannel* asWebMessagePortChannel() { return this; }

you may need to export this function since you are implementing it.  it may need to be tagged with WEBKIT_EXPORT.  please make sure that the component=shared_library build does not break.

> Source/WebKit/chromium/public/WebSerializedScriptValue.h:72
> +    WEBKIT_EXPORT static bool isFlattenable(WebTransferableReceipt*);

Why isn't this a member function of WebTransferableReceipt?  It seems like
something that should be a property.

> Source/WebKit/chromium/public/WebSerializedScriptValue.h:77
> +    WEBKIT_EXPORT static WebString flattenReceiptList(const WebString&,

this function is really quite confusing to me... reading the comment, it is
not so obvious what this function will do.

i thought after we chatted the other day that the plan was to do this final
flattening at the chromium level.  maybe i misunderstood something.

> Source/WebKit/chromium/public/WebTransferableReceipt.h:57
> +    virtual WebMessagePortChannel* asWebMessagePortChannel() { return 0; }

perhaps you should use the approach that we use for casting WebElement to WebInputElement?
define a global function in WebMessagePortChannel.h:

  WEBKIT_EXPORT WebMessagePortChannel* toWebMessagePortChannel(WebTransferableReceipt*);

we don't use the asFoo approach to RTTI anywhere else in the WebKit API, but we do use
the toFoo approach.  seems like we should be consistent.

> Source/WebKit/chromium/public/WebTransferableReceipt.h:64
> +    static PassOwnPtr<WebCore::TransferableReceipt> fromWeb(WebTransferableReceipt*);

Perhaps these would be better as global functions:

  PassOwnPtr<WebCore::TransferableReceipt> toTransferableReceipt(WebTransferableReceipt*);

> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:78
> +    for (size_t i = 0; i < receipts.size(); ++i) {

it seems like this workload of determining the unflattenable count could be
done by the embedder too.

it is interesting that SerializedScriptValue::flattenReceiptList does not
similarly determine the unflattenable count.  this seems to indicate that
it should not be done by the WebKit API either.  complexity should either
be in WebCore or in Chromium, not in the WebKit "API glue" layer.

> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:93
> +    return WebCore::SerializedScriptValue::flattenReceiptList(message, *coreReceipts);

no need for the WebCore:: prefix.

[Luke Zarko]

Created attachment 104550 [details]
Address comments; add explicit receipts in WebKit API

(In reply to comment #32)
> (From update of attachment 104212 [details])
> Thanx for putting this stuff together.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=104212&action=review
> 
> > Source/WebCore/ChangeLog:19
> > +            - Transferring out a MessagePort yields a MessagePortChannelTransferableReceipt.
> 
> The thing that's being transferred out is a 'port'. Would it make more sense to yield a 'portReceipt', 
> MessagePortReceipt or MessagePortTransferableReceipt?

OK (MessagePortReceipt).

> > Source/WebCore/dom/MessagePortChannel.cpp:51
> > +{
> 
> maybe ASSERT(m_messagePortChannel)

OK.

> > Source/WebCore/dom/MessagePortChannel.h:120
> > +        MessagePortChannel* messagePortChannel() const { return m_messagePortChannel.get(); }
> 
> Is this actually needed in the public interface? I don't see a caller for it. If not consider removing it.

Yes; it is used by the API layer (WebTransferableReceipt).

> > Source/WebKit/chromium/public/WebMessagePortChannel.h:45
> > +class WebMessagePortChannel : public WebTransferableReceipt {
> 
> Having PortChannel derived from Receipt seems like a shortcut. Should we introduce a WebMessagePortReceipt in the webkit api too?
> 
> I don't know about others, but to me it's pretty challenging to follow the logic of the transferable/receipt handling stuff from end-to-end. Seeing that the channel ISA receipt makes it more confusing since sometimes its being treated as a channel and other times its being treated as a receipt, but its not actually usable as both at the same time (afaik). But the interfaces may lead you to believe something else. (Or maybe i'm mistaken about why this derivation is here).
> 
> > Source/WebKit/chromium/public/WebTransferableReceipt.h:57
> > +    virtual WebMessagePortChannel* asWebMessagePortChannel() { return 0; }
> 
> If you buy that previous comment, this should probably be something like asMessagePortReceipt()

OK. Embedders are responsible for deleting receipts (as well as the array returned by toWebAll, but this was already the case before).

(In reply to comment #33)
> (From update of attachment 104212 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104212&action=review
> 
> > Source/WebKit/chromium/public/WebMessagePortChannel.h:52
> > +    virtual void postMessage(const WebString&, WebTransferableReceiptArray*) = 0;
> 
> why is the second parameter non-const?  it seems like it should be passed as |const Foo&| since you don't expect postMessage to mutate the array.

The second parameter may be NULL; since the receiver is responsible for the vector, it must deallocate the receipts contained therein.

> > Source/WebKit/chromium/public/WebSerializedScriptValue.h:72
> > +    WEBKIT_EXPORT static bool isFlattenable(WebTransferableReceipt*);
> 
> Why isn't this a member function of WebTransferableReceipt?  It seems like
> something that should be a property.

This is part of the mechanism for serialization, so I added it here. (WebTransferableReceipts don't
flatten themselves; WebSerializedScriptValue flattens them. It's a question asked of the latter
interface: do you flatten this receipt?)

> > Source/WebKit/chromium/public/WebSerializedScriptValue.h:77
> > +    WEBKIT_EXPORT static WebString flattenReceiptList(const WebString&,
> 
> this function is really quite confusing to me... reading the comment, it is
> not so obvious what this function will do.
> 
> i thought after we chatted the other day that the plan was to do this final
> flattening at the chromium level.  maybe i misunderstood something.

Yes, but since the operation of flattening is a form of serialization, it is implemented
as part of the serializer. The final flattening is requested at the Chromium level using
this interface. At the point where flattening is requested, the message has been reduced
to a WebString (see webworkerclient_proxy.cc's postMessageToWorkerObject for an example).

> 
> > Source/WebKit/chromium/public/WebMessagePortChannel.h:54
> > +    virtual WebMessagePortChannel* asWebMessagePortChannel() { return this; }
> 
> you may need to export this function since you are implementing it.  it may need to be tagged with WEBKIT_EXPORT.  please make sure that the component=shared_library build does not break.
>
> > Source/WebKit/chromium/public/WebTransferableReceipt.h:57
> > +    virtual WebMessagePortChannel* asWebMessagePortChannel() { return 0; }
> 
> perhaps you should use the approach that we use for casting WebElement to WebInputElement?
> define a global function in WebMessagePortChannel.h:
> 
>   WEBKIT_EXPORT WebMessagePortChannel* toWebMessagePortChannel(WebTransferableReceipt*);
> 
> we don't use the asFoo approach to RTTI anywhere else in the WebKit API, but we do use
> the toFoo approach.  seems like we should be consistent.

OK (modulo the component=shared_library check-- I can't build this on my machine because it is a Mac (?))

> 
> > Source/WebKit/chromium/public/WebTransferableReceipt.h:64
> > +    static PassOwnPtr<WebCore::TransferableReceipt> fromWeb(WebTransferableReceipt*);
> 
> Perhaps these would be better as global functions:
> 
>   PassOwnPtr<WebCore::TransferableReceipt> toTransferableReceipt(WebTransferableReceipt*);
> 

OK

> > Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:78
> > +    for (size_t i = 0; i < receipts.size(); ++i) {
> 
> it seems like this workload of determining the unflattenable count could be
> done by the embedder too.
> 
> it is interesting that SerializedScriptValue::flattenReceiptList does not
> similarly determine the unflattenable count.  this seems to indicate that
> it should not be done by the WebKit API either.  complexity should either
> be in WebCore or in Chromium, not in the WebKit "API glue" layer.

The embedder needs access to the number of unflattenable objects because it needs to
perform allocations with sizes that are determined by that number. Again, see
webworkerclient_proxy.cc: we have made the determination that we're going to be doing IPC,
so we issue an API request to flatten our message (which is at this point a simple WebString).
WebSerializedScriptValue::flattenReceiptList walks through the list of receipts and
shuffles those that need to be flattened back over to WebCore, which contains the serialization
logic necessary to actually change the message body. Nothing is done to unflattenable receipts
(no memory allocation, even); we don't (eg) wrap a WebMessagePortChannel to later ignore it.
After the new message is returned, we do whatever platform-specific task is necessary to
deal with the remaining unflattenables (here, we copy message port and routing IDs). While
we could certainly walk over the list in the embedder a second time--or just push_back onto the
vectors of ints--there's no harm in returning the number of unflattenable objects given that
it should be useful to almost any embedder that is calling flattenReceiptList.

> > Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:93
> > +    return WebCore::SerializedScriptValue::flattenReceiptList(message, *coreReceipts);
> 
> no need for the WebCore:: prefix.

OK

Additional changes:

Added logic in Transferable::transferAllOut to deal with Transferables that fail to transfer out (eg, MessagePorts with no entanglements).
Added logic in JSC SerializedScriptValue to prevent non-Transferred MessagePorts from appearing in message bodies; also added logic to prevent them from appearing in message bodies in legacy postMessage mode (normalizing with V8).