911972 - MessagePort and MessageChannel in workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Andrea Marchesini [:baku]]

Follow up for Bug 677638 Comment 59

Comment 1

[Andrea Marchesini [:baku]]

Attachment: WIP 1 - MessagePortProxy

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 798806 [details] [diff] [review]
WIP 1 - MessagePortProxy

I don't quite understand why mEntangledPortId is needed. Couldn't you just
pass the port when you currently pass mEntangledPortId.
Then the dispatched message should go to the port not passed as param.
Or do it the id there so that we don't need to addref/release on a wrong thread?
Is so, some comments please.

What is mProxysarray? We just append objects to it but never us it.
Is it just to keep objects alive? Some comments please.

I may not understand all of this yet, but some comments would be useful.

Comment 3

[SUN Haitao]

Any news on this?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

See this bug for a recent modification to the spec for MessageChannel/MessagePort

https://www.w3.org/Bugs/Public/show_bug.cgi?id=23685

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch 1 - MessagePortProxy

This first patch implements MessagePortProxy, a thread-safe object.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch 2 - Logic moved into MessagePortBase

In order to have the same logic in workers and on main-thread, this has been moved to MessagePortBase.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch 3 - MessageChannel and MessagePort in workers

This 3rd patch implements MessageChannel and MessagePort in workers.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch 4 - Worker and MainThread speaking to each other

This last patch let's MessagePort move between threads.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessageChannel and MessagePort in workers

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch 3 - Worker and MainThread speaking to each other

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8368250 [details] [diff] [review]
patch 1 - MessagePortProxy


>+class MessagePortProxy
>+{
>+public:
>+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MessagePortProxy)
>+
>+  MessagePortProxy(MessagePort* aPort)
>+    : mPort(aPort)
>+  {
>+  }
>+
>+  MessagePort*
>+  Port() const
>+  {
>+    return mPort;
>+  }
>+
>+  MessagePortProxy*
>+  EntangledProxy() const
>+  {
>+    return mEntangledProxy;
>+  }
>+
>+  void
>+  SetEntangledProxy(MessagePortProxy* aProxy)
>+  {
>+    mEntangledProxy = aProxy;
>+  }
>+
>+  already_AddRefed<PostMessageRunnable>
>+  Pop();
Pop() is an odd name for a method in a class called MessagePortProxy.
Perhaps TakeFirstPendingRunnable() ... that is a bit long.


>+  // CC methods
>+  void Unlink();
>+  void Traverse(nsCycleCollectionTraversalCallback& cb,
aCb

>+MessagePortProxy::Traverse(nsCycleCollectionTraversalCallback& cb,
s/cb/aCb

>+                           bool aNoLoop)
s/aNoLoop/aOnlyPortReference/ or something like that.




(I assume some things I saw are fixed in some other patch in this bug)

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8368299 [details] [diff] [review]
patch 2 - MessageChannel and MessagePort in workers

>+  if (NS_IsMainThread()) {
>+    // Get the JSContext for the target window
>+    nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(mPort->GetOwner());
>+    NS_ENSURE_STATE(sgo);

You're just moving this code, but actually, I think nsCxPusher would be simpler and safer here, 
since it ends up doing innerwindow check too...

So
nsCOMPtr<EventTarget> target = do_QueryInterface(mPort->GetOwner());
nsCxPusher pusher;
NS_ENSURE_STATE(pusher.Push(target));

(cx pushing is such a huge mess currently.)




>+  nsCOMPtr<EventTarget> target;
>+  if (NS_IsMainThread()) {
>+    target = do_QueryInterface(mPort->GetOwner());
>   }
> 
>-  ErrorResult error;
>-  nsRefPtr<nsDOMEvent> event =
>-    doc->CreateEvent(NS_LITERAL_STRING("MessageEvent"), error);
>-  if (error.Failed()) {
>-    return NS_OK;
>-  }
>+  nsRefPtr<nsDOMMessageEvent> event =
>+    new nsDOMMessageEvent(target, nullptr, nullptr);
You could just pass mPort as target




>+  nsresult rv = event->InitEvent(NS_LITERAL_STRING("MessageEvent"), false,
>+                                 true);
>   if (NS_FAILED(rv)) {
>     return NS_OK;
>   }
> 
>-  message->SetTrusted(true);
>+  rv = event->InitMessageEvent(NS_LITERAL_STRING("message"),
>+                               false /* non-bubbling */,
>+                               true /* cancelable */,
>+                               messageData,
>+                               EmptyString(),
>+                               EmptyString(),
>+                               NS_IsMainThread() ? mPort->GetOwner() : nullptr);

You InitEvent and then InitMessageEvent? Latter only please.

>+  nsRefPtr<nsDOMEvent> e = static_cast<nsDOMEvent*>(event.get());
nsDOMEvent* e = event->InternalDOMEvent();


> 
>   bool status;
>-  mPort->DispatchEvent(event, &status);
>+  mPort->DispatchEvent(e, &status);
but I don't understand why you need nsDOMEvent. DispatchEvent takes nsIDOMEvent* and bool&x



Could take a quick second look.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch 1 - MessagePortProxy

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessageChannel and MessagePort in workers

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8368300 [details] [diff] [review]
patch 3 - Worker and MainThread speaking to each other

You said on IRC you're still fixing some issues, so clearing review request.

Comment 16

[James Long (:jlongster)]

Is this the only bug blocking MessageChannel to be available in Firefox? Haven't seen movement on this for a while, any chance it's on the roadmap?

Comment 17

[Andrea Marchesini [:baku]]

(In reply to James Long (:jlongster) from comment #16)
> Is this the only bug blocking MessageChannel to be available in Firefox?
> Haven't seen movement on this for a while, any chance it's on the roadmap?

Yes. I have something written based on PBackground. I don't know when this can be reviewed but I hope to upload some working patch for this week or the next one.

There is a big limitation with my new patches: no transferable objects.
But this can be done in a follow up.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Move MessagePort code into a separated directory.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground - WIP

This is the missing pieces:

1. transferable objects in MessagePort.postMessage
2. worker.postMessage doesn't send a port to the main-thread
3. sometimes the ordering of the messages is wrong.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch 3 - transferable objects in MessagePort

This patch allows messagePort to transfer ArrayBuffers and MessagePorts to its entangled port.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: patch 3 - transferable objects in MessagePort

Comment 22

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground - WIP

This patch doesn't respect the ordering of the messages and it has to be updated.
But for now it can be used for testing ServiceWorkers.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: patch 3 - transferable objects in MessagePort

Comment 24

[Andrea Marchesini [:baku]]

Attachment: patch 4 - MessagePort transferred cross threads.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground

The shared queue is in a separated patch. I prefer to keep them separated, but if you want I can merge it for the purpose of the review.

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 8475111 [details] [diff] [review]
patch 1 - Move MessagePort code into a separated directory.

This patch moves MessageChannel/MessagePort files in a separated directory. With PBackground and the next patches I needed to add many files and it's cleaner to have them separated from the rest of dom/base.

Comment 27

[Andrea Marchesini [:baku]]

Attachment: patch 3 - MessagePort Shared Queue

This patch is what the spec calls the 'unshipped port message queue'. I would like to talk with smaug on IRC about this queue and what the spec says.

Comment 28

[Andrea Marchesini [:baku]]

Attachment: patch 4 - transferable objects in MessagePort

Transferable objects in MessagePort

Comment 29

[Andrea Marchesini [:baku]]

Attachment: patch 5 - MessagePort transferred cross threads.

Transferring messagePort to workers.

Comment 30

[Andrea Marchesini [:baku]]

Attachment: patch 3 - MessagePort Shared Queue

Much simpler queue implementation.

Comment 31

[Andrea Marchesini [:baku]]

Attachment: patch 4 - transferable objects in MessagePort

rebased

Comment 32

[Andrea Marchesini [:baku]]

Attachment: patch 3 - MessagePort unshipped port queue

A nice optimization avoids the creation of an actor when we don't need it.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: patch 4 - transferable objects in MessagePort

rebased

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8475111 [details] [diff] [review]
patch 1 - Move MessagePort code into a separated directory.

I guess this is fine. messagechannel is complicated enough.

(and I assume this patch is ok to review even if the other patches will be changed)

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8477847 [details] [diff] [review]
patch 2 - MessagePort/Channel with PBackground

So I assume this is about to be changed a bit.
Re-ask review if I'm wrong.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8478004 [details] [diff] [review]
patch 4 - transferable objects in MessagePort

ditto

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8478190 [details] [diff] [review]
patch 4 - transferable objects in MessagePort

There was another 'patch 4' in the queue. Dunno which one would be even the
one to review.

Comment 38

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground

This is the patch after the meeting we had.

The protocol is simpler and it works in an 'optimistic' way so that we have 1 single IPDL message for delivering data. and a "StopSendingData" request when this flooding has to be interrupted.

I still prefer the idea to have an actor for sending and receiving instead splitting the 2 operations. I think the code written in this way is more similar to what the spec say and it's easier to understand: in the end the MessagePort is bi-directional API, so it makes sense having 1 actor for both operations.

Comment 39

[Andrea Marchesini [:baku]]

Attachment: patch 3 - MessagePort unshipped port queue

Comment 40

[Andrea Marchesini [:baku]]

Attachment: patch 4 - transferable objects in MessagePort

Smaug, I don't know if this patch is for you or for bent.
It's moslty about the use of StructuredCloneAlgorithm.

Comment 41

[Andrea Marchesini [:baku]]

Attachment: patch 5 - MessagePort transferred cross threads.

Comment 42

[Andrea Marchesini [:baku]]

Comment on attachment 8480738 [details] [diff] [review]
patch 2 - MessagePort/Channel with PBackground

This is not a WIP :)

Comment 43

[Andrea Marchesini [:baku]]

Comment on attachment 8480744 [details] [diff] [review]
patch 4 - transferable objects in MessagePort

Review of attachment 8480744 [details] [diff] [review]:
-----------------------------------------------------------------

There is a bug in the cloning of the MessagePort. I wrote here the fix instead uploading a new patch.

::: dom/base/nsGlobalWindow.cpp
@@ +8091,5 @@
>  
> +    MessagePortCloneData* clonedPortData =
> +      scInfo->event->NewClonedPortData(aExtraData);
> +
> +    port->Clone(*clonedPortData);

if (!port->Clone(...)) return false.

::: dom/ipc/StructuredCloneUtils.cpp
@@ +216,5 @@
> +
> +    MessagePortCloneData* clonedPort =
> +      closure->mClosure.mMessagePorts.AppendElement();
> +
> +    port->Clone(*clonedPort);

if (!port->Clone(...)) return false.

::: dom/messagechannel/MessagePort.cpp
@@ +569,5 @@
>  
>    UpdateMustKeepAlive();
>  }
>  
> +void

boolean and return true everywhere.

::: dom/messagechannel/MessagePort.h
@@ +56,5 @@
>  
>    // Duplicate this message port. This method is used by the Structured Clone
>    // Algorithm and populates a MessagePortCloneData object with the information
>    // useful to create new MessagePort.
> +  virtual void

this must return a boolean.

@@ +99,5 @@
>    // Non WebIDL methods
>  
>    void UnshippedEntangle(MessagePort* aEntangledPort);
>  
> +  virtual void Clone(MessagePortCloneData& aData) MOZ_OVERRIDE;

boolean Clone()

::: dom/workers/MessagePort.cpp
@@ +197,5 @@
>  
>    Start();
>  }
>  
> +void

boolean and return false.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8480738 [details] [diff] [review]
patch 2 - MessagePort/Channel with PBackground

> struct StructuredCloneInfo {
>   PostMessageEvent* event;
>   bool subsumes;
>   nsPIDOMWindow* window;
>-  nsRefPtrHashtable<nsRefPtrHashKey<MessagePortBase>, MessagePortBase> ports;
>+
>+  // This hashtable contains the transferred ports and their clone data.
>+  nsRefPtrHashtable<nsRefPtrHashKey<MessagePortBase>, MessagePortCloneData> ports;
>+
>+  // This array is populated when the ports are cloned.
>+  nsTArray<nsRefPtr<MessagePortBase>> clonedPorts;
So 'ports' hashtable doesn't actually contain any ports. I think the name should be something else.
Perhaps portIdentifiers o



>   if (tag == SCTAG_DOM_MAP_MESSAGEPORT) {
>-    MessagePort* port = static_cast<MessagePort*>(aData);
>-    port->BindToOwner(scInfo->window);
>-    scInfo->ports.Put(port, nullptr);
>+    MessagePortCloneData* clonedPort = static_cast<MessagePortCloneData*>(aData);
clonedPort doesn't point to cloned port, but to cloned data, so I wonder if some other variable name would be better.


>+    nsRefPtr<MessagePort> port = new MessagePort(scInfo->window, *clonedPort);
... since port is the actual cloned port

>+    scInfo->clonedPorts.AppendElement(port);
>+    NS_RELEASE(clonedPort);
Don't use manual NS_RELEASE, but perhaps nsRefPtr<MessagePortCloneData> + dont_AddRef



>@@ -8061,70 +8066,57 @@ PostMessageTransferStructuredClone(JSCon
>                                    uint64_t* aExtraData)
> {
>   StructuredCloneInfo* scInfo = static_cast<StructuredCloneInfo*>(aClosure);
>   NS_ASSERTION(scInfo, "Must have scInfo!");
> 
>   MessagePortBase* port = nullptr;
>   nsresult rv = UNWRAP_OBJECT(MessagePort, aObj, port);
>   if (NS_SUCCEEDED(rv)) {
>-    nsRefPtr<MessagePortBase> newPort;
>-    if (scInfo->ports.Get(port, getter_AddRefs(newPort))) {
>+    nsRefPtr<MessagePortCloneData> clonedPort;
>+    if (scInfo->ports.Get(port, getter_AddRefs(clonedPort))) {
Again, clonedPort doesn't point to port


Ah, looks like we stop having exactly the same PostMessageTransferStructuredClone implemented twice.
Once in nsGlobalWindow.cpp and once in MessagePort.cpp. Good good.


> void
> PostMessageFreeTransferStructuredClone(uint32_t aTag, JS::TransferableOwnership aOwnership,
>                                        void *aContent, uint64_t aExtraData, void* aClosure)
> {
>-  StructuredCloneInfo* scInfo = static_cast<StructuredCloneInfo*>(aClosure);
>-  NS_ASSERTION(scInfo, "Must have scInfo!");
>-
>   if (aTag == SCTAG_DOM_MAP_MESSAGEPORT) {
>-    nsRefPtr<MessagePortBase> port(static_cast<MessagePort*>(aContent));
>-    scInfo->ports.Remove(port);
>+    nsRefPtr<MessagePortCloneData> clonedPort(static_cast<MessagePortCloneData*>(aContent));
>+    NS_RELEASE(clonedPort);
Does this even compile.
I assume you want
nsRefPtr + dont_AddRef and then drop NS_RELEASE.
And again, clonedPort doesn't actually point to a port so some clearer naming would be good.


>+nsresult
>+GenerateUUID(nsIUUIDGenerator* aUUIDGen, nsAString& aId)
>+{
>+  nsID id;
>+  nsresult rv = aUUIDGen->GenerateUUIDInPlace(&id);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return rv;
>+  }
>+
>+  char chars[NSID_LENGTH];
>+  id.ToProvidedString(chars);
>+  CopyASCIItoUTF16(chars, aId);
>+
>+  return NS_OK;
Why do we need to store this uuid in nsString, why not in nsCString or perhaps even in nsID?


>+bool
>+CheckPermission(nsIPrincipal* aPrincipal, bool aCallerChrome)
> {
>-  if (!gPrefInitialized) {
>-    Preferences::AddBoolVarCache(&gPrefEnabled, "dom.messageChannel.enabled");
>-    gPrefInitialized = true;
>-  }
>+  bool enabled = false;
>+  Preferences::GetBool("dom.messageChannel.enabled", &enabled);
Any reason to drop using VarCache? That is after all faster.


>+nsIPrincipal*
>+GetPrincipalFromWorkerPrivate(WorkerPrivate* aWorkerPrivate)
Hmm, wasn't something like this added in some other bug

> MessageChannel::Constructor(const GlobalObject& aGlobal, ErrorResult& aRv)
> {
>   nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
>-  if (!window) {
>-    aRv.Throw(NS_ERROR_UNEXPECTED);
>+  // window can be null in workers.
>+
>+  nsresult rv;
>+  nsCOMPtr<nsIUUIDGenerator> uuidgen =
>+    do_GetService("@mozilla.org/uuid-generator;1", &rv);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    aRv.Throw(rv);
>     return nullptr;
>   }
> 
>-  nsRefPtr<MessageChannel> channel = new MessageChannel(window);
>+  nsString portUUID1;
>+  aRv = GenerateUUID(uuidgen, portUUID1);
Tiny bit ugly code to require caller to get the service in order to use GenerateUUID.
Couldn't you just put GenerateUUID to nsContentUtils and let nsContentUtils to keep a pointer to nsIUUIDGenerator


How do we handle bfcaching?




>+MessagePort::Entangled(const nsTArray<MessagePortMessage>& aMessages)
This method needs comments.


> {
>-  MOZ_ASSERT(aMessagePort);
>-  MOZ_ASSERT(aMessagePort != this);
>+  MOZ_ASSERT(mState == StateEntangling);
>+  MOZ_ASSERT(mMessageQueue.IsEmpty());
> 
>-  Close();
>+  mState = StateEntangled;
> 
>-  mEntangledPort = aMessagePort;
>+  // If we have pending messages, these have to be sent.
>+  if (!mMessagePendingQueue.IsEmpty()) {
>+    nsTArray<MessagePortMessage> messages;
>+    MessagePortData::FromDataToMessages(mMessagePendingQueue, messages);
>+    mActor->SendPostMessages(messages);
>+    mMessagePendingQueue.Clear();
>+  }
>+
>+  // We were waiting for the entangling callback in order to disentangle this
>+  // port immediatelly after.
immediately

>+  if (mNextStep == NextStepDisentangle) {
>+    nsTArray<nsRefPtr<MessagePortData>> data;
>+    MessagePortData::FromMessagesToData(aMessages, data);
>+    mMessageQueue.AppendElements(data);
So here we put data to mMessageQueue...

>+    StartDisentangling();
>+    return;
>+  }
>+
>+  if (mNextStep == NextStepClose) {
>+    Close();
>+    return;
>+  }
>+
>+  MOZ_ASSERT(mNextStep == NextStepNone);
>+
>+  nsTArray<nsRefPtr<MessagePortData>> data;
>+  MessagePortData::FromMessagesToData(aMessages, data);
>+  mMessageQueue.AppendElements(data);
...And also here.

Couldn't you move if (mNextStep == NextStepClose) { higher up and have the AppendElements just once.


>+void
>+MessagePort::StartDisentangling()
>+{
>+  MOZ_ASSERT(mActor);
>+  MOZ_ASSERT(mState == StateEntangled);
>+
>+  mState = StateDisentangling;
>+  mNextStep = NextStepNone;
>+
>+  mActor->SendStopSendingData();
Hmm, how does this work. We have stuff in mMessageQueue. What happens to it? Please add comment about
Disentangle clearing the queue later.
>+class MessagePortCloneData
>+{
>+public:
>+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MessagePortCloneData)
>+
>+  nsString mUUID;
>+  nsString mDestinationUUID;
>+  uint32_t mSequenceID;
>+
>+private:
>+  ~MessagePortCloneData()
>+  { }
>+};
So you never initialize mSequenceID, and yet there are cases when MPCD is return without
its member variables set to any value.
So please add some default ctor which initialize mSequenceID.


I don't pretend to understand the whole setup properly yet.
Couple of iterations of reviewing is certainly needed.

Comment 45

[Andrea Marchesini [:baku]]

Thanks for reviewing it.

> Ah, looks like we stop having exactly the same
> PostMessageTransferStructuredClone implemented twice.
> Once in nsGlobalWindow.cpp and once in MessagePort.cpp. Good good.

Actually I have a patch to submit that removes the structuredclone algorithm from nsGlobalWindow completely.
But I want to see how the review process goes before submitting something new.

> >+    NS_RELEASE(clonedPort);
> Does this even compile.

It compiles :) But I applied your comments.


> >+nsIPrincipal*
> >+GetPrincipalFromWorkerPrivate(WorkerPrivate* aWorkerPrivate)
> Hmm, wasn't something like this added in some other bug

I didn't find it...

> How do we handle bfcaching?

Tell me more :)

Comment 46

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground

Comment 47

[Andrea Marchesini [:baku]]

smaug, I want to tell that the way nsGlobalWindow manages the transferred MessagePorts changes completely with patch 4. We don't have NS_Release or dont_AddRef things. We just use indexes.

But I cannot have the same code in patch 2 because the logic is different. Sorry for this.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

I guess my bfcache question would apply to non-worker cases too. How do we handle the case
when one port is in a non-bfcached, in other words active, window, and the other port is in bfached window. I'd expect the port in the bfcache doesn't actually do anything expect queues the messages
it gets.

Comment 49

[Andrea Marchesini [:baku]]

I would say: we should disable bfcache when MessagePorts are involved. How does it sound?

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could we instead kick the receiving document out of bfcache once a message arrives? So that we don't affect bfcaching if no one actually attempts to communicate with a bfcached document.

For example, if a page has a message channel with an <iframe>, and the user navigates the top-level page, then it seems silly to prevent the toplevel page from getting bfcached.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8480744 [details] [diff] [review]
patch 4 - transferable objects in MessagePort

>     *aTag = SCTAG_DOM_MAP_MESSAGEPORT;
>     *aOwnership = JS::SCTAG_TMO_CUSTOM;
>-    *aContent = clonedPort.forget().take();
>-    *aExtraData = 0;
>+    *aContent = 0;
*aContent = nullptr; ?


> struct
> StructuredCloneClosure
> {
>-  nsTArray<nsCOMPtr<nsIDOMBlob> > mBlobs;
>+  nsTArray<nsCOMPtr<nsIDOMBlob>> mBlobs;
>+  nsTArray<MessagePortCloneData> mMessagePorts;
> };
mMessagePorts doesn't contain any ports, so some better name is needed.
(This is very complicated code so need to be careful also with this kind of stuff)
I wonder if MessagePortCloneData should be called something like MessagePortIdentifier


>@@ -288,17 +302,37 @@ MessagePort::PostMessageMoz(JSContext* a
>                             const Optional<Sequence<JS::Value>>& aTransferable,
>                             ErrorResult& aRv)
> {
>   nsRefPtr<MessagePortData> data = new MessagePortData();
> 
>   // We *must* clone the data here, or the JS::Value could be modified
>   // by script
> 
>-  if (!WriteStructuredClone(aCx, aMessage, data->mBuffer, data->mClosure)) {
>+  JS::Rooted<JS::Value> transferable(aCx, JS::UndefinedValue());
>+  if (aTransferable.WasPassed()) {
>+    const Sequence<JS::Value>& realTransferable = aTransferable.Value();
>+
>+    // The input sequence only comes from the generated bindings code, which
>+    // ensures it is rooted.
>+    JS::HandleValueArray elements =
>+      JS::HandleValueArray::fromMarkedLocation(realTransferable.Length(),
>+                                               realTransferable.Elements());
>+
>+    JSObject* array =
>+      JS_NewArrayObject(aCx, elements);
>+    if (!array) {
>+      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
>+      return;
>+    }
>+    transferable.setObject(*array);
>+  }
ToJSValue doesn't work here?


>+    {
>+      const nsTArray<MessagePortCloneData>& ports =
>+        aData[i]->mClosure.mMessagePorts;
the array doesn't really contain any ports, just data, so better variable name.
Similar also elsewhere.

>+struct MessageTransferredPort
>+{
>+  nsString uuid;
>+  nsString destinationUuid;
>+  uint32_t sequenceId;
>+};
So I don't quite understand why we need nsString here. nsCString, could we just send nsID via ipdl?

Do we have worker tests for this all. Also cases when message is sent first for main thread port, but before the message is received,
the port is transferred to a worker.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8485998 [details] [diff] [review]
patch 2 - MessagePort/Channel with PBackground

Need consistent naming for MessagePortCloneData stuff.
either portData everywhere, or portIdentifier or some such evewhere, 
including the class name.

>+    MessagePortCloneData* clonedPortData = static_cast<MessagePortCloneData*>(aData);
>+    nsRefPtr<MessagePort> port = new MessagePort(scInfo->window, *clonedPortData);
>+    scInfo->clonedPorts.AppendElement(port);
>+
>+    // releasing this clonedPortData object.
>+    nsRefPtr<MessagePortCloneData> tmp = dont_AddRef(clonedPortData);
Just make clonedPortData to be nsRefPtr, no need for tmp

>+    MessagePortCloneData* clonedPortData(static_cast<MessagePortCloneData*>(aContent));
>+    nsRefPtr<MessagePortCloneData> tmp = dont_AddRef(clonedPortData);
Why you need two variables here?


>+
>+  void UpdateMustKeepAlive();
I'd like to see documentation about what this does and when it is called and why.

>+  enum {
>+    StateEntangling,
>+    StateEntangled,
>+    StateDisentangling,
>+    StateDisentangled
>+  } mState;
>+
>+  // This 'nexteStep' is useful when we are waiting to be entangled but the
>+  // content has called Clone() or Close().
>+  enum {
>+    NextStepNone,
>+    NextStepDisentangle,
>+    NextStepClose
>+  } mNextStep;
These states need _good_ comments. Like, where the messages are stored in which case and what happens
when a port is transferred etc.


Would like to see yet another updated patch. Sorry about doing this in several steps, but this is complicated, and I just need
to go through this few times. And this certainly needs a review from someone else too, like bent :)

Comment 53

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground

I like MessagePortIdentifier.

About UUID as nsID instead nsCString, the reason why I need it is because in the MessagePortIdentifier it can be that the UUID is 'empty'. In this case means that the port cannot be entangled (maybe because it has been already closed, or because it has been already disentangled).

I can do the same with a boolean, but, soon or late I have to send the ID to IPC and it has to be serialized somehow. I think it's better to use strings since the beginning.

But I'm totally open to a different approach.

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

You couldn't use all-zero uudi as empty?

Comment 55

[Andrea Marchesini [:baku]]

Attachment: patch 3 - MessagePort unshipped port queue

rebased.

Comment 56

[Andrea Marchesini [:baku]]

> >+  }
> ToJSValue doesn't work here?

No. It doesn't. I can propose a new ToJSValue() if you think it makes sense.
I wrote this code because I found it used in other part of gecko.

> Do we have worker tests for this all. Also cases when message is sent first
> for main thread port, but before the message is received,
> the port is transferred to a worker.

Yes. we have tests but in the next patch where we can transfer a port from the main-thread to workers and vice-versa.

Comment 57

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground

nsID instead nsCString.

Comment 58

[Andrea Marchesini [:baku]]

Attachment: patch 3 - MessagePort Shared Queue

rebased

Comment 59

[Andrea Marchesini [:baku]]

Attachment: patch 4 - transferable objects in MessagePort

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8486369 [details] [diff] [review]
patch 2 - MessagePort/Channel with PBackground



>+already_AddRefed<MessagePortIdentifier>
> MessagePort::Clone()
So this method should have a bit different name, since we're not cloning MessagePort, but
MessagePortIdentifier. The ::Clone has also rather significant side effects, so its name should
hint that the state of the original object may change.
And do we need to return non-null always?
I don't have a good suggestion for the method name.

>+  // This method is meant to keep alive the MessagePort when this object is
>+  // creating the actor and until the actor is entangled.
>+  // We release the object when the port is closed or disentangled.
>+  void UpdateMustKeepAlive();
Somewhere this all needs a comment about when disentangling might happen automatically.
(like, closing a window or so.)


>+  enum {
>+    // This is the first state of the MessagePort. At this point we are
>+    // creating a PBackground actor, then we wait until we receive the
>+    // entangled() message. All the postMessages requests are stored in
>+    // mMessagePendingQueue.
>+    // If the port is closed or cloned when we are in this state, we set the
>+    // mNextStep. This 'next' operation will be done when entangled() message
>+    // is received.
>+    StateEntangling,
What about the case of same-thread ports which aren't transfered anywhere?



>+    // When entangled() is received we send all the messages in the
>+    // mMessagePendingQueue to the actor and we change the state to
>+    // StateEntangled. At this point the port is entangled with the other. We
>+    // send and receive messages.
>+    // If the port queue is not enabled, the received messages are stored in
>+    // the mMessageQueue.
>+    StateEntangled,
>+
>+    // When the port is cloned or disentangled we want to stop receiving
>+    // messages. We call 'SendStopSendingData' to the actor and we wait for an
>+    // answer. All the messages received between now and the
>+    // 'StopSendingDataComfirmed are queued in the mMessageQueue but not
>+    // dispatched.
>+    StateDisentangling,
>+
>+    // When 'StopSendingDataConfirmed' is received, we can send the disentangle
>+    // request because we are 100% sure that we don't receive any other
>+    // message, so nothing will be lost.
send disentangle request where?

>+    // Disentangling the port we send all the messages from the mMessageQueue.
Send messages where?

>+    StateDisentangled
>+  } mState;
>+
>+  // This 'nexteStep' is useful when we are waiting to be entangled but the
s/nexteStep/nextStep/
s/useful/used/


>+  // content has called Clone() or Close().
>+  enum {
>+    NextStepNone,
>+    NextStepDisentangle,
>+    NextStepClose
>+  } mNextStep;

Enum values should be in form eEnumValueName

This needs still a carefully done second review.

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8486391 [details] [diff] [review]
patch 4 - transferable objects in MessagePort

>-  // This hashtable contains the transferred ports and their clone data.
>-  nsRefPtrHashtable<nsRefPtrHashKey<MessagePortBase>, MessagePortIdentifier> portIdentifiers;
>+  // This hashtable contains the transferred ports - used to avoid duplicates.
>+  nsTHashtable<nsRefPtrHashKey<MessagePortBase>> portIdentifiers;
Hmm, so the variable doesn't contain identifiers anymore, but just ports.
Rename the variable. transferredPorts perhaps?
(I wish we'd use the m-prefix here and elsewhere, but looks like struct doesn't use m for other variables, so
no need to change.)


>@@ -7956,20 +7969,19 @@ PostMessageReadTransferStructuredClone(J
>                                        uint64_t aExtraData,
>                                        void* aClosure,
>                                        JS::MutableHandle<JSObject*> returnObject)
> {
>   StructuredCloneInfo* scInfo = static_cast<StructuredCloneInfo*>(aClosure);
>   NS_ASSERTION(scInfo, "Must have scInfo!");
> 
>   if (tag == SCTAG_DOM_MAP_MESSAGEPORT) {
>-    nsRefPtr<MessagePortIdentifier> identifier =
>-      dont_AddRef(static_cast<MessagePortIdentifier*>(aData));
>-
>-    nsRefPtr<MessagePort> port = new MessagePort(scInfo->window, *identifier);
>+    MOZ_ASSERT(aData == 0);
>+    nsRefPtr<MessagePort> port =
>+      new MessagePort(scInfo->window, scInfo->event->GetPortIdentifier(aExtraData));
Could you add a comment what aExtraData is here. Or could you even rename aExtraData?

>+    if (!port->Clone(*identifier)) {
This Clone should be called something else, as I asked for the earlier patch

>+    nsTArray<nsRefPtr<MessagePortBase>> array;
>+    for (uint32_t i = 0; i < ports.Length(); ++i) {
>+      array.AppendElement(ports[i]);
>+    }
>+
>+    nsRefPtr<MessagePortList> portList =
>+      new MessagePortList(static_cast<dom::Event*>(event.get()), array);
>+    event->SetPorts(portList);
Hmm, MessagePortList is a gecko-ism.
Is there a bug open to get rid of it?
>+struct MessageTransferredPort
So this name is very odd, as I mentioned on IRC.
TransferredPortIdentifier or TransferredMessagePortIdentifier

Comment 62

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground

Comment 63

[Andrea Marchesini [:baku]]

Attachment: patch 3 - MessagePort unshipped port queue

rebased

Comment 64

[Andrea Marchesini [:baku]]

Attachment: patch 4 - transferable objects in MessagePort

Comment 65

[Andrea Marchesini [:baku]]

Attachment: patch 5 - MessagePort transferred cross threads.

Comment 66

[Andrea Marchesini [:baku]]

Attachment: patch 6 - BFCache

Comment 67

[Andrea Marchesini [:baku]]

Attachment: patch 5 - MessagePort transferred cross threads.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8487406 [details] [diff] [review]
patch 6 - BFCache

This is not doing what sicking suggested. This just disallows the doc to go to
bfcache. Sicking suggested kicking the doc out from bfcache (if it was there).

Comment 69

[SUN Haitao]

(In reply to Olli Pettay [:smaug] from comment #60)
> I don't have a good suggestion for the method name.

How about "Energize" (see also http://en.wikipedia.org/wiki/Transporter_%28Star_Trek%29)?

Comment 70

[Andrea Marchesini [:baku]]

Attachment: patch 6 - BFCache

Comment 71

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8488086 [details] [diff] [review]
patch 6 - BFCache

So this doesn't do what I'd expect.
It just once checks if the document is in bfcache.

We need something better, or not care about bfcache at all.
(Not doing it at all is a bit bad. We wouldn't close the port early enough, and pages might cause plenty of data to be stored in some to-be-delivered message queue since the port would look like being open)

Comment 72

[Andrea Marchesini [:baku]]

Attachment: patch 6 - BFCache

I hope this matches what we discussed on IRC.

Comment 73

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8488136 [details] [diff] [review]
patch 6 - BFCache

>+MessagePort::RemoveDocFromBFCache()
>+{
>+  if (!NS_IsMainThread()) {
>+    return;
>+  }
>+
>+  nsCOMPtr<nsPIDOMWindow> window = GetOwner();
nsPIDOMWindow* should be enough here.

Comment 74

[Andrea Marchesini [:baku]]

Attachment: patch 6 - BFCache

https://tbpl.mozilla.org/?tree=Try&rev=12f1eb5f80ce
fully green on try.

Comment 75

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground

the worker feature can be removed when the port is closed/disentangled.

Comment 76

[Andrea Marchesini [:baku]]

Attachment: patch 7 - blob

Comment 77

[Andrea Marchesini [:baku]]

Attachment: patch 8 - a port cannot transfer itself

I found this issue enabling MessagePort by default and running web-platform-tests.

Comment 78

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8514217 [details] [diff] [review]
patch 8 - a port cannot transfer itself

(The relevant piece of the spec is https://html.spec.whatwg.org/multipage/comms.html#dom-messageport-postmessage)

Comment 79

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This reminds me of the Swedish rhyme:

"Världens vigaste man, kröp in i sin häck och försvann".

Which roughly translates to

"The world's most flexible man, crawled into his butt and vanished".

Comment 80

[Andrea Marchesini [:baku]]

Attachment: patch 9 - ServiceWorkers

This is the last bit to have webplatform tests fully green.

Comment 81

[Andrea Marchesini [:baku]]

Attachment: patch 10 - portList can be null

Comment 82

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8515912 [details] [diff] [review]
patch 10 - portList can be null

>-      postMessage(e.data, e.ports);
>+      if (e.ports) {
>+        postMessage(e.data, e.ports);
>+      } else {
>+        postMessage(e.data);
>+      }
Requirement to do this looks like a spec bug to me.
void postMessage(any message, optional sequence<Transferable> transfer); should be probably
void postMessage(any message, optional sequence<Transferable>? transfer);
But not really about this bug, but might be good to file a spec bug.


>     event->SetTrusted(true);
>-    event->SetPorts(new MessagePortList(static_cast<dom::Event*>(event.get()),
>-                                        closure.mMessagePorts));
>+    if (closure.mMessagePorts.Length()) {
>+      event->SetPorts(new MessagePortList(static_cast<dom::Event*>(event.get()),
>+                                          closure.mMessagePorts));
>+    }
>+
As far as I see, this is wrong 
"Let new ports be an empty array." ... "Let the ports attribute of the event be initialised to the new ports array."

Comment 83

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #82)

> But not really about this bug, but might be good to file a spec bug.
Or maybe not a spec bug either, since if the array is always empty or contains some ports, we just don't need that change to the test.

Comment 84

[SUN Haitao]

Is there any huge amount of work left on this? It seems odd that Firefox Platform 2015 Roadmap (https://wiki.mozilla.org/Platform/Roadmap) lists "Message Port" as "planned to be done before or during Gecko 45 (End 2015)".

Comment 85

[Andrea Marchesini [:baku]]

No. This API is fully implemented. I'm just waiting for a review.

Comment 86

[SUN Haitao]

Is there anything can be done to help finishing the review?

Comment 87

[Andrew Overholt [:overholt]]

(In reply to SUN Haitao from comment #84)
> Is there any huge amount of work left on this? It seems odd that Firefox
> Platform 2015 Roadmap (https://wiki.mozilla.org/Platform/Roadmap) lists
> "Message Port" as "planned to be done before or during Gecko 45 (End 2015)".

That just means it's not a super-high priority.  It'll get done well before then :)

Comment 88

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8489546 [details] [diff] [review]
patch 2 - MessagePort/Channel with PBackground

Review of attachment 8489546 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good!

::: content/base/public/nsContentUtils.h
@@ +817,5 @@
>  
>    /**
> +   * Helper function that generates a UUID.
> +   */
> +  static nsresult GenerateUUID(nsID& aUUID);

I would call this "GenerateUUIDInPlace"

::: content/base/src/nsContentUtils.cpp
@@ +6989,5 @@
> +{
> +  nsresult rv;
> +
> +  if (!sUUIDGenerator) {
> +    rv = CallGetService("@mozilla.org/uuid-generator;1", &sUUIDGenerator);

This is racy if called on multiple threads... You might leak due to multiple addrefs in CallGetService. This needs to be made safer.

::: dom/base/Navigator.h
@@ +302,5 @@
>    virtual JSObject* WrapObject(JSContext* cx) MOZ_OVERRIDE;
>  
> +  // GetWindowFromGlobal returns the inner window for this global, if
> +  // any, else null.
> +  static already_AddRefed<nsPIDOMWindow> GetWindowFromGlobal(JSObject* aGlobal);

I'm not sure this makes sense here... Maybe move to nsContentUtils instead?

::: dom/messagechannel/MessageChannel.cpp
@@ +37,3 @@
>  
> +bool
> +CheckPermission(nsIPrincipal* aPrincipal, bool aCallerChrome)

Please add a main thread assert here. This isn't currently threadsafe.

@@ +66,5 @@
>    return isResource;
>  }
>  
> +nsIPrincipal*
> +GetPrincipalFromWorkerPrivate(WorkerPrivate* aWorkerPrivate)

MOZ_ASSERT(NS_IsMainThread())

@@ +115,5 @@
> +  MainThreadRun() MOZ_OVERRIDE
> +  {
> +    AssertIsOnMainThread();
> +
> +    nsIPrincipal* principal = GetPrincipalFromWorkerPrivate(mWorkerPrivate);

Need to handle null principals here

@@ +144,5 @@
> +      return false;
> +    }
> +
> +    return CheckPermission(doc->NodePrincipal(),
> +                           nsContentUtils::ThreadsafeIsCallerChrome());

You don't need ThreadsafeIsCallerChrome since you know you're on the main thread already.

::: dom/messagechannel/MessageChannel.h
@@ +31,5 @@
>    static bool Enabled(JSContext* aCx, JSObject* aGlobal);
>  
>  public:
> +  MessageChannel(nsPIDOMWindow* aWindow,
> +                 const nsID& aPortUUID1,

Nit: This needs a forward-declaration above.

::: dom/messagechannel/MessagePort.cpp
@@ +33,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +/* TODO

?

@@ +39,5 @@
> +2. postMessage from main-thread to workers
> +3. Blobs
> +*/
> +
> +class DispatchEventRunnable : public nsCancelableRunnable

Let's remove nsCancelableRunnable everywhere, use nsICancelableRunnable, and implement a real Cancel() function for these.

They're all meant to be used only on a single thread right?

@@ +51,5 @@
> +
> +  NS_IMETHOD
> +  Run()
> +  {
> +    nsRefPtr<DispatchEventRunnable> mKungFuDeathGrip(this);

Why is this needed? The event loop holds a ref to anything that is running.

@@ +97,2 @@
>  
> +    JS::Rooted<JS::Value> value(cx, JS::NullValue());

No need for the second arg, leave it undefined.

@@ +98,5 @@
> +    JS::Rooted<JS::Value> value(cx, JS::NullValue());
> +    if (mData->mBuffer.nbytes() &&
> +        !ReadStructuredClone(cx, mData->mBuffer.data(), mData->mBuffer.nbytes(),
> +                             mData->mClosure, &value)) {
> +      JS_ClearPendingException(cx);

Why? Shouldn't you report this? (I think you will automatically report by letting the AutoJSAPI go out of scope, but you should check)

@@ +118,4 @@
>  
> +    bool status;
> +    mPort->DispatchEvent(static_cast<dom::Event*>(event.get()), &status);
> +    return status ? NS_OK : NS_ERROR_FAILURE;

This seems unnecessary, the return value from this function will be ignored anyway.

@@ +138,5 @@
>  }
>  
>  NS_IMPL_CYCLE_COLLECTION_CLASS(MessagePort)
>  
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MessagePort,

Seems like we should be calling Close() here

@@ -347,5 @@
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mEntangledPort)
> -
> -  // Custom unlink loop because this array contains nsRunnable objects
> -  // which are not cycle colleactable.
> -  for (uint32_t i = 0, len = tmp->mMessageQueue.Length(); i < len; ++i) {

Hrm, why aren't you traversing mMessageQueue/mPendingMessageQueue here any more? They can hold blobs now.

@@ +164,5 @@
>  NS_IMPL_RELEASE_INHERITED(MessagePort, DOMEventTargetHelper)
>  
> +namespace {
> +
> +class MessagePortFeature : public workers::WorkerFeature

MOZ_FINAL

@@ +177,5 @@
> +  }
> +
> +  virtual bool Notify(JSContext* aCx, workers::Status aStatus) MOZ_OVERRIDE
> +  {
> +    if (aStatus >= Canceling) {

Are you sure this is right? Shouldn't it be anything > Running?

Also, Notify() can be called more than once, so you should probably null out mPort once you call close the first time to avoid calling it again.

@@ +220,5 @@
> +  mWorkerFeature = nullptr;
> +
> +  if (!mNeutered) {
> +    // Register this component to PBackground.
> +    mozilla::ipc::BackgroundChild::GetOrCreateForCurrentThread(this);

You can't do this, you don't have a stable refcount yet. You need a static |already_AddRefed<MessagePort> Create()| method here.

Also, GetOrCreateForCurrentThread() can fail (in theory)

@@ +228,5 @@
> +
> +  // The port has to keep itself alive until it's entangled.
> +  UpdateMustKeepAlive();
> +
> +  if (!NS_IsMainThread()) {

Reverse this to avoid ! and else

@@ +235,5 @@
> +
> +    mWorkerFeature = new MessagePortFeature(this);
> +    JSContext* cx = workerPrivate->GetJSContext();
> +    if (NS_WARN_IF(!workerPrivate->AddFeature(cx, mWorkerFeature))) {
> +      NS_WARNING("Failed to register the BroadcastChannel worker feature.");

Warning isn't enough, you should make this fail somehow.

@@ +248,5 @@
> +    mInnerID = GetOwner()->WindowID();
> +
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    if (obs) {
> +      obs->AddObserver(this, "inner-window-destroyed", false);

Is this really necessary? Can't we just rely on the cycle collector?

@@ +288,5 @@
>      return;
>    }
>  
> +  // This message has to be ignored.
> +  if (mState > eStateEntangled) {

Hm. This seems wrong. Shouldn't you throw if script calls postMessage() after clone/close?

@@ +293,5 @@
> +    return;
> +  }
> +
> +  // Not entangled yet, but already closed.
> +  if (mNextStep != eNextStepNone) {

Here too

@@ +298,5 @@
> +    return;
> +  }
> +
> +  // Not entangled yet.
> +  if (mState != eStateEntangled) {

This would make more sense as |mState == eStateEntangling|

@@ +306,5 @@
> +
> +  MOZ_ASSERT(mActor);
> +  MOZ_ASSERT(mMessagePendingQueue.IsEmpty());
> +
> +  nsTArray<nsRefPtr<MessagePortData>> array;

nsAutoTArray<..., 1>, below too.

@@ +329,5 @@
>  void
>  MessagePort::Dispatch()
>  {
> +  if (!mMessageQueueEnabled || mMessageQueue.IsEmpty() || mDispatchRunnable ||
> +      mState > eStateEntangled) {

What if |mState == eStateEntangling|?

@@ +339,5 @@
>  
> +  nsRefPtr<PostMessageRunnable> runnable = new PostMessageRunnable(this, data);
> +
> +  if (NS_FAILED(NS_DispatchToCurrentThread(runnable))) {
> +    NS_WARNING("Failed to dispatch to the current thread!");

This shouldn't ever fail I think, I'd just do |MOZ_ALWAYS_TRUE(NS_SUCCEEDED(...))|

@@ +366,3 @@
>  
> +  if (mState > eStateEntangled) {
> +    return;

Hm, shouldn't this throw? Calling close() more than once seems dumb.

@@ +368,5 @@
> +    return;
> +  }
> +
> +  // We don't care about stopping the sending of messages because from now all
> +  // the incoming messages will be ingnored.

They could still be expensive to serialize/deserialize, or hold on to large amounts of memory... I think we might want to reexamine this or have a followup about it.

@@ +508,5 @@
> +{
> +  nsRefPtr<MessagePortIdentifier> identifier = new MessagePortIdentifier();
> +
> +  if (mState > eStateEntangled) {
> +    return identifier.forget();

I don't understand what this does really... You're returning something with unset uuids... Why not just return null?

@@ +523,5 @@
> +  identifier->mSequenceID = mSequenceID + 1;
> +  identifier->mNeutered = false;
> +
> +  // Not entangled yet, we have to wait.
> +  if (mState < eStateEntangled) {

mState == eStateEntangling

@@ +568,5 @@
> +  MOZ_ASSERT(mActor);
> +
> +  mActor->SetPort(this);
> +
> +  mActor->SendEntangle(mUUID, mDestinationUUID, mSequenceID);

It seems like you should combine the constructor message and the entangle message. There's no reason to send two.

::: dom/messagechannel/MessagePort.h
@@ +28,5 @@
> +{
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MessagePortIdentifier)
> +
> +  MessagePortIdentifier()

Shouldn't the two UUIDs be passes as args here?

@@ +79,3 @@
>  };
>  
> +class MessagePortChild;

Nit: Move up with the other forward-declares.

@@ +151,3 @@
>  
> +  nsTArray<nsRefPtr<MessagePortData>> mMessageQueue;
> +  nsTArray<nsRefPtr<MessagePortData>> mMessagePendingQueue;

One is for messages that are queued for being sent, and the other is for messages that have been received right? Let's find better names because I don't know which is which.

@@ +153,5 @@
> +  nsTArray<nsRefPtr<MessagePortData>> mMessagePendingQueue;
> +
> +  nsID mUUID;
> +  nsID mDestinationUUID;
> +  uint32_t mSequenceID;

Would it make sense to just have a MessagePortIdentifier member?

@@ +199,5 @@
> +    eNextStepClose
> +  } mNextStep;
> +
> +  bool mIsKeptAlive;
> +  uint64_t mInnerID;

Nit: I'd move this up a bit so that the bools and enums can pack better.

::: dom/messagechannel/MessagePortChild.cpp
@@ +17,5 @@
> +
> +bool
> +MessagePortChild::RecvStopSendingDataConfirmed()
> +{
> +  MOZ_ASSERT(mPort);

I don't think any of these can be asserted... It's possible that the page called port.close() before these messages arrive, right?

::: dom/messagechannel/MessagePortChild.h
@@ +15,5 @@
> +
> +class MessagePortChild MOZ_FINAL : public PMessagePortChild
> +{
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MessagePortChild)

Why threadsafe? This should be tied to a single thread.

@@ +40,5 @@
> +    MOZ_ASSERT(!mPort);
> +  }
> +
> +  // This raw pointer is the parent and it's kept alive until this object is
> +  // not fully deleted.

Nit: This comment doesn't make a whole lot of sense to me...

::: dom/messagechannel/MessagePortData.h
@@ +12,5 @@
> +namespace dom {
> +
> +class MessagePortMessage;
> +
> +class MessagePortData MOZ_FINAL

I don't really understand why this class is needed...

::: dom/messagechannel/MessagePortParent.cpp
@@ +13,5 @@
> +MessagePortParent::MessagePortParent()
> +  : mService(MessagePortService::GetOrCreate())
> +  , mEntangled(false)
> +  , mCanSendData(true)
> +{

MOZ_ASSERT(mService) ?

Also in DEBUG builds it might be nice to memset mUUID to something (all 0?) so that you can tell in a debugger if it is valid or not.

@@ +27,5 @@
> +MessagePortParent::RecvEntangle(const nsID& aUUID,
> +                                const nsID& aDestinationUUID,
> +                                const uint32_t& aSequenceID)
> +{
> +  MOZ_ASSERT(mService, "Entangle is called after a shutdown!");

Correct message order can't be asserted since a hacked child could do anything. You need to handle out-of-order messages with swift death! Here and everywhere else in MessagePortParent.

@@ +37,5 @@
> +}
> +
> +bool
> +MessagePortParent::RecvPostMessages(
> +                                  const nsTArray<MessagePortMessage>& aMessages)

Is it ok for this to be empty?

@@ +43,5 @@
> +  if (!mEntangled) {
> +    return true;
> +  }
> +
> +  MOZ_ASSERT(mService, "PostMessage is called after a shutdown!");

Can't be asserted, below either.

@@ +83,5 @@
> +bool
> +MessagePortParent::RecvClose()
> +{
> +  if (mService) {
> +    MOZ_ASSERT(mService);

Not needed :)

::: dom/messagechannel/MessagePortParent.h
@@ +19,5 @@
> +  ~MessagePortParent();
> +
> +  virtual bool RecvEntangle(const nsID& aUUID,
> +                            const nsID& aDestinationUUID,
> +                            const uint32_t& aSequenceID) MOZ_OVERRIDE;

All these IPDL overrides can be private

@@ +22,5 @@
> +                            const nsID& aDestinationUUID,
> +                            const uint32_t& aSequenceID) MOZ_OVERRIDE;
> +
> +  virtual bool
> +  RecvPostMessages(const nsTArray<MessagePortMessage>& aMessages) MOZ_OVERRIDE;

Nit: You should be consistent with your formatting, however you're doing it (newlines after return type vs. not)

@@ +37,5 @@
> +  bool Entangled(const nsTArray<MessagePortMessage>& aMessages);
> +
> +  void Close();
> +
> +  bool CanSendData()

Nit: const

::: dom/messagechannel/MessagePortService.cpp
@@ +13,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +static MessagePortService* sInstance = nullptr;

This should be "gInstance" since it's not a static member. (Also initializing to null isn't needed)

@@ +15,5 @@
> +namespace dom {
> +
> +static MessagePortService* sInstance = nullptr;
> +
> +class MessagePortServiceData

Let's add MOZ_COUNT_CTOR/MOZ_COUNT_DTOR logging to all these non-refcounted classes

@@ +20,5 @@
> +{
> +public:
> +  MessagePortServiceData(const nsID& aDestinationUUID)
> +    : mDestinationUUID(aDestinationUUID)
> +    , mSequenceID(1)

Why 1 instead of 0?

@@ +24,5 @@
> +    , mSequenceID(1)
> +    , mParent(nullptr)
> +  { }
> +
> +  nsID mDestinationUUID;

Is this needed? Can't you get it from mParent?

@@ +30,5 @@
> +  uint32_t mSequenceID;
> +  MessagePortParent* mParent;
> +
> +  // MessagePortParent keeps the service alive, and we don't want a cycle.
> +  nsDataHashtable<nsUint32HashKey, MessagePortParent*> mNextParents;

Do we really need a hashtable here? Won't an array of |struct { uint32; MessagePortParent*; }| work instead? I doubt we'll have enough entries ever to warrant a hashtable.

@@ +31,5 @@
> +  MessagePortParent* mParent;
> +
> +  // MessagePortParent keeps the service alive, and we don't want a cycle.
> +  nsDataHashtable<nsUint32HashKey, MessagePortParent*> mNextParents;
> +  nsTArray<nsRefPtr<MessagePortData>> mMessages;

This should be FallibleTArray, and you should check for allocation failures. We don't want a runaway child to bring down the whole browser if we can help it.

@@ +49,5 @@
> +}
> +
> +// static
> +already_AddRefed<MessagePortService>
> +MessagePortService::GetOrCreate()

AssertIsOnBackgroundThread() here please!

@@ +59,5 @@
> +  return instance.forget();
> +}
> +
> +bool
> +MessagePortService::RequestEntangling(MessagePortParent* aParent,

I'm not sure it's worth doing but we could also enforce that a particular uuid belongs to a single process. We don't yet have any reason to allow message ports to be passed among processes, right?

@@ +68,5 @@
> +  MessagePortServiceData* data;
> +
> +  // If we already have a MessagePortServiceData, it means that we have 2
> +  // entangled ports.
> +  if (!mPorts.Get(aUUID, &data)) {

The comment talks about the opposite block from what you're testing here...

@@ +71,5 @@
> +  // entangled ports.
> +  if (!mPorts.Get(aUUID, &data)) {
> +    // Create the MessagePortServiceData for the destination.
> +    if (mPorts.Get(aDestinationUUID, &data)) {
> +      MOZ_CRASH("The creation of the 2 ports should be in sync.");

This should just be MOZ_ASSERT(false) because this can happen if a child misbehaves. We shouldn't have MOZ_CRASH on the parent side except for obvious programmer bugs. Make sure you remove them all!

@@ +79,5 @@
> +    data = new MessagePortServiceData(aUUID);
> +    mPorts.Put(aDestinationUUID, data);
> +
> +    data = new MessagePortServiceData(aDestinationUUID);
> +    mPorts.Put(aUUID, data);

Hrm, I kinda hate making 2 nsDataHashtables, 2 nsTArrays, and 6 nsIDs per message channel... Hopefully we can shrink this somehow?

@@ +103,5 @@
> +    // We activate this port, sending all the messages.
> +    data->mParent = aParent;
> +    nsTArray<MessagePortMessage> array;
> +    MessagePortData::FromDataToMessages(data->mMessages, array);
> +    bool rv = aParent->Entangled(array);

'rv' is always nsresult basically, let's just call it 'ok' or something.

@@ +141,5 @@
> +  data->mMessages.Clear();
> +
> +  ++data->mSequenceID;
> +
> +  // If we don't have a parent, we have to store the pending messages and wait.

This could be a large amount of memory... We need to figure out how to clean these up after a certain amount of time or something...

@@ +144,5 @@
> +
> +  // If we don't have a parent, we have to store the pending messages and wait.
> +  MessagePortParent* nextParent;
> +  if (!data->mNextParents.Get(data->mSequenceID, &nextParent)) {
> +    data->mMessages = messages;

SwapElements please

@@ +238,5 @@
> +    MOZ_CRASH("PostMessages() should be called just from the correct parent.");
> +    return false;
> +  }
> +
> +  // This cannot happen.

MOZ_ALWAYS_TRUE if that's right

@@ +269,5 @@
> +}
> +
> +namespace {
> +
> +struct ParentDestroyData

MOZ_STACK_CLASS, MOZ_FINAL

@@ +285,5 @@
> +ParentDestroyEnumerator(const uint32_t& aSequenceId, MessagePortParent* aParent,
> +                        void* aData)
> +{
> +  MOZ_ASSERT(aParent);
> +  ParentDestroyData* data = static_cast<ParentDestroyData*>(aData);

Nit: auto* to avoid typing the type twice

@@ +303,5 @@
> +                                  const nsID& aUUID)
> +{
> +  // When the last parent is deleted, this service is freed but this cannot be
> +  // done when the hashtables are written by CloseAll.
> +  nsRefPtr<MessagePortService> kungFuDeathGrip = this;

Rather than this how about you make MessagePortParent::ActorDestroy() hold a strong ref while calling ParentDestroy()? Self-refs are almost always unnecessary.

::: dom/messagechannel/MessagePortService.h
@@ +23,5 @@
> +
> +  static already_AddRefed<MessagePortService> GetOrCreate();
> +
> +  bool RequestEntangling(MessagePortParent* aParent,
> +                         const nsID& aUUID,

I don't get why all these methods need both MessagePortParent and nsID args... Since the nsID is always the one that belongs to the MessagePortParent just use that and add a |const nsID&| getter to MessagePortParent? That seems much more clear.

::: dom/messagechannel/PMessagePort.ipdl
@@ +10,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +struct MessagePortMessage {
> +  SerializedStructuredCloneBuffer data;

This is eventually just going to be ClonedMessageData right?

@@ +23,5 @@
> +  Entangle(nsID uuid, nsID destinationUuid, uint32_t sequenceId);
> +  PostMessages(MessagePortMessage[] messages);
> +  Disentangle(MessagePortMessage[] messages);
> +  StopSendingData();
> +  Close();

Documenting the message flow here would be really nice. E.g.:

  Shutdown sequence from child's perspective:
    1. SendStopSendingData();
    2. RecvStopSendingDataConfirmed();
    3. SendClose();
    4. RecvClosed();
    5. Recv__delete__();

(Or whatever the real sequence is)

::: ipc/glue/BackgroundParentImpl.cpp
@@ +13,5 @@
>  #include "nsXULAppAPI.h"
>  
>  using mozilla::ipc::AssertIsOnBackgroundThread;
>  
> +using namespace mozilla::dom;

Nit: Move inside the namespace blocks.

Comment 89

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8501203 [details] [diff] [review]
patch 7 - blob

Review of attachment 8501203 [details] [diff] [review]:
-----------------------------------------------------------------

I'll hold off on this one until you rebase over the BroadcastChannel stuff.

Comment 90

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8514545 [details] [diff] [review]
patch 9 - ServiceWorkers

Review of attachment 8514545 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/messagechannel/MessagePortService.h
@@ +50,3 @@
>  
>    void CloseAll(const nsID& aUUID);
> +  void MaybeShutdown();

Hm, I don't understand why this is related to service workers?

Comment 91

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8487407 [details] [diff] [review]
patch 5 - MessagePort transferred cross threads.

Review of attachment 8487407 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly fine to me! But I'll need to see the updated version when it's ready.

::: dom/workers/WorkerPrivate.cpp
@@ +377,5 @@
>        if (file) {
>          DOMFileImpl* fileImpl = static_cast<DOMFile*>(file)->Impl();
>          if (JS_WriteUint32Pair(aWriter, DOMWORKER_SCTAG_FILE, 0) &&
>              JS_WriteBytes(aWriter, &fileImpl, sizeof(fileImpl))) {
> +          closure->mClonedObjects.AppendElement(fileImpl);

This has all changed a little, so it'll need to be rebased

@@ +422,5 @@
> +               void* aClosure, JS::MutableHandle<JSObject*> aReturnObject)
> +  {
> +    MOZ_ASSERT(aClosure);
> +
> +    WorkerStructuredCloneClosure* closure =

Nit: auto*

@@ +426,5 @@
> +    WorkerStructuredCloneClosure* closure =
> +      static_cast<WorkerStructuredCloneClosure*>(aClosure);
> +
> +    if (aTag == SCTAG_DOM_MAP_MESSAGEPORT) {
> +      MOZ_ASSERT(aContent == 0);

Nit: MOZ_ASSERT(!aContent)

@@ +453,5 @@
> +           void** aContent, uint64_t *aExtraData)
> +  {
> +    MOZ_ASSERT(aClosure);
> +
> +    WorkerStructuredCloneClosure* closure =

Nit: auto*

@@ +456,5 @@
> +
> +    WorkerStructuredCloneClosure* closure =
> +      static_cast<WorkerStructuredCloneClosure*>(aClosure);
> +
> +    MessagePortBase *port = nullptr;

Nit: * on the left

@@ +461,5 @@
> +    nsresult rv = UNWRAP_OBJECT(MessagePort, aObj, port);
> +    if (NS_SUCCEEDED(rv)) {
> +      if (closure->mTransferredPorts.GetEntry(port)) {
> +        // No duplicate.
> +        return false;

At least a NS_WARNING here would be good.

@@ +467,5 @@
> +
> +      MessagePortIdentifier* identifier =
> +        closure->mMessagePortIdentifiers.AppendElement();
> +
> +      if (!port->CloneAndDisentangle(*identifier)) {

Hrm, this must be outdated, CloneAndDisentangle() now returns a MessagePortIdentifier...

@@ +474,5 @@
> +      closure->mTransferredPorts.PutEntry(port);
> +
> +      *aTag = SCTAG_DOM_MAP_MESSAGEPORT;
> +      *aOwnership = JS::SCTAG_TMO_CUSTOM;
> +      *aContent = 0;

Nit: nullptr

@@ +487,5 @@
> +  static void
> +  FreeTransfer(uint32_t aTag, JS::TransferableOwnership aOwnership,
> +               void *aContent, uint64_t aExtraData, void* aClosure)
> +  {
> +    // Nothing to do.

Really? What about calling Close() or something?

@@ +589,5 @@
>      AssertIsOnMainThread();
>  
>      NS_ASSERTION(aClosure, "Null pointer!");
>  
> +    WorkerStructuredCloneClosure* closure =

Nit: auto*

@@ +681,5 @@
> +  static void
> +  FreeTransfer(uint32_t aTag, JS::TransferableOwnership aOwnership,
> +               void *aContent, uint64_t aExtraData, void* aClosure)
> +  {
> +    // Nothing to do.

Please call the other one.

@@ +743,5 @@
> +  static void
> +  FreeTransfer(uint32_t aTag, JS::TransferableOwnership aOwnership,
> +               void *aContent, uint64_t aExtraData, void* aClosure)
> +  {
> +    // Nothing to do.

Here too.

@@ +833,5 @@
> +  static void
> +  FreeTransfer(uint32_t aTag, JS::TransferableOwnership aOwnership,
> +               void *aContent, uint64_t aExtraData, void* aClosure)
> +  {
> +    // Nothing to do.

And here...

Man we really need to try to get rid of these now!

@@ +1122,5 @@
> +  Write(JSContext* aCx, JS::Handle<JS::Value> aValue,
> +        JS::Handle<JS::Value> aTransferredValue,
> +        const JSStructuredCloneCallbacks *aCallbacks)
> +   {
> +    bool rv = mBuffer.write(aCx, aValue, aTransferredValue, aCallbacks,

Nit: s/rv/ok/

@@ +1139,5 @@
>      // cloning into worker when array goes out of scope.
> +    WorkerStructuredCloneClosure closure;
> +    closure.mClonedObjects.SwapElements(mClosure.mClonedObjects);
> +    MOZ_ASSERT(mClosure.mMessagePorts.IsEmpty());
> +    closure.mMessagePortIdentifiers.SwapElements(mClosure.mMessagePortIdentifiers);

This will be much simpler with Move()

::: dom/workers/WorkerStructuredClone.h
@@ +6,5 @@
> +#ifndef mozilla_dom_workers_WorkerStructuredClone_h
> +#define mozilla_dom_workers_WorkerStructuredClone_h
> +
> +#include "mozilla/dom/MessagePort.h"
> +#include "mozilla/dom/MessagePortBinding.h"

You should forward-declare everything you can, make the constructor/destructor live in WorkerPrivate.cpp so that you can keep the bindings stuff out of this header.

@@ +11,5 @@
> +
> +BEGIN_WORKERS_NAMESPACE
> +
> +struct
> +WorkerStructuredCloneClosure

MOZ_FINAL

Also, since this thing is kind of heavyweight I think we should make private copy-constructor and operator=, and force everyone to Move() it around if they need to.

@@ +26,5 @@
> +  // Information for the transferring.
> +  nsTArray<MessagePortIdentifier> mMessagePortIdentifiers;
> +
> +  // To avoid duplicates in the transferred ports.
> +  nsTHashtable<nsRefPtrHashKey<MessagePortBase>> mTransferredPorts;

I don't think we really need a hashset here... It's very unlikely that people are going to transfer a ton of ports, right? I think we just need an array.

Comment 92

[Andrea Marchesini [:baku]]

> > +/* TODO
> 
> ?

This is the list of what is not included in this patch. In the next ones I'll remove each entry of this TODO list.

> >  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MessagePort,
> 
> Seems like we should be calling Close() here

No because we keep MessagePort alive until this is not disconnected. The life-time management of MessagePort is a bit tricky and we cannot use UNLINK for this porpoise. This is also the reason why we use inner-window-destroyed.

> > +  // This message has to be ignored.
> > +  if (mState > eStateEntangled) {
> 
> Hm. This seems wrong. Shouldn't you throw if script calls postMessage()
> after clone/close?

The spec says that postmessage should ignore the messages in case the port is closed/disantangled.

> @@ +366,3 @@
> >  
> > +  if (mState > eStateEntangled) {
> > +    return;
> 
> Hm, shouldn't this throw? Calling close() more than once seems dumb.

This is what the spec says...

> @@ +508,5 @@
> > +{
> > +  nsRefPtr<MessagePortIdentifier> identifier = new MessagePortIdentifier();
> > +
> > +  if (mState > eStateEntangled) {
> > +    return identifier.forget();
> 
> I don't understand what this does really... You're returning something with
> unset uuids... Why not just return null?

By default an identifier contains the fact that it's neutered, so actually it contains some useful info.

> ::: dom/messagechannel/MessagePortChild.cpp
> @@ +17,5 @@
> > +
> > +bool
> > +MessagePortChild::RecvStopSendingDataConfirmed()
> > +{
> > +  MOZ_ASSERT(mPort);
> 
> I don't think any of these can be asserted... It's possible that the page
> called port.close() before these messages arrive, right?

Yes, but Close() don't set the port to null in the child immediately. This is done only if the SendingDataConfirmed() is received.

> ::: dom/messagechannel/MessagePortData.h
> @@ +12,5 @@
> > +namespace dom {
> > +
> > +class MessagePortMessage;
> > +
> > +class MessagePortData MOZ_FINAL
> 
> I don't really understand why this class is needed...

What do you mean? This is class is the Message sent/received by the MessagePort. Similar to BroadcastChannelMessage object.

> Also in DEBUG builds it might be nice to memset mUUID to something (all 0?)
> so that you can tell in a debugger if it is valid or not.

I did that. But then... a UUID fully 0 is a valid UUID, right?

> @@ +20,5 @@
> > +{
> > +public:
> > +  MessagePortServiceData(const nsID& aDestinationUUID)
> > +    : mDestinationUUID(aDestinationUUID)
> > +    , mSequenceID(1)
> 
> Why 1 instead of 0?

The idea is that sequenceID == 0 is invalid. We don't have any check for this, but it's useful for debugging it with gdb.

> > +  nsID mDestinationUUID;
> 
> Is this needed? Can't you get it from mParent?

It can happen that I don't have a parent when this object is created.
  
> > +// static
> > +already_AddRefed<MessagePortService>
> > +MessagePortService::GetOrCreate()
> 
> AssertIsOnBackgroundThread() here please!

I would. but I don't have such method available without a parent object. Right?
I could take it out from ParentImpl...

> > +bool
> > +MessagePortService::RequestEntangling(MessagePortParent* aParent,
> 
> I'm not sure it's worth doing but we could also enforce that a particular
> uuid belongs to a single process. We don't yet have any reason to allow
> message ports to be passed among processes, right?

Not yet...

> @@ +79,5 @@
> > +    data = new MessagePortServiceData(aUUID);
> > +    mPorts.Put(aDestinationUUID, data);
> > +
> > +    data = new MessagePortServiceData(aDestinationUUID);
> > +    mPorts.Put(aUUID, data);
> 
> Hrm, I kinda hate making 2 nsDataHashtables, 2 nsTArrays, and 6 nsIDs per
> message channel... Hopefully we can shrink this somehow?

The hashtable is gone. I don't know how to shrink all of this. But we need all of them, right?

> @@ +141,5 @@
> > +  data->mMessages.Clear();
> > +
> > +  ++data->mSequenceID;
> > +
> > +  // If we don't have a parent, we have to store the pending messages and wait.
> 
> This could be a large amount of memory... We need to figure out how to clean
> these up after a certain amount of time or something...

Follow up.

> ::: dom/messagechannel/PMessagePort.ipdl
> @@ +10,5 @@
> > +namespace mozilla {
> > +namespace dom {
> > +
> > +struct MessagePortMessage {
> > +  SerializedStructuredCloneBuffer data;
> 
> This is eventually just going to be ClonedMessageData right?

Right. But in the blob patch.

Comment 93

[Andrea Marchesini [:baku]]

Attachment: patch 2 - interdiff

Comment 94

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground

The full patch.

Comment 95

[Andrea Marchesini [:baku]]

> > +      MessagePortIdentifier* identifier =
> > +        closure->mMessagePortIdentifiers.AppendElement();
> > +
> > +      if (!port->CloneAndDisentangle(*identifier)) {
> 
> Hrm, this must be outdated, CloneAndDisentangle() now returns a
> MessagePortIdentifier...

mmm no. Sorry for this but my patches are evolved and, now CloneAndDisentangle() returns a boolean. The identifier is not ref-counted anymore.
This has been done in patch number 3 - about transferable objects.

> @@ +487,5 @@
> > +  static void
> > +  FreeTransfer(uint32_t aTag, JS::TransferableOwnership aOwnership,
> > +               void *aContent, uint64_t aExtraData, void* aClosure)
> > +  {
> > +    // Nothing to do.
> 
> Really? What about calling Close() or something?

Good point. We should send a message to the service. New patch just for this because it involves IPDL.


> Man we really need to try to get rid of these now!

\o/

> @@ +1139,5 @@
> >      // cloning into worker when array goes out of scope.
> > +    WorkerStructuredCloneClosure closure;
> > +    closure.mClonedObjects.SwapElements(mClosure.mClonedObjects);
> > +    MOZ_ASSERT(mClosure.mMessagePorts.IsEmpty());
> > +    closure.mMessagePortIdentifiers.SwapElements(mClosure.mMessagePortIdentifiers);
> 
> This will be much simpler with Move()

Tell me more.

Comment 96

[Andrea Marchesini [:baku]]

Attachment: patch 5 - MessagePort transferred cross threads.

the free stuff in a separate patch.

Comment 97

[Andrea Marchesini [:baku]]

Attachment: patch 5 - MessagePort transferred cross threads.

WorkerFeature should not be leaked. I had to add a check in mWorkerFeatures in WorkerPrivate because it can happen that we remove a feature when another one is notified. Having a runnable seems too much when we can just check if the feature exists before using it.

Comment 98

[Andrea Marchesini [:baku]]

Attachment: patch 7 - blob

rebased

Comment 99

[Andrea Marchesini [:baku]]

Attachment: patch 9 - SharedWorkers

SharedWorkers and ServiceWorkers need a particular life-time management for MessagePortService.

Comment 100

[Andrea Marchesini [:baku]]

Attachment: patch 9 - SharedWorkers

Comment 101

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8550351 [details] [diff] [review]
patch 2 - interdiff

Review of attachment 8550351 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly great but I want to see if we can't get rid of MessagePortData.

::: dom/messagechannel/MessageChannel.h
@@ +12,5 @@
>  #include "nsCycleCollectionParticipant.h"
>  #include "nsWrapperCache.h"
>  #include "nsCOMPtr.h"
>  
> +class nsID;

Is this needed now?

::: dom/messagechannel/MessagePort.cpp
@@ +54,5 @@
>  
>    NS_IMETHOD
>    Run()
>    {
>      nsRefPtr<DispatchEventRunnable> mKungFuDeathGrip(this);

This shouldn't be needed.

@@ +361,1 @@
>    MessagePortData::FromDataToMessages(array, messages);

I still don't understand why you're doing this extra copy (and why MessagePortData exists at all). If we can't get rid of MessagePortData entirely then at least we have to make sure that it doesn't copy all the data.

::: dom/messagechannel/MessagePort.h
@@ +34,5 @@
>      : mSequenceID(0)
>      , mNeutered(true)
>    { }
>  
> +  MessagePortIdentifier(const MessagePortIdentifier* aIdentifier)

Why not make this take a const reference instead?

@@ +41,5 @@
> +    MOZ_ASSERT(aIdentifier);
> +
> +    mUUID = aIdentifier->mUUID;
> +    mDestinationUUID = aIdentifier->mDestinationUUID;
> +    mSequenceID = aIdentifier->mSequenceID + 1;

This can all be done in the initializer list.

@@ +108,4 @@
>  
> +  static already_AddRefed<MessagePort>
> +  Create(nsPIDOMWindow* aWindow, const MessagePortIdentifier& aIdentifier,
> +         ErrorResult& aRv);

Neither of these really need to mess with ErrorResult since callers don't care about multiple error conditions. Returning null is all you need to do to signal an error.

@@ +154,5 @@
>    // We release the object when the port is closed or disentangled or when the
>    // window/worker is closed.
>    void UpdateMustKeepAlive();
>  
>    // Raw pointer because this goes 1:1 with the port.

This is no longer a "raw" pointer...

::: dom/messagechannel/MessagePortData.cpp
@@ +43,5 @@
>    for (uint32_t i = 0, len = aArray.Length(); i < len; ++i) {
>      nsRefPtr<MessagePortData> data = new MessagePortData();
>  
>      const SerializedStructuredCloneBuffer& buffer = aArray[i].data();
>      data->mBuffer.copy(buffer.data, buffer.dataLength);

This is the thing most likely to fail if you're low on memory...

But we shouldn't ever be copying this if we can avoid it!

::: dom/messagechannel/MessagePortData.h
@@ +25,5 @@
>    { }
>  
>    static void
>    FromDataToMessages(const nsTArray<nsRefPtr<MessagePortData>>& aData,
>                       nsTArray<MessagePortMessage>& aArray);

Why isn't this one fallible too?

::: dom/messagechannel/MessagePortParent.cpp
@@ +44,5 @@
>  MessagePortParent::RecvPostMessages(
>                                    const nsTArray<MessagePortMessage>& aMessages)
>  {
>    if (!mEntangled) {
>      return true;

Hm, I think you need to worry about grabbing blob references here too, otherwise they may leak

@@ +53,5 @@
> +  }
> +
> +  if (!mService) {
> +    NS_WARNING("Entangle is called after a shutdown!");
> +    return false;

I'd check this first above.

@@ +69,5 @@
>    }
>  
> +  if (!mService) {
> +    NS_WARNING("Entangle is called after a shutdown!");
> +    return false;

Same, handle this first.

::: dom/messagechannel/MessagePortService.cpp
@@ +13,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +static MessagePortService* gInstance;

I'd put this in an anonymous namespace

@@ +43,5 @@
> +    // MessagePortParent keeps the service alive, and we don't want a cycle.
> +    MessagePortParent* mParent;
> +  };
> +
> +  nsTArray<NextParent> mNextParents;

This should probably be fallible too

@@ +232,5 @@
>      data->mParent->Close();
>    }
>  
> +  for (uint32_t i = 0; i < data->mNextParents.Length(); ++i) {
> +    data->mNextParents[i].mParent->Close();

This can't mutate the array, right?

::: dom/messagechannel/PMessagePort.ipdl
@@ +18,5 @@
>  protocol PMessagePort
>  {
>    manager PBackground;
>  
> +  /* Many of these methods are useful just for the shutdown sequence. The

Nit: s/useful/used/

@@ +24,5 @@
> +     1. SendStopSendingData();
> +     2. RecvStopSendingDataConfirmed();
> +     3. SendClose();
> +     4. RecvClosed();
> +     5. Recv__delete__(); */

This order is only correct for ports that call close() right? What about for ports that are transferred?

Comment 102

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Do you have interdiffs for the rest of these parts? Or are they different enough that it just didn't make sense?

Comment 103

[Andrea Marchesini [:baku]]

> @@ +108,4 @@
> >  
> > +  static already_AddRefed<MessagePort>
> > +  Create(nsPIDOMWindow* aWindow, const MessagePortIdentifier& aIdentifier,
> > +         ErrorResult& aRv);
> 
> Neither of these really need to mess with ErrorResult since callers don't
> care about multiple error conditions. Returning null is all you need to do
> to signal an error.

Actually we do care in MessageChannel. I prefer to keep it.

a> ::: dom/messagechannel/MessagePortData.h
> @@ +25,5 @@
> >    { }
> >  
> >    static void
> >    FromDataToMessages(const nsTArray<nsRefPtr<MessagePortData>>& aData,
> >                       nsTArray<MessagePortMessage>& aArray);
> 
> Why isn't this one fallible too?

This will change in the next patch (blob) splitting these in 2 methods.

> @@ +232,5 @@
> >      data->mParent->Close();
> >    }
> >  
> > +  for (uint32_t i = 0; i < data->mNextParents.Length(); ++i) {
> > +    data->mNextParents[i].mParent->Close();
> 
> This can't mutate the array, right?

No. It just sends the Close() message.

Comment 104

[Andrea Marchesini [:baku]]

Attachment: interdiff patch 5

Comment 105

[Andrea Marchesini [:baku]]

I'll file followups to avoid the copying of data in RefcountedMessagePortMessage as we discussed on IRC.

Comment 106

[Andrea Marchesini [:baku]]

About the interdiff requests, the code is changed too much to generate interdiffs. Sorry :/

Comment 107

[Andrea Marchesini [:baku]]

any news for the second review?

Comment 108

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hrm, what happened to patch 2? Is there a new version I'm supposed to review?

Comment 109

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8551394 [details] [diff] [review]
patch 5 - MessagePort transferred cross threads.

Review of attachment 8551394 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but we need to talk about that change in NotifyFeatures before this lands.

::: dom/messagechannel/MessagePort.h
@@ +187,5 @@
>    // creating the actor and until the actor is entangled.
>    // We release the object when the port is closed or disentangled.
>    void UpdateMustKeepAlive();
>  
>    // Raw pointer because this goes 1:1 with the port.

Nit: No longer a raw pointer

::: dom/workers/WorkerPrivate.cpp
@@ +96,5 @@
>  #include "WorkerFeature.h"
>  #include "WorkerRunnable.h"
>  #include "WorkerScope.h"
>  #include "WorkerThread.h"
> +#include "WorkerStructuredClone.h"

Nit: alphabetize

@@ +372,5 @@
>  bool
>  WriteBlobOrFile(JSContext* aCx,
>                  JSStructuredCloneWriter* aWriter,
>                  FileImpl* aBlobOrFileImpl,
> +                WorkerStructuredCloneClosure* aClosure)

Nit: s/*/&/

@@ +500,5 @@
> +
> +      closure->mMessagePorts.AppendElement(port);
> +
> +      JS::Rooted<JSObject*> obj(aCx, port->WrapObject(aCx));
> +      if (!obj || !JS_WrapObject(aCx, &obj)) {

Wait, why do you need to call JS_WrapObject right after calling port->WrapObject?

@@ +520,5 @@
> +    MOZ_ASSERT(aClosure);
> +
> +    auto* closure = static_cast<WorkerStructuredCloneClosure*>(aClosure);
> +
> +    MessagePortBase* port = nullptr;

Nit: no need to initialize

@@ +524,5 @@
> +    MessagePortBase* port = nullptr;
> +    nsresult rv = UNWRAP_OBJECT(MessagePort, aObj, port);
> +    if (NS_SUCCEEDED(rv)) {
> +      if (NS_WARN_IF(closure->mTransferredPorts.Contains(port))) {
> +        // No duplicate.

Nit: s/duplicate/duplicates/

@@ +532,5 @@
> +      MessagePortIdentifier* identifier =
> +        closure->mMessagePortIdentifiers.AppendElement();
> +
> +      if (!port->CloneAndDisentangle(*identifier)) {
> +        return false;

Maybe also remove that last element you just created? Or just AppendElement after you know CloneAndDisentangle succeeded?

@@ +661,5 @@
>    MainThreadWorkerStructuredCloneCallbacks::Write,
>    MainThreadWorkerStructuredCloneCallbacks::Error,
> +  MainThreadWorkerStructuredCloneCallbacks::ReadTransfer,
> +  MainThreadWorkerStructuredCloneCallbacks::Transfer,
> +  MainThreadWorkerStructuredCloneCallbacks::FreeTransfer

You don't really need these, just put the WorkerStructuredCloneCallbacks versions here?

@@ +724,5 @@
>    ChromeWorkerStructuredCloneCallbacks::Write,
>    ChromeWorkerStructuredCloneCallbacks::Error,
> +  ChromeWorkerStructuredCloneCallbacks::ReadTransfer,
> +  ChromeWorkerStructuredCloneCallbacks::Transfer,
> +  ChromeWorkerStructuredCloneCallbacks::FreeTransfer

Ditto

@@ +815,5 @@
>    MainThreadChromeWorkerStructuredCloneCallbacks::Write,
>    MainThreadChromeWorkerStructuredCloneCallbacks::Error,
> +  MainThreadChromeWorkerStructuredCloneCallbacks::ReadTransfer,
> +  MainThreadChromeWorkerStructuredCloneCallbacks::Transfer,
> +  MainThreadChromeWorkerStructuredCloneCallbacks::FreeTransfer

Ditto

@@ +5287,5 @@
>    nsAutoTArray<WorkerFeature*, 30> features;
>    features.AppendElements(mFeatures);
>  
>    for (uint32_t index = 0; index < features.Length(); index++) {
> +    if (mFeatures.Contains(features[index]) &&

Hm, this is a little scary... Why is this needed? One feature being notified removes another?! I really hope we can undo this.

::: dom/workers/WorkerStructuredClone.h
@@ +20,5 @@
> +class WorkerStructuredCloneClosure MOZ_FINAL
> +{
> +public:
> +  WorkerStructuredCloneClosure()
> +  {}

I'd put the constructor (and add a destructor) in WorkerPrivate.cpp so that you don't get weird linker errors when including this file elsewhere.

@@ +23,5 @@
> +  WorkerStructuredCloneClosure()
> +  {}
> +
> +  // This is used for creating the new MessagePort it can be null if in workers.
> +  nsCOMPtr<nsPIDOMWindow> mParent;

Nit: forward-declare nsPIDOMWindow above, and maybe rename to "mParentWindow" since mParent is pretty ambiguous in worker-land

@@ +39,5 @@
> +  nsTArray<nsRefPtr<MessagePortBase>> mTransferredPorts;
> +
> +private:
> +  WorkerStructuredCloneClosure(const WorkerStructuredCloneClosure&);
> +  WorkerStructuredCloneClosure & operator=(const WorkerStructuredCloneClosure&);

Nit: add |= delete|

::: dom/workers/XMLHttpRequest.cpp
@@ +1538,5 @@
>        rv = NS_ERROR_DOM_DATA_CLONE_ERR;
>      }
>  
>      mBody.clear();
> +    mClosure.mClonedObjects.Clear();

Shouldn't you clear the other stuff here too?

Comment 110

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground

Comment 111

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Move MessagePort code into a separated directory.

rebased

Comment 112

[Andrea Marchesini [:baku]]

Attachment: patch 3 - MessagePort unshipped port queue

Comment 113

[Andrea Marchesini [:baku]]

Attachment: patch 4 - transferable objects in MessagePort

Comment 114

[Andrea Marchesini [:baku]]

Attachment: patch 2 - MessagePort/Channel with PBackground

Comment 115

[Andrea Marchesini [:baku]]

Comment on attachment 8581811 [details] [diff] [review]
patch 2 - MessagePort/Channel with PBackground

Ok, the main issue of this patch is that we don't free memory allocated for the cloning. This is implemented in a separate patch because I'm not sure you like what I have done.

Comment 116

[Andrea Marchesini [:baku]]

Attachment: patch 2b - Free of Structured Clone arrays

Comment 117

[Andrea Marchesini [:baku]]

Attachment: patch 2c - interdiff

Here the interdiff based on your previous comments.

Comment 118

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8582465 [details] [diff] [review]
patch 2b - Free of Structured Clone arrays

Review of attachment 8582465 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/StructuredCloneUtils.cpp
@@ +136,5 @@
> +                           aClosure, aClone)) {
> +    return false;
> +  }
> +
> +  aData.Clear();

It's weird to clear here inside Read (that's pretty surprising behavior). I wouldn't do it, let the caller call FreeStructuredClone when they're done with the data.

@@ +159,5 @@
> +  }
> +
> +  uint64_t* data;
> +  size_t size;
> +  buffer.steal(&data, &size);

Don't steal, just copy into the TArray and then let buffer clean up its data when it goes out of scope (no need for manual js_free). Otherwise you will leak below.

@@ +169,5 @@
> +
> +  memcpy(cloneData.Elements(), data, size);
> +  js_free(data);
> +
> +  aData.SwapElements(cloneData);

Since you're swapping you should assert at the beginning that aData is empty.

::: dom/messagechannel/MessagePort.cpp
@@ +118,4 @@
>        return NS_ERROR_FAILURE;
>      }
>  
> +    // The data is cleaned by ReadStructuredClone.

It shouldn't be!

@@ +477,5 @@
>  
> +  // We must convert the messages into SharedMessagePortMessages to avoid leaks.
> +  FallibleTArray<nsRefPtr<SharedMessagePortMessage>> data;
> +  if (!SharedMessagePortMessage::FromMessagesToShared(aMessages, data)) {
> +    // OOM, we cannot continue.

Should you notify the other end of the pipe somehow?

::: dom/messagechannel/SharedMessagePortMessage.cpp
@@ +45,5 @@
>                        FallibleTArray<nsRefPtr<SharedMessagePortMessage>>& aData)
>  {
>    aData.Clear();
>  
> +  if (!aData.SetCapacity(aArray.Length())) {

NS_WARN_IF

::: js/public/StructuredClone.h
@@ +147,5 @@
>  
>  JS_PUBLIC_API(bool)
>  JS_ClearStructuredClone(uint64_t *data, size_t nbytes,
>                          const JSStructuredCloneCallbacks *optionalCallbacks,
> +                        void *closure, bool freeData = true);

I don't think this is the right change. Instead you should probably add another function that releases transferables but explicitly does not free its data. I guess ask the JS folks what they want to do here?

Comment 119

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I don't understand what attachment 8581811 [details] [diff] [review] and attachment 8582506 [details] [diff] [review] are. I figured 8581811 was the final patch with the changes from 8582506 applied... But that doesn't look like it's true. What is the relationship here?

Comment 120

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8552614 [details] [diff] [review]
interdiff patch 5

Review of attachment 8552614 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/messagechannel/MessagePortService.cpp
@@ +18,2 @@
>  static MessagePortService* gInstance;
> +} // anonymous namespace

I'm horribly confused... This is part of the 'patch 2c - interdiff'

Comment 121

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Andrea, can you explain what's going on here? There are too many patches with too many interdiffs and duplicate changes... Maybe we need to obsolete everything and start fresh with new patches?

Comment 122

[Antonio Manuel Amaya Calvo (:amac)]

It would be *really* great to get this landed ASAP. We're trying to implement some service like apps in B2G (to mediate access to privileged/certified contents and allow content that would have to be privileged to be just normal content) and the lack of MessageChannel is making us made all kind of workarounds. Some of then don't even make much sense (or have a solution, really), like to be able to send a message from the service worker to the main thread we need a controlled client, but the first time the service app loads (because it gets a message/IAC from a client application) the SW is not registered, so we don't have any clients and can't answer unless we reload, but if we reload then we lose the message from the client. 

We could do a workaround loading the code on another iframe after the SW has been installed and doing the actual processing on that iframe... but it would be much nicer having this available (not to mention we could do away with all the code that we had to add just because we couldn't have this working).

Comment 123

[Andrea Marchesini [:baku]]

Attachment: single patch

I don't know if it helps but I have all the code in 1 single patch.
bent, if you prefer you can review that instead all of these patches.
How does it sound? This patch doesn't include blob because I have to rebase too many things.

Your comments are not applied yet.

I created this patch because it was needed for the madrid work-week.

Comment 124

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'll wait for a patch with previous comments already addressed so that I don't have to do it all again.

Comment 125

[Andrea Marchesini [:baku]]

Attachment: single patch

I applied all your comments but:

1. I have to steal the data from the buffer because otherwise it will be freed and I will have invalid elements.

2. I didn't have any feedback about the changes in the StructuredClonedAlgorithm.

Comment 126

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8597372 [details] [diff] [review]
single patch

Review of attachment 8597372 [details] [diff] [review]:
-----------------------------------------------------------------

Nowhere near done but here's some feedback to get started with.

Please please please track all changes as an interdiff so I don't have to review the same code again :)

And I guess hold off posting new patches until I'm done going through this one.

::: dom/base/NodeInfo.cpp
@@ +110,5 @@
>  NS_IMPL_CYCLE_COLLECTION_CLASS(NodeInfo)
>  
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_0(NodeInfo)
>  
> +static const char* kNodeInfoNSURIs[] = {

Unrelated changes in this file?

::: dom/base/ProcessGlobal.cpp
@@ +6,5 @@
>  
>  #include "ProcessGlobal.h"
>  
>  #include "nsContentCID.h"
> +#include "nsDOMClassInfoID.h"

Here too?

::: dom/base/nsContentUtils.cpp
@@ +7102,5 @@
> +nsContentUtils::GenerateUUIDInPlace(nsID& aUUID)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIUUIDGenerator> uuidGenerator =
> +    do_GetService("@mozilla.org/uuid-generator;1", &rv);

This is ok, but it does mean you pay the servicemanager overhead for every call. Could try caching the service if you can guarantee to do it on the main thread.

::: dom/ipc/StructuredCloneUtils.cpp
@@ +38,5 @@
> +
> +  StructuredCloneClosure& mClosure;
> +  nsPIDOMWindow* mWindow;
> +  nsTArray<nsRefPtr<MessagePort>> mMessagePorts;
> +  nsTHashtable<nsRefPtrHashKey<MessagePortBase>> mTransferredPorts;

Why use a hashtable? Seems very unlikely that you'll have an extremely large number of ports in one message, just use an array.

@@ +57,5 @@
>       uint32_t aData, void* aClosure)
>  {
>    MOZ_ASSERT(aClosure);
>  
> +  StructuredCloneClosureInternal* closure =

Nit: |auto*|, here and several other places below.

@@ +131,5 @@
> +
> +  StructuredCloneClosureInternal* closure =
> +    static_cast<StructuredCloneClosureInternal*>(aClosure);
> +
> +  if (aTag == SCTAG_DOM_MAP_MESSAGEPORT) {

Better to return false immediately if the tag isn't what you're looking for.

@@ +138,5 @@
> +
> +    ErrorResult rv;
> +    nsRefPtr<MessagePort> port =
> +      MessagePort::Create(closure->mWindow,
> +                          closure->mClosure.mMessagePortIdentifiers[aExtraData],

nsTArray doesn't have 64-bit indexes...

@@ +160,5 @@
> +  return false;
> +}
> +
> +bool
> +Transfer(JSContext* aCx, JS::Handle<JSObject*> aObj, void* aClosure,

Nit: call this "WriteTransfer"?

@@ +166,5 @@
> +         void** aContent, uint64_t *aExtraData)
> +{
> +  MOZ_ASSERT(aClosure);
> +
> +  StructuredCloneClosureInternal* closure =

Nit: auto*

@@ +169,5 @@
> +
> +  StructuredCloneClosureInternal* closure =
> +    static_cast<StructuredCloneClosureInternal*>(aClosure);
> +
> +  MessagePortBase *port = nullptr;

Nit: * on left, and you shouldn't need to initialize it.

@@ +171,5 @@
> +    static_cast<StructuredCloneClosureInternal*>(aClosure);
> +
> +  MessagePortBase *port = nullptr;
> +  nsresult rv = UNWRAP_OBJECT(MessagePort, aObj, port);
> +  if (NS_SUCCEEDED(rv)) {

return false immediately if rv failed.

@@ +197,5 @@
> +  return false;
> +}
> +
> +void
> +FreeTransfer(uint32_t aTag, JS::TransferableOwnership aOwnership,

Why implement this? Is it required, or could you just leave |nullptr| in |gCallbacks|?

@@ +237,5 @@
> +                                JS::MutableHandle<JS::Value> aClone,
> +                                nsPIDOMWindow* aParentWindow,
> +                                nsTArray<nsRefPtr<MessagePort>>& aMessagePorts)
> +{
> +  auto closure = const_cast<StructuredCloneClosure&>(aClosure);

Why do you need to un-const this? That's not good! Either change the signature or figure out another way to do this.

@@ +240,5 @@
> +{
> +  auto closure = const_cast<StructuredCloneClosure&>(aClosure);
> +  StructuredCloneClosureInternal internalClosure(closure, aParentWindow);
> +
> +  bool rv = !!JS_ReadStructuredClone(aCx, aData, aDataLength,

Why |!!|? It returns bool...

@@ +257,5 @@
> +                                JS::MutableHandle<JS::Value> aClone,
> +                                nsPIDOMWindow* aParentWindow,
> +                                nsTArray<nsRefPtr<MessagePort>>& aMessagePorts)
> +{
> +  return ReadStructuredCloneWithTransfer(aCx, (uint64_t*)aData.Elements(),

reinterpret_cast!

And you should at least assert that aData has a length divisible by sizeof(uint64_t), otherwise you'll walk off into someone else's memory.

@@ +268,5 @@
>                       JSAutoStructuredCloneBuffer& aBuffer,
>                       StructuredCloneClosure& aClosure)
>  {
> +  StructuredCloneClosureInternal internalClosure(aClosure, nullptr);
> +  JS::Rooted<JS::Value> transferable(aCx, JS::UndefinedValue());

JS::UndefinedValue() isn't needed, it's the default.

@@ +297,5 @@
> +  }
> +
> +  uint64_t* data;
> +  size_t size;
> +  buffer.steal(&data, &size);

Don't steal and free manually, let JSAutoStructuredCloneBuffer delete for you.

@@ +310,5 @@
> +
> +void
> +FreeStructuredClone(nsTArray<uint8_t>& aData, StructuredCloneClosure& aClosure)
> +{
> +  JS_ClearStructuredClone((uint64_t*)aData.Elements(), aData.Length(),

reinterpret_cast, and assert that the length is correct

::: dom/ipc/StructuredCloneUtils.h
@@ +46,5 @@
>                               aData.mClosure, aClone);
>  }
>  
>  bool
> +ReadStructuredCloneWithTransfer(JSContext* aCx, uint64_t* aData,

These "WithTransfer" functions, and adding MessagePortIdentifier to StructuredCloneClosure, are pretty specific to MessagePort. I would not add them here, just move them somewhere into dom/messagechannel and then make your own callbacks that call into these. Hopefully that will simplify things.

::: dom/ipc/nsIContentParent.h
@@ +36,5 @@
>  class BlobConstructorParams;
>  class BlobParent;
>  class ContentParent;
>  class File;
> +class FileImpl;

All this is BlobImpl now.

::: dom/messagechannel/MessagePort.h
@@ +24,5 @@
> +class WorkerFeature;
> +}
> +
> +// This class contains all the information to clone a MessagePort object.
> +struct MessagePortIdentifier final

Wait, what's the difference between this and TransferredMessagePortIdentifier? I think you should just rename the IPDL one to MessagePortIdentifier and use that everywhere. You don't want two nearly-identical types running around.

::: dom/messagechannel/PMessagePort.ipdl
@@ +4,5 @@
> +
> +include protocol PBackground;
> +include protocol PBlob;
> +
> +using struct mozilla::SerializedStructuredCloneBuffer from "ipc/IPCMessageUtils.h";

This isn't needed now, right?

@@ +18,5 @@
> +  uint32_t sequenceId;
> +  bool neutered;
> +};
> +
> +struct MessagePortMessage {

Nit: { on its own line

@@ +34,5 @@
> +     correct sequence for the child actor is:
> +     1. SendStopSendingData();
> +     2. RecvStopSendingDataConfirmed();
> +     3. SendClose();
> +     4. RecvClosed();

I forget why we need a |Closed| message. Couldn't we just receive the |__delete__| message instead, like we do when disentangling?

::: dom/messagechannel/SharedMessagePortMessage.cpp
@@ +55,5 @@
> +                      MessagePortChild* aActor,
> +                      const nsTArray<nsRefPtr<SharedMessagePortMessage>>& aData,
> +                      nsTArray<MessagePortMessage>& aArray)
> +{
> +  aArray.Clear();

Maybe just assert that aArray is empty here?

Also assert non-null aActor and that aData is not empty?

@@ +61,5 @@
> +
> +  PBackgroundChild* backgroundManager = aActor->Manager();
> +  MOZ_ASSERT(backgroundManager);
> +
> +  for (uint32_t i = 0, len = aData.Length(); i < len; ++i) {

Use range-for now. E.g.:

  for (auto& data : aData) { ... }

@@ +70,5 @@
> +      aData[i]->mClosure.mBlobImpls;
> +    if (!blobImpls.IsEmpty()) {
> +      message->blobsChild().SetCapacity(blobImpls.Length());
> +
> +      for (uint32_t i = 0, len = blobImpls.Length(); i < len; ++i) {

range-for again.

@@ +78,5 @@
> +        message->blobsChild().AppendElement(blobChild);
> +      }
> +    }
> +
> +    {

Why do you need an extra block?

@@ +85,5 @@
> +      nsTArray<TransferredMessagePortIdentifier>& transferredPorts =
> +        message->transferredPorts();
> +      transferredPorts.SetCapacity(identifiers.Length());
> +
> +      for (uint32_t t = 0; t < identifiers.Length(); ++t) {

range-for

@@ +143,5 @@
> +  return true;
> +}
> +
> +/* static */ bool
> +SharedMessagePortMessage::FromSharedToMessagesParent(

Hm, can't this be turned into a template somehow? This function is basically identical to FromSharedToMessagesChild

@@ +193,5 @@
> +  return true;
> +}
> +
> +/* static */ bool
> +SharedMessagePortMessage::FromMessagesToSharedParent(

Same

::: dom/messagechannel/SharedMessagePortMessage.h
@@ +28,5 @@
> +
> +  static void
> +  FromSharedToMessagesChild(
> +                      MessagePortChild* aActor,
> +                      const nsTArray<nsRefPtr<SharedMessagePortMessage>>& aData,

You actually modify aData here so don't pretend that this is const.

Same for the rest of these functions.

::: dom/messagechannel/moz.build
@@ +13,5 @@
> +    'MessagePortList.h',
> +    'MessagePortParent.h',
> +]
> +
> +SOURCES += [

UNIFIED_SOURCES

::: dom/workers/WorkerStructuredClone.h
@@ +16,5 @@
> +struct MessagePortIdentifer;
> +}
> +}
> +
> +BEGIN_WORKERS_NAMESPACE

No need for this in new code, just continue your previous namespace block and add |namespace workers {|

@@ +21,5 @@
> +
> +class WorkerStructuredCloneClosure final
> +{
> +public:
> +  WorkerStructuredCloneClosure();

Nit: Comment where these are implemented.

@@ +24,5 @@
> +public:
> +  WorkerStructuredCloneClosure();
> +  ~WorkerStructuredCloneClosure();
> +
> +  // This is used for creating the new MessagePort it can be null if in workers.

Nit: run-on sentence.

@@ +30,5 @@
> +
> +  nsTArray<nsCOMPtr<nsISupports>> mClonedObjects;
> +
> +  // The transferred port created on the 'other side' of the Structured Clone
> +  // Algorithm.

Nit: I actually don't understand this comment. Is this the ports that have already been transferred, so guaranteed to be empty on the sending side? That sounds like what I would expect to be held in mTransferredPorts. So this must be something else. Needs a clearer comment.

@@ +39,5 @@
> +
> +  // To avoid duplicates in the transferred ports.
> +  nsTArray<nsRefPtr<MessagePortBase>> mTransferredPorts;
> +
> +  void Clear();

You're mixing members and functions here. Put all the members at the top and then all the functions below? Or the other way around, I don't care so much. But don't mix :)

@@ +42,5 @@
> +
> +  void Clear();
> +
> +private:
> +  WorkerStructuredCloneClosure(const WorkerStructuredCloneClosure&);

Nit: = delete

@@ +46,5 @@
> +  WorkerStructuredCloneClosure(const WorkerStructuredCloneClosure&);
> +  WorkerStructuredCloneClosure & operator=(const WorkerStructuredCloneClosure&) = delete;
> +};
> +
> +END_WORKERS_NAMESPACE

de-macro-ize

::: js/public/StructuredClone.h
@@ +147,5 @@
>  
>  JS_PUBLIC_API(bool)
>  JS_ClearStructuredClone(uint64_t* data, size_t nbytes,
>                          const JSStructuredCloneCallbacks* optionalCallbacks,
> +                        void *closure, bool freeData = true);

I said previously:

> I don't think this is the right change. Instead you should
> probably add another function that releases transferables but
> explicitly does not free its data. I guess ask the JS folks
> what they want to do here?

Comment 127

[Andrea Marchesini [:baku]]

> > +static const char* kNodeInfoNSURIs[] = {
> 
> Unrelated changes in this file?

These changes are needed because of the moving of MessagePort.cpp in a separate directory.

> ::: dom/base/nsContentUtils.cpp
> @@ +7102,5 @@
> > +nsContentUtils::GenerateUUIDInPlace(nsID& aUUID)
> > +{
> > +  nsresult rv;
> > +  nsCOMPtr<nsIUUIDGenerator> uuidGenerator =
> > +    do_GetService("@mozilla.org/uuid-generator;1", &rv);
> 
> This is ok, but it does mean you pay the servicemanager overhead for every
> call. Could try caching the service if you can guarantee to do it on the
> main thread.

I cannot guarantee this. MessageChannel can be used in workers. I can cache the UUIDGenerator in the main-thread and have a runtime one in workers, but I don't know if this is nice to have. Or cache it in WorkerPrivate. What do you think?


> @@ +237,5 @@
> > +                                JS::MutableHandle<JS::Value> aClone,
> > +                                nsPIDOMWindow* aParentWindow,
> > +                                nsTArray<nsRefPtr<MessagePort>>& aMessagePorts)
> > +{
> > +  auto closure = const_cast<StructuredCloneClosure&>(aClosure);
> 
> Why do you need to un-const this? That's not good! Either change the
> signature or figure out another way to do this.

I do this because JS_ReadStructuredClone wants a void*. I can change the JS_ReadStructuredClone or all the places where we use const StructredCloneClosure.

> 
> @@ +297,5 @@
> > +  }
> > +
> > +  uint64_t* data;
> > +  size_t size;
> > +  buffer.steal(&data, &size);
> 
> Don't steal and free manually, let JSAutoStructuredCloneBuffer delete for
> you.

I cannot. If the data is freed by the JSAutoStructuredCloneBuffer, after buffer.clear() all my objects are freed and the code crashes. What you suggest works without transferred objects only.

> 
> ::: dom/messagechannel/SharedMessagePortMessage.cpp
> @@ +55,5 @@
> > +                      MessagePortChild* aActor,
> > +                      const nsTArray<nsRefPtr<SharedMessagePortMessage>>& aData,
> > +                      nsTArray<MessagePortMessage>& aArray)
> > +{
> > +  aArray.Clear();
> 
> Maybe just assert that aArray is empty here?
> 
> Also assert non-null aActor and that aData is not empty?

aData can be empty when we send the Disentangle message.


> 
> ::: js/public/StructuredClone.h
> @@ +147,5 @@
> >  
> >  JS_PUBLIC_API(bool)
> >  JS_ClearStructuredClone(uint64_t* data, size_t nbytes,
> >                          const JSStructuredCloneCallbacks* optionalCallbacks,
> > +                        void *closure, bool freeData = true);
> 
> I said previously:
> 
> > I don't think this is the right change. Instead you should
> > probably add another function that releases transferables but
> > explicitly does not free its data. I guess ask the JS folks
> > what they want to do here?

ok. maybe this will solve also the memcpy + free issue.

I have an interdiff with your first comments.

Comment 128

[Andrea Marchesini [:baku]]

Attachment: interdiff

Comment 129

[Andrea Marchesini [:baku]]

Attachment: rebased full patch

Comment 130

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Andrea Marchesini (:baku) from comment #127)
> I cannot guarantee this. MessageChannel can be used in workers. I can cache
> the UUIDGenerator in the main-thread and have a runtime one in workers, but
> I don't know if this is nice to have. Or cache it in WorkerPrivate. What do
> you think?

Just get the cache mechanism to work like the others in nsContentUtils and then call (and cache) it on the main thread before you create the first worker in RuntimeService::Init() like we do for IndexedDatabaseManager.

> I do this because JS_ReadStructuredClone wants a void*. I can change the
> JS_ReadStructuredClone or all the places where we use const
> StructredCloneClosure.

You can't un-const a StructredCloneClosure, you don't know that the caller will always be able to handle that. If you just need to turn it into a void* then do the const_cast on the address as you pass it to JS_ReadStructuredClone.

> I cannot. If the data is freed by the JSAutoStructuredCloneBuffer, after
> buffer.clear() all my objects are freed and the code crashes. What you
> suggest works without transferred objects only.

Oh I see. That's subtle, add a comment?

Comment 131

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8597372 [details] [diff] [review]
single patch

Review of attachment 8597372 [details] [diff] [review]:
-----------------------------------------------------------------

Ok! All done.

Still a bit concerned about MessagePortService lifetime and a few other issues.

::: dom/base/nsGlobalWindow.cpp
@@ +7977,5 @@
>    bool subsumes;
>    nsPIDOMWindow* window;
> +
> +  // This hashtable contains the transferred ports - used to avoid duplicates.
> +  nsTHashtable<nsRefPtrHashKey<MessagePortBase>> transferredPorts;

I think you're better off with an nsTArray. You won't have enough elements to make a hashtable faster, I don't think.

@@ +8102,5 @@
>    StructuredCloneInfo* scInfo = static_cast<StructuredCloneInfo*>(aClosure);
>    NS_ASSERTION(scInfo, "Must have scInfo!");
>  
>    if (tag == SCTAG_DOM_MAP_MESSAGEPORT) {
> +    MOZ_ASSERT(aData == 0);

!aData

::: dom/messagechannel/MessageChannel.cpp
@@ +14,5 @@
>  #include "nsContentUtils.h"
>  #include "nsPIDOMWindow.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsIDocument.h"
> +#include "nsIPrincipal.h"

Nit: Alphabetize?

@@ +16,5 @@
> +#include "nsServiceManagerUtils.h"
> +#include "nsIDocument.h"
> +#include "nsIPrincipal.h"
> +
> +using namespace mozilla::dom::workers;

Nit: Move this inside your other namespace block to prevent leaking into other UNIFIED_SOURCES

@@ +64,5 @@
>    if (NS_FAILED(uri->SchemeIs("resource", &isResource))) {
>      return false;
>    }
>  
>    return isResource;

Please check with bz to make sure this is correct. I don't know why we would whitelist all resource:// pages.

@@ +98,5 @@
> +}
> +
> +// A WorkerMainThreadRunnable to synchronously dispatch the call of
> +// CheckPermission() from the worker thread to the main thread.
> +class CheckPermissionRunnable final : public WorkerMainThreadRunnable

Hrm, I don't like this too much... Can't you check whether MessageChannel is enabled before you launch the worker? Like we do for IndexedDB (search for "mIndexedDBAllowed"). Then you wouldn't have to block every worker when it starts up...

@@ +107,5 @@
> +
> +  CheckPermissionRunnable(WorkerPrivate* aWorkerPrivate)
> +    : WorkerMainThreadRunnable(aWorkerPrivate)
> +    , mResult(false)
> +    , mCallerChrome(nsContentUtils::ThreadsafeIsCallerChrome())

This is overkill, just call aWorkerPrivate->UsesSystemPrincipal()

@@ +190,5 @@
>  /* static */ already_AddRefed<MessageChannel>
>  MessageChannel::Constructor(const GlobalObject& aGlobal, ErrorResult& aRv)
>  {
>    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
> +  // window can be null in workers.

Nit: Move this above where you set window

::: dom/messagechannel/MessagePort.cpp
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "MessagePort.h"
> +#include "SharedMessagePortMessage.h"

Nit: This should just go in the main list

@@ +40,2 @@
>  {
> +friend class MessagePort;

Nit: Indent

@@ +47,5 @@
> +    : mPort(aPort)
> +  { }
> +
> +  NS_IMETHOD
> +  Run()

Hm, you should probably bail out if mPort is null (you got canceled)?

And you need 'override'

@@ +49,5 @@
> +
> +  NS_IMETHOD
> +  Run()
> +  {
> +    mPort->mDispatchRunnable = nullptr;

MOZ_ASSERT(mPort->mDispatchRunnable == this) before?

@@ +56,5 @@
> +    return NS_OK;
> +  }
> +
> +  NS_IMETHOD
> +  Cancel()

override

@@ +84,5 @@
> +    MOZ_ASSERT(aPort);
> +    MOZ_ASSERT(aData);
> +  }
> +
> +  NS_IMETHODIMP Run()

NS_IMETHOD, and override

And handle being canceled?

@@ +86,5 @@
> +  }
> +
> +  NS_IMETHODIMP Run()
> +  {
> +    AutoJSAPI jsapi;

Nit: Move this closer to where you use it.

@@ +117,5 @@
> +      if (!ok) {
> +        return NS_ERROR_FAILURE;
> +      }
> +
> +      FreeStructuredClone(mData->mData, mData->mClosure);

Hm, shouldn't you call this even if Read fails?

@@ +138,5 @@
> +    event->SetSource(mPort);
> +
> +    nsTArray<nsRefPtr<MessagePortBase>> array;
> +    for (uint32_t i = 0; i < ports.Length(); ++i) {
> +      array.AppendElement(ports[i]);

Hrm, I hope |array.AppendElements(ports)| works here? Otherwise you at least need a SetCapacity() call before appending elements.

@@ +145,5 @@
> +    nsRefPtr<MessagePortList> portList =
> +      new MessagePortList(static_cast<dom::Event*>(event.get()), array);
> +    event->SetPorts(portList);
> +
> +    bool status;

Nit: call this "dummy" or "ignored" or something.

@@ +151,5 @@
> +    return NS_OK;
> +  }
> +
> +  NS_IMETHOD
> +  Cancel()

override

@@ +180,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_CLASS(MessagePort)
>  
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MessagePort,
>                                                  DOMEventTargetHelper)

Hrm, your base class isn't DOMEventTargetHelper! It's MessagePortBase. Change this everywhere.

@@ -386,5 @@
> -  // Custom unlink loop because this array contains nsRunnable objects
> -  // which are not cycle colleactable.
> -  for (uint32_t i = 0, len = tmp->mMessageQueue.Length(); i < len; ++i) {
> -    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMessageQueue[i]->mPort);
> -    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMessageQueue[i]->mSupportsArray);

This went unaddressed in a previous review (comment 88). Why aren't you traversing these anymore?

@@ +289,5 @@
> +  mMessageQueueEnabled = false;
> +  mState = aState;
> +  mNextStep = eNextStepNone;
> +  mIsKeptAlive = false;
> +  mInnerID = 0;

Couldn't all these be done in the constructor?

@@ +290,5 @@
> +  mState = aState;
> +  mNextStep = eNextStepNone;
> +  mIsKeptAlive = false;
> +  mInnerID = 0;
> +  mWorkerFeature = nullptr;

Not needed

@@ +320,5 @@
> +    mWorkerFeature = new MessagePortFeature(this);
> +    JSContext* cx = workerPrivate->GetJSContext();
> +    if (NS_WARN_IF(!workerPrivate->AddFeature(cx, mWorkerFeature))) {
> +      NS_WARNING("Failed to register the BroadcastChannel worker feature.");
> +      mWorkerFeature = nullptr;

Rather than assigning and then nulling if something failed just do:

  nsAutoPtr<MessagePortFeature> feature = new MessagePortFeature(this);
  if (NS_WARN_IF(!workerPrivate->AddFeature(cx, feature))) {
    aRv.Throw(NS_ERROR_FAILURE);
    return;
  }
  mWorkerFeature = Move(feature);

@@ +329,3 @@
>  }
>  
>  MessagePort::~MessagePort()

Nit: Move this together with your constructor?

@@ +348,2 @@
>  {
> +  nsRefPtr<SharedMessagePortMessage> data = new SharedMessagePortMessage();

Move this down closer to where you actually use it.

@@ +355,5 @@
>      const Sequence<JS::Value>& realTransferable = aTransferable.Value();
>  
> +    // Here we want to check if the transerable object list contains
> +    // this port. No other checks are done.
> +    for (uint32_t i = 0; i < realTransferable.Length(); ++i) {

range-for.

@@ +360,5 @@
> +      if (!realTransferable[i].isObject()) {
> +        continue;
> +      }
> +
> +      JS::Rooted<JSObject*> obj(aCx, &realTransferable[i].toObject());

Maybe just root the Sequence (use SequenceRooter) before entering the loop rather than rooting every object?

@@ +365,5 @@
> +      if (!obj) {
> +        continue;
> +      }
> +
> +      MessagePortBase *port = nullptr;

Nit: * on left, and do you need to initialize it if you check the return value of UNWRAP_OBJECT?

@@ +381,2 @@
>      // The input sequence only comes from the generated bindings code, which
>      // ensures it is rooted.

Oh, this comment makes it sound like you don't need to do any rooting in the loop above.

@@ +405,1 @@
>      return;

Hm, can you check this earlier and avoid all the work you just did?

@@ +414,5 @@
> +  }
> +
> +  // Not entangled yet, but already closed.
> +  if (mNextStep != eNextStepNone) {
> +    return;

Same here, could you do this check earlier and avoid work?

@@ +417,5 @@
> +  if (mNextStep != eNextStepNone) {
> +    return;
> +  }
> +
> +  RemoveDocFromBFCache();

Hm, you did several early returns above, are you sure you shouldn't call this no matter what?

@@ +481,4 @@
>  
> +    // This avoids loops.
> +    nsRefPtr<MessagePort> port = mUnshippedEntangledPort;
> +    mUnshippedEntangledPort = nullptr;

nsRefPtr<MessagePort> port = Move(mUnshippedEntangledPort);

@@ +542,2 @@
>  void
> +MessagePort::Entangled(const nsTArray<MessagePortMessage>& aMessages)

If this took an rvalue arg then you could avoid copying the message data... We should look into this now that IPDL hands out rvalue refs.

@@ +552,5 @@
> +    SharedMessagePortMessage::FromSharedToMessagesChild(mActor,
> +                                                        mMessageForTheOtherPortQueue,
> +                                                        messages);
> +    mActor->SendPostMessages(messages);
> +    mMessageForTheOtherPortQueue.Clear();

Maybe do this before calling SendPostMessages?

@@ +557,5 @@
> +  }
> +
> +  // We must convert the messages into SharedMessagePortMessages to avoid leaks.
> +  FallibleTArray<nsRefPtr<SharedMessagePortMessage>> data;
> +  if (!SharedMessagePortMessage::FromMessagesToSharedChild(aMessages, data)) {

NS_WARN_IF?

@@ +567,5 @@
> +    Close();
> +    return;
> +  }
> +
> +  mMessageQueue.AppendElements(data);

Can you SwapElements here? mMessageQueue should be empty, right?

@@ +606,5 @@
> +
> +  RemoveDocFromBFCache();
> +
> +  FallibleTArray<nsRefPtr<SharedMessagePortMessage>> data;
> +  if (!SharedMessagePortMessage::FromMessagesToSharedChild(aMessages, data)) {

NS_WARN_IF?

@@ +639,5 @@
> +  nsTArray<MessagePortMessage> messages;
> +  SharedMessagePortMessage::FromSharedToMessagesChild(mActor, mMessageQueue,
> +                                                      messages);
> +  mActor->SendDisentangle(messages);
> +  mMessageQueue.Clear();

Do this before calling SendDisentangle()?

@@ +653,5 @@
> +{
> +  aIdentifier.mNeutered = true;
> +
> +  if (mState > eStateEntangled) {
> +    return true;

This is weird, you will return true, but you haven't set all the members of aIdentifier yet. In at least one place it is passed in uninitialized, so you'll have garbage there...

@@ +659,5 @@
> +
> +  // We already have a 'next step'. We have to consider this port as already
> +  // cloned/closed/disentangled.
> +  if (mNextStep != eNextStepNone) {
> +    return true;

Same

@@ +681,5 @@
> +    if (mMessageQueue.IsEmpty()) {
> +      aIdentifier.mSequenceID = mIdentifier.mSequenceID;
> +
> +      // Disentangle can delete this object.
> +      nsRefPtr<MessagePort> kungFuDeathGrip(this);

Hm, why do you need this? You're not calling Disentangle(), and you return as soon as you're done with UpdateMustKeepAlive().

In any case your caller should make sure you don't die.

@@ +731,5 @@
> +    mozilla::ipc::BackgroundChild::GetForCurrentThread();
> +  if (actor) {
> +    ActorCreated(actor);
> +  } else {
> +    mozilla::ipc::BackgroundChild::GetOrCreateForCurrentThread(this);

This can fail, though I don't know what you'd do to handle it. Maybe just MOZ_CRASH.

@@ +745,5 @@
> +void
> +MessagePort::ActorCreated(mozilla::ipc::PBackgroundChild* aActor)
> +{
> +  MOZ_ASSERT(aActor);
> +  MOZ_ASSERT(mState == eStateEntangling);

MOZ_ASSERT(!mActor)

@@ +813,5 @@
> +    if (obs) {
> +      obs->RemoveObserver(this, "inner-window-destroyed");
> +    }
> +
> +    Close();

It's not clear to me that this will always do the right thing... There are lots of states that affect how Close() behaves. Are you sure that this is enough?

@@ +834,5 @@
> +  if (!doc) {
> +    return;
> +  }
> +
> +  nsIBFCacheEntry* bfCacheEntry = doc->GetBFCacheEntry();

Should this use an nsCOMPtr?

::: dom/messagechannel/MessagePort.h
@@ +94,3 @@
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(MessagePort,
>                                             DOMEventTargetHelper)

MessagePortBase!

@@ +137,5 @@
>    ~MessagePort();
>  
> +  enum State {
> +    // When a port is created by a MessageChannel it is entangled with the
> +    // other. They both run on the same thread, save event loop and the

Nit: s/save/same/

@@ +199,5 @@
>  
> +  nsRefPtr<MessagePort> mUnshippedEntangledPort;
> +
> +  nsTArray<nsRefPtr<SharedMessagePortMessage>> mMessageQueue;
> +  nsTArray<nsRefPtr<SharedMessagePortMessage>> mMessageForTheOtherPortQueue;

How about "mMessages" and "mMessagesForTheOtherPort"?

@@ +209,1 @@
>    bool mMessageQueueEnabled;

Nit: Move this to the end with your other bool.

::: dom/messagechannel/MessagePortChild.cpp
@@ +10,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +MessagePortChild::MessagePortChild()

This could be inlined in the header, right?

@@ +18,5 @@
> +bool
> +MessagePortChild::RecvStopSendingDataConfirmed()
> +{
> +  MOZ_ASSERT(mPort);
> +  mPort->StopSendingDataConfirmed();

MOZ_ASSERT(!mPort) after this?

@@ +42,5 @@
> +bool
> +MessagePortChild::RecvClosed()
> +{
> +  if (mPort) {
> +    mPort->Closed();

Here too

::: dom/messagechannel/MessagePortChild.h
@@ +4,5 @@
> +
> +#ifndef mozilla_dom_MessagePortChild_h
> +#define mozilla_dom_MessagePortChild_h
> +
> +#include "mozilla/dom/PMessagePortChild.h"

Also include nsISupportsImpl.h (for NS_INLINE_DECL_REFCOUNTING) and mozilla/Assertions.h

@@ +10,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class MessagePort;
> +class PostMessageRunnable;

Not needed.

@@ +25,5 @@
> +    mPort = aPort;
> +  }
> +
> +  virtual bool
> +  RecvEntangled(nsTArray<MessagePortMessage>&& aMessages) override;

These IPDL messages can all be private.

@@ +35,5 @@
> +
> +  virtual bool RecvClosed() override;
> +
> +private:
> +  ~MessagePortChild() {

Nit: { on its own line

::: dom/messagechannel/MessagePortParent.cpp
@@ +51,5 @@
> +    return false;
> +  }
> +
> +  if (!mEntangled) {
> +    return true;

Is returning true (but discarding the messages) the right thing to do here? When can this happen? Discarding sounds like a bad idea... And we need to make sure we have a test that covers this.

@@ +60,5 @@
> +    return false;
> +  }
> +
> +  if (messages.IsEmpty()) {
> +    return false;

I would do both of these checks where you could return false before checking mEntangled.

@@ +77,5 @@
> +    return false;
> +  }
> +
> +  if (!mEntangled) {
> +    return true;

Same question here, is this desirable?

@@ +82,5 @@
> +  }
> +
> +  if (!mService) {
> +    NS_WARNING("Entangle is called after a shutdown!");
> +    return false;

Check this before checking mEntangled.

::: dom/messagechannel/MessagePortParent.h
@@ +14,5 @@
> +
> +class MessagePortParent final : public PMessagePortParent
> +{
> +public:
> +  MessagePortParent();

I think you should take a UUID here to set as mUUID, then make mUUID const.

@@ +20,5 @@
> +
> +  bool Entangle(const nsID& aUUID, const nsID& aDestinationUUID,
> +                const uint32_t& aSequenceID);
> +
> +  bool Entangled(const nsTArray<MessagePortMessage>& aMessages);

You ignore this return everywhere so just make it return void. Do the unused thing inside Entangled().

::: dom/messagechannel/MessagePortService.cpp
@@ +14,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +namespace {
> +StaticRefPtr<MessagePortService> gInstance;

This should just be a raw pointer. Your MessagePortParent objects should own the service exclusively.

@@ +20,5 @@
> +
> +class MessagePortServiceData final
> +{
> +public:
> +  MessagePortServiceData(const nsID& aDestinationUUID)

You might want to make a copy constructor that calls MOZ_COUNT_CTOR too, otherwise if you ever use the autogenerated one it will underflow the leak stats counter.

@@ +60,5 @@
> +  return gInstance;
> +}
> +
> +bool
> +MessagePortService::RequestEntangling(MessagePortParent* aParent,

MOZ_ASSERT(aParent)?

@@ +70,5 @@
> +  // If we don't have a MessagePortServiceData, we must create 2 of them for
> +  // both ports.
> +  if (!mPorts.Get(aParent->ID(), &data)) {
> +    // Create the MessagePortServiceData for the destination.
> +    if (mPorts.Get(aDestinationUUID, &data)) {

Pass nullptr here, not &data.

@@ +104,5 @@
> +    data->mParent = aParent;
> +    FallibleTArray<MessagePortMessage> array;
> +    if (!SharedMessagePortMessage::FromSharedToMessagesParent(aParent,
> +                                                               data->mMessages,
> +                                                               array)) {

Nit: indent is off by 1

@@ +109,5 @@
> +      return false;
> +    }
> +
> +    bool ok = aParent->Entangled(array);
> +    data->mMessages.Clear();

Clear() before calling Entangled()

@@ +122,5 @@
> +  if (!nextParent) {
> +    return false;
> +  }
> +
> +  nextParent->mSequenceID = aSequenceID;

Hm, I never really thought about it before, but what happens if something sends sequences out of order? Say |data->mSequenceID == 1|, and then you receive messages for SequenceID 3, then SequenceID 2. Those will go into your array out of order. Is that possible? Or something you should handle?

Seems like you could enforce order if you want:

  if (!data->mNextParents.IsEmpty() &&
      aSequenceID <= data->mNextParents[data->mNextParents.Length() - 1].>mSequenceID) {
    return false;
  }

Otherwise, if you want to allow this, you should insert in sorted order. That will let you use binary search in Disentangle.

@@ +135,5 @@
> +                  FallibleTArray<nsRefPtr<SharedMessagePortMessage>>& aMessages)
> +{
> +  MessagePortServiceData* data;
> +  if (!mPorts.Get(aParent->ID(), &data)) {
> +    MOZ_ASSERT(false, "Unknown MessagePortParent should not happend.");

Nit: s/happend/happen/

@@ +146,5 @@
> +  }
> +
> +  // Let's put the messages in the correct order. |aMessages| contains the
> +  // unsent messages so they have to go first.
> +  if (!aMessages.AppendElements(data->mMessages)) {

if aMessages is empty (likely, I bet) then you could SwapElements() instead to save allocation. And you wouldn't need to call Clear().

@@ +155,5 @@
> +
> +  ++data->mSequenceID;
> +
> +  // If we don't have a parent, we have to store the pending messages and wait.
> +  uint32_t nextParentId = 0;

Weird, this is your index counter in data->mNextParents... Call it "index"? "nextParentId" is very confusing.

@@ +172,5 @@
> +    return true;
> +  }
> +
> +  // Let's activate the new part with the pending messages.
> +  data->mMessages.Clear();

You just called this above.

@@ +227,5 @@
> +  if (data->mParent) {
> +    data->mParent->Close();
> +  }
> +
> +  for (uint32_t i = 0; i < data->mNextParents.Length(); ++i) {

range-for.

@@ +234,5 @@
> +
> +  nsID destinationUUID = data->mDestinationUUID;
> +  mPorts.Remove(aUUID);
> +
> +  CloseAll(destinationUUID);

After you call this you should be able to assert that the UUID doesn't exist as anyone else's destination, right? Just do a DEBUG-only EnumerateRead() looking for aUUID and MOZ_ASSERT if you see it.

@@ +240,5 @@
> +}
> +
> +// This service can be dismissed when there are not active ports.
> +void
> +MessagePortService::MaybeShutdown()

Hm, I don't think this is a good idea... You should probably never do this manually. Just null out gInstance in your destructor. That way the service is entirely owned by MessagePortParent objects. I'd just remove this method entirely.

@@ +265,5 @@
> +  }
> +
> +  MOZ_ALWAYS_TRUE(mPorts.Get(data->mDestinationUUID, &data));
> +
> +  if (!data->mMessages.AppendElements(aMessages)) {

You could swap here id data->mMessages is empty.

@@ +275,5 @@
> +    FallibleTArray<MessagePortMessage> messages;
> +    if (!SharedMessagePortMessage::FromSharedToMessagesParent(data->mParent,
> +                                                              data->mMessages,
> +                                                              messages)) {
> +      return true;

I think this is wrong... Everywhere else you return false for OOM, right?

@@ +279,5 @@
> +      return true;
> +    }
> +
> +    unused << data->mParent->SendReceiveData(messages);
> +    data->mMessages.Clear();

Clear before SendReceiveData

@@ +296,5 @@
> +  }
> +
> +  // This port has to be closed.
> +  if (data->mParent == aParent) {
> +    CloseAll(aParent->ID());

Since you call CloseAll regardless I think this flow would be clearer:

  if (data->mParent != aParent) {
    // Remove element in data->mNextParents...
  }
  CloseAll(aParent->ID());

@@ +301,5 @@
> +    return;
> +  }
> +
> +  // We don't want to send a message to this parent.
> +  for (uint32_t i = 0; i < data->mNextParents.Length(); ++i) {

range-for.

::: dom/messagechannel/MessagePortService.h
@@ +6,5 @@
> +#define mozilla_dom_MessagePortService_h
> +
> +#include "nsISupportsImpl.h"
> +#include "nsHashKeys.h"
> +#include "nsClassHashtable.h"

Nit: Alphabetize?

@@ +12,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class MessagePortParent;
> +class MessagePortServiceData;

Make this an inner class of MessagePortService?

@@ +20,5 @@
> +{
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(MessagePortService)
> +
> +  static MessagePortService* GetOrCreate();

This should really return already_AddRefed

@@ +34,5 @@
> +  bool ClosePort(MessagePortParent* aParent);
> +
> +  bool PostMessages(
> +                 MessagePortParent* aParent,
> +                 FallibleTArray<nsRefPtr<SharedMessagePortMessage>>& aMessages);

All these should probably take rvalue refs to avoid copies.

::: dom/workers/MessagePort.cpp
@@ +203,2 @@
>  {
>    NS_WARNING("Haven't implemented structured clone for these ports yet!");

Is this coming in a different bug?

::: dom/workers/WorkerPrivate.cpp
@@ +699,5 @@
> +      MOZ_ASSERT(aExtraData < closure->mMessagePortIdentifiers.Length());
> +
> +      ErrorResult rv;
> +      nsRefPtr<MessagePortBase> port =
> +        dom::MessagePort::Create(closure->mParentWindow,

dom:: shouldn't be needed

::: ipc/glue/BackgroundChildImpl.cpp
@@ +12,5 @@
>  #include "mozilla/dom/PBlobChild.h"
>  #include "mozilla/dom/cache/ActorUtils.h"
>  #include "mozilla/dom/indexedDB/PBackgroundIDBFactoryChild.h"
>  #include "mozilla/dom/ipc/BlobChild.h"
> +#include "mozilla/dom/MessagePortChild.h"

Nit: I think this should go between "mozilla/Assertions.h" and "mozilla/dom/PBlobChild.h"

::: ipc/glue/BackgroundParentImpl.cpp
@@ +16,5 @@
>  #include "mozilla/dom/indexedDB/ActorsParent.h"
>  #include "mozilla/dom/ipc/BlobParent.h"
>  #include "mozilla/ipc/BackgroundParent.h"
>  #include "mozilla/ipc/BackgroundUtils.h"
> +#include "mozilla/dom/MessagePortParent.h"

Nit: move just after "ContentParent.h"

Comment 132

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Just waiting on one interdiff to glance over (for review in comment 131). Then we should be all done!

Comment 133

[Andrea Marchesini [:baku]]

Attachment: rebased full patch

Comment 134

[Andrea Marchesini [:baku]]

> 
> @@ +98,5 @@
> > +}
> > +
> > +// A WorkerMainThreadRunnable to synchronously dispatch the call of
> > +// CheckPermission() from the worker thread to the main thread.
> > +class CheckPermissionRunnable final : public WorkerMainThreadRunnable
> 
> Hrm, I don't like this too much... Can't you check whether MessageChannel is
> enabled before you launch the worker? Like we do for IndexedDB (search for
> "mIndexedDBAllowed"). Then you wouldn't have to block every worker when it
> starts up...

we should this also for BroadcastChannel. Actually in any exposed API with prefs.
I prefer to keep what I implemented and remove the preference in the next cycle if it's OK for you.

> @@ +47,5 @@
> > +    : mPort(aPort)
> > +  { }
> > +
> > +  NS_IMETHOD
> > +  Run()
> 
> Hm, you should probably bail out if mPort is null (you got canceled)?

MOZ_ASSERT(mPort); Run cannot be called after a Cancel() right? I added a MOZ_ASSERT(mPort).

> 
> @@ -386,5 @@
> > -  // Custom unlink loop because this array contains nsRunnable objects
> > -  // which are not cycle colleactable.
> > -  for (uint32_t i = 0, len = tmp->mMessageQueue.Length(); i < len; ++i) {
> > -    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMessageQueue[i]->mPort);
> > -    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMessageQueue[i]->mSupportsArray);
> 
> This went unaddressed in a previous review (comment 88). Why aren't you
> traversing these anymore?

because SharedMessagePortMessage obj doesn't have nothing traversable. Just BlobImpl maybe, and they don't need to be traversed.

> > +      MessagePortBase *port = nullptr;
> 
> Nit: * on left, and do you need to initialize it if you check the return
> value of UNWRAP_OBJECT?

just to avoid warning messages.

> 
> @@ +405,1 @@
> >      return;
> 
> Hm, can you check this earlier and avoid all the work you just did?

No, if you transfer things, they have to be neutered also if the operation fails.
 
> 
> @@ +417,5 @@
> > +  if (mNextStep != eNextStepNone) {
> > +    return;
> > +  }
> > +
> > +  RemoveDocFromBFCache();
> 
> Hm, you did several early returns above, are you sure you shouldn't call
> this no matter what?

Yes, because we remove the doc from BFCache only if we have fully received a message. If the operation failed before, we don't do this step.

> 
> @@ +542,2 @@
> >  void
> > +MessagePort::Entangled(const nsTArray<MessagePortMessage>& aMessages)
> 
> If this took an rvalue arg then you could avoid copying the message data...
> We should look into this now that IPDL hands out rvalue refs.

I still have to copy it if it contains blobs right?
 

> 
> @@ +567,5 @@
> > +    Close();
> > +    return;
> > +  }
> > +
> > +  mMessageQueue.AppendElements(data);
> 
> Can you SwapElements here? mMessageQueue should be empty, right?

We populate the messageQueue directly if the 2 ports are not shipped yet. So no. There is a test for this case.

> 
> @@ +653,5 @@
> > +{
> > +  aIdentifier.mNeutered = true;
> > +
> > +  if (mState > eStateEntangled) {
> > +    return true;
> 
> This is weird, you will return true, but you haven't set all the members of
> aIdentifier yet. In at least one place it is passed in uninitialized, so
> you'll have garbage there...

The issue here is this: what about if you transfer a port that has been already transfer? This is supported by the spec. The port will be neutered also on the other side. I don't need to populate all the rest of aIdentifier. A comment can help.

> 
> @@ +813,5 @@
> > +    if (obs) {
> > +      obs->RemoveObserver(this, "inner-window-destroyed");
> > +    }
> > +
> > +    Close();
> 
> It's not clear to me that this will always do the right thing... There are
> lots of states that affect how Close() behaves. Are you sure that this is
> enough?

Close should be able to do the correct operation. In case we are entangled, it sends a message to the parent for instance.
Yeah, it should be totally ok.

> 
> @@ +20,5 @@
> > +
> > +  bool Entangle(const nsID& aUUID, const nsID& aDestinationUUID,
> > +                const uint32_t& aSequenceID);
> > +
> > +  bool Entangled(const nsTArray<MessagePortMessage>& aMessages);
> 
> You ignore this return everywhere so just make it return void. Do the unused
> thing inside Entangled().

I'm not ignoring it. I use it in RequestEntangling.
 
> ::: dom/messagechannel/MessagePortService.cpp
> @@ +14,5 @@
> > +namespace mozilla {
> > +namespace dom {
> > +
> > +namespace {
> > +StaticRefPtr<MessagePortService> gInstance;
> 
> This should just be a raw pointer. Your MessagePortParent objects should own
> the service exclusively.

No because it can happen that I don't have MessagePortParents alive. When a port is sent to another thread, the current MessagePort can do all the operation to destroy the MessagePort before that the other side creates the MessagePortParent.

> Hm, I never really thought about it before, but what happens if something
> sends sequences out of order? Say |data->mSequenceID == 1|, and then you
> receive messages for SequenceID 3, then SequenceID 2. Those will go into
> your array out of order. Is that possible? Or something you should handle?

I handle it. The parent can be also in the wrong order. This is absolutely impossible to test because it means that 1 thread is busy doing something but it's able to send the port to another thread before entangling it.

> Otherwise, if you want to allow this, you should insert in sorted order.
> That will let you use binary search in Disentangle.

I really hope we have 1 pending parent, max 2. No more.

> 
> @@ +146,5 @@
> > +  }
> > +
> > +  // Let's put the messages in the correct order. |aMessages| contains the
> > +  // unsent messages so they have to go first.
> > +  if (!aMessages.AppendElements(data->mMessages)) {
> 
> if aMessages is empty (likely, I bet) then you could SwapElements() instead
> to save allocation. And you wouldn't need to call Clear().

It can happen that aMessage is not empty. There is a test for this too. Think about this scenario:

for(100 times) messagePort.postMessage(42);
... after the first delivery:
worker.postMessage("", [ messagePort]);


> @@ +265,5 @@
> > +  }
> > +
> > +  MOZ_ALWAYS_TRUE(mPorts.Get(data->mDestinationUUID, &data));
> > +
> > +  if (!data->mMessages.AppendElements(aMessages)) {
> 
> You could swap here id data->mMessages is empty.

Same here: it can happen that the destination is not ready to receive data.

> @@ +301,5 @@
> > +    return;
> > +  }
> > +
> > +  // We don't want to send a message to this parent.
> > +  for (uint32_t i = 0; i < data->mNextParents.Length(); ++i) {
> 
> range-for.

I have to remove items. Can I use range-for here too?

> 
> ::: dom/workers/MessagePort.cpp
> @@ +203,2 @@
> >  {
> >    NS_WARNING("Haven't implemented structured clone for these ports yet!");
> 
> Is this coming in a different bug?

I think we don't want to support this. Otherwise we can do something like a sharedWorker sending its own port to a different thread.

> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +699,5 @@
> > +      MOZ_ASSERT(aExtraData < closure->mMessagePortIdentifiers.Length());
> > +
> > +      ErrorResult rv;
> > +      nsRefPtr<MessagePortBase> port =
> > +        dom::MessagePort::Create(closure->mParentWindow,
> 
> dom:: shouldn't be needed

It conflicts with workers::MessagePort.

Comment 135

[Andrea Marchesini [:baku]]

Attachment: interdiff

I still have to apply your suggestions from comment 130. I want to talk with you on IRC about that before.

Comment 136

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8610128 [details] [diff] [review]
rebased full patch

Review of attachment 8610128 [details] [diff] [review]:
-----------------------------------------------------------------

This as is doesn't compile. With those two changes it compiles correctly.

::: dom/workers/WorkerStructuredClone.h
@@ +15,5 @@
> +namespace dom {
> +
> +class MessagePortBase;
> +
> +class workers {

namespace workers?

@@ +21,5 @@
> +// This class is implemented in WorkerPrivate.cpp
> +class WorkerStructuredCloneClosure final
> +{
> +private:
> +  WorkerStructuredCloneClosure(const WorkerStructuredCloneClosure&) delete;

= delete

Comment 137

[Andrea Marchesini [:baku]]

Attachment: rebased full patch

Comment 139

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8610156 [details] [diff] [review]
interdiff

Review of attachment 8610156 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/messagechannel/MessagePortService.cpp
@@ +29,5 @@
>      MOZ_COUNT_CTOR(MessagePortServiceData);
>    }
>  
> +  MessagePortServiceData(const MessagePortServiceData& aOther) = delete;
> +  MessagePortServiceData& operator=(const MessagePortServiceData&) = delete;

It doesn't really matter but this feels like it should be in a private: section

@@ +74,5 @@
>    // If we don't have a MessagePortServiceData, we must create 2 of them for
>    // both ports.
>    if (!mPorts.Get(aParent->ID(), &data)) {
>      // Create the MessagePortServiceData for the destination.
> +    if (mPorts.Get(aDestinationUUID, nullptr)) {

I think you can just leave off the second arg.

@@ +248,5 @@
>  
>    CloseAll(destinationUUID);
> +
> +#ifdef DEBUG
> +  mPorts.EnumerateRead(CloseAllDebugCheck, const_cast<nsID*>(&aUUID));

You could use a lambda for this too, but the way you have it is fine also.

@@ +315,5 @@
> +    // We don't want to send a message to this parent.
> +    for (uint32_t i = 0; i < data->mNextParents.Length(); ++i) {
> +      if (aParent == data->mNextParents[i].mParent) {
> +       data->mNextParents.RemoveElementAt(i);
> +       break;

Nit: looks like indent is off here

Comment 140

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8610514 [details] [diff] [review]
rebased full patch

This looks great! r=me.

Comment 141

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8610514 [details] [diff] [review]
rebased full patch

Er, wait, I guess there's one more interdiff coming (per comment 135) right?

Comment 142

[Noemí Freire (:noemi)]

setting v2 dependency. Thanks!

Comment 143

[Andrea Marchesini [:baku]]

Attachment: interdiff

Comment 144

[Andrea Marchesini [:baku]]

Attachment: interdiff 2

Comment 145

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8621033 [details] [diff] [review]
interdiff

Review of attachment 8621033 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentUtils.cpp
@@ +7122,5 @@
>    nsresult rv;
> +
> +  // We can store this nsIUUDGenerator and use it everywhere because it is
> +  // thread-safe.
> +  if (!sUUIDGenerator) {

This is a race, but you're fixing in another interdiff.

::: dom/messagechannel/MessagePortUtils.cpp
@@ +31,5 @@
>    nsTArray<nsRefPtr<MessagePortBase>> mTransferredPorts;
>  };
>  
> +struct
> +StructuredCloneClosureInternalReadOnly

MOZ_STACK_CLASS right?

Nit: normally "struct" goes on the same line

Comment 146

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8621057 [details] [diff] [review]
interdiff 2

Review of attachment 8621057 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 147

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok, I'll stamp the final patch as soon as you get it up!

Comment 148

[Andrea Marchesini [:baku]]

Attachment: full patch

Comment 149

[Andrea Marchesini [:baku]]

Attachment: id.patch

Comment 150

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8621548 [details] [diff] [review]
full patch

Great! Looks good. r=me.

Comment 151

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8621550 [details] [diff] [review]
id.patch

Looks fine.

Comment 152

[Andrea Marchesini [:baku]]

Attachment: PostMessageEvent patch

Comment 153

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8621649 [details] [diff] [review]
PostMessageEvent patch

Review of attachment 8621649 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/PostMessageEvent.h
@@ +7,5 @@
> +#ifndef mozilla_dom_PostMessageEvent_h
> +#define mozilla_dom_PostMessageEvent_h
> +
> +#include "js/StructuredClone.h"
> +#include "nsAutoPtr.h"

Not needed?

Though you should add nsCOMPtr

@@ +45,5 @@
> +    mSupportsArray.AppendElement(aSupports);
> +    return true;
> +  }
> +
> +  bool Write(JSContext* aCx, JS::Handle<JS::Value> aMessage,

This is the only method that you want to expose besides the constructor and runnable stuff. Make the rest private? To do so you'll have to make the structured clone functions private static functions of PostMessageEvent, but that seems like a fair tradeoff.

Comment 154

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d67d747b3eb

Comment 155

[Ryan VanderMeulen [:RyanVM]]

Backed out for hitting semi-frequent test_post_message_advanced.html timeouts on linux64 debug e10s.
https://treeherder.mozilla.org/logviewer.html#?job_id=10776729&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/53a99d02925f

Comment 156

[Ryan VanderMeulen [:RyanVM]]

The timeouts were hitting linux64 and OSX opt builds as well.

Comment 157

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/964d75a56702

Comment 158

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/964d75a56702

Comment 159

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do we still need the MessageChannel::Enabled function? Can't we enable this everywhere by default now?

Comment 160

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, just saw bug 952139. I guess we can keep this code for now in case we need to emergency-disable it due to a security bug or some such.

Comment 161

[Andrea Marchesini [:baku]]

Release Note Request (optional, but appreciated)
[Why is this notable]: MessagePort API is a powerful API and finally it's enabled by default in firefox.
[Suggested wording]: MessageChannel and MessagePort API enabled

Comment 162

[Andrea Marchesini [:baku]]

(In reply to Jonas Sicking (:sicking) from comment #160)
> Ah, just saw bug 952139. I guess we can keep this code for now in case we
> need to emergency-disable it due to a security bug or some such.

Correct, I'll remove the preference and the MessageChannel::Enabled function in the next cycle.

Comment 163

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Release note added to Firefox 41.0a2

Comment 164

[Chris Mills [:cmills]]

"Available in workers" - information added to the following pages (and their associated member pages):

https://developer.mozilla.org/en-US/docs/Web/API/Channel_Messaging_API
https://developer.mozilla.org/en-US/docs/Web/API/MessagePort
https://developer.mozilla.org/en-US/docs/Web/API/MessageChannel
https://developer.mozilla.org/en-US/docs/Web/API/Channel_Messaging_API/Using_channel_messaging

I've also added it into the following:

https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers

and checked that a note has been added to the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/41