Last Comment Bug 666748 - Replace ContentParent::GetSingleton to allow for multiple content processes
: Replace ContentParent::GetSingleton to allow for multiple content processes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Windows 7
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Benjamin Smedberg [:bsmedberg]
:
Mentors:
Depends on: 669640
Blocks: 641683
  Show dependency treegraph
 
Reported: 2011-06-23 14:19 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2013-09-24 09:16 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Support multiple, rev. 1 (unfinished) (21.32 KB, patch)
2011-07-06 07:47 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Review
Remove ContentParent::GetSingleton ready for review, rev. 2 (21.58 KB, patch)
2011-07-25 12:52 PDT, Benjamin Smedberg [:bsmedberg]
josh: review+
Details | Diff | Review
Remove ContentParent::GetSingleton, updated to review comments, rev. 3 (21.03 KB, patch)
2011-07-27 11:56 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Review

Description Benjamin Smedberg [:bsmedberg] 2011-06-23 14:19:03 PDT
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 [:bsmedberg] 2011-07-06 07:47:55 PDT
Created attachment 544235 [details] [diff] [review]
Support multiple, rev. 1 (unfinished)

This patch needs message-manager love.
Comment 2 Olli Pettay [:smaug] 2011-07-22 05:28:36 PDT
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 [:bsmedberg] 2011-07-25 12:52:40 PDT
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.
Comment 4 Josh Matthews [:jdm] 2011-07-26 09:52:19 PDT
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 2011-07-27 11:02:46 PDT
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 [:bsmedberg] 2011-07-27 11:56:43 PDT
Created attachment 548857 [details] [diff] [review]
Remove ContentParent::GetSingleton, updated to review comments, rev. 3
Comment 7 Benjamin Smedberg [:bsmedberg] 2011-08-19 07:17:51 PDT
http://hg.mozilla.org/mozilla-central/rev/b764f55dd37d

Note You need to log in before you can comment on or make changes to this bug.