Bug 1488993 - Fix PreallocatedProcessManager blocker management (v2). r=mconley,smaug
☠☠ backed out by b0e69b136883 ☠ ☠
authorJed Davis <jld@mozilla.com>
Thu, 22 Nov 2018 00:06:07 +0000
changeset 507379 cea43bc88c7a564f0ca72995f6eac6b0d118f3f1
parent 507378 fe77c09dee3107e675162b760a6efa9b5f0bcce3
child 507380 8ede2ebe6b7ab639e729614cb3609776955a6bc2
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()) {