Replace ContentParent::GetSingleton to allow for multiple content processes

RESOLVED FIXED in mozilla9

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

unspecified
mozilla9
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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)

Updated

6 years ago
Assignee: nobody → benjamin
(Assignee)

Comment 1

6 years ago
Created attachment 544235 [details] [diff] [review]
Support multiple, rev. 1 (unfinished)

This patch needs message-manager love.
(Assignee)

Updated

6 years ago
Depends on: 669640

Comment 2

6 years ago
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?
(Assignee)

Comment 3

6 years ago
Created attachment 548257 [details] [diff] [review]
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.
Attachment #544235 - Attachment is obsolete: true
Attachment #548257 - Flags: review?(josh)

Comment 4

6 years ago
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.
(Assignee)

Comment 6

6 years ago
Created attachment 548857 [details] [diff] [review]
Remove ContentParent::GetSingleton, updated to review comments, rev. 3
Attachment #548257 - Attachment is obsolete: true
(Assignee)

Comment 7

6 years ago
http://hg.mozilla.org/mozilla-central/rev/b764f55dd37d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.