Closed Bug 1133594 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
e10s m5+ ---
firefox38 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 4 obsolete files)

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.
Attached patch rename-script-executor (obsolete) — Splinter Review
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.
Attachment #8565161 - Flags: review?(bugs)
Attached patch rename-load-frame-script (obsolete) — Splinter Review
A bit more renaming.
Attachment #8565162 - Flags: review?(bugs)
Attached patch refactor-common (obsolete) — Splinter Review
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.
Attachment #8565165 - Flags: review?(bugs)
Attached patch process-script (obsolete) — Splinter Review
This patch adds process scripts.
Attachment #8565168 - Flags: review?(bugs)
(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.
Attachment #8565161 - Flags: review?(bugs) → review-
, 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
Attachment #8565162 - Flags: review?(bugs) → review-
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.)
Attachment #8565165 - Flags: review?(bugs) → review-
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.)
Attachment #8565168 - Flags: review?(bugs) → 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.)
Attachment #8565161 - Attachment is obsolete: true
Attachment #8566374 - Flags: review?(bugs)
Attachment #8565162 - Attachment is obsolete: true
Attachment #8566375 - Flags: review?(bugs)
Attachment #8565165 - Attachment is obsolete: true
Attachment #8566376 - Flags: review?(bugs)
Attached patch hide-mmSplinter Review
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.
Attachment #8566377 - Flags: review?(bugs)
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
Attachment #8565168 - Attachment is obsolete: true
Attachment #8566378 - Flags: review?(bugs)
Attachment #8566374 - Flags: review?(bugs) → review+
Attachment #8566375 - Flags: review?(bugs) → review+
Comment on attachment 8566376 [details] [diff] [review]
refactor-common v2

You need to update uuid of nsIContentFrameMessageManager
Attachment #8566376 - Flags: review?(bugs) → review+
Attachment #8566377 - Flags: review?(bugs) → review+
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.
Attachment #8566378 - Flags: review?(bugs) → review+
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.
Depends on: 1167371
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.