Bug 1488993 - Fix PreallocatedProcessManager blocker management (v2). r=mconley,smaug
authorJed Davis <jld@mozilla.com>
Wed, 28 Nov 2018 20:42:00 +0000
changeset 507798 e0e1a14b592fd45b1e4c8dffe51d68dbe84cd265
parent 507797 5b0d04272c75c8cf03e38dc372ef950e43dfb1ba
child 507799 1b92017b48651531b42b12856acf0f36001168ae
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmconley, smaug
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1488993 - Fix PreallocatedProcessManager blocker management (v2). r=mconley,smaug This fixes/adjusts two things about how content process preallocation is blocked: 1. Processes aren't registered as blockers until after they launch successfully. The main goal is to not leak a blocker if launch fails, but we don't need to block *while* launching synchronously, because this all happens on the main thread. 2. Preallocated processes themselves aren't blockers. The main goal here is so that async preallocation doesn't need extra complexity to avoid being blocked by itself when launch completes. This mostly doesn't affect actual behavior, because we currently support at most one preallocated process. The difference is the window from when the process is sent its first PBrowserConstructor until when it's next idle, where there is now no longer a blocker, but this seems to be relatively short (~100ms) and we don't even try to launch a new process until at least 1s + an idle runnable. This patch does not explicitly RemoveBlocker in ActorDestroy like the first attempt did, because it's unnecessary: this is handled in the ipc:content-shutdown observer. Differential Revision: https://phabricator.services.mozilla.com/D8939
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -626,18 +626,16 @@ bool ContentParent::sEarlySandboxInit = 
   RefPtr<ContentParent> process =
     new ContentParent(/* aOpener = */ nullptr,
                       /* aRecordingFile = */ EmptyString());
-  PreallocatedProcessManager::AddBlocker(process);
   if (!process->LaunchSubprocess(PROCESS_PRIORITY_PREALLOC)) {
     return nullptr;
   return process.forget();
 /*static*/ void
@@ -897,23 +895,23 @@ ContentParent::GetNewOrUsedBrowserProces
       p->mActivateTS = TimeStamp::Now();
       return p.forget();
   // Create a new process from scratch.
   RefPtr<ContentParent> p = new ContentParent(aOpener, aRemoteType, recordReplayState, recordingFile);
+  if (!p->LaunchSubprocess(aPriority)) {
+    return nullptr;
+  }
   // Until the new process is ready let's not allow to start up any preallocated processes.
-  if (!p->LaunchSubprocess(aPriority)) {
-    return nullptr;
-  }
   if (recordReplayState == eNotRecordingOrReplaying) {
   p->mActivateTS = TimeStamp::Now();
   return p.forget();
--- a/dom/ipc/PreallocatedProcessManager.cpp
+++ b/dom/ipc/PreallocatedProcessManager.cpp
@@ -207,17 +207,23 @@ PreallocatedProcessManagerImpl::AddBlock
 PreallocatedProcessManagerImpl::RemoveBlocker(ContentParent* aParent)
   uint64_t childID = aParent->ChildID();
-  MOZ_ASSERT(mBlockers.Contains(childID));
+  // This used to assert that the blocker existed, but preallocated
+  // processes aren't blockers anymore because it's not useful and
+  // interferes with async launch, and it's simpler if content
+  // processes don't need to remember whether they were preallocated.
+  // (And preallocated processes can't AddBlocker when taken, because
+  // it's possible for a short-lived process to be recycled through
+  // Provide() and Take() before reaching RecvFirstIdle.)
   if (!mPreallocatedProcess && mBlockers.IsEmpty()) {