From c47b196d02b5e0ceab7741aa48c84489772937bb Mon Sep 17 00:00:00 2001
From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com>
Date: Thu, 2 Jun 2022 20:12:50 -0700
Subject: [PATCH] fix: render process crash handling (#34430)

* fix: crash when renderer process is reused

Could occur when a renderer crashes and the same-origin URL is loaded again
which leads to reusing the renderer process.

* test: renderer process crash recovery

* fix: handle case which leads to render frame DCHECK

* fix: lint

Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>
---
 .../api/electron_api_web_frame_main.cc        |  9 ++++++-
 shell/browser/electron_browser_client.cc      |  3 +++
 spec-main/api-web-frame-main-spec.ts          | 27 +++++++++++++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/shell/browser/api/electron_api_web_frame_main.cc b/shell/browser/api/electron_api_web_frame_main.cc
index a18f4caaf8d05..31f27d1c4539b 100644
--- a/shell/browser/api/electron_api_web_frame_main.cc
+++ b/shell/browser/api/electron_api_web_frame_main.cc
@@ -189,13 +189,20 @@ const mojo::Remote<mojom::ElectronRenderer>& WebFrameMain::GetRendererApi() {
 }
 
 void WebFrameMain::MaybeSetupMojoConnection() {
+  if (render_frame_disposed_) {
+    // RFH may not be set yet if called between when a new RFH is created and
+    // before it's been swapped with an old RFH.
+    LOG(INFO) << "Attempt to setup WebFrameMain connection while render frame "
+                 "is disposed";
+    return;
+  }
+
   if (!renderer_api_) {
     pending_receiver_ = renderer_api_.BindNewPipeAndPassReceiver();
     renderer_api_.set_disconnect_handler(base::BindOnce(
         &WebFrameMain::OnRendererConnectionError, weak_factory_.GetWeakPtr()));
   }
 
-  // Render frame should exist when this method is called.
   DCHECK(render_frame_);
 
   // Wait for RenderFrame to be created in renderer before accessing remote.
diff --git a/shell/browser/electron_browser_client.cc b/shell/browser/electron_browser_client.cc
index 36dfe72633da3..1148e1a86c974 100644
--- a/shell/browser/electron_browser_client.cc
+++ b/shell/browser/electron_browser_client.cc
@@ -436,6 +436,9 @@ void ElectronBrowserClient::RenderProcessWillLaunch(
       new extensions::MessagingAPIMessageFilter(process_id, browser_context));
 #endif
 
+  // Remove in case the host is reused after a crash, otherwise noop.
+  host->RemoveObserver(this);
+
   // ensure the ProcessPreferences is removed later
   host->AddObserver(this);
 }
diff --git a/spec-main/api-web-frame-main-spec.ts b/spec-main/api-web-frame-main-spec.ts
index 7f947f7f54922..bf3106a6efb70 100644
--- a/spec-main/api-web-frame-main-spec.ts
+++ b/spec-main/api-web-frame-main-spec.ts
@@ -224,6 +224,33 @@ describe('webFrameMain module', () => {
       expect(w.webContents.mainFrame).to.equal(mainFrame);
       expect(mainFrame.url).to.equal(crossOriginUrl);
     });
+
+    it('recovers from renderer crash on same-origin', async () => {
+      const server = await createServer();
+      // Keep reference to mainFrame alive throughout crash and recovery.
+      const { mainFrame } = w.webContents;
+      await w.webContents.loadURL(server.url);
+      w.webContents.forcefullyCrashRenderer();
+      await w.webContents.loadURL(server.url);
+      // Log just to keep mainFrame in scope.
+      console.log('mainFrame.url', mainFrame.url);
+    });
+
+    // Fixed by #34411
+    it('recovers from renderer crash on cross-origin', async () => {
+      const server = await createServer();
+      // 'localhost' is treated as a separate origin.
+      const crossOriginUrl = server.url.replace('127.0.0.1', 'localhost');
+      // Keep reference to mainFrame alive throughout crash and recovery.
+      const { mainFrame } = w.webContents;
+      await w.webContents.loadURL(server.url);
+      w.webContents.forcefullyCrashRenderer();
+      // A short wait seems to be required to reproduce the crash.
+      await new Promise(resolve => setTimeout(resolve, 100));
+      await w.webContents.loadURL(crossOriginUrl);
+      // Log just to keep mainFrame in scope.
+      console.log('mainFrame.url', mainFrame.url);
+    });
   });
 
   describe('webFrameMain.fromId', () => {