585173 - Add "process message manager"

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

This would be pretty similar to the current message managers, but
would allow communication without knowing anything about xul:browsers or 
content windows.
The use case is communication from content process JS components to chrome process.

Comment 1

[Florian Hänel [:heeen]]

Whats the status on this? Is there something we can help with?

Comment 2

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

This isn't assigned to anyone ;)

Comment 3

[Benjamin Smedberg]

I think this should be WONTFIX, unless there is a clear and compelling use case for it.

Comment 4

[Doug Turner (:dougt)]

I am thinking about the toolkit components that are written in js.  Callers of these things are be in JS or in C++.  For example, lets consider the content pref service.  It is used by native code in the child process.  For example, it does something like: 

nsCOMPtr<nsIContentPrefService> cpsvc = do_GetService("@mozilla.org/content-pref/service;1", &rv);

The caller has no idea if it is script or c++.  The implementation:

 - is a js component that lives in the content process.
 - directly accesses sqlite
 - doesn't know anything about windows/docshells

So, one approach is to #ifdef and add a check in all callers to toolkit componetns to to see if we are in the child process, and if we are, proxy over using some new protocol / pidl method.

Another approach would be to simple let the toolkit js component in the child process message over to the parent.

Another approach would be to have the js component use the frame message manager.  somehow.  This would require the js component in the child process to figure out which window or docshell it should use.  This might be a bit of guessing -- or it might just be to grab the top most docshell.

Comment 5

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

Attachment: patch

So this adds process manager to chrome and content processes.
In chrome process it uses nsIFrameMessageManager interface and in
content process it has also sendSyncMessage.

var ppm = Components.classes["@mozilla.org/parentprocessmessagemanager;1"]
                    .getService(Components.interfaces.nsIFrameMessageManager);

var cpm = Components.classes["@mozilla.org/childprocessmessagemanager;1"]
                    .getService(Components.interfaces.nsISyncMessageSender);

Note, be careful when using the API. It has similar memory management limitations as (also global) observer service; listeners should be
removed manually unless they should stay alive until shutdown.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 468659 [details] [diff] [review]
patch


>+static PRBool
>+IsChromeProcess()
>+{
>+  nsCOMPtr<nsIXULRuntime> rt = do_GetService("@mozilla.org/xre/runtime;1");
>+  if (rt) {
>+    PRUint32 type = nsIXULRuntime::PROCESS_TYPE_DEFAULT;
>+    rt->GetProcessType(&type);
>+    return type == nsIXULRuntime::PROCESS_TYPE_DEFAULT;

Maybe add NS_ABORT_IF_FALSE(rt, "We should have a xre runtime");

also, no need to set type before calling GetProcessType -- it will set the value and not fail.

>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIContentFrameMessageManager,
>+                                     !mChrome && !mIsProcessManager)
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsISyncMessageSender, !mChrome)
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIChromeFrameMessageManager,
>+                                     mChrome && !mIsProcessManager)


This is getting more complex and probably worth a comment specifically explaining why we do not return success when mIsProcessManager is nonnull.

>+ContentChild::RecvAsyncMessage(const nsString& aMsg, const nsString& aJSON)
>+{
>+  nsRefPtr<nsFrameMessageManager> cpm = nsFrameMessageManager::sChildProcessManager;
>+  if (cpm) {
>+    cpm->ReceiveMessage(static_cast<nsIContentFrameMessageManager*>(cpm.get()),
>+                        aMsg, PR_FALSE, aJSON, nsnull, nsnull);
>+  }
>+  return true;
>+}

if cpm is null, shouldn't we return false? 


>+bool
>+ContentParent::RecvSyncMessage(const nsString& aMsg, const nsString& aJSON,
>+                               nsTArray<nsString>* aRetvals)
>+{
>+  nsRefPtr<nsFrameMessageManager> ppm = nsFrameMessageManager::sParentProcessManager;
>+  if (ppm) {
>+    ppm->ReceiveMessage(static_cast<nsIContentFrameMessageManager*>(ppm.get()),
>+                        aMsg,PR_TRUE, aJSON, nsnull, aRetvals);
>+  }
>+  return true;
>+}

and here.


>+bool
>+ContentParent::RecvAsyncMessage(const nsString& aMsg, const nsString& aJSON)
>+{
>+  nsRefPtr<nsFrameMessageManager> ppm = nsFrameMessageManager::sParentProcessManager;
>+  if (ppm) {
>+    ppm->ReceiveMessage(static_cast<nsIContentFrameMessageManager*>(ppm.get()),
>+                        aMsg, PR_FALSE, aJSON, nsnull, nsnull);
>+  }
>+  return true;
>+}

same.


>diff --git a/dom/ipc/remote-test.js b/dom/ipc/remote-test.js
>--- a/dom/ipc/remote-test.js
>+++ b/dom/ipc/remote-test.js
>@@ -1,11 +1,20 @@
> dump("Loading remote script!\n");
> dump(content + "\n");
> 
>+var cpm = Components.classes["@mozilla.org/childprocessmessagemanager;1"]

nit: i suggested the name, but maybe it is more than a mouthful.  how about:

@mozilla.org/message-manager/child;1

you could also have:

@mozilla.org/message-manager/global;1


I'd really like to see a test js component that uses this code.  Maybe it isn't strictly needed, but that is basically what we hope to use this for.

Comment 7

[Alon Zakai (:azakai)]

I tested this patch by writing an xpcshell test for another bug. It works nicely. Some questions though:

1. Why do we still have a globalmessagemanager? Doesn't the parentprocess one do the same thing?
2. In my xpcshell test, using the globalmessagemanager didn't work, only the parentprocess one. Otherwise the child sends messages and no one hears them. Is that intentional?

Comment 8

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

(In reply to comment #7)
> I tested this patch by writing an xpcshell test for another bug. It works
> nicely.
Great.

> 1. Why do we still have a globalmessagemanager? Doesn't the parentprocess one
> do the same thing?
No, it does not do the same thing.
global mm gets messages from a TabChildGlobal and the target for the messages
is the <xul:browser>. So message listener knows which "tab" sent the message.
That is not the case with process mm


> 2. In my xpcshell test, using the globalmessagemanager didn't work, only the
> parentprocess one. Otherwise the child sends messages and no one hears them. Is
> that intentional?
I don't understand this.
xpcshell test doesn't have (chrome) DOMWindow, right? globalmessagemanager
needs one. and it needs a remote xul:browser

Comment 9

[Alon Zakai (:azakai)]

(In reply to comment #8)
> (In reply to comment #7)
> > I tested this patch by writing an xpcshell test for another bug. It works
> > nicely.
> Great.
> 
> > 1. Why do we still have a globalmessagemanager? Doesn't the parentprocess one
> > do the same thing?
> No, it does not do the same thing.
> global mm gets messages from a TabChildGlobal and the target for the messages
> is the <xul:browser>. So message listener knows which "tab" sent the message.
> That is not the case with process mm
> 

Thanks for the explanation.

To make sure I understand: Can the parentprocessmanager respond to messages? (As it doesn't know their origin?) It can do so by responding in sync messages, of course - but I guess not in any other way?

> 
> > 2. In my xpcshell test, using the globalmessagemanager didn't work, only the
> > parentprocess one. Otherwise the child sends messages and no one hears them. Is
> > that intentional?
> I don't understand this.
> xpcshell test doesn't have (chrome) DOMWindow, right? globalmessagemanager
> needs one. and it needs a remote xul:browser

Yes, that was probably the issue. So - very nice, this patch lets us do messaging in xpcshell tests now, without DOMWindows or xul:browsers.

The xpcshell test I wrote is here if anyone wants to take a look, maybe it can be useful in writing tests for other bugs:

https://bug584842.bugzilla.mozilla.org/attachment.cgi?id=469620

Comment 10

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

(In reply to comment #9)

> To make sure I understand: Can the parentprocessmanager respond to messages?
> (As it doesn't know their origin?)
It does know their origin. The origin is the child process mm.

 It can do so by responding in sync messages,
> of course - but I guess not in any other way?
It can send async messages.

Comment 11

[Alon Zakai (:azakai)]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > To make sure I understand: Can the parentprocessmanager respond to messages?
> > (As it doesn't know their origin?)
> It does know their origin. The origin is the child process mm.

Well yes, but there could be more than one (once we have multiple child processes).

> 
>  It can do so by responding in sync messages,
> > of course - but I guess not in any other way?
> It can send async messages.

Yes, but it wouldn't know to which child process to send it (again, once we have multiple child processes?).

Comment 12

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

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > 
> > > To make sure I understand: Can the parentprocessmanager respond to messages?
> > > (As it doesn't know their origin?)
> > It does know their origin. The origin is the child process mm.
> 
> Well yes, but there could be more than one (once we have multiple child
> processes).
And when that happens, the API will change to include some process ID.


> Yes, but it wouldn't know to which child process to send it (again, once we
> have multiple child processes?).
Again, the API will change then.
We don't have multiple child processes yet.

Comment 13

[Alon Zakai (:azakai)]

Sounds good, just making sure I understood the current API.

Comment 14

[Benjamin Smedberg]

I would like to have the option of multiple child processes in the Mozilla 2.0 branch series. Can we just design the API correctly now, so that we don't have to modify it again in a few months?

Comment 15

[Doug Turner (:dougt)]

bsmedberg, i think that is out of scope for this specific bug.  Fennec needs something yesterday.  Multiple process support can be a follow up.

Comment 16

[Benjamin Smedberg]

I think that the design decisions here are going to incur significant technical debt later, and that we should make the parent-side "global" message manager non-singleton in the following way:

Have a global observer notification when a new content process is created (either nsIObserver or something equivalent). That will be passed the parent-global message manager on which listeners can be installed.

Comment 17

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

(In reply to comment #16)
> Have a global observer notification when a new content process is created
> (either nsIObserver or something equivalent).
> That will be passed the
> parent-global message manager on which listeners can be installed.
Not parent -global, but parent-process message manager.

But I don't understand what you mean.

Comment 18

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

The current API could be forwards compatible if we add the process id as an
optional parameter to sendAsyncMessage and add/removeMessageListener.
Also, when adding support for multiple child processes, we could extend
messages to contain .processID.
Then, when a new process is created, it could send a message to parent process, 
which would have a listener, which listens for messages from all processes.
And at that point parent process could add process specific listeners.

This all could be done afterwards.

Comment 19

[Benjamin Smedberg]

Hrm, I was thinking that there would be one parent message manager per content process. Comment 18 is talking about having one global parent message manager, and adding some context parameter to the messages.

I tend to think one per-content-process will be the simpler solution, but I don't strongly care.

Comment 20

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

 
> if cpm is null, shouldn't we return false? 
> 
No. If no one has used process message manager, there we don't have one, and
then it is ok to just forget the message.

Comment 21

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

 
> @mozilla.org/message-manager/child;1
> 
> you could also have:
> 
> @mozilla.org/message-manager/global;1

I want to have "process" somewhere in the name, since process message managers
are a bit different than other message managers.

Comment 22

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

Attachment: patch

I hope alon's tests are good enough for this too.
(well, this does have test.xul tests)

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 470402 [details] [diff] [review]
patch

>+  nsCOMPtr<nsIXULRuntime> rt = do_GetService("@mozilla.org/xre/runtime;1");
>+  NS_ABORT_IF_FALSE(rt, "We should have a xre runtime");

nit:
s/should/must

>+  if (!ctx) {
>+    nsContentUtils::ThreadJSContextStack()->GetSafeJSContext(&ctx);
>+  }

nit: no need for braces around one statement.

>+#ifdef MOZ_IPC
>+bool SendAsyncMessageToChildProcess(void* aCallbackData,
>+                                    const nsAString& aMessage,
>+                                    const nsAString& aJSON)
>+{
>+  mozilla::dom::ContentParent* cp =
>+    mozilla::dom::ContentParent::GetSingleton(PR_FALSE);
>+  if (cp) {
>+    return cp->SendAsyncMessage(nsString(aMessage), nsString(aJSON));
>+  }

Should we assert or something if cp is false.  I am worried a bit that someone will not realize the content process hasn't started and not understand that their events are dropped.

>-    NS_ASSERTION(mContext || (aChrome && !aParentManager),
>+    NS_ASSERTION(mContext || (aChrome && !aParentManager) || aProcessManager,
>                  "Should have mContext in non-global manager!");

probably should update this assertion text.


>+ContentParent::RecvSyncMessage(const nsString& aMsg, const nsString& aJSON,
>+                               nsTArray<nsString>* aRetvals)
>+{
>+  nsRefPtr<nsFrameMessageManager> ppm = nsFrameMessageManager::sParentProcessManager;
>+  if (ppm) {
>+    ppm->ReceiveMessage(static_cast<nsIContentFrameMessageManager*>(ppm.get()),

r+ w/ nits addressed.

Comment 24

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

> nit: no need for braces around one statement.
Per coding style even a single-line controlled statements should have braces.

Comment 25

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

Attachment: patch

Comment 26

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

I guess this can't be pushed to m-c before b5

Comment 27

[Doug Turner (:dougt)]

marking blocking because it blocks other b1 bugs.

Comment 28

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/1d7e7ccda33c

Comment 29

[Mark Banner (:standard8)]

After discussion with Olli, I pushed a bustage fix for some configurations of builds (e.g. shared, non-ipc like Thunderbird uses):

http://hg.mozilla.org/mozilla-central/rev/51d17027ff45