From 4bb1a054be4162349d335dbe0b270d920f90f018 Mon Sep 17 00:00:00 2001
From: Nika Layzell <nika@thelayzells.com>
Date: Tue, 9 Jan 2024 00:48:17 +0000
Subject: [PATCH] Bug 1871625 - Move AppShutdownConfirmed check in
 nsFrameLoader to a more appropriate place, r=smaug

This check was introduced in bug 1811195, but was added outside of the
normal opening flow, meaning that failures after this check would not be
handled properly, leading to potential assertion failures or unhandled
content process creation failures.

This patch adds a clarifying comment to ensure that we don't add checks
to the wrong place in the future, and moves the check into
`ContentParent::CreateBrowser` such that it is handled in a similar way
to other content process creation failures.

In addition, extra logic was added to ensure `mInitialized` gets set on
TryRemoteBrowserInternal failure, as new checks have been added before
the `mInitialized` check, and this seems likely to happen more in the
future.

Differential Revision: https://phabricator.services.mozilla.com/D197541
---
 dom/base/nsFrameLoader.cpp | 12 +++++++-----
 dom/ipc/ContentParent.cpp  |  8 ++++++++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/dom/base/nsFrameLoader.cpp b/dom/base/nsFrameLoader.cpp
index 198c79a29f02..2c0e9859f8d7 100644
--- a/dom/base/nsFrameLoader.cpp
+++ b/dom/base/nsFrameLoader.cpp
@@ -2800,17 +2800,19 @@ bool nsFrameLoader::TryRemoteBrowserInternal() {
 }
 
 bool nsFrameLoader::TryRemoteBrowser() {
-  // Creating remote browsers may result in creating new processes, but during
-  // parent shutdown that would add just noise, so better bail out.
-  if (AppShutdown::IsInOrBeyond(ShutdownPhase::AppShutdownConfirmed)) {
-    return false;
-  }
+  // NOTE: Do not add new checks to this function, it exists to ensure we always
+  // MaybeNotifyCrashed for any errors within TryRemoteBrowserInternal. If new
+  // checks are added, they should be added into that function, not here.
 
   // Try to create the internal remote browser.
   if (TryRemoteBrowserInternal()) {
     return true;
   }
 
+  // We shouldn't TryRemoteBrowser again, even if a check failed before we
+  // initialize mInitialized within TryRemoteBrowserInternal.
+  mInitialized = true;
+
   // Check if we should report a browser-crashed error because the browser
   // failed to start.
   if (XRE_IsParentProcess() && mOwnerContent && mOwnerContent->IsXULElement()) {
diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
index db060dd3b57a..75b584bc3b80 100644
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -1444,6 +1444,14 @@ already_AddRefed<RemoteBrowser> ContentParent::CreateBrowser(
       "BrowsingContext must not have BrowserParent, or have previous "
       "BrowserParent cleared");
 
+  // Don't bother creating new content browsers after entering shutdown. This
+  // could lead to starting a new content process, which may significantly delay
+  // shutdown, and the content is unlikely to be displayed.
+  if (AppShutdown::IsInOrBeyond(ShutdownPhase::AppShutdownConfirmed)) {
+    NS_WARNING("Ignoring remote browser creation request during shutdown");
+    return nullptr;
+  }
+
   nsAutoCString remoteType(aRemoteType);
   if (remoteType.IsEmpty()) {
     remoteType = DEFAULT_REMOTE_TYPE;