A very common pattern when using the message manager is to load a frame script that then loads a JSM. That way there is one JSM per content process. This is useful if you need to register observers or a content policy or any other global thing. We've lived with the JSM approach for a while and it works okay, but it's a difficult pattern to explain to add-on developers. The add-on developers wonder why their frame script is being loaded more than once per process, which seems broken to them. They also don't understand why JSMs fix the problem, since they don't really understand how JSM loading works either. (See bug 1048164 for more pain.) We can improve the documentation, but I think it's probably better to make the API match what developers find most natural. So these patches introduce "process scripts", which are similar to frame scripts. They run once per content process (as well as once in the main process). They have access to the child process message manager. And they're loaded using the parent process message manager.

This patch renames some stuff related to frame scripts to "remote" scripts, which is the generic term I'd like to use to refer to frame scripts and process scripts.

A bit more renaming.

This patch moves some of the utility functions that are available on frame scripts (dump, btoa, etc.) to a more central place. This way they can be reused by process scripts.

This patch adds process scripts.

(In reply to Bill McCloskey (:billm) from comment #0) > A very common pattern when using the message manager is to load a frame > script that then loads a JSM. That way there is one JSM per content process. That is surprising to me (and not what was in mind when message managers were implemented). I would have thought there would be JS implemented services which communicate via process message manager. > The add-on developers wonder why their frame script is being loaded more > than once per process, which seems broken to them. Because frame script, as its name hints is for one browser frame/tab. > So these patches introduce "process scripts", which are similar to frame > scripts. They run once per content process (as well as once in the main > process). They have access to the child process message manager. And they're > loaded using the parent process message manager. I guess that is fine.

Comment on attachment 8565161 [details] [diff] [review] rename-script-executor "RemoteScript" doesn't really make sense, given that this stuff maybe used for in-process case too. Perhaps MessageManagerScript. So s/nsRemoteScript/MessageManagerScript/ I know it is a bit long, but have better suggestion now.

, but don't have...

Oh, I see the next patch uses the same naming... Need to figure out some good name.

Comment on attachment 8565162 [details] [diff] [review] rename-load-frame-script So I think nsIRemoteMessageManager should be something like nsIMessageManagerGlobal

Comment on attachment 8565162 [details] [diff] [review] rename-load-frame-script er, I was looking at wrong patch. But this should anyhow use something else than "RemoteScript", since that really misleading. MessageManagerScript?

Comment on attachment 8565165 [details] [diff] [review] refactor-common So this is the patch which should probably use nsIMessageManagerGlobal and not nsIRemoteMessageManager. (all these 3 first patches look ok, except the naming.)

We need tests for this. I'm especially worried about memory leaks. (Still trying to understand why we wouldn't leak in case some mm script in the process global accesses child process mm and keeps a ref to it, for example)

(In reply to Olli Pettay [:smaug] (high review load) from comment #12) > We need tests for this. I'm especially worried about memory leaks. > (Still trying to understand why we wouldn't leak in case some mm script in > the > process global accesses child process mm and keeps a ref to it, for example) I suspect we would probably leak in that case. I could add an xpcom-shutdown observer that calls ProcessGlobal::Shutdown. I think that should fix the problem. I'm also wondering if perhaps the childprocessmessagemanager service should actually return a reference to the ProcessGlobal (rather than the message manager). The ProcessGlobal is strictly more useful.

Comment on attachment 8565168 [details] [diff] [review] process-script >+StaticRefPtr<ProcessGlobal> ProcessGlobal::sInstance; So this is worrisome. If component manager releases the last ref to the child process mm, but the process global still keeps the mm alive, cycle collector can't release this stuff, because sInstance unknown to cycle collector. >+ProcessGlobal::~ProcessGlobal() >+{ >+ sInstance = nullptr; sInstance = nullptr; doesn't really make sense. How could sInstance be non-null if refcnt is 0 ;) >+void >+ProcessGlobal::Shutdown() >+{ >+ sInstance = nullptr; >+} So one option is to make sure this is called at right time so that the unknown edge to CC is released. Or, child process mm could perhaps own PG, and tell about the edge to the CC. >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(ProcessGlobal) >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mGlobal) 2 spaces is enough >+class ProcessGlobal : >+ public nsRemoteScriptExecutor, >+ public nsIContentProcessMessageManager, >+ public nsIGlobalObject, >+ public nsIScriptObjectPrincipal, >+ public nsSupportsWeakReference, >+ public mozilla::dom::ipc::MessageManagerCallback 2 spaces for indentation >+ virtual JSObject* GetGlobalJSObject() MOZ_OVERRIDE { { goes to the next line This all looks surprisingly simple :) (just take care of that memory management, or explain why it isn't an issue, and re-ask review.)

Ah, yes, perhaps accessing cpmm could return ProcessGlobal.

(but even then, better to not use StaticRefPtr)

(and happy to review new versions of patches for this still this week.)

This patch isn't really necessary, but I thought I might need it when I started rewriting the patch. It makes the code a little nicer, but I'll leave it up to you.

Thanks Olli. I changed things so the childprocessmessagemanager service now returns the ProcessGlobal. The ProcessGlobal now owns the actual message manager. I also realized that ProcessGlobal needs to be wrapper cached. I don't know this part of the code too well, so could you check over all the cycle collection and QI macros carefully? I just pushed this to try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=819a15bce36d

Comment on attachment 8566376 [details] [diff] [review] refactor-common v2 You need to update uuid of nsIContentFrameMessageManager

Comment on attachment 8566378 [details] [diff] [review] process-script v2 >+[scriptable, builtinclass, uuid(9ca95410-b253-11e4-ab27-0800200c9a66)] >+interface nsIContentProcessMessageManager : nsIMessageManagerGlobal >+{ >+}; I wonder if we really need to have this interface. But ok, if it is really needed. And I guess we may want to add something there later, like getter for all the FrameMessageManager globals. >+[scriptable, builtinclass, uuid(7e1e1a20-b24f-11e4-ab27-0800200c9a66)] >+interface nsIProcessScriptLoader : nsISupports >+{ >+ /** >+ * Load a script in the (remote) frame. Misleading comment. Load a script in the (possibly remote) process.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/84c196dc0c64 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ab5a44691d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a70302aa6104 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c97594ee58d9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f0d6ad53a7 And a separate commit because I forgot to fix the comment: https://hg.mozilla.org/integration/mozilla-inbound/rev/25e29e6871e6

https://hg.mozilla.org/mozilla-central/rev/84c196dc0c64 https://hg.mozilla.org/mozilla-central/rev/f7ab5a44691d https://hg.mozilla.org/mozilla-central/rev/a70302aa6104 https://hg.mozilla.org/mozilla-central/rev/c97594ee58d9 https://hg.mozilla.org/mozilla-central/rev/b9f0d6ad53a7 https://hg.mozilla.org/mozilla-central/rev/25e29e6871e6

Can anyone provide an example how to use the process script in extensions? Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #27) > Can anyone provide an example how to use the process script in extensions? > > Honza Nothing special, see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/processsingleton/MainProcessSingleton.js#88. Note that chrome URLs don't work so you either have to convert them to something else (as that code does) or use a resource URL.

https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Message_Manager/Process_scripts https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIProcessScriptLoader Marking as dev-doc-complete, but please let me know if you need anything else.