From 12409da39fea83a72f727ed6c179a99ab5e979bf Mon Sep 17 00:00:00 2001 From: samuelmaddock Date: Thu, 2 Jun 2022 15:09:13 -0400 Subject: [PATCH 1/4] 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. --- shell/browser/electron_browser_client.cc | 3 +++ 1 file changed, 3 insertions(+) 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); } From 84d34e2239b17914dc50dce7a6e0f2a10fba768e Mon Sep 17 00:00:00 2001 From: samuelmaddock Date: Thu, 2 Jun 2022 15:09:30 -0400 Subject: [PATCH 2/4] test: renderer process crash recovery --- spec-main/api-web-frame-main-spec.ts | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) 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', () => { From c7f62acccbde23c0fdcbce9a39f4b682399b6960 Mon Sep 17 00:00:00 2001 From: samuelmaddock Date: Thu, 2 Jun 2022 17:45:09 -0400 Subject: [PATCH 3/4] fix: handle case which leads to render frame DCHECK --- shell/browser/api/electron_api_web_frame_main.cc | 8 +++++++- 1 file changed, 7 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..715c60c04f9ec 100644 --- a/shell/browser/api/electron_api_web_frame_main.cc +++ b/shell/browser/api/electron_api_web_frame_main.cc @@ -189,13 +189,19 @@ const mojo::Remote& 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. From 114b55d11a5e119448d7761fab71db8c99173895 Mon Sep 17 00:00:00 2001 From: samuelmaddock Date: Thu, 2 Jun 2022 17:55:14 -0400 Subject: [PATCH 4/4] fix: lint --- shell/browser/api/electron_api_web_frame_main.cc | 3 ++- 1 file changed, 2 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 715c60c04f9ec..31f27d1c4539b 100644 --- a/shell/browser/api/electron_api_web_frame_main.cc +++ b/shell/browser/api/electron_api_web_frame_main.cc @@ -192,7 +192,8 @@ 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"; + LOG(INFO) << "Attempt to setup WebFrameMain connection while render frame " + "is disposed"; return; }