566024 - Add a 'global' messageManager

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Alon Zakai (:azakai)]

Various small bits of functionality need modifications for electrolysis (e.g., the addonManager, and likely many others). It would be nice to not have to write a new IPDL protocol with all the relevant boilerplate. The mobile UI code, for example, has a nice way to send messages chrome<-->content, using the messageManager and it's JS API which looks something like this:

  Browser.messagePasser.addMessageListener("Foo",
this);
  [..]
  receiveFoo: function
receiveFoo(message, location) {
  [..]
  sendMessage("Foo", location);

However the messageManager is tied to the DOM (the browser/window objects, etc.). For non-DOM code - say, the addonManager, which is an XPCOM component written in JS - there is currently no way to use the messageManager. However the C++ code for the messageManager should be reusable for a 'global messageManager' which would be usable by arbitrary (priviliged) JS code.

Comment 1

[Alon Zakai (:azakai)]

Attachment: Proof of concept patch for discussion purposes (remotes the addonManager)

This patch is *not* meant as a serious implementation. It is mainly to promote discussion about what is the right way to do this. It basically is a working patch to remote the addonManager JS component. The patch works in the e10s sense - messages are passed, functions called in the right place, etc., but addons are not actually installed due to an unrelated permissions issue.

1. Please ignore the C++ parts, they are a crude hack just to get things working. I tacked on some functions to an existing service, which has nothing to do with the current topic (it was just the fastest way to get something working). Should a new service be created for this?

2. The C++ basically just saves the first actual messageManager on the parent and child, as the 'global' one. This is of course wrong for several reasons. Probably new messageManagers should be created?

3. The JS code here shows the motivation for this entire issue: We e10s-enable an existing JS component by just adding a block of code to it - without changing any of the existing code at all. The JS code basically tells the child to forward calls to the parent, and the parent to listen to them, and apply them normally. This seems to me to be a clean and efficient approach for this specific problem, which again was the whole motivation.

(The JS code here still has some boilerplate/redundancy in it. That can be improved, optimally I would like to just call an 'electrolify()' function on the component with some parameters as to what should be remoted, etc.)

Comment 2

[Benjamin Stover (:stechz)]

I really hope we have a way for components to use the message manager.  We need it for form history.  The components are being rewritten in JS for easier maintainability, which is a good thing.

I understand we don't want arbitrary components in these processes, but what about a manifest of allowed ones?

Comment 3

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

So just add a new component and implement is using the nsFrameMessageManager.h/.cpp as base class.
See that is done in TabChild.cpp/nsInProcessMessageManager.cpp. Exclude all the
event handling.
I'm not sure what to do with javascript context though. If loadFrameScript isn't needed, the there shouldn't be need for any special javascript context.

Comment 4

[Benjamin Smedberg]

Hrm, this is not the change I was imagining.

This patch adds a global message manager in the *content* process. I don't think this is a good idea. I think that all messages in the content process should be associated with the message manager for that content/IFrameEmbedding.

What I was imagining is that the content process code would stay exactly the same. But that in the chrome process, JS components would have a way of being notified that new toplevel content was created, and could get that message manager and call .addMessageListener/.loadFrameScript on it. You might need some extra context parameters in message listeners so that you didn't hold message managers alive in closures unnecessarily.

Comment 5

[Benjamin Stover (:stechz)]

> I'm not sure what to do with javascript context though. If loadFrameScript
> isn't needed, the there shouldn't be need for any special javascript context.

We really need loadFrameScript.  Ideally for us, it would load that script on every browser created so far, and listen for new browsers being created as well.

> This patch adds a global message manager in the *content* process. I don't
> think this is a good idea. I think that all messages in the content process
> should be associated with the message manager for that content/IFrameEmbedding.

Yes, that's exactly what we need for login manager and satchel.  No need for global message manager in content.

Comment 6

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

So the global message manager would be on chrome side only? And it would 
send messages to content process TabChildGlobals (i.e. content process per tab message managers)?
That sounds even simpler to do. The global message manager would be the
parent of all window level message managers in chrome.
Though, I'm not quite sure which JSContext the global manager should use.
Perhaps it could cache the context when addMessageListener is called and use
that when calling the listener.

Comment 7

[Benjamin Stover (:stechz)]

> Perhaps it could cache the context when addMessageListener is called and use
> that when calling the listener.

That sounds like the correct thing to do.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #5)

> > This patch adds a global message manager in the *content* process. I don't
> > think this is a good idea. I think that all messages in the content process
> > should be associated with the message manager for that content/IFrameEmbedding.
> 
> Yes, that's exactly what we need for login manager and satchel.  No need for
> global message manager in content.

Agreed

Comment 9

[Alon Zakai (:azakai)]

(In reply to comment #4)
> This patch adds a global message manager in the *content* process. I don't
> think this is a good idea. I think that all messages in the content process
> should be associated with the message manager for that content/IFrameEmbedding.
> 
> What I was imagining is that the content process code would stay exactly the
> same. But that in the chrome process, JS components would have a way of being
> notified that new toplevel content was created, and could get that message
> manager and call .addMessageListener/.loadFrameScript on it. You might need
> some extra context parameters in message listeners so that you didn't hold
> message managers alive in closures unnecessarily.

This was how I initially wanted to do things, but the problem is that if you are a JS component (called from C++, that in turn was originally called from JS) in the content process, then you don't have a handle to the TabChild relevant for you. In other words you don't know what tab called you.

If we had one process per tab then it would be trivial to add a global singleton to the (only existing) TabChild, but in the current architecture (multiple content tabs in one process), is there a good way to do this? It seems like we can make a note of the current tab when called from some tab's JS code, but storing that naively in a global location sounds problematic (in particular it assumes the code is not and will not be multithreaded).

Comment 10

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

(In reply to comment #6)
> Though, I'm not quite sure which JSContext the global manager should use.
> Perhaps it could cache the context when addMessageListener is called and use
> that when calling the listener.
Actually, I think we use the context from the window in which the <browser> element is in. Simple, just pass the context from child mm to parent, and
parent uses either its own mContext or the one from child.




(In reply to comment #9)
> This was how I initially wanted to do things, but the problem is that if you
> are a JS component (called from C++, that in turn was originally called from
> JS) in the content process, then you don't have a handle to the TabChild
> relevant for you. In other words you don't know what tab called you.
> 
> If we had one process per tab then it would be trivial to add a global
> singleton to the (only existing) TabChild, but in the current architecture
> (multiple content tabs in one process), is there a good way to do this? It
> seems like we can make a note of the current tab when called from some tab's JS
> code, but storing that naively in a global location sounds problematic (in
> particular it assumes the code is not and will not be multithreaded).
At least in common cases something in the web page triggers something in the
components. So usually components get some reference to the content page
(element, document, window?). And from those one can access the current TabChild
using Docshelltreeowner. Would that be enough? Or are there components which
need to run in content process and randomly notify chrome process about something?

Comment 11

[Alon Zakai (:azakai)]

(In reply to comment #10)
> At least in common cases something in the web page triggers something in the
> components. So usually components get some reference to the content page
> (element, document, window?). And from those one can access the current
> TabChild
> using Docshelltreeowner. Would that be enough? Or are there components which
> need to run in content process and randomly notify chrome process about
> something?

I am not sure if this covers all the necessary cases, but even assuming it does, I think it would mean adding a lot of additional parameters to function calls. For example, in the addon manager, we have

1. Some web page's JS calls InstallTrigger.install(), which runs some C++ code. This code knows the window that called it.

2. That C++ code then calls the actual addonManager component which is written in JS. That call has no need to pass a window, document, etc. - it just sends the necessary information about what addons to install (their URIs, icons, etc.).

(Note that not passing a window makes sense since the addons will be installed globally, and not just for this tab.)

So adding another parameter here - which might as well just be the messageManager, btw - clutters the addonManager API. In the non-e10s case, that parameter would be entirely unnecessary, but even in the e10s case it seems odd. A 'global' messageManager as in the above patch avoids all that. And this same issue would recur in other JS components.

But again, I would prefer not to create an additional global messageManager, if there were an elegant way to do it that doesn't change API calls in various places needlessly.

Comment 12

[Benjamin Smedberg]

It seems like the correct fix here might be to implement InstallTrigger.install as a JS function and do the remoting in there, instead of having the addonmanager component do it.

Comment 13

[Alon Zakai (:azakai)]

(In reply to comment #12)
> It seems like the correct fix here might be to implement InstallTrigger.install
> as a JS function and do the remoting in there, instead of having the
> addonmanager component do it.

Well, that would require a rewrite of the amInstallTrigger code (which was recently rewritten into the current form). And it might require rewriting additional code as well in other components. Whereas adding a global messageManager in content removes the need for all that rewriting.

Comment 14

[Doug Turner (:dougt)]

it is clear that we need js components that run in the chrome process to be able to install frame scripts.  This work should be completed as soon as possible as it blocks some of the fennec work (login manager comes to mind).

if we find a concrete requirement for installing frame scripts from a content process, we can address that in another bug.

Smaug, could you expose the message manager to js components.

Comment 15

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

Will take couple of days before I get to this.
(Merging m-c to e10s takes time)

Comment 16

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

Attachment: almost there

Comment 17

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

Attachment: patch

So this works reasonable well, but asking some feedback if this is the behavior we want.
The patch is for m-c, but should apply almost cleanly to e10s (may need some --fuzz).
Includes tests.

This should fix also Bug 567290.

Comment 18

[Benjamin Smedberg]

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

Hrm. This patch actually adds a global message manager. I figured that what you'd actually do is just fire an observer notification whenever we created a new message manager and let code hook up to message managers as they are created. I'm not sure either way is better, but wondered if you had considered that option.

Comment 19

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

(In reply to comment #18)
> (From update of attachment 447836 [details] [diff] [review])
> Hrm. This patch actually adds a global message manager.
Yes. This bug is "Add a 'global' messageManager" ;)

> I figured that what
> you'd actually do is just fire an observer notification whenever we created a
> new message manager and let code hook up to message managers as they are
> created. I'm not sure either way is better, but wondered if you had considered
> that option.
No I haven't considered that. Interesting idea.
Not as easy to use though. And doesn't allow one to send scripts to all
existings TabChildGlobals if your code runs after messageManagers have been
created.

Comment 20

[Benjamin Stover (:stechz)]

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

On vacation for a week.  Doug, could you give feedback on this?

Comment 21

[Doug Turner (:dougt)]

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

This looks good.

why the change to dom/base/nsGlobalWindow.h?

I do not understand the change to  nsFrameLoader::GetMessageManager.  Isn't nsFrameMessageManager
 a nsIChromeFrameMessageManager… always?


+  PRBool IsGlobal() { return mGlobal; }
+  PRBool IsWindowLevel() { return mParentManager && mParentManager->IsGlobal(); }


These are somewhat confusing names.

Probably could clean this up and get jst to r+

Comment 22

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

(In reply to comment #21)
> (From update of attachment 447836 [details] [diff] [review])
> This looks good.
> 
> why the change to dom/base/nsGlobalWindow.h?
IIRC, I couldn't access mMessageManager in nsGlobalWindow::CleanUp
I wouldn't remove the protected. But I can re-test.


> 
> I do not understand the change to  nsFrameLoader::GetMessageManager.  Isn't
> nsFrameMessageManager
>  a nsIChromeFrameMessageManager… always?
You mean in nsFrameLoader::GetMessageManager? I just wanted to be sure that
whatever .messageManager is, it is the same thing what QI gives.


> 
> +  PRBool IsGlobal() { return mGlobal; }
> +  PRBool IsWindowLevel() { return mParentManager &&
> mParentManager->IsGlobal(); }
> 
> 
> These are somewhat confusing names.
Really? We have global messageManager, we have window level messageManager
and we have frame level messageManager.
Though, if you have suggestions for better names, I'd be happy the
change these.

Comment 23

[Benjamin Smedberg]

With this patch, how does a client of the global message manager know when a new window is created, so that it can .loadFrameScript?

Comment 24

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

It doesn't, but of course client can always use pending scripts.

If there is a need for notifications for new windows, that could be added sure.
Either by having special message name for those, or using
another listener mechanism.
That is anyway an additional feature for messageManager. Is that needed for each
messageManager or just for global one?

Comment 25

[Benjamin Smedberg]

pending scripts?

My use-case is to implement window.InstallTrigger using the message manager: when each client-side manager is first created, we run a script that hooks up a DOMWindowCreated listener, and then sets window.wrappedJSObject.InstallTrigger = { };

I don't think it's needed for individual message managers, because the UI code knows when a new browser is created, and can explicitly initialize it.

Comment 26

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

(In reply to comment #25)
> pending scripts?
You can add pending scripts which are run when new message managers become
available. Those scripts should be used with care, since the url is
kept alive for the life time of messageManager.
But basically you can have a script which is run whenever a new
tab is added:
globalMessageManager.loadFrameScript("chrome://some-initializer-script.js", true);


> 
> My use-case is to implement window.InstallTrigger using the message manager:
> when each client-side manager is first created, we run a script that hooks up a
> DOMWindowCreated listener, and then sets window.wrappedJSObject.InstallTrigger
> = { };

So the initializer script could add DOMWindowCreated DOM event listener.

Comment 27

[Doug Turner (:dougt)]

next steps?

Comment 28

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

Review?

Comment 29

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

...if bsmedberg isn't too strongly against the API.
Something new can be added to make it easier to detect when a new
tab is added, if needed, but in some other bug, I hope.

Comment 30

[Benjamin Smedberg]

I'm not against it. I'm worried that it might not completely solve the jetpack needs, but I'm pretty sure it solves the mochitest/fennec needs, so let's get it in and iterate later if necessary.

Comment 31

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

http://hg.mozilla.org/mozilla-central/rev/b84d0be52070