Closed Bug 666748 Opened 8 years ago Closed 8 years ago

Replace ContentParent::GetSingleton to allow for multiple content processes

Categories

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

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(1 file, 2 obsolete files)

We want multiple content processes in desktop Firefox (and probably mobile as well, if the memory tradeoffs are good). This means we need to replace ContentParent::GetSingleton with a function which can hand out content processes as necessary. We'll obviously want to experiment with a pool, process-per-tab, and other policies, but for now let's start with something simple like a pref to govern how many content processes are created and then hand out tabs randomly to processes. Fennec will of course default to one process for now.
Assignee: nobody → benjamin
This patch needs message-manager love.
Depends on: 669640
How much is there still to do? Could we land even the partial patch to
e10s repo, but so that only one process is actually used?
Smaug dealt with the message manager bits on top of this patch, so this should be ready for review.
Attachment #544235 - Attachment is obsolete: true
Attachment #548257 - Flags: review?(josh)
Comment on attachment 548257 [details] [diff] [review]
Remove ContentParent::GetSingleton ready for review, rev. 2

Review of attachment 548257 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, modulo the minor comments and one major concern.

::: content/base/src/nsFrameMessageManager.cpp
@@ -779,4 +779,5 @@
> >  bool SendAsyncMessageToChildProcess(void* aCallbackData,
> >                                      const nsAString& aMessage,
> >                                      const nsAString& aJSON)
> >  {
> > +#if 0 // Need code for multiple content processes

I can't find the bug with smaug's work in it right now. Does it subsume this block entirely, and has it already landed? This will break mobile as-is.

::: dom/ipc/ContentParent.cpp
@@ -161,7 +163,9 @@
> > -    if (gSingleton && !gSingleton->IsAlive())
> > +    if (!gContentParents)
> > -        gSingleton = nsnull;
> > +        gContentParents = new nsTArray<ContentParent*>();
NaN more ...
> > +    PRInt32 maxContentProcesses = Preferences::GetInt("dom.ipc.processCount", 1);
> > -        nsRefPtr<ContentParent> parent = new ContentParent();
> > +    if (maxContentProcesses < 1)
> > -        gSingleton = parent;
NaN more ...

PK11_GenerateRandom, please. rand() is not cryptogaphically sound.

(kidding!)

@@ -367,3 +396,3 @@
> >      NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> > -    //If the previous content process has died, a new one could have
> > +    if (gContentParents) {
> > -    //been started since.
> > +        gContentParents->RemoveElement(this);

This shouldn't be necessary, since we remove ourselves in ActorDestroy.

@@ -654,4 +682,4 @@
> >              return NS_ERROR_NOT_AVAILABLE;
> >      }
> >      else if (!strcmp(aTopic, "child-memory-reporter-request")) {
> > -        SendPMemoryReportRequestConstructor();
> > +        unused << SendPMemoryReportRequestConstructor();

There's a bug filed about fixing this the right way (ie. we actually want to take some action if it fails). Just revert this hunk for now, please.
Attachment #548257 - Flags: review?(josh) → review+
Comment on attachment 548257 [details] [diff] [review]
Remove ContentParent::GetSingleton ready for review, rev. 2

>diff --git a/widget/src/android/nsWindow.cpp b/widget/src/android/nsWindow.cpp
>--- a/widget/src/android/nsWindow.cpp
>+++ b/widget/src/android/nsWindow.cpp
>+            nsTArray<ContentParent*> cplist;
>+            ContentParent::GetAll();

Er, I do think you want to pass something to GetAll.
http://hg.mozilla.org/mozilla-central/rev/b764f55dd37d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.