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)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: billm, Assigned: billm)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
We also forward some information at the start of process here: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp?rev=8f30bbb70986#1436
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.
Assignee | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
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•10 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-
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8603605 -
Attachment is obsolete: true
Attachment #8603651 -
Flags: review?(bugs)
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
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.
Comment 12•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 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.
Description
•