1133594 - [e10s] Permit loading "process scripts" into content processes

Status: RESOLVED FIXED

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

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename-script-executor

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.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename-load-frame-script

A bit more renaming.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: refactor-common

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.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: process-script

This patch adds process scripts.

Comment 5

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

(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 6

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

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.

Comment 7

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

, but don't have...

Comment 8

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

Oh, I see the next patch uses the same naming...

Need to figure out some good name.

Comment 9

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

Comment on attachment 8565162 [details] [diff] [review]
rename-load-frame-script

So I think nsIRemoteMessageManager should be something like
nsIMessageManagerGlobal

Comment 10

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

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 11

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

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.)

Comment 12

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

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)

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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 14

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

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.)

Comment 15

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

Ah, yes, perhaps accessing cpmm could return ProcessGlobal.

Comment 16

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

(but even then, better to not use StaticRefPtr)

Comment 17

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

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

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename-script-executor v2

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename-load-frame-script v2

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: refactor-common v2

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: hide-mm

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.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: process-script v2

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 23

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

Comment on attachment 8566376 [details] [diff] [review]
refactor-common v2

You need to update uuid of nsIContentFrameMessageManager

Comment 24

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

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.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 26

[Carsten Book [:Tomcat]]

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

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Honza

Comment 28

[Dave Townsend [:mossop]]

(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.

Comment 29

[Will Bamberg [:wbamberg]]

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.