Bug 1646088 - Part 1: Keep processes alive during process switches, r=kmag

Differential Revision: https://phabricator.services.mozilla.com/D79888

docshell/base/CanonicalBrowsingContext.cpp

@@ -421,8 +421,7 @@ void CanonicalBrowsingContext::LoadURI(const nsAString& aURI,
   LoadURI(loadState, true);
 }
 
-void CanonicalBrowsingContext::PendingRemotenessChange::ProcessReady(
+void CanonicalBrowsingContext::PendingRemotenessChange::ProcessReady() {
-    ContentParent* aContentParent) {
   if (!mPromise) {
     return;
   }
@@ -431,18 +430,15 @@ void CanonicalBrowsingContext::PendingRemotenessChange::ProcessReady(
   if (mPrepareToChangePromise) {
     mPrepareToChangePromise->Then(
         GetMainThreadSerialEventTarget(), __func__,
-        [self = RefPtr{this}, contentParent = RefPtr{aContentParent}](bool) {
+        [self = RefPtr{this}](bool) { self->Finish(); },
-          self->Finish(contentParent);
-        },
         [self = RefPtr{this}](nsresult aRv) { self->Cancel(aRv); });
     return;
   }
 
-  Finish(aContentParent);
+  Finish();
 }
 
-void CanonicalBrowsingContext::PendingRemotenessChange::Finish(
+void CanonicalBrowsingContext::PendingRemotenessChange::Finish() {
-    ContentParent* aContentParent) {
   if (!mPromise) {
     return;
   }
@@ -482,13 +478,13 @@ void CanonicalBrowsingContext::PendingRemotenessChange::Finish(
     // browser to determine if it is remote, so update the value.
     browserElement->SetAttr(
         kNameSpaceID_None, nsGkAtoms::remote,
-        aContentParent ? NS_LITERAL_STRING("true") : NS_LITERAL_STRING("false"),
+        mContentParent ? NS_LITERAL_STRING("true") : NS_LITERAL_STRING("false"),
         /* notify */ true);
 
     // The process has been created, hand off to nsFrameLoaderOwner to finish
     // the process switch.
     ErrorResult error;
-    frameLoaderOwner->ChangeRemotenessToProcess(aContentParent,
+    frameLoaderOwner->ChangeRemotenessToProcess(mContentParent,
                                                 mReplaceBrowsingContext, error);
     if (error.Failed()) {
       Cancel(error.StealNSResult());
@@ -507,7 +503,7 @@ void CanonicalBrowsingContext::PendingRemotenessChange::Finish(
     RefPtr<nsFrameLoader> frameLoader = frameLoaderOwner->GetFrameLoader();
     RefPtr<BrowserParent> newBrowser = frameLoader->GetBrowserParent();
     if (!newBrowser) {
-      if (aContentParent) {
+      if (mContentParent) {
         // Failed to create the BrowserParent somehow! Abort the process switch
         // attempt.
         Cancel(NS_ERROR_UNEXPECTED);
@@ -537,7 +533,7 @@ void CanonicalBrowsingContext::PendingRemotenessChange::Finish(
     return;
   }
 
-  if (NS_WARN_IF(!aContentParent)) {
+  if (NS_WARN_IF(!mContentParent)) {
     Cancel(NS_ERROR_FAILURE);
     return;
   }
@@ -575,7 +571,7 @@ void CanonicalBrowsingContext::PendingRemotenessChange::Finish(
   // Update which process is considered the current owner
   uint64_t inFlightProcessId = target->OwnerProcessId();
   target->SetInFlightProcessId(inFlightProcessId);
-  target->SetOwnerProcessId(aContentParent->ChildID());
+  target->SetOwnerProcessId(mContentParent->ChildID());
 
   auto resetInFlightId = [target, inFlightProcessId] {
     if (target->GetInFlightProcessId() == inFlightProcessId) {
@@ -608,7 +604,7 @@ void CanonicalBrowsingContext::PendingRemotenessChange::Finish(
   // Create and initialize our new BrowserBridgeParent.
   TabId tabId(nsContentUtils::GenerateTabId());
   RefPtr<BrowserBridgeParent> bridge = new BrowserBridgeParent();
-  nsresult rv = bridge->InitWithProcess(embedderBrowser, aContentParent,
+  nsresult rv = bridge->InitWithProcess(embedderBrowser, mContentParent,
                                         windowInit, chromeFlags, tabId);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     Cancel(rv);
@@ -661,6 +657,13 @@ void CanonicalBrowsingContext::PendingRemotenessChange::Clear() {
     mTarget->mPendingRemotenessChange = nullptr;
   }
 
+  // When this PendingRemotenessChange was created, it was given a
+  // `mContentParent`.
+  if (mContentParent) {
+    mContentParent->RemoveKeepAlive();
+    mContentParent = nullptr;
+  }
+
   mPromise = nullptr;
   mTarget = nullptr;
   mPrepareToChangePromise = nullptr;
@@ -765,23 +768,27 @@ CanonicalBrowsingContext::ChangeRemoteness(const nsAString& aRemoteType,
   }
 
   if (aRemoteType.IsEmpty()) {
-    change->ProcessReady(nullptr);
+    change->ProcessReady();
   } else {
-    ContentParent::GetNewOrUsedBrowserProcessAsync(
+    change->mContentParent = ContentParent::GetNewOrUsedLaunchingBrowserProcess(
         /* aFrameElement = */ nullptr,
         /* aRemoteType = */ aRemoteType,
         /* aPriority = */ hal::PROCESS_PRIORITY_FOREGROUND,
         /* aOpener = */ nullptr,
-        /* aPreferUsed = */ false)
+        /* aPreferUsed = */ false);
-        ->Then(
+    if (!change->mContentParent) {
+      change->Cancel(NS_ERROR_FAILURE);
+      return promise.forget();
+    }
+
+    // Add a KeepAlive used by this ContentParent, which will be cleared when
+    // the change is complete. This should prevent the process dying before
+    // we're ready to use it.
+    change->mContentParent->AddKeepAlive();
+    change->mContentParent->WaitForLaunchAsync()->Then(
         GetMainThreadSerialEventTarget(), __func__,
-            [change](ContentParent* aContentParent) {
+        [change](ContentParent*) { change->ProcessReady(); },
-              MOZ_RELEASE_ASSERT(
+        [change](LaunchError) { change->Cancel(NS_ERROR_FAILURE); });
-                  aContentParent,
-                  "Null process from GetNewOrUsedBrowserProcessAsync");
-              change->ProcessReady(aContentParent);
-            },
-            [change](LaunchError aError) { change->Cancel(NS_ERROR_FAILURE); });
   }
   return promise.forget();
 }

docshell/base/CanonicalBrowsingContext.h

@@ -205,13 +205,14 @@ class CanonicalBrowsingContext final : public BrowsingContext {
     friend class CanonicalBrowsingContext;
 
     ~PendingRemotenessChange();
-    void ProcessReady(ContentParent* aContentParent);
+    void ProcessReady();
-    void Finish(ContentParent* aContentParent);
+    void Finish();
     void Clear();
 
     RefPtr<CanonicalBrowsingContext> mTarget;
     RefPtr<RemotenessPromise::Private> mPromise;
     RefPtr<GenericPromise> mPrepareToChangePromise;
+    RefPtr<ContentParent> mContentParent;
 
     uint64_t mPendingSwitchId;
     bool mReplaceBrowsingContext;

dom/ipc/ContentParent.cpp

@@ -971,12 +971,11 @@ already_AddRefed<ContentParent> ContentParent::GetUsedBrowserProcess(
 
 /*static*/
 already_AddRefed<ContentParent>
-ContentParent::GetNewOrUsedBrowserProcessInternal(Element* aFrameElement,
+ContentParent::GetNewOrUsedLaunchingBrowserProcess(Element* aFrameElement,
                                                    const nsAString& aRemoteType,
                                                    ProcessPriority aPriority,
                                                    ContentParent* aOpener,
-                                                  bool aPreferUsed,
+                                                   bool aPreferUsed) {
-                                                  bool aIsSync) {
   MOZ_LOG(ContentParent::GetLog(), LogLevel::Debug,
           ("GetNewOrUsedProcess for type %s",
            NS_ConvertUTF16toUTF8(aRemoteType).get()));
@@ -988,9 +987,9 @@ ContentParent::GetNewOrUsedBrowserProcessInternal(Element* aFrameElement,
       && contentParents.Length() >= maxContentParents) {
     MOZ_LOG(ContentParent::GetLog(), LogLevel::Debug,
             ("GetNewOrUsedProcess: returning Large Used process"));
-    return GetNewOrUsedBrowserProcessInternal(
+    return GetNewOrUsedLaunchingBrowserProcess(
         aFrameElement, NS_LITERAL_STRING(DEFAULT_REMOTE_TYPE), aPriority,
-        aOpener, /*aPreferUsed =*/false, aIsSync);
+        aOpener, /*aPreferUsed =*/false);
   }
 
   // Let's try and reuse an existing process.
@@ -1013,7 +1012,7 @@ ContentParent::GetNewOrUsedBrowserProcessInternal(Element* aFrameElement,
            NS_ConvertUTF16toUTF8(aRemoteType).get()));
 
   contentParent = new ContentParent(aOpener, aRemoteType);
-  if (!contentParent->BeginSubprocessLaunch(aIsSync, aPriority)) {
+  if (!contentParent->BeginSubprocessLaunch(aPriority)) {
     // Launch aborted because of shutdown. Bailout.
     contentParent->LaunchSubprocessReject();
     return nullptr;
@@ -1041,82 +1040,89 @@ ContentParent::GetNewOrUsedBrowserProcessAsync(Element* aFrameElement,
                                                ContentParent* aOpener,
                                                bool aPreferUsed) {
   // Obtain a `ContentParent` launched asynchronously.
-  RefPtr<ContentParent> contentParent = GetNewOrUsedBrowserProcessInternal(
+  RefPtr<ContentParent> contentParent = GetNewOrUsedLaunchingBrowserProcess(
-      aFrameElement, aRemoteType, aPriority, aOpener, aPreferUsed,
+      aFrameElement, aRemoteType, aPriority, aOpener, aPreferUsed);
-      /* aIsSync = */ false);
   if (!contentParent) {
     // In case of launch error, stop here.
     return LaunchPromise::CreateAndReject(LaunchError(), __func__);
   }
-
+  return contentParent->WaitForLaunchAsync(aPriority);
-  MOZ_ASSERT(!contentParent->IsDead());
-  if (!contentParent->IsLaunching()) {
-    // `contentParent` is already ready and initialized.
-    return LaunchPromise::CreateAndResolve(contentParent, __func__);
-  }
-
-  // We have located a process that hasn't finished initializing. Let's race
-  // against whoever launched it (and whoever else is already racing). Once
-  // the race is complete, the winner will finish the initialization.
-  RefPtr<ProcessHandlePromise> ready =
-      contentParent->mSubprocess->WhenProcessHandleReady();
-  return ready->Then(
-      GetCurrentThreadSerialEventTarget(), __func__,
-      // On resolve.
-      [contentParent, aPriority]() {
-        if (contentParent->IsLaunching()) {
-          if (!contentParent->LaunchSubprocessResolve(/* aIsSync = */ false,
-                                                      aPriority)) {
-            contentParent->LaunchSubprocessReject();
-            return LaunchPromise::CreateAndReject(LaunchError(), __func__);
-          }
-          contentParent->mActivateTS = TimeStamp::Now();
-        } else if (contentParent->IsDead()) {
-          // This could happen if we're racing against a sync launch and it
-          // failed.
-          return LaunchPromise::CreateAndReject(LaunchError(), __func__);
-        }
-        return LaunchPromise::CreateAndResolve(contentParent, __func__);
-      },
-      // On reject.
-      [contentParent]() {
-        if (contentParent->IsLaunching()) {
-          contentParent->LaunchSubprocessReject();
-        }
-        return LaunchPromise::CreateAndReject(LaunchError(), __func__);
-      });
 }
 
 /*static*/
 already_AddRefed<ContentParent> ContentParent::GetNewOrUsedBrowserProcess(
     Element* aFrameElement, const nsAString& aRemoteType,
     ProcessPriority aPriority, ContentParent* aOpener, bool aPreferUsed) {
-  RefPtr<ContentParent> contentParent = GetNewOrUsedBrowserProcessInternal(
+  RefPtr<ContentParent> contentParent = GetNewOrUsedLaunchingBrowserProcess(
-      aFrameElement, aRemoteType, aPriority, aOpener, aPreferUsed,
+      aFrameElement, aRemoteType, aPriority, aOpener, aPreferUsed);
-      /* aIsSync = */ true);
+  if (!contentParent || !contentParent->WaitForLaunchSync(aPriority)) {
-  if (!contentParent) {
     // In case of launch error, stop here.
     return nullptr;
   }
-
-  MOZ_ASSERT(!contentParent->IsDead());
-  if (!contentParent->IsLaunching()) {
-    // `contentParent` is already ready and initialized
   return contentParent.forget();
 }
 
-  // We have located a process that hasn't finished initializing. We may be
+RefPtr<ContentParent::LaunchPromise> ContentParent::WaitForLaunchAsync(
-  // racing against whoever launched it (and whoever else is already racing).
+    ProcessPriority aPriority) {
-  // Since we're sync, we win the race and finish the initialization.
+  MOZ_DIAGNOSTIC_ASSERT(!IsDead());
-  const bool launchSuccess = contentParent->mSubprocess->WaitForProcessHandle();
+  if (!IsLaunching()) {
+    return LaunchPromise::CreateAndResolve(this, __func__);
+  }
+
+  // We've started an async content process launch.
+  Telemetry::Accumulate(Telemetry::CONTENT_PROCESS_LAUNCH_IS_SYNC, 0);
+
+  // We have located a process that hasn't finished initializing. Let's race
+  // against whoever launched it (and whoever else is already racing). Once
+  // the race is complete, the winner will finish the initialization.
+  return mSubprocess->WhenProcessHandleReady()->Then(
+      GetCurrentThreadSerialEventTarget(), __func__,
+      // On resolve.
+      [self = RefPtr{this}, aPriority]() {
+        if (self->IsLaunching()) {
+          if (!self->LaunchSubprocessResolve(/* aIsSync = */ false,
+                                             aPriority)) {
+            self->LaunchSubprocessReject();
+            return LaunchPromise::CreateAndReject(LaunchError(), __func__);
+          }
+          self->mActivateTS = TimeStamp::Now();
+        } else if (self->IsDead()) {
+          // This could happen if we're racing against a sync launch and it
+          // failed.
+          return LaunchPromise::CreateAndReject(LaunchError(), __func__);
+        }
+        return LaunchPromise::CreateAndResolve(self, __func__);
+      },
+      // On reject.
+      [self = RefPtr{this}]() {
+        if (self->IsLaunching()) {
+          self->LaunchSubprocessReject();
+        }
+        return LaunchPromise::CreateAndReject(LaunchError(), __func__);
+      });
+}
+
+bool ContentParent::WaitForLaunchSync(ProcessPriority aPriority) {
+  MOZ_DIAGNOSTIC_ASSERT(!IsDead());
+  if (!IsLaunching()) {
+    return true;
+  }
+
+  // We've started a sync content process launch.
+  Telemetry::Accumulate(Telemetry::CONTENT_PROCESS_LAUNCH_IS_SYNC, 1);
+
+  // We're a process which hasn't finished initializing. We may be racing
+  // against whoever launched it (and whoever else is already racing). Since
+  // we're sync, we win the race and finish the initialization.
+  bool launchSuccess = mSubprocess->WaitForProcessHandle();
   if (launchSuccess &&
-      contentParent->LaunchSubprocessResolve(/* aIsSync = */ true, aPriority)) {
+      LaunchSubprocessResolve(/* aIsSync = */ true, aPriority)) {
-    contentParent->mActivateTS = TimeStamp::Now();
+    mActivateTS = TimeStamp::Now();
-    return contentParent.forget();
+    return true;
   }
   // In case of failure.
-  contentParent->LaunchSubprocessReject();
+  LaunchSubprocessReject();
-  return nullptr;
+  return false;
 }
 
 /*static*/
@@ -1982,8 +1988,8 @@ bool ContentParent::ShouldKeepProcessAlive() {
     return true;
   }
 
-  if (!sBrowserContentParents) {
+  if (mNumKeepaliveCalls > 0) {
-    return false;
+    return true;
   }
 
   // If we have already been marked as dead, don't prevent shutdown.
@@ -1991,6 +1997,10 @@ bool ContentParent::ShouldKeepProcessAlive() {
     return false;
   }
 
+  if (!sBrowserContentParents) {
+    return false;
+  }
+
   auto contentParents = sBrowserContentParents->Get(mRemoteType);
   if (!contentParents) {
     return false;
@@ -2050,6 +2060,22 @@ void ContentParent::NotifyTabDestroying() {
   StartForceKillTimer();
 }
 
+void ContentParent::AddKeepAlive() {
+  // Something wants to keep this content process alive.
+  ++mNumKeepaliveCalls;
+}
+
+void ContentParent::RemoveKeepAlive() {
+  MOZ_DIAGNOSTIC_ASSERT(mNumKeepaliveCalls > 0);
+  --mNumKeepaliveCalls;
+
+  if (ManagedPBrowserParent().Count() == 0 && !ShouldKeepProcessAlive() &&
+      !TryToRecycle()) {
+    MarkAsDead();
+    MaybeAsyncSendShutDownMessage();
+  }
+}
+
 void ContentParent::StartForceKillTimer() {
   if (mForceKillTimer || !mIPCOpen) {
     return;
@@ -2230,15 +2256,9 @@ void ContentParent::AppendSandboxParams(std::vector<std::string>& aArgs) {
 }
 #endif  // XP_MACOSX && MOZ_SANDBOX
 
-bool ContentParent::BeginSubprocessLaunch(bool aIsSync,
+bool ContentParent::BeginSubprocessLaunch(ProcessPriority aPriority) {
-                                          ProcessPriority aPriority) {
   AUTO_PROFILER_LABEL("ContentParent::LaunchSubprocess", OTHER);
 
-  // Note that, in case of race, we can have a launch started as async
-  // and finished as sync.
-  Telemetry::Accumulate(Telemetry::CONTENT_PROCESS_LAUNCH_IS_SYNC,
-                        static_cast<uint32_t>(aIsSync));
-
   if (!ContentProcessManager::GetSingleton()) {
     // Shutdown has begun, we shouldn't spawn any more child processes.
     return false;
@@ -2379,7 +2399,10 @@ bool ContentParent::LaunchSubprocessResolve(bool aIsSync,
 
 bool ContentParent::LaunchSubprocessSync(
     hal::ProcessPriority aInitialPriority) {
-  if (!BeginSubprocessLaunch(/* aIsSync = */ true, aInitialPriority)) {
+  // We've started a sync content process launch.
+  Telemetry::Accumulate(Telemetry::CONTENT_PROCESS_LAUNCH_IS_SYNC, 1);
+
+  if (!BeginSubprocessLaunch(aInitialPriority)) {
     return false;
   }
   const bool ok = mSubprocess->WaitForProcessHandle();
@@ -2392,7 +2415,10 @@ bool ContentParent::LaunchSubprocessSync(
 
 RefPtr<ContentParent::LaunchPromise> ContentParent::LaunchSubprocessAsync(
     hal::ProcessPriority aInitialPriority) {
-  if (!BeginSubprocessLaunch(/* aIsSync = */ false, aInitialPriority)) {
+  // We've started an async content process launch.
+  Telemetry::Accumulate(Telemetry::CONTENT_PROCESS_LAUNCH_IS_SYNC, 0);
+
+  if (!BeginSubprocessLaunch(aInitialPriority)) {
     // Launch aborted because of shutdown. Bailout.
     LaunchSubprocessReject();
     return LaunchPromise::CreateAndReject(LaunchError(), __func__);
@@ -2432,6 +2458,7 @@ ContentParent::ContentParent(ContentParent* aOpener,
       mJSPluginID(aJSPluginID),
       mRemoteWorkerActorData("ContentParent::mRemoteWorkerActorData"),
       mNumDestroyingTabs(0),
+      mNumKeepaliveCalls(0),
       mLifecycleState(LifecycleState::LAUNCHING),
       mIsForBrowser(!mRemoteType.IsEmpty()),
       mCalledClose(false),

dom/ipc/ContentParent.h

@@ -213,6 +213,28 @@ class ContentParent final
           hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND,
       ContentParent* aOpener = nullptr, bool aPreferUsed = false);
 
+  /**
+   * Get or create a content process, but without waiting for the process
+   * launch to have completed. The returned `ContentParent` may still be in the
+   * "Launching" state.
+   *
+   * Can return `nullptr` in the case of an error.
+   *
+   * Use the `WaitForLaunchAsync` or `WaitForLaunchSync` methods to wait for
+   * the process to be fully launched.
+   */
+  static already_AddRefed<ContentParent> GetNewOrUsedLaunchingBrowserProcess(
+      Element* aFrameElement, const nsAString& aRemoteType,
+      hal::ProcessPriority aPriority =
+          hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND,
+      ContentParent* aOpener = nullptr, bool aPreferUsed = false);
+
+  RefPtr<ContentParent::LaunchPromise> WaitForLaunchAsync(
+      hal::ProcessPriority aPriority =
+          hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND);
+  bool WaitForLaunchSync(hal::ProcessPriority aPriority =
+                             hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND);
+
   /**
    * Get or create a content process for a JS plugin. aPluginID is the id of the
    * JS plugin
@@ -357,6 +379,11 @@ class ContentParent final
   /** Notify that a tab was destroyed during normal operation. */
   void NotifyTabDestroyed(const TabId& aTabId, bool aNotifiedDestroying);
 
+  // Manage the set of `KeepAlive`s on this ContentParent which are preventing
+  // it from being destroyed.
+  void AddKeepAlive();
+  void RemoveKeepAlive();
+
   TestShellParent* CreateTestShell();
 
   bool DestroyTestShell(TestShellParent* aTestShell);
@@ -751,18 +778,9 @@ class ContentParent final
   // Common implementation of LaunchSubprocess{Sync,Async}.
   // Return `true` in case of success, `false` if launch was
   // aborted because of shutdown.
-  bool BeginSubprocessLaunch(bool aIsSync, ProcessPriority aPriority);
+  bool BeginSubprocessLaunch(ProcessPriority aPriority);
   void LaunchSubprocessReject();
   bool LaunchSubprocessResolve(bool aIsSync, ProcessPriority aPriority);
-  // Return `nullptr` in case of error.
-  // Return a `ContentParent` in case of success. This `ContentParent`
-  // may either be ready and initialized or in the process of initializing
-  // asynchronously. In the latter case, the caller is responsible for
-  // finishing initialization.
-  static already_AddRefed<ContentParent> GetNewOrUsedBrowserProcessInternal(
-      Element* aFrameElement, const nsAString& aRemoteType,
-      ProcessPriority aPriority, ContentParent* aOpener, bool aPreferUsed,
-      bool aIsSync);
 
   // Common initialization after sub process launch.
   bool InitInternal(ProcessPriority aPriority);
@@ -1403,6 +1421,8 @@ class ContentParent final
   // NotifyTabDestroying() but not called NotifyTabDestroyed().
   int32_t mNumDestroyingTabs;
 
+  uint32_t mNumKeepaliveCalls;
+
   // The process starts in the LAUNCHING state, and transitions to
   // ALIVE once it can accept IPC messages.  It remains ALIVE only
   // while remote content is being actively used from this process.