1174624 - [B2G] Create internal inter-app communication API in order to communicate in background thread.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mantaroh Yoshinaga[:mantaroh]]

This bug is fork from bug 1168271.

If BroadcastChannel API allow cross-origin communication in chrome code,
The keyboard apps is hopeful perfomance improvement.

The communication of Keyboard apps (Inputmethod API) is using chrome thread.
So current communication is consumed the main thread resource.
(If main thread is slow, affect keyboard experience)

It will avoid by using PBackground thread.(e.g. BroadcastChannel API).

BroadcastChannel API is not allow cross-origin communication.
(Keyboard app is communicate with keyboard app and content.)

So BroadcastChannel API have better to allow cross-origin communication.
However, it is used by internal apps only. 
So it should add [ChromeOnly] attribute.

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: WIP-bug1174624.diff

WIP:
Implemented BroadcastChannel.postMessageToChromeOnly method.
Its specific is as follow.
 - can called from chrome code.
   (added [ChromeOnly] attribute)
 - can receive message(posted postMessageToChromeOnly) to chrome only.
 - can communicate inter-apps.

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

Current implementation have some problem.
It only have one event listener in spite of it have 2 sending method.
(Receiving listener is 'onMessage'. sending method is 'postMessage' and 'postMessageToChromeOnly'.)

So I think that this method bring a some confusion.
Because it should be careful about caller method location.
e.g. When event listener registration called from chrome code, It registered as chrome listener which can receive postMessageToChromeOnly() message.

baku:
I think that should separate event listener like onMessageChromeMessage or add new API like BroadcastChanelInternal.

What do you think of this idea?

Comment 3

[Andrea Marchesini [:baku]]

> What do you think of this idea?

I think a clean way to implement this is:

[Constructor(DOMString channel),
 Exposed=(Window,Worker),
 Func="CrossOriginBroadcastChannel::IsEnabled"]
interface CrossOriginBroadcastChannel : BroadcastChannel {}

3 points:

1. I really would like to have a feedback from sicking about this new API
2. Maybe we should find a better name.
3. Sorry for the delay.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

baku,
Thank you for good idea!

sicking:
I'm going to implement new App-Communication API (like CrossOriginBroadcastChannel) in order to improve keyboard app.(bug 1168271)
Input-Method API is communicate with Content and App and Chrome process.

Already exist BroadcastChannel API, but BroadcastChannel is not support Cross-Origin communication.
So, I would like to expanding the BroadcastChannel API.

New API would be as follow.

[Constructor(DOMString channel),
 Exposed=(Window,Worker),
 Func="CrossOriginBroadcastChannel::IsEnabled"]
interface CrossOriginBroadcastChannel : BroadcastChannel {
  [ChromeOnly]
  attribute EventHandler oncrossoriginmessage;

  [ChromeOnly, Throws]
  void postChromeOriginMessage(any mesage);
};

What do you think of this new API?

Comment 5

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

Can't we simply let chrome code instantiate a BroadcastChannel instance, which would then broadcast to any other chrome pages? Why is a new interface needed?

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Jonas Sicking (:sicking) from comment #5)
> Can't we simply let chrome code instantiate a BroadcastChannel instance,
> which would then broadcast to any other chrome pages? Why is a new interface
> needed?

I would like to improve the keyboard app performance issue(bug 1168271).
However, BroadcastChannel is not support cross-origin.[1]
So I think it is better to expand the BroadcastChannel in order to communicate Keyboard App and Content App directly.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/broadcastchannel/BroadcastChannelParent.cpp#102

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga from comment #6)
> (In reply to Jonas Sicking (:sicking) from comment #5)
> > Can't we simply let chrome code instantiate a BroadcastChannel instance,
> > which would then broadcast to any other chrome pages? Why is a new interface
> > needed?
> 
> I would like to improve the keyboard app performance issue(bug 1168271).
> However, BroadcastChannel is not support cross-origin.[1]
> So I think it is better to expand the BroadcastChannel in order to
> communicate Keyboard App and Content App directly.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/broadcastchannel/
> BroadcastChannelParent.cpp#102

I misunderstood your comment(#comment 5).
Did you mean that BroadcastChannel API identify the caller?

- If caller is chrome, BroadcastChannel API allow cross-origin connection.
- If caller is not chrome, BroadcastChannel API disallow cross-origin connection.

I think it is feasible using 'nsContentUtils::IsCallerChrom'.[1]

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#2004

Comment 8

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

I was proposing something like that yes, but I don't actually think that would work.

But before we get into details, I need to better understand what you want to accomplish.

Do you want chrome-JS code in the child process that the keyboard app runs in, to exchange messages with chrome-JS in the parent process?

Or do you want chrome-JS in the child process that the keyboard app runs in, to exchange messages with chrome-JS in app child processes?

Note that either of these aren't really "cross origin" since it's the chrome origin talking to the chrome origin. I.e. it's not "cross origin", but rather "cross process".


Or do you want chrome-JS in the child or parent process to be able to talk to webpages in a child process?

Or do you want chrome-JS in the child or parent process to be able to talk to the system app?


Also, why do you want this? I.e. what is the problem with the current code that you are trying to solve?

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Jonas Sicking (:sicking) from comment #8)
> Also, why do you want this? I.e. what is the problem with the current code
> that you are trying to solve?

First, the issue that I want to resolve is keyboard app event.
This event is traveling around the following processes:

forms.js(Content Process) <--> Keyboard.jsm(Main Process) <--> MozKeyboard.js(Keyboard App Process)

However, for some events interaction with Keyboard.jsm is not necessary.
e.g. "SendKey:Result:XX" Event:
When Keyboard.jsm received this event, Keyboard.jsm simply replaces the prefix of event and send to MozKeyboard.js.[1]
So, those event should be sent from forms.js to MozKeyboard.js directly.

I'm concerned that if the process of Keyboard.jsm is heavy, it will affect Keyboard performance.
If we can bypass the main messaging thread, we can resolve this issue.

I previously made a prototype of this comparing the difference in performance as shown in attachment 8610351 [details]. On the right is the existing implementation, on the left is the prototype that bypasses the main messaging thread. I artificially slowed down the main process for the demo to simulate the system under load.

This prototype works by removing the cross-origin checks from BroadcoastChannel API.

In this bug I want to implement this optimization in some form that can be checked in so I focused on extending the BroadcastChannel API for chrome callers. I focused on the BroadcastChannel API because this API is using PBackground[2] and this API can send without main messaging thread.

However, the origin of forms.js is different from origin of Keyboard App(MozKeyboard.js).
I thought that it will be necessary to expand BroadcastChannel API in order to send message directly.

If you know a way to send message to app directly, could you please teach me?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.jsm#220
[2] http://mxr.mozilla.org/mozilla-central/source/ipc/glue/PBackground.ipdl

Comment 10

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

The tricky part is that we need to ensure that not any process can pretend to be the "Keyboard App Process" and send messages to the parent process which the parent process then forward to the content app.

I.e. we need IPC security here. We can't just rely on security checks in the child process since child processes can get hacked.

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Jonas Sicking (:sicking) from comment #10)
> The tricky part is that we need to ensure that not any process can pretend
> to be the "Keyboard App Process" and send messages to the parent process
> which the parent process then forward to the content app.
> 
> I.e. we need IPC security here. We can't just rely on security checks in the
> child process since child processes can get hacked.
Thank you for your reply.

I think most key point of resolving this issue is that to use background
thread instead of main messaging thread. So PBackground will became
candidate.

My previous way of ensuring security was adding [ChromeOnly] attribute.
I thought that it is enough to prevent malevolent code.
However, I perceived that it is not enough by your comment.

I think that I can use PermissionManager.[1]
There are same bug filed.(Bug 776834 / Bug 813758 ..etc)

If application have a 'input' permission, caller is Keyboard App process.
Because InputMethod API requeire 'input' permission[2].

I think that ensure security adding by the access limitation to API.
e.g.
 - The interface of using from Keyboard App(send message to content app)
check that caller is Keyboard-App.
   The way of check is using PermissionManager.

 - The interface of using from Content App(send message to keyboard app)
check that caller is current content app and caller is chrome code.

What do you think this way?

[1]
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIPermissionManager.idl
[2] https://developer.mozilla.org/en-US/Apps/Build/App_permissions#input

Comment 12

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

Yes, adding [ChromeOnly] isn't enough. That will only perform some security checks in the child process.

What we need here is to do security checks in the parent process after the message has been received by the parent on the PBackground thread.

However you can't run JS there, so I'm not sure there is much benefit to using BroadcastChannel API there?

Comment 13

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Jonas Sicking (:sicking) from comment #12)
> Yes, adding [ChromeOnly] isn't enough. That will only perform some security
> checks in the child process.
> 
> What we need here is to do security checks in the parent process after the
> message has been received by the parent on the PBackground thread.
> 
> However you can't run JS there, so I'm not sure there is much benefit to
> using BroadcastChannel API there?
Thank you for your suggestion.

I think that I can security check in PBackground process.
Its way is PermissionManager's security check using Principal Information.
(PBackground can get child process's principal information.[1])

If receiving principal which haven't 'input-manager' or 'input' permission, those message was sent from excepting keyboard-related app.

So, I think this way can prevent malicious apps attack.

Does this way mean the same as your comment #12 's security check?

[1] http://mxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundParentImpl.cpp#371

Comment 14

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

Yes, that sounds right.

I don't understand how you're going to use the BroadcastChannel API/implementation to do that though?

But if you have a plan then that's great. You should go ahead and write it and I can look at the patches if you want.

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Jonas Sicking (:sicking) from comment #14)
> Yes, that sounds right.
> 
> I don't understand how you're going to use the BroadcastChannel
> API/implementation to do that though?
I think that we should not implement to the BroadcastChannel API.
Because this way is not related BroadcastChannel now.
I mentioned that most important key of this problem is using background thread in comment #11.
Perhaps, The result of this resolving implementation will be other internal API.

> But if you have a plan then that's great. You should go ahead and write it
> and I can look at the patches if you want.
Of course! I want to receive your advice.
I'll start to create this implementation.

Thanks.

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: WIP: Part1 : Add internal API in order to communicate with Keyboard apps.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Mantaroh, this look like a great progress. Please keep the C++ part simple and only communication-related, i.e. only pass the same message from one process to another -- I would like to make sure future Gaia engineers who only speaks JavaScript can update InputMethod API on his/her own just like what I do from time to time. It is worthwhile to put more comments on the WebIDL too IMHO.

Thanks!

Comment 18

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: WIP: Part1 : Add internal API in order to communicate with Keyboard apps.

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: WIP: Part2. Add the IPC Define in order to communicate Keyboard apps.

Hi Tim,

(In reply to Tim Guan-tin Chien [:timdream] (OOO Nov 16-26; please needinfo) from comment #17)
> Mantaroh, this look like a great progress. Please keep the C++ part simple
> and only communication-related, i.e. only pass the same message from one
> process to another -- I would like to make sure future Gaia engineers who
> only speaks JavaScript can update InputMethod API on his/her own just like
> what I do from time to time. It is worthwhile to put more comments on the
> WebIDL too IMHO.
Thank you for your advice. I improved the part1 patch which added the some comment.(comment #18)

I would like to implement the IPC communication logic to the inputmethod module.
(This logic will use only InputMethod API.) 

What do you think about this implementation plan?

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8685256 [details] [diff] [review]
WIP: Part2. Add the IPC Define in order to communicate Keyboard apps.

It looks like the C part takes a JS object as message data so it fits my previous expectation. I am not really sure if I understand the .h files correctly though.

Please rename all "Keyboard" to "InputMethod" for consistency.

Comment 21

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: WIP: Part1 : Add InputmethodChannel API in order to communicate with Keyboard apps.

Comment 22

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: WIP: Part2. Add the IPC Define in order to communicate Keyboard apps.

Thanks Tim,

(In reply to Tim Guan-tin Chien [:timdream] (OOO Nov 16-26; please needinfo) from comment #20)
> Please rename all "Keyboard" to "InputMethod" for consistency.
I modified the internal API name as |InputMethodChannel|.
I plan to continue implementation using this name.

Thanks

Comment 23

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: WIP : Part3 : Add the logic of connection with ipc.

Comment 24

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part1 : Add definition of InputmethodChannel API in order to improve the keyboard apps.

Hi Olli,
I'm going to implement the |InputMethodChannel| API which can use only from InputMethod API(b2g). This API can communicate with MozKeyboard[1] and forms.js[2] directly. So We can improve the keyboard app's performance.

Could you confirm the this patch?

[1] https://dxr.mozilla.org/mozilla-central/rev/7cd2d806bd069c0260ff73f023ac85f892b863bf/dom/inputmethod/MozKeyboard.js
[2] https://dxr.mozilla.org/mozilla-central/rev/7cd2d806bd069c0260ff73f023ac85f892b863bf/dom/inputmethod/forms.js

Comment 25

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part1 : Add definition of InputmethodChannel API in order to improve the keyboard apps.

Hi Olli,
Sorry, Previous patch included unnecessary code.
I removed that code.

Comment 26

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

Comment on attachment 8687804 [details] [diff] [review]
Part1 : Add definition of  InputmethodChannel API in order to improve the keyboard apps.

>+EventHandlerNonNull*
>+InputMethodChannel::GetOnmessage()
>+{
>+  if (NS_IsMainThread()) {
>+    return GetEventHandler(nsGkAtoms::onmessage, EmptyString());
>+  }
>+  return GetEventHandler(nullptr, NS_LITERAL_STRING("message"));
>+}
>+
>+void
>+InputMethodChannel::SetOnmessage(EventHandlerNonNull* aCallback)
>+{
>+  if (NS_IsMainThread()) {
>+    SetEventHandler(nsGkAtoms::onmessage, EmptyString(), aCallback);
>+  } else {
>+    SetEventHandler(nullptr, NS_LITERAL_STRING("message"), aCallback);
>+  }
>+}
Please use IMPL_EVENT_HANDLER to implement event handler stuff.


>+
>+void
>+InputMethodChannel::AddEventListener(const nsAString& aType,
>+                                     EventListener* aCallback,
>+                                     bool aCapture,
>+                                     const dom::Nullable<bool>& aWantsUntrusted,
>+                                     ErrorResult& aRv)
>+{
>+  DOMEventTargetHelper::AddEventListener(aType, aCallback, aCapture,
>+                                         aWantsUntrusted, aRv);
>+
>+  if (aRv.Failed()) {
>+    return;
>+  }
>+}
>+
>+void
>+InputMethodChannel::RemoveEventListener(const nsAString& aType,
>+                                      EventListener* aCallback,
>+                                      bool aCapture,
>+                                      ErrorResult& aRv)
>+{
>+  DOMEventTargetHelper::RemoveEventListener(aType, aCallback, aCapture, aRv);
>+
>+  if (aRv.Failed()) {
>+    return;
>+  }
>+}
Why the need for these methods?
And 
>+  if (aRv.Failed()) {
>+    return;
>+  }
looks useless when you return anyway from the method.


>+++ b/dom/webidl/InputMethodChannel.webidl
>@@ -0,0 +1,34 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * 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/.
>+ */
>+
>+/**
>+ * InputMethodChannel API implements a use InputMethod API as Internal API.
>+ * This API is use only from InputMethod Internal Implementation codes.
>+ */
>+[Constructor,ChromeOnly]
>+interface InputMethodChannel : EventTarget {
>+
>+  /**
>+   * Send message to the InputMethod Manager(MozKeyboard.js) or
>+   * Contents App(forms.js).
>+   * Note :
>+   *  The message which sent from same app can't receive.
>+   *  e.g. MozKeyboard.js can't receive themselves message
>+   */
>+  [Throws]
>+  void postMessage(any message);
>+
>+  /**
>+   * Close communication channel which Keyboard and contents.
>+   */
>+  void close();
>+
>+  /**
>+   * Receive the message from the InputMethod Manager(MozKeyboard.js) or
>+   * Keyboard App(forms.js).
>+   */
>+  attribute EventHandler onmessage;
>+};
So looks fine, but I wonder if you've considered (re-)using MessagePorts here?
The API is almost the same. But since I don't know how this new API is supposed to be used, hard to
say whether MessagePorts would work here.
Our current MessagePorts implementation doesn't seem accept transferring across processes, but baku might have ideas whether that is easy to change
(that is something we need to change anyway for out-of-process service/shared workers).

Or perhaps this all will still look more like BroadcastChannel?
Anyhow, both MessagePort and BroadcastChannel implementations are complicated, and I wouldn't want to add yet another as complicated implementation for cross js context messaging, if
possible.

r- anyway because of small issues in c++, but I'd like to also understand how the API is supposed to be used.
What runs in which process and how many users per each channel and so.
(if a new API is added, I wouldn't add the bizarre special case MessagePorts have related to calling start() when onmessage is set.)

Comment 27

[Mantaroh Yoshinaga[:mantaroh]]

Thanks Olli,

(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 8687804 [details] [diff] [review]
> So looks fine, but I wonder if you've considered (re-)using MessagePorts
> here?
Umm, I've thought only about creating the new API similar to BroadcastChannel API.
If Channel Messaging APIs can communicate with inter-process, I'd like to use this API.

> Our current MessagePorts implementation doesn't seem accept transferring
> across processes, but baku might have ideas whether that is easy to change
> (that is something we need to change anyway for out-of-process
> service/shared workers).
Hi baku,

I saw the bug 911972. It's my understanding that MessagePort API can send message across processes, because those API use to PBackground[1]. I think that we can use MessagePorts as inter-app communication method in b2g.
Is my understanding correct?

If my understanding is right, I'll try use ChromeWorker and MessagePort in order to communicate with keyboard app and content app.

Could you get your opinion about using MessagePort as way of inter-app communication?

[1] https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/ipc/glue/PBackground.ipdl

Comment 28

[Andrea Marchesini [:baku]]

> Is my understanding correct?

Yes. MessagePort works cross processes/threads.

> Could you get your opinion about using MessagePort as way of inter-app
> communication?

The simplest way of passing a port is to support transferable objects in some kind of multi-process postMessage.
And if this is built on top of StructuredCloneHolder, it will work by default.

Comment 29

[Mantaroh Yoshinaga[:mantaroh]]

Thanks baku,

(In reply to Andrea Marchesini (:baku) from comment #28)
> > Could you get your opinion about using MessagePort as way of inter-app
> > communication?
> 
> The simplest way of passing a port is to support transferable objects in
> some kind of multi-process postMessage.
> And if this is built on top of StructuredCloneHolder, it will work by
> default.
Thank you for your advice.

I faced the problem about the way of providing the MessagePort to contents apps.

The background of this bug is improvement of KeyboardApps's communication which go through Main Process in B2G. The InputMethod API is communicated with Keyboard-Apps(ChildProcess)[1] and Contents-Apps(ChildProcess)[2] via Main-Process[3]. If we can replace this communication way to MessagePort, we can delete current communication implements.

However I faced some problems. It is that the way of delivering MessagePort to Keyboard and Contents Apps. 
I've tried as follow :
 - Create MessagePort in Main-Process, And deliver it to each Apps when Keyboard Initialized.
 - Use to SharedWorker's MessagePort.
 - Create MessagePort in Contents-Process, And deliver each Apps using Main-Process.

The above way is failed, because occurred by limitation of same-origin.
I succeed provide the MessagePort each apps when I use the MessagePort in same-origin apps.

My question point is..
Do you have idea which the way of providing MessagePort in cross-orign environment like B2G's iframe.


[1] https://dxr.mozilla.org/mozilla-central/rev/099f695d31326c39595264c34988a0f4b7cbc698/dom/inputmethod/MozKeyboard.js
[2] https://dxr.mozilla.org/mozilla-central/rev/099f695d31326c39595264c34988a0f4b7cbc698/dom/inputmethod/forms.js
[3] https://dxr.mozilla.org/mozilla-central/rev/099f695d31326c39595264c34988a0f4b7cbc698/dom/inputmethod/Keyboard.jsm

Comment 30

[Andrea Marchesini [:baku]]

This is more an architecture question, so I would like to have fabrice to answer here.
What I think is that we can write a tiny layer on top of MessagePort for skip the cross-origin check. But the real issue is that, as far as I know, in b2g we don't have a concept of 'transferring' data between apps and this is needed for MessagePort in order to neuter the port that is transferred to the other app.
For how it works in gecko, a MessagePort X can populate a MessagePortIdentifier, a fully transferable data struct, that can be used by another MessagePort to 'be' X in another context. Can we use this at the same way we transfer the current implementation of IPC MessagePort?

Comment 31

[Mantaroh Yoshinaga[:mantaroh]]

Thanks baku!

(In reply to Andrea Marchesini (:baku) from comment #30)
> What I think is that we can write a tiny layer on top of MessagePort for
> skip the cross-origin check. 
I tried to modify some code which skip cross-origin check when creating SharedWorker.
(add skip code in BasicPrincipal.cpp[1].)

So we can communicate with each apps using by MessagePort. And It is go through Worker.
This result will expect to improve keyboard apps performance, 
because this way is not going through Main-Process.


[1] https://dxr.mozilla.org/mozilla-central/rev/35916735b8afc5b0732e00f9aeb56bf846bba7f4/caps/BasePrincipal.cpp#273


> But the real issue is that, as far as I know,
> in b2g we don't have a concept of 'transferring' data between apps and this
> is needed for MessagePort in order to neuter the port that is transferred to
> the other app.
InputMethod API  communicate using by serialized string. So we need not use MessagePort's transfer object. I think that using only MessagePort's message object is enough to improve keyboard apps.


Hi Tim,
I think that using SharedWorker with MessagePort become alternative of InputMethod internal communcation. So I would like to use SharedWorker and MessagePort.
What do you think about this way?

Thanks.

Comment 32

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am fine with any solution as long as there isn't a need to port too many logic into C++ (beyond security/origin check, for example). Thanks!

Comment 33

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Fabrice, I wonder if we can get your help on the architectural questions raised by Andrea in comment 31. As I understand it, the solution we are considering is to set up a MessageChannel in the parent process and pass the ports to the child processes: content app and keyboard.

This is really not my domain so forgive me if I've got the concepts confused, but I'm told that we can't use postMessage to just pass these ports to the child iframes representing the app and keyboard but instead we need to use the message manager. Normally, if you use postMessage to pass a MessagePort, the original port is neutered. From what I understand, we'd need to do something similar when passing the port between processes using the message manager.

Does that make any sense? And can you advise where to get started, or anyone who might help mentor Mantaroh with this?

As background, we'd trying to eliminate routing messages between the app and keyboard app through the parent process for performance reasons.

Also, Andrea, does that description match what you had in mind?

Comment 34

[Andrea Marchesini [:baku]]

> Also, Andrea, does that description match what you had in mind?

Correct. I think it will not be too complex to extend the message manager in order to support the transferring of MessagePort.

Comment 35

[Mantaroh Yoshinaga[:mantaroh]]

Hi baku,

Thank you so much for your advice in mozlando.
I was able to step forward!

And so I have a question for you.

When will WriteIPCParams be called?
This function called from IPCMessageUtils[1]. However I can't find the caller of this Write() function.

[1] https://dxr.mozilla.org/mozilla-central/rev/412e4d7ce98ca4dbc37de133d0f26d7e1a59946f/ipc/glue/IPCMessageUtils.h#702

Thanks.

Comment 36

[Andrea Marchesini [:baku]]

> When will WriteIPCParams be called?
> This function called from IPCMessageUtils[1]. However I can't find the
> caller of this Write() function.

That is IPC code. When we send StructuredCloneData via IPDL, those Writes are called.

Comment 37

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: [WIP] Adding Transfer object to nsFrameMessageManager.

This patch will occur IPC error as follow.
Continue to investigation.

----------------------
###!!! [Parent][MessageChannel] Error: (msgtype=0x2A0081,name=PBrowser::Msg_Dest
roy) Channel error: cannot send/recv

Comment 38

[Mantaroh Yoshinaga[:mantaroh]]

Hi baku,

Thank you so much for your advice of MessagePort implementation.
I've trying the implement the sendSyncMessage with MessagePort.

Then, I have a some questions.
Please let me confirm again.

1) Should I add the MessagePortIdentifier to StructuredCloneData?
 According to Mozlando, I should have MessagePortIdentifier in StructuredCloneData[1] from my understanding. 
And We should serialize the MessageCloneData when called |StructuredCloneData::WriteIPCParams| using by |WriteParam| function.[2]
Is it right?

[1] https://dxr.mozilla.org/mozilla-central/rev/cb66ffeb6725e8344818e8e2f707ae2eaeb953b4/dom/ipc/StructuredCloneData.h
[2] https://dxr.mozilla.org/mozilla-central/rev/cb66ffeb6725e8344818e8e2f707ae2eaeb953b4/dom/ipc/StructuredCloneData.cpp#85

2) Are there other implementation part of this which we should create IPDL?

 I tried to Add to the transferable object parameter in the |nsIMessageSender::SendAsyncMessage|[3][4], and then give this transferable object to |StructuredCloneHolder| in |StructuredCloneData::Write|[5].
 However it happen to crash in content process before calling the |StructuredCloneData::WriteIPCParams|[6].
 Should I implement other IPC parts?

[3] https://dxr.mozilla.org/mozilla-central/rev/cb66ffeb6725e8344818e8e2f707ae2eaeb953b4/dom/base/nsIMessageManager.idl#282
[4] https://dxr.mozilla.org/mozilla-central/rev/cb66ffeb6725e8344818e8e2f707ae2eaeb953b4/dom/base/nsFrameMessageManager.cpp#815
[5] https://dxr.mozilla.org/mozilla-central/rev/cb66ffeb6725e8344818e8e2f707ae2eaeb953b4/dom/ipc/StructuredCloneData.cpp#66
[6] https://dxr.mozilla.org/mozilla-central/rev/cb66ffeb6725e8344818e8e2f707ae2eaeb953b4/dom/ipc/StructuredCloneData.cpp#85

Comment 39

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: [WIP] Adding Transfer object to nsFrameMessageManager.

Comment 40

[Andrea Marchesini [:baku]]

> 1) Should I add the MessagePortIdentifier to StructuredCloneData?

No. You should just change the constructor of StructuredCloneData in order to support the transferring of ports.

> 2) Are there other implementation part of this which we should create IPDL?

Not really. But I'm not sure about this.

>  I tried to Add to the transferable object parameter in the
> |nsIMessageSender::SendAsyncMessage|[3][4], and then give this transferable
> object to |StructuredCloneHolder| in |StructuredCloneData::Write|[5].
>  However it happen to crash in content process before calling the
> |StructuredCloneData::WriteIPCParams|[6].

Can I see your code? What's the crash about?

Comment 41

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: [WIP] Add Transfer object to nsFrameMessageManager.

Thanks baku.

(In reply to Andrea Marchesini (:baku) from comment #40)
> Can I see your code? What's the crash about?
The cause of crash is array size of PortIdentifier was 0. Because I forgot transferring PortIdentifier information. So occurred assertion error.[1]
So I added the implementation of transferring the identifier information.

In current implementation, We used the |ClonedMessageData| instead of |StructuredCloneData| when sending async message. And It was not copy to PortIdentifier when convert from |StructuredCloneData| to |ClonedMessageData|[3]. If we expanded the |StructuredCloneData|, We should expand |ClonedMessageData|.

On the other hand, I think that we may transfer identifier without using |StructuredCloneData|. We can send port identifier as parameter of async message.
I implemented the this way. 

What do you think this implementation?

[1] https://dxr.mozilla.org/mozilla-central/rev/29258f59e5456a1a518ccce6b473b50c1173477e/dom/base/StructuredCloneHolder.cpp#1047
[2] https://dxr.mozilla.org/mozilla-central/rev/29258f59e5456a1a518ccce6b473b50c1173477e/dom/ipc/PContent.ipdl#1189
[3] https://dxr.mozilla.org/mozilla-central/rev/29258f59e5456a1a518ccce6b473b50c1173477e/dom/base/nsFrameMessageManager.cpp#1991

Comment 42

[Andrea Marchesini [:baku]]

Comment on attachment 8704499 [details] [diff] [review]
[WIP] Add Transfer object to nsFrameMessageManager.

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

It's going in the right direction, but there is a lot to do yet. Mainly:

1. MessageIdentifiers should be part of the clonedMessageData.
2. The serialization of it should call the serialization of MessageIdentifiers
3. indentation :)

::: dom/base/nsFrameLoader.cpp
@@ +2490,5 @@
>      if (aCpows && (!mgr || !mgr->Wrap(aCx, aCpows, &cpows))) {
>        return NS_ERROR_UNEXPECTED;
>      }
> +
> +	nsTArray<MessagePortIdentifier> identifiers;

data should already contain the identifiers. Why this array?

::: dom/base/nsFrameMessageManager.cpp
@@ +48,5 @@
>  #include "nsQueryObject.h"
>  #include <algorithm>
>  
> +#include "mozilla/dom/MessagePort.h"
> +#include "mozilla/dom/MessagePortList.h"

move those in the previous block, in alphabetic order.

@@ +629,5 @@
>    ErrorResult rv;
> +  if (aTransfer.isNull() || aTransfer.isUndefined()) {
> +	aData.Write(aCx, v, rv);
> +  } else {
> +	aData.Write(aCx, v, t, rv);

I guess you can directly use aData.Write(aCx, v, t, rv);

@@ +826,5 @@
> +                                            nsIPrincipal* aPrincipal,
> +                                            JSContext* aCx,
> +                                            uint8_t aArgc)
> +{
> +	StructuredCloneData data;

2 spaces of indentation.

@@ +1183,5 @@
> +      JS::Rooted<JSObject*> transferredList(cx);
> +
> +      if (!ports.IsEmpty()) {
> +        RefPtr<MessagePortList> portList = new MessagePortList(aTarget, ports);
> +        transferredList = portList->WrapObject(cx, nullptr);

check if this fails.

@@ +2046,5 @@
>      if (aCpows && !cc->GetCPOWManager()->Wrap(aCx, aCpows, &cpows)) {
>        return NS_ERROR_UNEXPECTED;
>      }
> +
> +	nsTArray<MessagePortIdentifier> identifiers;

this is not needed, right?

::: dom/ipc/ContentBridgeChild.cpp
@@ +62,5 @@
>  ContentBridgeChild::RecvAsyncMessage(const nsString& aMsg,
>                                       const ClonedMessageData& aData,
>                                       InfallibleTArray<jsipc::CpowEntry>&& aCpows,
> +                                     const IPC::Principal& aPrincipal,
> +                                     InfallibleTArray<MessagePortIdentifier>&& identifiers)

This should be included in ClonedMessageData.

::: dom/ipc/StructuredCloneData.cpp
@@ +85,5 @@
> +StructuredCloneData::Write(JSContext* aCx,
> +                           JS::Handle<JS::Value> aValue,
> +                           JS::Handle<JS::Value> aTransfer,
> +                           ErrorResult &aRv)
> +{

Can you reuse this Write() in the previous one? I mean, calling |Write(aCx, aValue, JS::UndefinedValue, aRv)| in the previous ::Write.

@@ +105,1 @@
>  StructuredCloneData::WriteIPCParams(IPC::Message* aMsg) const

You should change this, right?

Comment 43

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add Transfer object to nsFrameMessageManager.

Thanks baku!

(In reply to Andrea Marchesini (:baku) from comment #42)
> Comment on attachment 8704499 [details] [diff] [review]
> [WIP] Add Transfer object to nsFrameMessageManager.
> 
> Review of attachment 8704499 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/ipc/StructuredCloneData.cpp
> @@ +105,1 @@
> >  StructuredCloneData::WriteIPCParams(IPC::Message* aMsg) const
> 
> You should change this, right?
I think that this change is not necessary, because We aren't use StructuredCloneData as IPC Parameter.[1]
|WriteIPCParams| was called when calling |GetXPCOMProcessAttributes| and |SyncMessage| and |RpcMessage| only.[2][3][4]

[1] https://dxr.mozilla.org/mozilla-central/rev/e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c/dom/ipc/PContent.ipdl#1180
[2] https://dxr.mozilla.org/mozilla-central/rev/e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c/dom/ipc/PContent.ipdl#751
[3] https://dxr.mozilla.org/mozilla-central/rev/e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c/dom/ipc/PContent.ipdl#879
[4] https://dxr.mozilla.org/mozilla-central/rev/e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c/dom/ipc/PContent.ipdl#883


I modified using to the ClonedMessageData. I confirmed that can transfer MessagePort using nsFrameMessageManager.
Could you confirm this patch?

Thanks.

Comment 44

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: [WIP] Part2. Change to use the nsIGlobalObject in MessagePort.

Hi baku,

Attachment #8705502 [details] [diff] has a problem that can't send message from parent to child process via MessagePort.
The reason is that |nsFrameMessageManager| can't create nsPIDOMWindow from nsIGlobalObject of child process.

I think that We can create MessagePort without nsPIDOMWindow. So I tried to created the patch.

If you know about the way of getting nsPIDOMWindowobject in child process, Please let me know.


Detail flow is as follow.
 - |StructuredCloneHolder| set the |mParent| from parameter of |ReadFromBuffer|.[1]
 - |ReadFromBuffer| was called from |StructuredCloneData|.[2]
 - |StructuredCloneHolder| created the nsPIDOMWindow from |mParent|.[3]
   However, can't do_QueryInterfaces. So |window| is NULL.
 - |MessagePort| was created nsIGlobalObject when receive postMessage.[4]
   However, can't create this object, because |mParentObject| is NULL.

[1] https://dxr.mozilla.org/mozilla-central/rev/e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c/dom/base/StructuredCloneHolder.cpp#343
[2] https://dxr.mozilla.org/mozilla-central/rev/e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c/dom/ipc/StructuredCloneData.cpp#62
[3] https://dxr.mozilla.org/mozilla-central/rev/e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c/dom/base/StructuredCloneHolder.cpp#1045
[4] https://dxr.mozilla.org/mozilla-central/rev/e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c/dom/messagechannel/MessagePort.cpp#104

Comment 45

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add Transfer object to nsFrameMessageManager.

Hi baku.

I changed the parameter order and squashed two patches. Previous patch affected existing code.
Could you please confirm this patch?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2fdb6790412

Comment 46

[Andrea Marchesini [:baku]]

Comment on attachment 8709782 [details] [diff] [review]
Add Transfer object to nsFrameMessageManager.

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

Great! Almost perfect. I want to see these comments applied and add a test.

::: dom/base/StructuredCloneHolder.cpp
@@ +1047,5 @@
>      MOZ_ASSERT(aExtraData < mPortIdentifiers.Length());
>      const MessagePortIdentifier& portIdentifier = mPortIdentifiers[aExtraData];
>  
>      // aExtraData is the index of this port identifier.
> +    nsIGlobalObject* global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));

this is not needed.

@@ +1055,3 @@
>      ErrorResult rv;
>      RefPtr<MessagePort> port =
> +      MessagePort::Create(global, portIdentifier, rv);

use mParent directly.

::: dom/base/nsFrameMessageManager.cpp
@@ +842,5 @@
>                                               JS::Handle<JS::Value> aObjects,
>                                               JSContext* aCx,
>                                               uint8_t aArgc)
>  {
> +  return DispatchAsyncMessage(aMessageName, aJSON, aObjects, nullptr, 

no extra space.

@@ +1144,5 @@
>        JS::Rooted<JS::Value> json(cx, JS::NullValue());
>        if (aCloneData && aCloneData->DataLength()) {
>          ErrorResult rv;
> +        JSContext* jCont = nsContentUtils::GetCurrentJSContextForThread();
> +        aCloneData->Read(jCont, &json, rv);

why don't use 'cx' ?

@@ +1179,5 @@
>                  JS_DefineProperty(cx, param, "sync", syncv, JSPROP_ENUMERATE) &&
>                  JS_DefineProperty(cx, param, "json", json, JSPROP_ENUMERATE) && // deprecated
>                  JS_DefineProperty(cx, param, "data", json, JSPROP_ENUMERATE) &&
>                  JS_DefineProperty(cx, param, "objects", cpowsv, JSPROP_ENUMERATE);
> +	  

no extra spaces.

@@ +1181,5 @@
>                  JS_DefineProperty(cx, param, "data", json, JSPROP_ENUMERATE) &&
>                  JS_DefineProperty(cx, param, "objects", cpowsv, JSPROP_ENUMERATE);
> +	  
> +      if (!ports.IsEmpty()) {
> +        ok = ok && JS_DefineProperty(cx, param, "ports", transferredList, JSPROP_ENUMERATE);

I would create 'ports' in any case.
Create a MessagePortList in any case and add it here also if ports.IsEmpty().

::: dom/base/nsFrameMessageManager.h
@@ +213,5 @@
>  
>    nsresult DispatchAsyncMessage(const nsAString& aMessageName,
> +	  const JS::Value& aJSON,
> +	  const JS::Value& aObjects,
> +                      nsIPrincipal* aPrincipal,

indentation.

::: dom/ipc/StructuredCloneData.cpp
@@ +66,5 @@
>  StructuredCloneData::Write(JSContext* aCx,
>                             JS::Handle<JS::Value> aValue,
>                             ErrorResult &aRv)
>  {
> +    Write(aCx, aValue, JS::UndefinedHandleValue, aRv);

2 spaces, no 4.

::: dom/messagechannel/MessageChannel.cpp
@@ +68,5 @@
>    }
>  
>    RefPtr<MessageChannel> channel = new MessageChannel(aWindow);
>  
> +  nsCOMPtr<nsIGlobalObject> globalObject = do_QueryInterface(aWindow);

This is not needed. Instead having a aWindow, use a more generic nsISupports in MessageChannel and in MessagePort.

::: dom/messagechannel/MessagePort.cpp
@@ +121,5 @@
>  
>      ErrorResult rv;
>      JS::Rooted<JS::Value> value(cx);
>  
> +    mData->Read(globalObject, cx, &value, rv);

same here.

@@ +281,5 @@
>  NS_IMPL_ISUPPORTS(ForceCloseHelper, nsIIPCBackgroundChildCreateCallback)
>  
>  } // namespace
>  
> +MessagePort::MessagePort(nsIGlobalObject* aGlobalObject)

here too

@@ +299,5 @@
>    MOZ_ASSERT(!mWorkerFeature);
>  }
>  
>  /* static */ already_AddRefed<MessagePort>
> +MessagePort::Create(nsIGlobalObject* aGlobal, const nsID& aUUID,

and there.

::: dom/messagechannel/MessagePortIPC.h
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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/. */
> +
> +#ifndef MessagePortIPC_h__

mozilla_dom_MessagePortIPC_h

@@ +6,5 @@
> +#ifndef MessagePortIPC_h__
> +#define MessagePortIPC_h__
> +
> +#include "mozilla/dom/PMessagePort.h"
> +#include "ipc/IPCMessageUtils.h"

alphabetic order.

@@ +17,5 @@
> +
> +namespace IPC {
> +template <>
> +struct ParamTraits<mozilla::dom::MessagePortIdentifier>
> +{

All of this is not needed because MessagePortIdentifier is already supported by PMessagePort.ipdl.
Why do you need this file and this ParamTraits<> ?

::: dom/messagechannel/moz.build
@@ +14,5 @@
>      'MessagePortParent.h',
>  ]
>  
> +EXPORTS.ipc += [
> +    'MessagePortIPC.h',

Get rid of this.

Comment 47

[Andrea Marchesini [:baku]]

If it's not too much work for you, can you split this patch in 3 patches?

1. MessageChannel/MessagePort using nsIParent instead nsPIDOMWindow
2. all the rest
3. the test

Thanks!

Comment 48

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add Transfer object to nsFrameMessageManager.

Hi baku,

Thank you very much for your review.

(In reply to Andrea Marchesini (:baku) from comment #46)
> Comment on attachment 8709782 [details] [diff] [review]
> Add Transfer object to nsFrameMessageManager.
> 
> Review of attachment 8709782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great! Almost perfect. I want to see these comments applied and add a test.
OK. I'll add the test for this patch.

> ::: dom/messagechannel/MessageChannel.cpp
> @@ +68,5 @@
> >    }
> >  
> >    RefPtr<MessageChannel> channel = new MessageChannel(aWindow);
> >  
> > +  nsCOMPtr<nsIGlobalObject> globalObject = do_QueryInterface(aWindow);
> 
> This is not needed. Instead having a aWindow, use a more generic nsISupports
> in MessageChannel and in MessagePort.
Oh, Sorry.
I'm misunderstood serializing object of IPDL.
The member of MessagePortIdentifier is primitive types, So I'm not need to add the Serializing code. Am I right?

I addressed the patch.
Could you review this patch again?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fee1e5eeb80f

Comment 49

[Andrea Marchesini [:baku]]

Comment on attachment 8711515 [details] [diff] [review]
Add Transfer object to nsFrameMessageManager.

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

It seems to me that you uploaded the wrong patch. Can you apply all my previous comments? Thanks!

::: dom/base/StructuredCloneHolder.cpp
@@ +1046,1 @@
>  

you don't need this. remove it. Just use mParent in MessagePort::Create()

@@ +1050,3 @@
>      ErrorResult rv;
>      RefPtr<MessagePort> port =
> +    MessagePort::Create(global.get(), portIdentifier, rv);

mParent instead global.get()

::: dom/base/nsFrameMessageManager.cpp
@@ +1158,5 @@
> +        ports = aCloneData->TakeTransferredPorts();
> +      }
> +      JS::Rooted<JSObject*> transferredList(cx);
> +
> +      if (!ports.IsEmpty()) {

What about if here you do:

JS::Rooted<JSObject*> transferredList(cx);
RefPtr<MessagePortList> portList = new MessagePortList(aTarget, ports);
transferredList = portList->WrapObject(cx, nullptr);
if (!NS_WARN_IF(transferredList)) {
  return NS_ERROR_UNEXPECTED;
}

without checking if ports is empty or not.

@@ +1179,5 @@
>                  JS_DefineProperty(cx, param, "json", json, JSPROP_ENUMERATE) && // deprecated
>                  JS_DefineProperty(cx, param, "data", json, JSPROP_ENUMERATE) &&
>                  JS_DefineProperty(cx, param, "objects", cpowsv, JSPROP_ENUMERATE);
> +
> +      if (!ports.IsEmpty()) {

and here just do:

JS_DefineProperty(cx, param, "objects", cpowsv, JSPROP_ENUMERATE) &&
JS_DefineProperty(cx, param, "ports", transferredList, JSPROP_ENUMERATE);

::: dom/ipc/DOMTypes.ipdlh
@@ +23,5 @@
>    nsID;
>    void_t;
>  };
>  
> +struct MessagePortIdentifier

Remove all of these changes

Comment 50

[Mantaroh Yoshinaga[:mantaroh]]

Thanks baku.

Sorry for taking your precious time.
I have a one question about comment #49.

(In reply to Andrea Marchesini (:baku) from comment #49)
> Comment on attachment 8711515 [details] [diff] [review]
> ::: dom/ipc/DOMTypes.ipdlh
> @@ +23,5 @@
> >    nsID;
> >    void_t;
> >  };
> >  
> > +struct MessagePortIdentifier
> 
> Remove all of these changes
MessagePortIdentifier was used by MessagePortMessage[1] and ClonedMessageData[2]. In my understand, generated data structures can be used in several protocols if they are defined in a separate |.ipdl| file. Is it right?

[1] https://dxr.mozilla.org/mozilla-central/rev/bd8bb6298d90770f97843e9d7dc711cc0f87d02f/dom/messagechannel/PMessagePort.ipdl#21
[2] https://dxr.mozilla.org/mozilla-central/rev/bd8bb6298d90770f97843e9d7dc711cc0f87d02f/dom/ipc/DOMTypes.ipdlh#27

If I defined MessagePortIdentifier in |PMessagePort.ipdl|, |ClonedMessageData| can't refer to MessagePortIdentifier. So I think that I had better to define |MessagePortIdentifier| in ipdlh.

Comment 51

[Andrea Marchesini [:baku]]

> > Remove all of these changes
> MessagePortIdentifier was used by MessagePortMessage[1] and
> ClonedMessageData[2]. In my understand, generated data structures can be
> used in several protocols if they are defined in a separate |.ipdl| file. Is
> it right?

Absolutely. You are right.

Comment 52

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add Transfer object to nsFrameMessageManager.

Thanks baku.

(In reply to Andrea Marchesini (:baku) from comment #51)
> > > Remove all of these changes
> > MessagePortIdentifier was used by MessagePortMessage[1] and
> > ClonedMessageData[2]. In my understand, generated data structures can be
> > used in several protocols if they are defined in a separate |.ipdl| file. Is
> > it right?
> 
> Absolutely. You are right.
I would like to separate define of |MessagePortIdentifier| from ipdl files, because |MessagePortIdentifier| was reffered by two ipdl files.(comment #50)
So I added the B to the DOMTypes.ipdl.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68013fdf52bd

Comment 53

[Andrea Marchesini [:baku]]

Comment on attachment 8712571 [details] [diff] [review]
Add Transfer object to nsFrameMessageManager.

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

I still want to see a test. Don't land this code without a proper mochitest.

::: dom/base/nsFrameMessageManager.cpp
@@ +829,4 @@
>                                          JSContext* aCx,
>                                          uint8_t aArgc)
>  {
> +  return DispatchAsyncMessage(aMessageName, aJSON, aObjects, aPrincipal, aTransfers, aCx,

80chars.

::: dom/ipc/StructuredCloneData.h
@@ +106,5 @@
>               JS::Handle<JS::Value> aValue,
>               ErrorResult &aRv);
>  
> +  void Write(JSContext* aCx,
> +	  JS::Handle<JS::Value> aValue,

indentation.

::: dom/messagechannel/MessageChannel.cpp
@@ +68,5 @@
>    }
>  
>    RefPtr<MessageChannel> channel = new MessageChannel(aWindow);
>  
> +  nsCOMPtr<nsISupports> supports = do_QueryInterface(aWindow);

no needs for this. Just use aWindow here.

@@ +74,5 @@
>    if (NS_WARN_IF(aRv.Failed())) {
>      return nullptr;
>    }
>  
> +  channel->mPort2 = MessagePort::Create(supports, portUUID2, portUUID1, aRv);

same here.

::: dom/messagechannel/MessagePort.cpp
@@ +279,5 @@
>  
>  } // namespace
>  
> +MessagePort::MessagePort(nsISupports* aSupports)
> +  : DOMEventTargetHelper(static_cast<nsIGlobalObject*>(aSupports))

this looks totally wrong :)
Don't do this. Instead do:

MessagePort::MessagePort(nsISupports* aSupports)
  : mInnerID(0)
  ...
{
  ...

  MOZ_ASSERT(aSupports);
  nsCOMPtr<nsIGlobalObject> globalObject = do_QueryInterface(aSupports);
  if (NS_WARN_IF(!globalObject)) {
    return;
  }

  BindToOwner(globalObject);
}

Comment 54

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add mochitest for MessagePort of nsFrameMessageManager.

This mochitest patch was removed from bug 1224989.(see bug 1224989,comment #6)
Carrying forward r+ from baku(see bug 1224989,comment #7).

Comment 55

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add Transfer object to nsFrameMessageManager.

Thanks baku!

I fixed comment #53.

(In reply to Andrea Marchesini (:baku) from comment #53)
> Comment on attachment 8712571 [details] [diff] [review]
> Add Transfer object to nsFrameMessageManager.
> ::: dom/messagechannel/MessagePort.cpp
> @@ +279,5 @@
> >  
> >  } // namespace
> >  
> > +MessagePort::MessagePort(nsISupports* aSupports)
> > +  : DOMEventTargetHelper(static_cast<nsIGlobalObject*>(aSupports))
> 
> this looks totally wrong :)
> Don't do this. Instead do:
> 
> MessagePort::MessagePort(nsISupports* aSupports)
>   : mInnerID(0)
>   ...
> {
>   ...
> 
>   MOZ_ASSERT(aSupports);
This Assertion removed, because Worker pass the nullptr[1].

Could please confirm this patch again?

[1] https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/workers/WorkerPrivate.cpp#6382

Comment 56

[Andrea Marchesini [:baku]]

Comment on attachment 8714205 [details] [diff] [review]
Add Transfer object to nsFrameMessageManager.

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

This plus the test. Good.

Comment 58

[Andrea Marchesini [:baku]]

Comment on attachment 8714205 [details] [diff] [review]
Add Transfer object to nsFrameMessageManager.

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

Ops... sorry. I meant r+.

Comment 59

[Mantaroh Yoshinaga[:mantaroh]]

Thanks baku!

I run try again.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e040d418692

Comment 60

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add mochitest for MessagePort of nsFrameMessageManager.

Oh,, The test of android failed.
Because test is using to new window. So I added the skip code when android environment.

baku,
Sorry. I've not confirmed android result. The result of android mochitest is failed. 
Could you confirm chrome.ini ?

Comment 61

[Andrea Marchesini [:baku]]

Comment on attachment 8715145 [details] [diff] [review]
Add mochitest for MessagePort of nsFrameMessageManager.

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

::: dom/messagechannel/tests/mm_messageChannel.js
@@ +1,1 @@
> +

extra line

Comment 62

[Andrea Marchesini [:baku]]

> Oh,, The test of android failed.
> Because test is using to new window. So I added the skip code when android
> environment.

Can you tell me more? Why does it fail?

Comment 63

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add mochitest for MessagePort of nsFrameMessageManager.

I'm attach the wrong file. reattached.

Comment 64

[Andrea Marchesini [:baku]]

Comment on attachment 8715177 [details] [diff] [review]
Add mochitest for MessagePort of nsFrameMessageManager.

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

Please, answer comment 62.

::: dom/messagechannel/tests/mm_messageChannel.js
@@ +1,2 @@
> +
> +function debug(msg) {

remove the empty line.

Comment 65

[Mantaroh Yoshinaga[:mantaroh]]

Hi baku,

Sorry late reply.
(In reply to Andrea Marchesini (:baku) from comment #64)
> Please, answer comment 62.
The fennec used SameParentProcessMessageManagerCallback as MessageManagerCallback in |nsFrameMessageManger|.[1]
In this case, PortIdentifier doesn't copied.

We can reproduce this crash using by communicating with same process. So I'll fix |nsFrameMessageManager| code in order to communicate with same process, and added tests.

[1] https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/base/nsFrameMessageManager.cpp#1872

Comment 66

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add mochitest for MessagePort of nsFrameMessagemanager.

Add same process test.

Comment 67

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add the code of setting PortIdentifiers when communicating with same process.

I added the copying to PortIdentifier in |nsFrameMessagemanager|. It is create MessagePort when communicating with same process.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a99a65f36b9b

Comment 68

[Mantaroh Yoshinaga[:mantaroh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a89ec1230b3f

Comment 69

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add mochitest for MessagePort of nsFrameMessagemanager.

There are two problem of this test failure.

First, current patch can't communicate with same process.(comment #65)
Second is this chrome test is used remote browser.(attachment 8716175 [details] [diff] [review]) However Fennec can't use remote browser option. 

So I fixed tests and code of communicating with same process.

Comment 70

[Mantaroh Yoshinaga[:mantaroh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b2304b2300e

Comment 71

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add the code of setting PortIdentifiers when communicating with same process.

I remove the unnecessary space.

Comment 72

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8717337 [details] [diff] [review]
Add mochitest for MessagePort of nsFrameMessagemanager.

Hi baku,

I fixed problem of fennec.(see comment #69)
Would you please confirm those patches?

Comment 73

[Andrea Marchesini [:baku]]

Comment on attachment 8717768 [details] [diff] [review]
Add the code of setting PortIdentifiers when communicating with same process.

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

::: dom/base/nsFrameMessageManager.cpp
@@ +2202,5 @@
>      Telemetry::Accumulate(Telemetry::IPC_SAME_PROCESS_MESSAGE_COPY_OOM_KB, aData.DataLength());
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  mData.PortIdentifiers().AppendElements(aData.PortIdentifiers());

in theory mData should be a copy of aData.
You should change how Copy() works.

Comment 74

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add the code of setting PortIdentifiers when communicating with same process.

Thanks baku,

(In reply to Andrea Marchesini (:baku) from comment #73)
> Comment on attachment 8717768 [details] [diff] [review]
> in theory mData should be a copy of aData.
> You should change how Copy() works.
I moved port identifier copy code to Copy().

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bb65c818a53&group_state=expanded

Comment 75

[Andrea Marchesini [:baku]]

Comment on attachment 8719643 [details] [diff] [review]
Add the code of setting PortIdentifiers when communicating with same process.

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

::: dom/base/StructuredCloneHolder.h
@@ +209,5 @@
>      MOZ_ASSERT(mSupportsTransferring);
>      return Move(mTransferredPorts);
>    }
>  
> +  nsTArray<MessagePortIdentifier>& PortIdentifiers() const

I think this must be used also for adding new identifiers, somewhere.
So, remove const and mutable. or tell me more :)

@@ +309,5 @@
>  
>    // This array contains the identifiers of the MessagePorts. Based on these we
>    // are able to reconnect the new transferred ports with the other
>    // MessageChannel ports.
> +  mutable nsTArray<MessagePortIdentifier> mPortIdentifiers;

tell me more about this mutable.

Comment 76

[Mantaroh Yoshinaga[:mantaroh]]

Thanks baku!

(In reply to Andrea Marchesini (:baku) from comment #75)
> ::: dom/base/StructuredCloneHolder.h
> @@ +209,5 @@
> >      MOZ_ASSERT(mSupportsTransferring);
> >      return Move(mTransferredPorts);
> >    }
> >  
> > +  nsTArray<MessagePortIdentifier>& PortIdentifiers() const
> 
> I think this must be used also for adding new identifiers, somewhere.
> So, remove const and mutable. or tell me more :)
I think so. The reason of this change is in order to call the function of const object.[1]
However as you mentioned, this member should be modifiable. So I added the mutable qualifiers.

[1] https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/dom/ipc/StructuredCloneData.cpp#29

I can change the this parameter to non-const. But this modify will make to unsafe..

Comment 77

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d2bc2159809
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e7a30a8f6c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/aac3a7267a2f

Comment 78

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d2bc2159809
https://hg.mozilla.org/mozilla-central/rev/8e7a30a8f6c8
https://hg.mozilla.org/mozilla-central/rev/aac3a7267a2f