776825 - Separate message managers into senders and broadcasters

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We have multiple content processes now.  We have DOM APIs in JS that use the process message managers to implement remoted DOM APIs.

I have no idea what would happen if we used this code with multiple content processes running.  It would likely either break, or if it's looping over all child processes, leak sensitive data to processes that don't have rights (as well as wasting CPU time).

I came across this in bug 776649, because I need to get a process context (PBrowserParent or PContentParent) in order to apply security checks.

This is extremely critical.  If we can't get a fix soon we're going to need to rewrite our remoted JS DOM-API impls in C++.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

A simpler potential bandaid here may be removing parentprocessmessagemanager and changing the JS impls to use the frame message managers.

Comment 2

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

We have a tree of parent process managers in the parent process.
Bug 669640

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There are three problems here.

 (1) This implementation leaks information to processes that don't have permission to access it.  A content process could listen to mm messages and see all a user's contacts, all telephony events, etc.

 (2) On higher-end phones, we could have 20 or more content processes.  It's completely unacceptable to wake up every process in the system just because we don't track the origin of messages.  What would you say if I tried to land a patch that delivered every input event Firefox received to every tab, or, whenever we repainted one tab, went ahead and repainted *every* tab.  It would not fly.

 (3) The performance problems caused by this wouldn't show up in local testing.  Developers would only have one content process open to test their API, and test with small messages.  But then in the field, code would send a 1MB in-memory blob, and it would unexpectedly end up being 20x more expensive as it was broadcast to every process in an active system.  This is the old kind of problem with synchronous disk IO, it bites hardest in the extreme cases, not the common case.

So here's what I'm going to do

 - disable ppmm's ability to send messages for b2g, at least.  If it's not used elsewhere maybe everything.
 - keep addMessageListener/removeMessageListener working the same way
 - add a parameter to messages that's the specific "child message manager" they originated from.  (That name is very confusing ...)
 - in current ppmm users, store the origin mm as part of requests, and only respond to that

I guess I'll have to see whether any current ppmm users really need a "broadcast" functionality, but I sort of hope not.

Comment 4

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

Don't understand 1), nor 2) or 3) :)
There is a parent process mm for each child process and there is the top level manager.

All the messages have already pointer to the manager they are coming from. It is the .target.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What happens when this code

http://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/ContactService.jsm#127

is called.

Comment 6

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

That code explicitly uses the global process message manager, so the message is sent to all the
child processes. I don't know why the code does that and not aMessage.target.sendAsyncMessage().

global process message manager is no different to global frame message manager or window level
message manager. All those broadcast messages to all the descendant managers.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That is a foot-nuclear-missile.  We need to turn that off, or make it extremely explicit what's happening.

Is there any documentation of this hierarchy?

Comment 8

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

Don't know about documentation.

I guess after dealing with DOM trees and event handling long enough I don't quite see the
problem with message manager trees either. If the API is easy to use wrong way, it obviously needs
some changes (but I really like the consistency and simplicity of the API).

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For example, do messages sent to "process mms" also get dispatched to "frame mms"?  The MDC page [1] doesn't mention process mms at all.

For messages that might end up being delivered to every content process in the system (really, any message that might dispatched to a list of listeners), I think we should at least add a string parameter that must say "broadcast", so that it's clear to authors and readers that that's the semantics.

But better docs would certainly help too! ;)

[1] https://developer.mozilla.org/en/The_message_manager

Comment 10

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> For example, do messages sent to "process mms" also get dispatched to "frame
> mms"?
No

Better docs would help, yes.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This bug needs to block any further JS cross-process code.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Make async message-manager messages not broadcast to child managers by default, add a new interface for sending broadcasting messages, and rename sendAsyncMessage to postMessage

I apologize for making the interface here less clean, but we found out that every (non-xul-fennec) current user of parentprocessmessagemanager, including some who should have known better, created security and perf bugs with it :/.  I agree that that's partially a failure of review, but some of this code was even written by message manager ninjas from the xul-fennec days.  This interface is just too much of a footgun.

Here is what I absolutely think should be changed: parentprocessmessagemanager should either not have a broadcasting interface, or it should be very explicit.  There are other things in this patch that I changed that I care less about.

There's a lot of design space to achieve this goal.  Here's what this patch does
 - split the send-message-to-other-mm interface off from broadcast-message.  I did this for the common IDL so that all mm's continue to have the same interface.  This means parentprocessmessagemanager can still broadcast messages, but it's very clear when it's doing so.

 - added some documentation based on my admittedly still rather fuzzy understanding of message manager.  Would really appreciate you taking a look over it.

 - rename sendAsyncMessage to postMessage.  This was done for two reasons: to be more familiar to programmers used to DOM postMessage, and to more closely match the usage of the win32 API that the name "postMessage" was derived from: PostMessage() delivers a message asynchronously, and SendMessage() delivers the message synchronously.  There may be a reason why you didn't use the name postMessage originally --- would love to know.

Alright, let's let the iteration begin :).

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: sed patch that changes current calllers

Fails the messagemanager tests, of course, but not worth bothering with until we settle on the interface changes.

Comment 14

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

Comment on attachment 645652 [details] [diff] [review]
Make async message-manager messages not broadcast to child managers by default, add a new interface for sending broadcasting messages, and rename sendAsyncMessage to postMessage


>+ * The second type is the "window message manager".  It is like frame
>+ * message managers, but can broadcast messages to all the frame
>+ * element sub-elements of the enclosing window.

There is also global message manager, under which all the window managers are


>+ *
>+ * The third type is the "process message manager", which is somewhat
>+ * special.  In child processes, there is exactly one global process
>+ * message manager.  The child-process message manager has a
>+ * correlated parent-process message manager in its parent process.
>+ * Messages sent from the child-process message manager are delivered
>+ * to its parent-process message manager.
>+ *
>+ * In the parent process, however, there is a global parent-process
>+ * message manager which contains each parent-side message manager for
>+ * the subprocesses in an hierarchy.  Parent-side message sending code
>+ * can send messages to specific child processes, or broadcast
>+ * messages to all child processes.
>+ *
>+ * Process message managers are not hierarchically above "window
>+ * message managers", meaning that they don't broadcast messages to
>+ * windows.
>+ *
>+ * WARNING: great care must be taken in the use of parent-process
>+ * message managers.  Broadcasting messages to all child processes can
>+ * easily cause security and performance problems.  Instead,
>+ * parent-process message managers should usually only reply to the
>+ * originating child-process message manager, as in the following
>+ * example
>+ *
>+ *  const ParentProcessListener = {
>+ *    receiveMessage: function(aMessage) {
>+ *      let childMM = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager);
>+ *      switch (aMessage.name) {
>+ *      case "Foo:Request":
>+ *        // service request
>+ *        childMM.postMessage("Foo:Response", { data });
>+ *      }
>+ *    }
>+ *  };
>+ */
>+
> [scriptable, function, uuid(938fcb95-3d63-46be-aa72-94d08fd3b418)]
> interface nsIFrameMessageListener : nsISupports
> {
>   /**
>    * This is for JS only.
>    * receiveMessage is called with one parameter, which has the following
>    * properties:
>    *   {
>    *     name:    %message name%,
>    *     sync:    %true or false%.
>    *     json:    %json object or null%,
>+   *     target:  %the targeted message manager object%
>    *     objects: %array of handles or null, always null if sync is false%
>    *   }
>    * @note objects property isn't implemented yet.
>    *
>    * if the message is synchronous, possible return value is sent back
>    * as JSON.
>    *
>    * When the listener is called, 'this' value is the target of the message.
>+   *
>+   * XXX what happens if a listener mutates the argument to
>+   * receiveMessage?  Does each listener get its own copy?
Each listener gets its own copy


>+   *
>+   * XXX if there are multiple receivers of synchronous messages, and
>+   * all of them return values, which return value(s) are sent back to
>+   * content? 
All. That is why the return value in the client is an array


>+ What's returned to the sender if a listener of a
>+   * synchronous message doesn't return a value?
Return value of syncMessage is always an array.


>+   *
>+   * XXX if we pass an argument to this function, and the listener can
>+   * return a value, why is the signature void/(void) instead of
>+   * jsval/(jsval)?
>    */
Because XPIDL hasn't always been capable to handle what was needed here.

> [scriptable, builtinclass, uuid(9be42627-a5db-456f-8de2-9097da45a8c3)]
> interface nsIFrameMessageManager : nsISupports
> {
>-  void addMessageListener(in AString aMessage, in nsIFrameMessageListener aListener);
>-  void removeMessageListener(in AString aMessage, in nsIFrameMessageListener aListener);
>-  [implicit_jscontext,optional_argc]
>-  void sendAsyncMessage([optional] in AString messageName, [optional] in jsval obj);
>+  /**
>+   * Register |listener| to receive |messageName|.  All listener
>+   * callbacks for a particular message are invoked when that message
>+   * is received.
>+   *
>+   * The message manager holds a strong ref to |listener|.  (XXX
>+   * right?) 
Of course

>+ Client code should call removeMessageListener() to avoid
>+   * leaking their listeners.
Sure, though stuff is cycle collected like event listeners, so things are in fmm when the
frame goes away



>+   *
>+   * XXX what happens if a caller registers the same listener for the
>+   * same message twice?
Same what happens with event listeners, nothing.



>+  /**
>+   * Send |messageName| and |obj| to the "other side" of this message
>+   * manager.  This invokes listeners who registered for
>+   * |messageName|.  The semantics are similar to postMessage(), but
>+   * the format of the data sent is different.
Which is why the method name shouldn't be the same. 
What would postMessage do in global/window/global process message managers?

>+   * If this interface is used with a window message manager, all
>+   * frame message managers will be broadcast to.  (XXX right?)
Yes



> interface nsISyncMessageSender : nsIFrameMessageManager
> {
>   /**
>-   * Returns an array of JSON objects.
>+   * Like |postMessage()|, except blocks until all listeners of the
>+   * message have been invoked.  An array is returned which contains
>+   * the return values from each listener invoked.  (XXX is that
>+   * correct?)
Yes. Except that it isn't like postMessage


r- mainly because we shouldn't reuse very well known method name postMessage for something which does different thing.
Also, the patch obviously can't land if all the callers aren't fixed too.

Comment 15

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

Though, I guess postMessage is already inconsistent (window vs. workers), but let's not add yet another flavor of inconsistencies.

Comment 16

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

Comment on attachment 645652 [details] [diff] [review]
Make async message-manager messages not broadcast to child managers by default, add a new interface for sending broadcasting messages, and rename sendAsyncMessage to postMessage


>+  [implicit_jscontext, optional_argc]
>+  void postMessageAndBroadcastToAllChildMessageManagers(
>+    [optional] in AString messageName,
>+    [optional] in jsval obj);
Also, it makes no sense for child part of the message managers to have this method.

Parent only stuff can be put to nsIChromeFrameMessageManager

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 645652 [details] [diff] [review]
> Make async message-manager messages not broadcast to child managers by
> default, add a new interface for sending broadcasting messages, and rename
> sendAsyncMessage to postMessage
> 
> 
> >+ * The second type is the "window message manager".  It is like frame
> >+ * message managers, but can broadcast messages to all the frame
> >+ * element sub-elements of the enclosing window.
> 
> There is also global message manager, under which all the window managers are
> 

I see.  And they're not connected to the process managers, right?

> >+ What's returned to the sender if a listener of a
> >+   * synchronous message doesn't return a value?
> Return value of syncMessage is always an array.
> 

Right, but if a listener doesn't return a value, what happens?  Does the array get an |undefined| entry, or is the array is one element shorter?  For example, if there were two listeners, and neither returned a value, would [ undefined, undefined ] be returned, or [ ]?

> >+   *
> >+   * XXX if we pass an argument to this function, and the listener can
> >+   * return a value, why is the signature void/(void) instead of
> >+   * jsval/(jsval)?
> >    */
> Because XPIDL hasn't always been capable to handle what was needed here.
> 

I will gladly stay away from this then! ;)

> >+  /**
> >+   * Send |messageName| and |obj| to the "other side" of this message
> >+   * manager.  This invokes listeners who registered for
> >+   * |messageName|.  The semantics are similar to postMessage(), but
> >+   * the format of the data sent is different.
> Which is why the method name shouldn't be the same. 

How do you feel about making the name more similar, but slightly different?  Maybe postNamedMessage() or something like that?  Here and below, I'm not entirely sure what differences you're referring to.

> What would postMessage do in global/window/global process message managers?
> 

I expect parentprocessmessagemanager.postMessage() to do nothing.  (Should probably throw an exception, in fact.)  For the other two, are there not corresponding objects on the "child side" on which listeners could register to receive the message?  OK, I see why not now.  So they would be like parentprocessmessagemanager.

> 
> r- mainly because we shouldn't reuse very well known method name postMessage
> for something which does different thing.

Based on what you wrote, maybe it makes sense to have two different APIs: one for the "aggregating" message managers on the parent side that don't have a corresponding object on the child side(s), and a separate one for the 1:1 message managers.  (And, I guess, continuing to have a third one in order to expose the sendSyncMessage().)  It's not obvious to me that the same sendAsyncMessage() method does quite different things for each type of message manager.

Since we'll be touching ~200KB of code according to attachment 645653 [details] [diff] [review], we might as well spend some time on naming.  For the 1:1 managers, do you agree with sticking closer to the win32/DOM naming convention for post/send, or would you prefer something else?  Here's a strawman proposal

 nsIFrameMessageManager:
   // Asynchronously delivers this message to our "other side".
   postNamedMessage();

 nsIAggregatingMessageManager:
   // Asynchronously delivers this message to all transitively aggregated message managers.
   broadcastNamedMessageToAggregates();

(And then the other interface to expose sendSyncMessage().)

Comment 18

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #17)
> 
> I see.  And they're not connected to the process managers, right?
No. There are two trees. 
globalMM->windowMMs->frameMMs
and
globalProcessMM->processMMs


> 
> Right, but if a listener doesn't return a value, what happens?  Does the
> array get an |undefined| entry, or is the array is one element shorter?  For
> example, if there were two listeners, and neither returned a value, would [
> undefined, undefined ] be returned, or [ ]?
> 
> 
> How do you feel about making the name more similar, but slightly different? 
> Maybe postNamedMessage() or something like that?  Here and below, I'm not
> entirely sure what differences you're referring to.
I really prefer having async in the name.


> Based on what you wrote, maybe it makes sense to have two different APIs:
> one for the "aggregating" message managers on the parent side that don't
> have a corresponding object on the child side(s), and a separate one for the
> 1:1 message managers.  (And, I guess, continuing to have a third one in
> order to expose the sendSyncMessage().)  It's not obvious to me that the
> same sendAsyncMessage() method does quite different things for each type of
> message manager.
> 
There is only one implementation of sendAsyncMessage, so it does the same thing.



> Since we'll be touching ~200KB of code according to attachment 645653 [details] [diff] [review]
> [details] [diff] [review], we might as well spend some time on naming.  For
> the 1:1 managers, do you agree with sticking closer to the win32/DOM naming
> convention for post/send, or would you prefer something else?
send is good, post not so much. I don't understand the need to change the method name.


>  nsIAggregatingMessageManager:
>    // Asynchronously delivers this message to all transitively aggregated
> message managers.
>    broadcastNamedMessageToAggregates();
broadcastAsyncMessage()

Comment 19

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

nsIAggregatingMessageManager sounds pretty bad.
Perhaps nsIContainerMessageManager

I still wonder why this is such a problem. I haven't heard similar problems with
fmm vs window mm, and the setup is the same

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Olli Pettay [:smaug] from comment #18)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #17)
> > 
> > 
> > Right, but if a listener doesn't return a value, what happens?  Does the
> > array get an |undefined| entry, or is the array is one element shorter?  For
> > example, if there were two listeners, and neither returned a value, would [
> > undefined, undefined ] be returned, or [ ]?
> > 

I'll assume it's the former, or see if I can decipher the jsapi, or let you correct the comment ;).

> > Based on what you wrote, maybe it makes sense to have two different APIs:
> > one for the "aggregating" message managers on the parent side that don't
> > have a corresponding object on the child side(s), and a separate one for the
> > 1:1 message managers.  (And, I guess, continuing to have a third one in
> > order to expose the sendSyncMessage().)  It's not obvious to me that the
> > same sendAsyncMessage() method does quite different things for each type of
> > message manager.
> > 
> There is only one implementation of sendAsyncMessage, so it does the same
> thing.
> 

There's only one *implementation*, but

  mm.sendAsyncMessage(...)

might do something entirely different than another call to

  mm.sendAsyncMessage(...)

and it's not obvious at the callsite.

(In reply to Olli Pettay [:smaug] from comment #19)
> nsIAggregatingMessageManager sounds pretty bad.
> Perhaps nsIContainerMessageManager
> 
> I still wonder why this is such a problem. I haven't heard similar problems
> with
> fmm vs window mm, and the setup is the same

This appears not to be used much currently.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Separate message managers into "senders" and "broadcasters". Interface changes only.

Unlike the previous interface, doesn't fix the C++, but it's somewhat nontrivial in this case.  Would like to settle on an interface first.

Incorporates above feedback and makes long comment at top more precise.

Comment 22

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

Comment on attachment 646067 [details] [diff] [review]
Separate message managers into "senders" and "broadcasters".  Interface changes only.

>+ * Child-side message managers can send synchronous messages to their
>+ * parent side, but not the other way around.
A bit oddly said. Perhaps something like
"Only child-side message managers can send synchronous messages."

>+ *  const ParentProcessListener = {
>+ *    receiveMessage: function(aMessage) {
>+ *      let childMM = aMessage.target.QueryInterface(Ci.nsIMessageSender);
We should make sure this QI isn't needed. .target should expose the necessary APIs automatically.
(Need to have right classinfo in nsDOMClassInfo)

>+interface nsIMessageListener : nsISupports
> {
>   /**
>    * This is for JS only.
>    * receiveMessage is called with one parameter, which has the following
>    * properties:
>    *   {
>    *     name:    %message name%,
>    *     sync:    %true or false%.
>    *     json:    %json object or null%,
>+   *     target:  %the targeted message manager object%
You can blame me, but in case of fmm target is the browser/iframe element, not mm, since it is hard to get back to
dom element from mm, but easy from element to mm (and this was the original behavior).
In other cases target is the mm (also on client side).



I like :)

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

My plan here is to
 - write patch to fix the C++, add new DOMClassInfo feature
 - write patch to update all in-tree callers
 - prepare documentation after proving out interface
 - send notice of API change to ... m.d.platform + m.d.extensions?
 - start working on followup work on top of the patch queue
 - then after waiting X units of time, land the whole schmeer

What's an appropriate X and unit of time?  1, "week"?

Comment 24

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

The changes here will be done to the code which bug 759427 modifies quite a bit, so need
to coordinate which one goes in first (I assume bug 759427).

Comment 25

[Philipp von Weitershausen [:philikon]]

yoink

Comment 26

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (rebased): Separate message managers into "senders" and "broadcasters". Interface changes only.

Comment 27

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): Update implementations of the late nsIFrameMessageManager

Comment 28

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v1): Fix uses of the late nsIFrameMessageManager

Comment 29

[Philipp von Weitershausen [:philikon]]

Try build for v1 patches Parts 1-3: https://tbpl.mozilla.org/?tree=Try&rev=8308cb820c8a

Comment 30

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

Comment on attachment 649914 [details] [diff] [review]
Part 3 (v1): Fix uses of the late nsIFrameMessageManager

Have you checked how many addons use nsIFrameMessageManager?

Comment 31

[Philipp von Weitershausen [:philikon]]

(In reply to Olli Pettay [:smaug] from comment #30)
> Have you checked how many addons use nsIFrameMessageManager?

I have now. There are a total of 12 occurrences across all of AMO:

* 6 of these occurrences are in the same JS library across different addons all written by SHIMODA "Piro" Hiroshi:

  if (target instanceof Components.interfaces.nsIFrameMessageManager)

  This can easily be adjusted to require nsIMessageSender when available. Piro is very responsive, I've worked with him before. I'll be happy to reach out to him.

* Adblock Plus declares a lazy service getter for parentprocessmessagemanager but doesn't actually seem to use it in its code. I can reach out to Wladimir.

* The remaining 5 occurrences are (outdated) copies or forks of Adblock Plus.

I think we have nothing to worry about.

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 649914 [details] [diff] [review]
Part 3 (v1): Fix uses of the late nsIFrameMessageManager

A lot of the ppmm users need to be fixed, but you know this and the bugs are on file :).

For this pattern

-    let mm = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager);
+    let mm = aMessage.target.QueryInterface(Ci.nsIMessageSender);

we want to (as smaug said) set up DOM classinfo support so we don't need to QI.  /me waves hands some more.

Comment 33

[Philipp von Weitershausen [:philikon]]

Attachment: IDL conundrum

So cjones and I realized that nsIChromeFrameMessageManager doesn't fit into the new separation of broadcasters and senders very well. I can see three solutions, neither of which really blow my socks off. I've described them at length, including the IDL code, in this file. It all starts with the XXXphilikon comment. Interested in your feedback, smaug and cjones. Thanks!

Comment 34

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

Comment on attachment 650421 [details]
IDL conundrum


> * I see three solutions to untangling this hierarchy:
> *
> * (a) We don't care that the message managers on frameloaders are
> * broadcasters and keep nsIChromeFrameMessageManager as an extension
> * of nsITreeItemMessageManager and nsIMessageBroadcaster.
> *
> * (b) We make the message managers on frameloaders implement
> * nsIMessageSender. The global and window message managers implement
> * nsITreeItemMessageManager, an extension of nsIMessageBroadcaster
> * (see below).
Also gppm should implement nsITreeItemMessageManager


> The frame script loading stuff that used to be on
> * nsIChromeFrameMessageManager moves to an entirely separate
> * interface nsIFrameScriptLoader. As a result, consumers will have to
> * QI their message manager to this interface if they want to load a
> * frame script (a small drawback of this solution).
> *
> * (c) We keep nsIChromeFrameMessageManager, but it's merely an
> * extension of nsIMessageListenerManager named
> * nsIFrameScriptMessageManager below. From that we derive
> * nsITreeItemFrameMessageManager which is implemented by the global
> * and window message managers. It adds the tree traversal methods and
> * `broadcastAsyncMessage()` (ideally it would just inherit from
> * nsIMessageBroadcaster, but XPIDL doens't seem to support multiple
> * inheritance). For the message managers on frameloaders we have
> * nsIFrameloaderMessageManager which extends
> * nsIFrameScriptMessageManager by `sendAsyncMessage()` (ideally it
> * would just mix in nsIMessageSender).
> *
> */
>

B sounds ok, especially if we can get classinfo working properly in each case.

Comment 35

[Philipp von Weitershausen [:philikon]]

(In reply to Olli Pettay [:smaug] from comment #34)
> Also gppm should implement nsITreeItemMessageManager

Good point.

> B sounds ok, especially if we can get classinfo working properly in each
> case.

(b) is probably what I would prefer as well. Can you elaborate on the classinfo stuff? Can we make it so that message managers automatically expose nsIFrameScriptLoader?

Comment 36

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

If you look at nsDOMClassInfo, it has stuff for messagemanager, the client part.
We could, I hope add stuff for parent part too, and conditionally
have NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(MessageManager) inside NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsFrameMessageManager).
(There is NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL )
You'd need to add stuff to nsDOMClassInfoClasses.h too.

Comment 37

[Philipp von Weitershausen [:philikon]]

I just realized that we might as well fold nsITreeItemMessageManager (childCount and getChildAt) into nsIMessageBroadcaster. After all the whole point of being a broadcaster is that it has child-MMs to delegate the message to. Otherwise it would just be a regular sender. Yay fewer interfaces!

Comment 38

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): IDL changes

Alright, here's a reworked version of the interface changes that implements option (b):

* nuke nsIChromeFrameMessageManager by factoring the frame script loading capabilities into nsIFrameScriptLoader

* fold nsITreeItemFrameMessageManager into nsIMessageBroadcaster, because it only makes sense to be a broadcaster if you have child MMs

Comment 39

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v2): Update implementations of the late nsIFrameMessageManager

Update the C++ implementations to reflect the v2 interface changes. Most important change is that I introduced another flag on nsFrameMessageManager (mIsBroadcaster) to make the QueryInterface map work correctly in all cases. This parameter defaults to false in the ctor, and only in a three places we actually pass true here:

* global message manager
* its children, the window message managers
* parent process message manager

Comment 40

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v2): Fix uses of the late nsIFrameMessageManager

This is much like v1, except it also has to do s/nsIChromeFrameMessageManager/nsIMessageBroadcaster. The additional QI'ing to nsIFrameScriptLoader really isn't that bad, so I'm quite happy we went with option (b) here.

Comment 41

[Philipp von Weitershausen [:philikon]]

Try build for v2 patches part 1-3: https://tbpl.mozilla.org/?tree=Try&rev=a105399b53b1

Comment 42

[Philipp von Weitershausen [:philikon]]

(In reply to Olli Pettay [:smaug] from comment #36)
> If you look at nsDOMClassInfo, it has stuff for messagemanager, the client
> part.

Yes. I don't really understand it, to be honest:

* The "ContentFrameMessageManager" thing is defined as DOMCI_DATA_NO_CLASS. Not sure what that means.

* The NS_DEFINE_CLASSINFO_DATA macro uses nsEventTargetSH instead of nsFrameMessageManager. It also makes it a window-global with nsIXPCScriptable::IS_GLOBAL_OBJECT which doesn't really make much sense to me... I can actually access an object called "ContentFrameMessageManager" in a regular HTML page, but I'm not sure what it does and why.

* The DOM_CLASSINFO_MAP contains nsIDOMEventTarget. I don't really understand why, apart from the fact that it seems to match the usage of nsEventTargetSH above.

> We could, I hope add stuff for parent part too, and conditionally
> have NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(MessageManager) inside
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsFrameMessageManager).
> (There is NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL )

I imagine it will look something like this:

  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(ContentFrameMessageManager, !mChrome)
  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(ChromeMessageBroadcaster,
                                                   mChrome && mIsBroadcaster)
  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(ChromeMessageSender,
                                                   mChrome && !mIsBroadcaster)

Comment 43

[Philipp von Weitershausen [:philikon]]

(This may or may not represent my current mental state correctly: http://mozillamemes.tumblr.com/post/29300345308/lost-in-boilerplate-hell-halp)

Comment 44

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Olli, why are there interface objects for things like SyncMessageSender?

Comment 45

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2.1): IDL changes

Had nsIFrameloader::messageManager incorrectly declared as an nsIMessageBroadcaster, even though it clearly only communicates to one OOP frame.

Comment 46

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v2.1): Update implementations of the late nsIFrameMessageManager

Update nsFrameloader::GetMessageManager() implementation for correct return type.

Comment 47

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v2.1): Fix uses of the late nsIFrameMessageManager

Turns out I was missing a few more QI's to nsIFrameScriptLoader. Maybe we can take those out again if we figure out the ClassInfo stuff, but for now they can't hurt.

Comment 48

[Philipp von Weitershausen [:philikon]]

Try build for v2.1 patches part 1-3: https://tbpl.mozilla.org/?tree=Try&rev=111c1c7e5e59

Looking a lot less like a Christmas tree :). Will tackle the remaining failures tomorrow.

Comment 49

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

Comment on attachment 651253 [details] [diff] [review]
Part 1 (v2.1): IDL changes


>+ * The global MMg and window MMw's are message broadcasters implementing
>+ * nsIMessageBroadcaster while the frame MMp's are simple message senders
>+ * (nsIMessageSender). Their counterparts in the content processes are
>+ * message senders implementing nsIContentFrameMessageManager.
>+ *
>+ *                    nsIMessageListenerManager
>+ *                  /                           \
>+ * nsIMessageSender                               nsIMessageBroadcaster
>+ *       |
>+ * nsISyncMessageSender (content process only)

content process only? really? what about In-process mm?

>+ *       |
>+ * nsIContentFrameMessageManager (content process only)
ditto



>+[scriptable, builtinclass, uuid(7f23767d-0f39-40c1-a22d-d3ab8a481f9d)]
>+interface nsIMessageSender : nsIMessageListenerManager
> {
>   /**
>    * Returns an array of JSON objects.
>+   * Send |messageName| and |obj| to the "other side" of this message
>+   * manager.  This invokes listeners who registered for
>+   * |messageName|.
>+   *
>+   * See nsIMessageListener::receiveMessage() for the format of the
>+   * data delivered to listeners.
>+   */
>+  [implicit_jscontext, optional_argc]
>+  void sendAsyncMessage([optional] in AString messageName,
>+                        [optional] in jsval obj);
>+};
sendAsyncMessage doesn't return anything, so remove the comment about JSON.


>+
>+[scriptable, builtinclass, uuid(83be5862-2996-4685-ae7d-ae25bd795d50)]
>+interface nsISyncMessageSender : nsIMessageSender
>+{
>+  /**
>+   * Like |sendAsyncMessage()|, except blocks the sender until all
>+   * listeners of the message have been invoked.  An array is returned
>+   * which contains the return values from each listener invoked.
>    */
>   [implicit_jscontext,optional_argc]
>-  jsval sendSyncMessage([optional] in AString messageName, [optional] in jsval obj);
>+  jsval sendSyncMessage([optional] in AString messageName,
>+                        [optional] in jsval obj);
> };
Or just move the comment here.

Comment 50

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

Comment on attachment 651255 [details] [diff] [review]
Part 2 (v2.1): Update implementations of the late nsIFrameMessageManager


> 
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsFrameMessageManager)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIContentFrameMessageManager)
>-  NS_INTERFACE_MAP_ENTRY_AGGREGATED(nsIFrameMessageManager,
>-                                    (mChrome ?
>-                                       static_cast<nsIFrameMessageManager*>(
>-                                         static_cast<nsIChromeFrameMessageManager*>(this)) :
>-                                       static_cast<nsIFrameMessageManager*>(
>-                                         static_cast<nsIContentFrameMessageManager*>(this))))
>+
>+  /* nsFrameMessageManager implements nsIMessageSender and nsIMessageBroadcaster,
>+   * both of which descend from nsIMessageListenerManager. QI'ing to
>+   * nsIMessageListenerManager is therefore ambiguous and needs explicit casts
>+   * depending on which child interface applies. */
>+  NS_INTERFACE_MAP_ENTRY_AGGREGATED(nsIMessageListenerManager,
>+                                    (mIsBroadcaster ?
>+                                       static_cast<nsIMessageListenerManager*>(
>+                                         static_cast<nsIMessageBroadcaster*>(this)) :
>+                                       static_cast<nsIMessageListenerManager*>(
>+                                         static_cast<nsIMessageSender*>(this))))
>+
>+  /* Message managers in child process implement nsIMessageSender and
>+     nsISyncMessageSender. */
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIMessageSender, !mChrome)
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsISyncMessageSender, !mChrome)
>+
>+  /* Message managers in the chrome process are either broadcasters (if they have
>+     subordinate/child message managers) or they're simple message senders. */
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIMessageSender, mChrome && !mIsBroadcaster)

You have nsIMessageSender twice.
NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIMessageSender, !mChrome || !mIsBroadcaster)
should work

> nsresult
>-nsFrameMessageManager::SendAsyncMessageInternal(const nsAString& aMessage,
>-                                                const StructuredCloneData& aData)
>+nsFrameMessageManager::DispatchAsyncMessageInternal(const nsAString& aMessage,
>+                                                    const StructuredCloneData& aData,
>+                                                    ShouldBroadcast aBroadcast)

>+nsresult
>+nsFrameMessageManager::DispatchAsyncMessageInternal(const nsAString& aMessageName,
>+                                                    const jsval& aObject,
>+                                                    JSContext* aCx,
>+                                                    PRUint8 aArgc,
>+                                                    ShouldBroadcast aBroadcast)
> {
This is now odd... Two DispatchAsyncMessageInternal methods taking different params and it is not clear
from the name which one does what. Could you merge them.



> NS_IMETHODIMP
> nsFrameMessageManager::Atob(const nsAString& aAsciiString,
>                             nsAString& aBinaryData)
> {
>   return NS_OK;
> }
> 
>+
Extra newline

>+  bool mChrome;     // true if we're in the chrome process
>+  bool mGlobal;     // true if 
If what?

Comment 51

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

Comment on attachment 651256 [details] [diff] [review]
Part 3 (v2.1): Fix uses of the late nsIFrameMessageManager

I think we need to get classinfo bits fixed. This becomes too dirty.

Comment 52

[Philipp von Weitershausen [:philikon]]

(In reply to Olli Pettay [:smaug] from comment #51)
> I think we need to get classinfo bits fixed. This becomes too dirty.

Ok. I'll split the QueryInterface(nsIFrameScriptLoader) bits out of Part 3 and will give the ClassInfo stuff a shot. I still have no idea what I'm doing, though. Could you take a look at my questions in comment 42?

Comment 53

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

(In reply to Philipp von Weitershausen [:philikon] from comment #42)
> * The "ContentFrameMessageManager" thing is defined as DOMCI_DATA_NO_CLASS.
> Not sure what that means.
It used to be DOMCI_DATA and peterv changed it recently. I don't know what the new macro does.


> 
> * The NS_DEFINE_CLASSINFO_DATA macro uses nsEventTargetSH instead of
> nsFrameMessageManager.
for contentmessagemanager, which implements also nsIDOMEventTarget



 It also makes it a window-global with
> nsIXPCScriptable::IS_GLOBAL_OBJECT which doesn't really make much sense to
> me...
On TabChildGlobal it makes a lot of sense, since messagemanager is the global object.


> I can actually access an object called "ContentFrameMessageManager" in
> a regular HTML page, but I'm not sure what it does and why.
We should indeed hide the constructor. You can't access object implementing 
ContentFrameMessageManager


> 
> * The DOM_CLASSINFO_MAP contains nsIDOMEventTarget. I don't really
> understand why, apart from the fact that it seems to match the usage of
> nsEventTargetSH above.
Because TabChildGlobal is nsIDOMEventTarget

 
> I imagine it will look something like this:
> 
>  
> NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(ContentFrameMessageManager,
> !mChrome)
>   NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(ChromeMessageBroadcaster,
>                                                    mChrome && mIsBroadcaster)
>   NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(ChromeMessageSender,
>                                                    mChrome &&
> !mIsBroadcaster)
Looks ok

Comment 54

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

Btw, one macro I didn't know about before today, and which could be good for this stuff is
NS_DEFINE_CHROME_ONLY_CLASSINFO_DATA. All these interfaces should be chrome only, content
shouldn't know about them

Comment 55

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

(I hope that macro still works, since the only use of it, ChromeWorker, was removed when workers
moved to new bindings.)

Comment 56

[Philipp von Weitershausen [:philikon]]

Thanks, Olli! I'll give this a shot.

Comment 57

[Philipp von Weitershausen [:philikon]]

(In reply to Olli Pettay [:smaug] from comment #50)
> > nsresult
> >-nsFrameMessageManager::SendAsyncMessageInternal(const nsAString& aMessage,
> >-                                                const StructuredCloneData& aData)
> >+nsFrameMessageManager::DispatchAsyncMessageInternal(const nsAString& aMessage,
> >+                                                    const StructuredCloneData& aData,
> >+                                                    ShouldBroadcast aBroadcast)
> 
> >+nsresult
> >+nsFrameMessageManager::DispatchAsyncMessageInternal(const nsAString& aMessageName,
> >+                                                    const jsval& aObject,
> >+                                                    JSContext* aCx,
> >+                                                    PRUint8 aArgc,
> >+                                                    ShouldBroadcast aBroadcast)
> > {
> This is now odd... Two DispatchAsyncMessageInternal methods taking different
> params and it is not clear from the name which one does what. Could you merge them.

Merging them isn't really possible. They basically do the same. Classic method overloading. One signature is used for the nested broadcasting, the other is used when directly invoking from methods. But I can easily given them different names.

Comment 58

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2.2): IDL changes. r=smaug

Comment 59

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v2.2): Update implementations of the late nsIFrameMessageManager. r=smaug

Comment 60

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2.5 (v1): ClassInfo for chrome message managers

Comment 61

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

Comment on attachment 652155 [details] [diff] [review]
Part 2.5 (v1): ClassInfo for chrome message managers


>+  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(ContentFrameMessageManager, !mChrome)
This shouldn't be here. It is already in InProcessMessageManager and in TabChildGlobal

> 
>+  DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF(ChromeMessageBroadcaster, nsIMessageBroadcaster)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIFrameScriptLoader)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIMessageListenerManager)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIMessageBroadcaster)
>+  DOM_CLASSINFO_MAP_END
>+
>+  DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF(ChromeMessageSender, nsIMessageSender)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIFrameScriptLoader)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIMessageListenerManager)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIMessageSender)
>+  DOM_CLASSINFO_MAP_END
>+

Could you try nsISupports as the latter param for _CLASS_IF
This whole setup is quite unique, but nsISupports might hide stuff from global scope.
(never tried that, but was just reading the code).
But if nsISupports doesn't work, this is ok too.

Comment 62

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2.5 (v2): ClassInfo for chrome message managers. r=smaug

Addressed smaug's review comments, including the ones made on IRC (ContentFrameMessageManager is now based on nsISupports, which means we don't litter useless interface objects into global window scopes.)

Comment 63

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v2.2): Fix uses of the late nsIFrameMessageManager

... now with a lot less QI'ing!

Comment 64

[Philipp von Weitershausen [:philikon]]

Try build for v2.2 patches: https://tbpl.mozilla.org/?tree=Try&rev=9864579c7139

Comment 65

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

Comment on attachment 652227 [details] [diff] [review]
Part 3 (v2.2): Fix uses of the late nsIFrameMessageManager

Looks like we have stuff which should use
messsage.target.sendAsyncMessage and not ppm.broadcastAsyncMessage().

Comment 66

[Philipp von Weitershausen [:philikon]]

(In reply to Olli Pettay [:smaug] from comment #65)
> Comment on attachment 652227 [details] [diff] [review]
> Part 3 (v2.2): Fix uses of the late nsIFrameMessageManager
> 
> Looks like we have stuff which should use
> messsage.target.sendAsyncMessage and not ppm.broadcastAsyncMessage().

For sure. These are already being tackled in individual bugs.

Comment 67

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #64)
> Try build for v2.2 patches:
> https://tbpl.mozilla.org/?tree=Try&rev=9864579c7139

Hmm, somehow incomplete. New try build with patches rebased on top of latest m-c: https://tbpl.mozilla.org/?tree=Try&rev=21af8e44d2f0

Comment 68

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v2.3): Fix uses of the late nsIFrameMessageManager. r=smaug

Rebased on top of latest m-c and an attempt at fixing all test failures found by the last try run. New try build: https://tbpl.mozilla.org/?tree=Try&rev=4669af658898

Comment 69

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v2.4): Fix uses of the late nsIFrameMessageManager. r=smaug

Stupid typo caused the last remaining test failure in the previous try build. This should fix it. Here's a new try build with the fix: https://tbpl.mozilla.org/?tree=Try&rev=aad10f66d2a3 *fingers crossed*

I'm pretty sure this is pretty much ready to land, although I can already see one conflict arising in bug 777203. Rebasing is pretty trival for me, so I'd rather wait for that bug to land first. In the mean time, should I escalate this issue to dev.platform as originally proposed by cjones, or should we do that post-mortem? (I would almost prefer the latter.)

Comment 70

[Philipp von Weitershausen [:philikon]]

Try is green, this is good to go. Unless there are any objections, I will inform folks of this change on dev-platform, update MDN docs, and then land as soon as bug 777203 has landed.

As pointed out in comment 31, there's only two add-ons that refer to the interface we're removing. Lots of add-ons use sendAsyncMessage, but AFAICT only in frame scripts and other nsIMessageSender scenarios. If they're doing it in an nsIMessageBroadcaster scenario, they're doing something wrong anyway.

Comment 71

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v2.3): Update implementations of the late nsIFrameMessageManager. r=smaug

Resolve conflict with bug 579517 which landed in the mean time.

Comment 72

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v2.5): Fix uses of the late nsIFrameMessageManager. r=smaug

Resolve (expected) conflict with bug 777203 that just landed.

Comment 73

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

CC'ing the metro team since they are using XUL Fennec as a base and working on "elm" so they'll need to update too.

Comment 74

[Philipp von Weitershausen [:philikon]]

(In reply to Mark Finkle (:mfinkle) from comment #73)
> CC'ing the metro team since they are using XUL Fennec as a base and working
> on "elm" so they'll need to update too.

Part 3 should already update all their code.

Comment 75

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #74)
> (In reply to Mark Finkle (:mfinkle) from comment #73)
> > CC'ing the metro team since they are using XUL Fennec as a base and working
> > on "elm" so they'll need to update too.
> 
> Part 3 should already update all their code.

And by that I mean, make it work. This bug is not about removing inappropriate uses of broadcasting. That will be tackled in follow-ups.

Comment 76

[Jim Mathies [:jimm]]

(In reply to Philipp von Weitershausen [:philikon] from comment #74)
> (In reply to Mark Finkle (:mfinkle) from comment #73)
> > CC'ing the metro team since they are using XUL Fennec as a base and working
> > on "elm" so they'll need to update too.
> 
> Part 3 should already update all their code.

We have a heavily modified clone of mobile/xul in browser/metro that hasn't merged to mc yet. That's ok though the changes to mobile code look minimal. Unless you're up for checking out elm, updating, and building on win8 to test :), I can do the updates when we merge mc over after this lands.

Comment 77

[Philipp von Weitershausen [:philikon]]

Landed as one revision (since the changes are not even close to atomic):

https://hg.mozilla.org/integration/mozilla-inbound/rev/5acb2a155d12

Comment 78

[Philipp von Weitershausen [:philikon]]

This actually made the mozilla17 cut-off.

Comment 79

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5acb2a155d12

Comment 80

[Will Elwood (:Will)]

For the record, my addon code uses sendAsyncMessage in an nsIMessageBroadcaster scenario (which comment 70 suggests is wrong). The message is sent from bootstrap.js to my frame scripts during addon shutdown to signal that they should also shut down. Is there a more correct way to achieve this in a restartless addon?

Also, I don't use nsIFrameMessageManager but nsIChromeFrameMessageManager (having followed the e10s examples from places like https://wiki.mozilla.org/Mobile/Fennec/Extensions/Electrolysis ) so perhaps comment 31 hasn't caught all addons that might be affected by these changes.

Finally, I'm not sure if documentation has been updated yet, but I couldn't see any mention of this change on the Firefox 17 for Developers page at least.

Comment 81

[Jeferson Hultmann]

I also came across the issue reported in comment #80

Comment 82

[Jorge Villalobos [:jorgev] (he/him)]

philikon, do you have any suggestions for the last two comments? Is there any documentation that explains it?

Comment 83

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

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl
has pretty good documentation