Closed Bug 1162838 Opened 10 years ago Closed 10 years ago

Allow sending initial process data to content processes

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch init-data (obsolete) (deleted) — Splinter Review
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.
Attached patch patch v2 (obsolete) (deleted) — Splinter Review
Found a bug in the previous version.
Attachment #8603144 - Attachment is obsolete: true
Attachment #8603144 - Flags: review?(bugs)
Attachment #8603605 - Flags: review?(bugs)
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-
Attached patch patch v3 (deleted) — Splinter Review
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: