Allow sending initial process data to content processes

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 8603144 [details] [diff] [review]
init-data

It's not uncommon to write JS code where the main process keeps track of some state and broadcasts changes to all the content processes. When a new process starts, it needs to read all the state from the main process. Usually this is done with a sync message to ensure we have the state before anything important happens.

It would be nice if we could use one sync message to transfer all this data. We already have something like that, GetXPCOMProcessAttributes, but it only works for C++ code. I'd like to add arbitrary JS data to that message.

The basic idea is that there's a JS object in the parent process that anyone can modify. Whenever a new child process starts, it uses structured clone to transfer this data from the parent. Then anyone in the child can read that data.

This patch depends on some code in bug 803783.
Attachment #8603144 - Flags: review?(bugs)
Wait, can't you instead send this data when the process is starting rather than blocking the new process in a sync message? (e.g. |BrowserConfiguration| in http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#543)
Maybe. BrowserConfiguration isn't exactly right since it's per-browser. But maybe we could find a time when an async message from the parent would work. It would also need to be before any frame scripts run.

This is easier though. Note that the patch doesn't add any new sync calls. It just piggybacks on an existing one.
Yes please! Launching a child process and then immediately having it block to ask the parent for something is a pattern we should kill with fire.
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #4)
> Yes please! Launching a child process and then immediately having it block
> to ask the parent for something is a pattern we should kill with fire.

Word.
Let's see what smaug wants. Keep in mind that this patch doesn't add any extra sync calls and it will allow us to remove one at startup (in RemoteAddonsChild).

For what I need now, ForwardKnownInfo should work. I'm worried about using it, though, because the info will be received after XPCOM services have already started in the child. That's not a problem now, but it could be in the future. It might mean that JS authors will use a sync message if they need the data that early, which defeats the purpose of this patch.

I think the only advantage of using ForwardKnownInfo is that it will make things easier if we want to remove the GetXPCOMProcessAttributes message in the future.
Created attachment 8603605 [details] [diff] [review]
patch v2

Found a bug in the previous version.
Attachment #8603144 - Attachment is obsolete: true
Attachment #8603144 - Flags: review?(bugs)
Attachment #8603605 - Flags: review?(bugs)

Comment 8

3 years ago
Comment on attachment 8603605 [details] [diff] [review]
patch v2

update uuid of nsIProcessScriptLoader and nsIContentProcessMessageManager, though
I think no new things should be added to nsIProcessScriptLoader here, but a new interface  which
only the global ppmm implements.


>+void
>+ProcessGlobal::SetInitialProcessData(JS::HandleValue aInitialData)
>+{
Could we MOZ_ASSERT here that this is the global parent process mm.

>+  if (mIsProcessManager) {
>+    mozilla::HoldJSObjects(this);
>+  }
So we want this only for (mIsProcessManager && (IsBroadcaster() || !mChrome))


>+nsFrameMessageManager::~nsFrameMessageManager()
>+{
>+  if (mIsProcessManager) {
>+    mozilla::DropJSObjects(this);
>+  }
ditto


>+nsFrameMessageManager::GetInitialProcessData(JSContext* aCx, JS::MutableHandleValue aResult)
>+{
>+  if (mChrome && !mIsBroadcaster) {
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
So we could MOZ_ASSERT there if we just exposed the interface on the global ppmm


>+
>+  JS::RootedValue init(aCx, mInitialProcessData);
>+  if (mChrome && init.isPrimitive()) {
Don't you want isUndefined() check here (or whatever the default for JS::Value is). Someone might want to set
initialProcessData to point to some number or null, and that would be primitive.


Super nice API (after those minor fixes)
Attachment #8603605 - Flags: review?(bugs) → review-
Created attachment 8603651 [details] [diff] [review]
patch v3
Attachment #8603605 - Attachment is obsolete: true
Attachment #8603651 - Flags: review?(bugs)
Comment on attachment 8603651 [details] [diff] [review]
patch v3

I may need to tweak classinfo a bit in a different bug
(since it looks a bit broken already without this patch).

I assume 
buffer.steal(&initialData->data, &initialData->dataLength);
does the right thing, OwningSerializedStructuredCloneBuffer will then release the data, right?
Attachment #8603651 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4e131df428

> I assume 
> buffer.steal(&initialData->data, &initialData->dataLength);
> does the right thing, OwningSerializedStructuredCloneBuffer will then release the data, right?

Yes.
https://hg.mozilla.org/mozilla-central/rev/cd4e131df428
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.