666748 - Replace ContentParent::GetSingleton to allow for multiple content processes

Status: RESOLVED FIXED

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

Description

[Benjamin Smedberg]

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.

Comment 1

[Benjamin Smedberg]

Attachment: Support multiple, rev. 1 (unfinished)

This patch needs message-manager love.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 3

[Benjamin Smedberg]

Attachment: Remove ContentParent::GetSingleton ready for review, rev. 2

Smaug dealt with the message manager bits on top of this patch, so this should be ready for review.

Comment 4

[Josh Matthews [:jdm]]

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.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 6

[Benjamin Smedberg]

Attachment: Remove ContentParent::GetSingleton, updated to review comments, rev. 3

Comment 7

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/b764f55dd37d