Last Comment Bug 677638 - (MessageChannel) Implement HTML5's channel messaging API
: (MessageChannel) Implement HTML5's channel messaging API
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
-- normal with 13 votes (vote)
: mozilla26
Assigned To: Andrea Marchesini [:baku]
:
: Andrew Overholt [:overholt]
Mentors:
: 741618 (view as bug list)
Depends on: 984990 984991 1140776
Blocks: 914974 911972 914430 952139 983928
  Show dependency treegraph
 
Reported: 2011-08-09 12:00 PDT by James Leigh
Modified: 2015-03-10 10:13 PDT (History)
39 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
WIP 1 - Interfaces (12.65 KB, patch)
2013-08-23 03:29 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
WIP 2 - Structured Clone Algorithm (9.82 KB, patch)
2013-08-23 03:30 PDT, Andrea Marchesini [:baku]
bugs: feedback+
Details | Diff | Splinter Review
WIP 3 - PostMessage (122 bytes, patch)
2013-08-23 03:30 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
WIP 3 - PostMessage (11.85 KB, patch)
2013-08-23 03:33 PDT, Andrea Marchesini [:baku]
bugs: feedback-
Details | Diff | Splinter Review
patch 1 - WebIDL interfaces (12.65 KB, patch)
2013-08-26 10:25 PDT, Andrea Marchesini [:baku]
bugs: review+
Details | Diff | Splinter Review
patch 2 - structured clone algorithm (9.82 KB, patch)
2013-08-26 10:26 PDT, Andrea Marchesini [:baku]
bugs: review+
Details | Diff | Splinter Review
patch 3 - postMessage (16.08 KB, patch)
2013-08-26 10:27 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 4 - Start() method (16.41 KB, patch)
2013-08-26 10:29 PDT, Andrea Marchesini [:baku]
bugs: review-
Details | Diff | Splinter Review
patch 5 - unshipped message port queue (2.50 KB, patch)
2013-08-27 04:34 PDT, Andrea Marchesini [:baku]
bugs: review-
Details | Diff | Splinter Review
patch 3 - postMessage (16.11 KB, patch)
2013-08-27 05:44 PDT, Andrea Marchesini [:baku]
bugs: review+
Details | Diff | Splinter Review
patch 4 - Start() method (16.42 KB, patch)
2013-08-28 07:45 PDT, Andrea Marchesini [:baku]
bugs: review+
Details | Diff | Splinter Review
patch 5 - cycle collection and timer (9.36 KB, patch)
2013-08-28 07:46 PDT, Andrea Marchesini [:baku]
bugs: review-
Details | Diff | Splinter Review
patch 6 - unshipped message port queue (3.80 KB, patch)
2013-08-28 07:48 PDT, Andrea Marchesini [:baku]
bugs: review+
Details | Diff | Splinter Review
patch 5 - cycle collection and runnable (9.75 KB, patch)
2013-08-29 06:08 PDT, Andrea Marchesini [:baku]
bugs: review+
Details | Diff | Splinter Review
patch 1 - WebIDL interfaces (12.64 KB, patch)
2013-08-29 15:54 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 2 - structured clone algorithm (9.85 KB, patch)
2013-08-29 15:55 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 - postMessage (16.15 KB, patch)
2013-08-29 15:56 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 4 - Start() method (16.42 KB, patch)
2013-08-29 15:59 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 5 - cycle collection and runnable (9.55 KB, patch)
2013-08-29 16:00 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 6 - unshipped message port queue (3.96 KB, patch)
2013-08-29 16:00 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 7 - pref (21.91 KB, patch)
2013-09-03 03:29 PDT, Andrea Marchesini [:baku]
bugs: review+
Details | Diff | Splinter Review
patch 7 - pref (21.56 KB, patch)
2013-09-03 03:46 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review

Description User image James Leigh 2011-08-09 12:00:46 PDT
User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/534.30 (KHTML, like Gecko) Ubuntu/11.04 Chromium/12.0.742.112 Chrome/12.0.742.112 Safari/534.30

Steps to reproduce:

Channel messaging appears to be unimplemented.

The example code from the HTML5 specification produces an error. The line of code "var channel = new MessageChannel();" from the HTML5 spec below causes an exception in the error console.

http://dev.w3.org/html5/postmsg/#channel-messaging


Actual results:

"Error: MessageChannel is not defined" appears in the error console


Expected results:

Independent pieces of code (e.g. running in different browsing contexts) should be able to communicate directly using channel messaging.
Comment 1 User image Olli Pettay [:smaug] 2011-08-10 01:30:32 PDT
Use postMessage?

Just curious,
do you have some real world use case for MessageChannel's ports?
Comment 2 User image James Leigh 2011-08-10 05:54:13 PDT
When writing a reusable js library it is important not to clobber other messages that might be intended for other libraries within the page. By using a message channel all communication within that channel can be controlled.

By restricting all communication to previously authorized message channels, the chances of an malicious message received from an untrusted source is reduced.

It's in the HTML5 spec draft and two implementations already exist: WebKit and Opera.
Comment 3 User image Olli Pettay [:smaug] 2011-08-10 07:07:26 PDT
yeah, I can see use cases for ports, it is mainly the
MessageChannel I'm curious about. Since, IMO, MessageChannel is rather
awkward and it would be probably easier to have
var portToOtherWindow = otherWindow.createPort("name");
or some such.
Comment 4 User image Ioana (away) 2011-09-21 04:38:16 PDT
This issue reproduces exactly as in the bug description on:
Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110920 Firefox/9.0a1 (20110920085517).
Comment 5 User image Florian Bender 2012-04-02 09:15:23 PDT
I agree with :smaug. This will make it a lot easier to use MessageChannel. Plus, it should provide better security by design, IMO.
Comment 6 User image Yehuda Katz 2012-09-27 10:42:02 PDT
This is also implemented in Internet Explorer[1][2].

[1] http://msdn.microsoft.com/en-us/library/ie/hh673525(v=vs.85).aspx
[2] http://msdn.microsoft.com/en-us/library/windows/apps/hh441303.aspx
Comment 8 User image Mike Williamson 2013-04-13 13:26:55 PDT
Hi - this bug has been dormant for awhile, I was wondering if there were any plans for it?

We're building a fairly large, HTML5 app, which uses multiple webworkers, and the only way we've found for one worker to communicate with another so far is to pass each worker a connected MessagePort.

I'm assuming this bug needs engineering resources?  It seems like implementing the entire channeling API would be fairly non-trivial, but we'd love to expand our platform base beyond just Chrome.
Comment 9 User image Brendan Eich [:brendan] 2013-08-03 19:45:44 PDT
Cc'ing Johnny and Jonas - can we get this bug assigned? Looks like it is _de riguer_.

See also https://news.ycombinator.com/item?id=6154170.

/be
Comment 10 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-08-04 00:08:52 PDT
Johnny, anyone on the DOM team that can help? I'm reluctant to pile more stuff on Khuey or Bent given that we already have a number of critical things depending on them.
Comment 11 User image Mounir Lamouri (:mounir) 2013-08-05 04:29:10 PDT
I hope to be able to get (part of) this feature implemented as a side effect of bug 876397.
Comment 12 User image Andrew Overholt [:overholt] 2013-08-07 18:47:17 PDT
baku will likely have time when he gets back from vacation
Comment 13 User image Johnny Stenback (:jst, jst@mozilla.com) 2013-08-08 12:27:16 PDT
Over to baku per Andrew's comment. Andrea, if you don't have time once you're back, let me and Andrew know and we'll hunt down other resources to help here.
Comment 14 User image Andrea Marchesini [:baku] 2013-08-20 13:30:16 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #13)
> Over to baku per Andrew's comment. Andrea, if you don't have time once
> you're back, let me and Andrew know and we'll hunt down other resources to
> help here.

I start working on it tomorrow. Sounds fun.
Comment 15 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-20 13:35:43 PDT
Please make sure you look at bug 907060, we need to make MessagePort objects pretty flexible.
Comment 16 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-08-23 00:51:21 PDT
There are also some challenges here when it comes to worker lifetimes. Bent should be able to explain the problem as well as a potential solution.
Comment 17 User image Andrea Marchesini [:baku] 2013-08-23 03:29:30 PDT
Created attachment 794588 [details] [diff] [review]
WIP 1 - Interfaces
Comment 18 User image Andrea Marchesini [:baku] 2013-08-23 03:30:12 PDT
Created attachment 794589 [details] [diff] [review]
WIP 2 - Structured Clone Algorithm
Comment 19 User image Andrea Marchesini [:baku] 2013-08-23 03:30:46 PDT
Created attachment 794590 [details] [diff] [review]
WIP 3 - PostMessage
Comment 20 User image Andrea Marchesini [:baku] 2013-08-23 03:33:15 PDT
Created attachment 794591 [details] [diff] [review]
WIP 3 - PostMessage
Comment 21 User image Olli Pettay [:smaug] 2013-08-23 04:08:16 PDT
Comment on attachment 794591 [details] [diff] [review]
WIP 3 - PostMessage


>+
>+  // We can't simply call dispatchEvent on the window because doing so ends
>+  // up flipping the trusted bit on the event, and we don't want that to
>+  // happen because then untrusted content can call postMessage on a chrome
>+  // window if it can get a reference to it.
content sure shouldn't get access to chrome window.

>+
>+  nsIPresShell *shell = doc->GetShell();
>+  nsRefPtr<nsPresContext> presContext;
>+  if (shell) {
>+    presContext = shell->GetPresContext();
>+  }


>+
>+  message->SetTrusted(mTrusted);
These events should be always trusted.

>+
>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  nsEventDispatcher::Dispatch(mPort,
>+                              presContext,
>+                              message->GetInternalNSEvent(),
>+                              message,
>+                              &status);
Just use the normal DispatchEvent on mPort


But I see this is existing code in nsGlobalWindow and that code is on crack, and yes, apparently I did review it.
Comment 22 User image Andrea Marchesini [:baku] 2013-08-26 10:25:28 PDT
Created attachment 795513 [details] [diff] [review]
patch 1 - WebIDL interfaces
Comment 23 User image Andrea Marchesini [:baku] 2013-08-26 10:26:48 PDT
Created attachment 795515 [details] [diff] [review]
patch 2 - structured clone algorithm

This second patch makes MessagePort transferable via window.postMessage.
Comment 24 User image Andrea Marchesini [:baku] 2013-08-26 10:27:50 PDT
Created attachment 795516 [details] [diff] [review]
patch 3 - postMessage

Implementation of postMessage in MessagePort
Comment 25 User image Andrea Marchesini [:baku] 2013-08-26 10:29:01 PDT
Created attachment 795518 [details] [diff] [review]
patch 4 - Start() method

This patch implements start() method and the queue of events.
Comment 26 User image Andrea Marchesini [:baku] 2013-08-27 04:34:08 PDT
Created attachment 795968 [details] [diff] [review]
patch 5 - unshipped message port queue

If this test covers all the unshipped message port queue usage, then we don't have to implement anything more than what we already have.
Comment 27 User image :Ms2ger (⌚ UTC+1/+2) 2013-08-27 04:41:57 PDT
Comment on attachment 795513 [details] [diff] [review]
patch 1 - WebIDL interfaces

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

::: dom/base/MessagePort.h
@@ +28,5 @@
> +
> +  nsPIDOMWindow*
> +  GetParentObject() const
> +  {
> +     return GetOwner();

We don't usually do 3-space indentation
Comment 28 User image :Ms2ger (⌚ UTC+1/+2) 2013-08-27 04:45:58 PDT
Comment on attachment 795516 [details] [diff] [review]
patch 3 - postMessage

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

::: dom/base/MessagePort.cpp
@@ +216,5 @@
> +  }
> +
> +  nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(doc);
> +  nsCOMPtr<nsIDOMEvent> event;
> +  domDoc->CreateEvent(NS_LITERAL_STRING("MessageEvent"), getter_AddRefs(event));

Call the version on nsIDocument

@@ +218,5 @@
> +  nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(doc);
> +  nsCOMPtr<nsIDOMEvent> event;
> +  domDoc->CreateEvent(NS_LITERAL_STRING("MessageEvent"), getter_AddRefs(event));
> +  if (!event)
> +    return NS_OK;

{}

@@ +286,5 @@
> +  scInfo.event = event;
> +  scInfo.port = this;
> +
> +  JS::Rooted<JS::Value> transferable(aCx, aTransfer.WasPassed()
> +                                            ? aTransfer.Value() : JSVAL_VOID);

Why not

  JS::Handle<JS::Value> transferable = aTransfer.WasPassed()
                                       ? aTransfer.Value()
                                       : JS::UndefinedHandleValue;
Comment 29 User image Andrea Marchesini [:baku] 2013-08-27 05:44:35 PDT
Created attachment 795997 [details] [diff] [review]
patch 3 - postMessage
Comment 30 User image Mark Hammond [:markh] 2013-08-27 05:56:59 PDT
*** Bug 741618 has been marked as a duplicate of this bug. ***
Comment 31 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-27 07:21:12 PDT
Note that you want to avid JSVAL_* no matter what.  Use JS::UndefinedValue() instead if you need that value.
Comment 32 User image Olli Pettay [:smaug] 2013-08-27 10:02:06 PDT
Comment on attachment 795513 [details] [diff] [review]
patch 1 - WebIDL interfaces

>+class MessagePort MOZ_FINAL : public nsDOMEventTargetHelper
>+{
>+public:
>+  NS_DECL_ISUPPORTS_INHERITED
>+  NS_REALLY_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper)
>+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(MessagePort,
>+                                           nsDOMEventTargetHelper)
>+
>+  MessagePort(nsPIDOMWindow* aWindow);
>+  ~MessagePort();
>+
>+  nsPIDOMWindow*
>+  GetParentObject() const
>+  {
>+     return GetOwner();
>+  }
nsDOMEventTargetHelper has already GetParentObject
Comment 33 User image Olli Pettay [:smaug] 2013-08-27 10:28:26 PDT
Comment on attachment 795515 [details] [diff] [review]
patch 2 - structured clone algorithm

PostMessageRead/WriteStructuredClone will need some clean ups at some point
Comment 34 User image Olli Pettay [:smaug] 2013-08-27 11:48:12 PDT
Comment on attachment 795513 [details] [diff] [review]
patch 1 - WebIDL interfaces


>+class MessageChannel MOZ_FINAL : public nsISupports
>+                               , public nsWrapperCache
MessageChannel wouldn't have to inherit nsISupports, but
doesn't matter much.
Comment 35 User image Olli Pettay [:smaug] 2013-08-27 13:00:17 PDT
Comment on attachment 795968 [details] [diff] [review]
patch 5 - unshipped message port queue

Please add also a test for the case when one of the ports isn't started yet, so
that messages to it don't go to the unshipped port queue.

And don't just post messages in a loop, but add some timeouts and such.
Comment 36 User image Olli Pettay [:smaug] 2013-08-27 13:44:24 PDT
Comment on attachment 795997 [details] [diff] [review]
patch 3 - postMessage

>+struct StructuredCloneInfo {
{ to the next line.

>+  PostMessageRunnable* event;
>+  MessagePort* port;
mEvent, mPort
(I know, we're not super consistent when naming things in structs.)


> MessagePort::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
>-                         const Optional<JS::Handle<JS::Value> >& aTransfer)
>+                         const Optional<JS::Handle<JS::Value> >& aTransfer,
>+                         ErrorResult& aRv)
> {
This doesn't handle the queue, but I think it will be added in some other patch...
Comment 37 User image Olli Pettay [:smaug] 2013-08-27 14:18:21 PDT
Comment on attachment 795518 [details] [diff] [review]
patch 4 - Start() method

># HG changeset patch
># Parent a4af4ae5eae7963cf16c769fca696688389947b3
># User Andrea Marchesini <amarchesini@mozilla.com>
>Bug 677638 - Start() method
>
>diff --git a/dom/base/MessagePort.cpp b/dom/base/MessagePort.cpp
>--- a/dom/base/MessagePort.cpp
>+++ b/dom/base/MessagePort.cpp
>@@ -21,40 +21,49 @@
> namespace mozilla {
> namespace dom {
> 
> class PostMessageRunnable : public nsRunnable
> {
>   public:
>     NS_DECL_NSIRUNNABLE
> 
>-    PostMessageRunnable(MessagePort* aPort)
>-      : mPort(aPort)
>-      , mMessage(nullptr)
>+    PostMessageRunnable()
>+      : mMessage(nullptr)
>       , mMessageLen(0)
>     {
>     }
> 
>     ~PostMessageRunnable()
>     {
>-      NS_ASSERTION(!mMessage, "Message should have been deserialized!");
>+      // Ensure that the buffer is freed
>+      if (mMessage) {
>+        JSAutoStructuredCloneBuffer buffer;
>+        buffer.adopt(mMessage, mMessageLen);
>+      }
>     }
> 
>     void SetJSData(JSAutoStructuredCloneBuffer& aBuffer)
>     {
>       NS_ASSERTION(!mMessage && mMessageLen == 0, "Don't call twice!");
>       aBuffer.steal(&mMessage, &mMessageLen);
>     }
> 
>     bool StoreISupports(nsISupports* aSupports)
>     {
>       mSupportsArray.AppendElement(aSupports);
>       return true;
>     }
> 
>+    void Dispatch(MessagePort* aPort)
>+    {
>+      mPort = aPort;
>+      NS_DispatchToCurrentThread(this);
>+    }
>+
>   private:
>     nsRefPtr<MessagePort> mPort;
>     uint64_t* mMessage;
>     size_t mMessageLen;
> 
>     nsTArray<nsCOMPtr<nsISupports> > mSupportsArray;
> };
> 
>@@ -176,16 +185,18 @@ JSStructuredCloneCallbacks kPostMessageC
>   nullptr
> };
> 
> } // anonymous namespace
> 
> NS_IMETHODIMP
> PostMessageRunnable::Run()
> {
>+  MOZ_ASSERT(mPort);
>+
>   // Ensure that the buffer is freed even if we fail to post the message
>   JSAutoStructuredCloneBuffer buffer;
>   buffer.adopt(mMessage, mMessageLen);
>   mMessage = nullptr;
>   mMessageLen = 0;
> 
>   // Get the JSContext for the target window
>   nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(mPort->GetOwner());
>@@ -235,27 +246,28 @@ PostMessageRunnable::Run()
> 
>   message->SetTrusted(true);
> 
>   bool status;
>   mPort->DispatchEvent(event, &status);
>   return status ? NS_OK : NS_ERROR_FAILURE;
> }
> 
>-NS_IMPL_CYCLE_COLLECTION_INHERITED_1(MessagePort, nsDOMEventTargetHelper,
>-                                     mEntangledPort)
>+NS_IMPL_CYCLE_COLLECTION_INHERITED_2(MessagePort, nsDOMEventTargetHelper,
>+                                     mEntangledPort, mMessageQueue)
So the queue contains PostMessageRunnable objects, which aren't cycle collectable objects.
Those runnables may keep some nsISupports objects alive, so they should be cycle collected.
Perhaps you could just manually traverse/unlink the members of the PostMessageRunnable objects.
(nsRunnable has threadsafe refcounting so we may not want make PostMessageRunnable itself cycle collectable)


> MessagePort::Start()
> {
>-  // TODO
>+  if (mMessageQueueEnabled) {
>+    return;
>+  }
>+
>+  mMessageQueueEnabled = true;
>+
>+  while (!mMessageQueue.IsEmpty()) {
>+    nsRefPtr<PostMessageRunnable> event = mMessageQueue.ElementAt(0);
>+    mMessageQueue.RemoveElementAt(0);
>+
>+    event->Dispatch(this);
>+  }
Is this quite right? What should happen if some message event listener transfer the port to another location?

"4. Move all the events in the port message queue of original port to the port message queue of new port, if any, leaving the new port's port message queue in its initial disabled state."



Could you also post a patch which contains all the changes.
Comment 38 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-08-28 02:58:22 PDT
Andrea: I'm worried about landing support for MessageChannel without also supporting passing ports to Workers. If we do that it becomes a lot more complicated for authors to test if MessageChannel-to-workers is supported or not since they can't simply do

if ("MessageChannel" in window) {
  ...
}

Do you have plans for implementing MessageChannel support for workers as well? It would be super awesome if you did.
Comment 39 User image Olli Pettay [:smaug] 2013-08-28 03:31:11 PDT
Yes, I was thinking that too yesterday, and I think we will need to have worker support too.
Comment 40 User image Andrea Marchesini [:baku] 2013-08-28 07:40:19 PDT
> Do you have plans for implementing MessageChannel support for workers as
> well? It would be super awesome if you did.

Yes. It's part of the plan. I just wrote the first part about window-to-window but I'm going to implement MessagePort/MessageChannel for workers as well. My plan is to land main-thread and workers patches all together.
Comment 41 User image Andrea Marchesini [:baku] 2013-08-28 07:45:38 PDT
Created attachment 796666 [details] [diff] [review]
patch 4 - Start() method

Cycle collection and timer are implemented in the following patch. This fixes just mPort and mEvent.
Comment 42 User image Andrea Marchesini [:baku] 2013-08-28 07:46:53 PDT
Created attachment 796668 [details] [diff] [review]
patch 5 - cycle collection and timer

Here, we have cycle collection and timer, plus a test for the scenario you described:
an event listener that transfers the port.
Comment 43 User image Andrea Marchesini [:baku] 2013-08-28 07:48:59 PDT
Created attachment 796669 [details] [diff] [review]
patch 6 - unshipped message port queue

Tests for unshipped message port queue.
Comment 44 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-08-28 09:40:13 PDT
Andrea: You rock! :)
Comment 45 User image Olli Pettay [:smaug] 2013-08-28 10:35:00 PDT
Yes he does!

(And I'll review the new patches tomorrow.)
Comment 46 User image Olli Pettay [:smaug] 2013-08-29 05:32:07 PDT
Comment on attachment 796668 [details] [diff] [review]
patch 5 - cycle collection and timer


>+MessagePort::Dispatch()
>+{
>+  if (!mMessageQueueEnabled || mMessageQueue.IsEmpty() || mTimer) {
>+    return;
>+  }
> 
>-    event->Dispatch(this);
>+  nsRefPtr<PostMessageRunnable> event = mMessageQueue.ElementAt(0);
>+  mMessageQueue.RemoveElementAt(0);
>+
>+  event->Dispatch(this);
>+
>+  mTimer = do_CreateInstance("@mozilla.org/timer;1");
>+  if (mTimer) {
>+    mTimer->InitWithFuncCallback(TimerCallback, this, 0,
>+                                 nsITimer::TYPE_ONE_SHOT);
>   }
> }
Don't understand the use of timer. Also, this is unsafe use because timer isn't cancelled
when 'this' is deleted. TimerCallback can be called after 'this' is deleted and 
TimerCallback ends up accessing deleted object.
Use runnable if possible
Comment 47 User image Olli Pettay [:smaug] 2013-08-29 05:41:36 PDT
Comment on attachment 796669 [details] [diff] [review]
patch 6 - unshipped message port queue


>+      if (expectedNumber > 102) {
>+        runTests();
>+      }
number 102 could use some comment

>+    }
>+
>+    a.port1.onmessage = function(evt) {
>+      testEvent(evt.data, 2);
>+    };
>+
>+    a.port2.onmessage = function(evt) {
>+      testEvent(evt.data, 1);
>+    };
>+
>+    b.port2.onmessage = function(evt) {
>+      testEvent(evt.data, 3);
>+    };
Could you perhaps do b.port1.addEventListener("message", function() { ok(false, "shouldn't be called")})
Comment 48 User image Andrea Marchesini [:baku] 2013-08-29 06:08:05 PDT
Created attachment 797201 [details] [diff] [review]
patch 5 - cycle collection and runnable

Is this what you mean with nsRunnable? What about the cycle collection and the runnable...?
Comment 49 User image Olli Pettay [:smaug] 2013-08-29 14:36:08 PDT
Comment on attachment 797201 [details] [diff] [review]
patch 5 - cycle collection and runnable

>+    nsresult
>+    Run()
NS_IMETHOD Run()
                       mEntangledPort, mMessageQueue)
>+NS_IMPL_CYCLE_COLLECTION_CLASS(MessagePort)
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MessagePort,
>+                                                nsDOMEventTargetHelper)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mEntangledPort)
>+
>+  // Custom unlink loop because this array contains nsRunnable objects
>+  // which are not cycle colleactable.
>+  while (!tmp->mMessageQueue.IsEmpty()) {
>+    ImplCycleCollectionUnlink(tmp->mMessageQueue[0]->mPort);
>+    ImplCycleCollectionUnlink(tmp->mMessageQueue[0]->mSupportsArray);
>+    tmp->mMessageQueue.RemoveElementAt(0);
>+  }
>+
>+  if (tmp->mDispatchRunnable) {
>+    ImplCycleCollectionUnlink(tmp->mDispatchRunnable->mPort);
>+  }
Could you just use NS_IMPL_CYCLE_COLLECTION_UNLINK macro for everything.
NS_IMPL_CYCLE_COLLECTION_UNLINK(mMessageQueue[0]->mPort)
and so on


>+  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) {
>+    ImplCycleCollectionTraverse(cb, tmp->mMessageQueue[i]->mPort,
>+                                "tmp->mMessageQueue[i]->mPort", 0);
>+    ImplCycleCollectionTraverse(cb, tmp->mMessageQueue[i]->mSupportsArray,
>+                                "tmp->mMessageQueue[i]->mSupportsArray", 0);
>+  }
>+
>+  if (tmp->mDispatchRunnable) {
>+    ImplCycleCollectionTraverse(cb, tmp->mDispatchRunnable->mPort,
>+                                "tmp->mDispatchRunnable-->port", 0);
s/--/-/
>+  }
Use macros for traverse?
Comment 50 User image Andrea Marchesini [:baku] 2013-08-29 15:54:55 PDT
Created attachment 797544 [details] [diff] [review]
patch 1 - WebIDL interfaces

rebased.
Comment 51 User image Andrea Marchesini [:baku] 2013-08-29 15:55:44 PDT
Created attachment 797546 [details] [diff] [review]
patch 2 - structured clone algorithm

rebased
Comment 52 User image Andrea Marchesini [:baku] 2013-08-29 15:56:35 PDT
Created attachment 797547 [details] [diff] [review]
patch 3 - postMessage

rebased + comments
Comment 53 User image Andrea Marchesini [:baku] 2013-08-29 15:59:27 PDT
Created attachment 797548 [details] [diff] [review]
patch 4 - Start() method

rebased + comments
Comment 54 User image Andrea Marchesini [:baku] 2013-08-29 16:00:19 PDT
Created attachment 797550 [details] [diff] [review]
patch 5 - cycle collection and runnable
Comment 55 User image Andrea Marchesini [:baku] 2013-08-29 16:00:53 PDT
Created attachment 797551 [details] [diff] [review]
patch 6 - unshipped message port queue
Comment 56 User image Andrea Marchesini [:baku] 2013-09-03 03:29:09 PDT
Created attachment 798789 [details] [diff] [review]
patch 7 - pref
Comment 57 User image Olli Pettay [:smaug] 2013-09-03 03:40:25 PDT
Comment on attachment 798789 [details] [diff] [review]
patch 7 - pref

Could you use  Preferences::AddBoolVarCache for the pref
and then just return the current value in MessageChannel::PrefEnabled().
Calling GetBool all the time might show up in some odd profiles.

test_messageChannel_pref.html, please call SimpleTest.waitForExplicitFinish();
before running any tests.

I believe no need to add anything to all.js. Default for the pref should be false
Comment 58 User image Andrea Marchesini [:baku] 2013-09-03 03:46:33 PDT
Created attachment 798794 [details] [diff] [review]
patch 7 - pref
Comment 59 User image Andrea Marchesini [:baku] 2013-09-03 03:53:33 PDT
After a chat with smaug, we decided to land what we have here, disabled by pref.
The worker patch of MessagePort will be a follow up.

The reason why we decided this is because: here are already have 7 patches and it's hard to keep them up-to-date.
Plus, the worker world is changing a lot lately and MessagePort/MessageChannel uses code that is still under development.

The pref will be removed only when worker and main-thread MessagePort/Channel are fully implemented.
Comment 62 User image SUN Haitao 2013-09-03 23:25:12 PDT
It seems that the behavior of the current implementation is not compatible with the specification (and all other engines). 

The following code snippet works fine in all other engines, but causes a DataCloneError in Gecko:
    targetWindow.postMessage({}, '*', [channel.port2]);

And the following code which works fine in Gecko, causes a DataCloneError in all other engines:
    targetWindow.postMessage({ports:[channel.port2]}, '*');
Comment 63 User image Olli Pettay [:smaug] 2013-09-04 03:18:05 PDT
File a followup please. This stuff isn't finalized yet (nor enabled).
Comment 64 User image Andrea Marchesini [:baku] 2013-09-04 03:20:50 PDT
Yep, this is a follow up, but the spec says this:
http://www.whatwg.org/specs/web-apps/current-work/multipage/web-messaging.html#posting-messages

We should check what the other engines do and maybe file a bug to the spec... ?
Comment 65 User image Olli Pettay [:smaug] 2013-09-04 04:10:54 PDT
SUN Haitao, thanks for catching that bug!
Comment 66 User image SUN Haitao 2013-09-04 05:14:01 PDT
I've filed a separate bug for the behavior difference. See Bug 912456.
Comment 67 User image SUN Haitao 2013-09-04 05:25:37 PDT
(In reply to Olli Pettay [:smaug] from comment #65)
> SUN Haitao, thanks for catching that bug!

I'm working on a cross-engine inter-app communication framework (see https://github.com/sunhaitao/hail-them) which quite needs window-to-window message channel. Currently almost a half of all codes are used to make a hackish support for Gecko.
Comment 68 User image Asa Dotzler [:asa] 2013-09-13 12:34:48 PDT
Release note could look something like "Firefox now supports HTML5's channel messaging API."  Feel free to suggest better wording if you have it. Thanks.
Comment 69 User image Olli Pettay [:smaug] 2013-09-13 12:49:23 PDT
No. This is all disabled by default, see comment 59.
Comment 70 User image Lukas Blakk [:lsblakk] use ?needinfo 2013-09-13 13:07:16 PDT
Minusing as per comment 69, please nominate when this is getting preffed on by default.
Comment 71 User image Cyril 2013-09-17 15:08:37 PDT
We have developed a near-polyfill to support MessageChannel in Firefox and IE

https://github.com/tildeio/MessageChannel.js

It is used in a library that allows to communicate with embedded untrusted content

https://github.com/tildeio/oasis.js

(In reply to SUN Haitao from comment #67)
> (In reply to Olli Pettay [:smaug] from comment #65)
> > SUN Haitao, thanks for catching that bug!
> 
> I'm working on a cross-engine inter-app communication framework (see
> https://github.com/sunhaitao/hail-them) which quite needs window-to-window
> message channel. Currently almost a half of all codes are used to make a
> hackish support for Gecko.
Comment 72 User image SUN Haitao 2013-09-19 06:53:22 PDT
Even (In reply to Cyril from comment #71)
> We have developed a near-polyfill to support MessageChannel in Firefox and IE
> 
> https://github.com/tildeio/MessageChannel.js

Thanks. And I've already included a fallback solution that only use cross-document messaging. But an real implementation that works right should be much better.

> It is used in a library that allows to communicate with embedded untrusted
> content
> 
> https://github.com/tildeio/oasis.js

It seems that we are developing very similar solutions for almost the same problem: To use 'documents' as APIs in ANY modern browser engine. So anyone can introduce (many kinds of) new APIs that compatible with any engine without time consuming standardization. I believe it's important to make the Web first-class.
Comment 73 User image Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2013-11-23 07:29:20 PST
Since this is resolved, is there any other bug that will be resolved when feature becomes pref-ed on by default ?
Comment 74 User image Andrew Overholt [:overholt] 2013-11-29 12:30:12 PST
(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #73)
> Since this is resolved, is there any other bug that will be resolved when
> feature becomes pref-ed on by default ?

Andrea, do we have such a bug?  I don't see one, either.
Comment 75 User image Andrea Marchesini [:baku] 2014-02-05 06:03:57 PST
This bug fixes just a part of the MessageChannel/MessagePort bug.
We still need to fix bug 912456, bug 911972 and bug 917254.

Note You need to log in before you can comment on or make changes to this bug.