Repository: electron/electron (language: cpp)

Issue:
[Bug]: Crash in WebFrameMain::Connect() -> GetInterface -> GetInterfaceByName
### Preflight Checklist

- [X] I have read the [Contributing Guidelines](https://github.com/electron/electron/blob/main/CONTRIBUTING.md) for this project.
- [X] I agree to follow the [Code of Conduct](https://github.com/electron/electron/blob/main/CODE_OF_CONDUCT.md) that this project adheres to.
- [X] I have searched the [issue tracker](https://www.github.com/electron/electron/issues) for a bug report that matches the one I want to file, without success.

### Electron Version

17, 18

### What operating system are you using?

Windows

### Operating System Version

Windows 10 19042.985

### What arch are you using?

x64

### Last Known Working Electron version

14

### Expected Behavior

No crash

### Actual Behavior

Electron crashes
![2022-04-21 10_07_53-Source Not Found (Debugging) - Microsoft Visual Studio](https://user-images.githubusercontent.com/10595986/164416772-c4800f31-d165-4639-88e3-23be25d8fe8c.png)

This does not happen consistently but fairly often and can be reproduced in the app.
I have not been able yet to make a small/standalone reproducable example

### Testcase Gist URL

_No response_

### Additional Information

_No response_


Discussion / hints:
Some testing results:
15.0.0 -> working
16.0.0 -> working
16.2.3 -> working
17.0.0 -> crash
17.3.1 -> crash
17.4.1 -> crash
18.0.4 -> crash

I can see that each time before the crash it seems like a renderer is crashing first and then 2-3s later electron crashes
Thanks for including the stacktrace! This seems to be occurring when a speculative RenderFrameHost is created upon navigating with `webContents.loadURL`.

Here we prevent emitting the "frame-created" event with speculative render frames. A potential fix would be to prevent calling `WebFrameMain::Connect` with speculative render frames.

https://github.com/electron/electron/blob/6a1748da0608428fe902a3fc04a27899cd9803ad/shell/browser/api/electron_api_web_contents.cc#L1525-L1539

To write a reproducible test case, it will have something to do with cross-origin navigation.

Fix patch:
diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc
index f27eb39170c74..19dd04b58ea9f 100644
--- a/shell/browser/api/electron_api_web_contents.cc
+++ b/shell/browser/api/electron_api_web_contents.cc
@@ -1506,7 +1506,7 @@ void WebContents::HandleNewRenderFrame(
 
   auto* web_frame = WebFrameMain::FromRenderFrameHost(render_frame_host);
   if (web_frame)
-    web_frame->Connect();
+    web_frame->MaybeSetupMojoConnection();
 }
 
 void WebContents::OnBackgroundColorChanged() {
diff --git a/shell/browser/api/electron_api_web_frame_main.cc b/shell/browser/api/electron_api_web_frame_main.cc
index d39943e4d64cb..f8bc7c9efcd11 100644
--- a/shell/browser/api/electron_api_web_frame_main.cc
+++ b/shell/browser/api/electron_api_web_frame_main.cc
@@ -96,13 +96,15 @@ void WebFrameMain::Destroyed() {
 void WebFrameMain::MarkRenderFrameDisposed() {
   render_frame_ = nullptr;
   render_frame_disposed_ = true;
+  TeardownMojoConnection();
 }
 
 void WebFrameMain::UpdateRenderFrameHost(content::RenderFrameHost* rfh) {
   // Should only be called when swapping frames.
   render_frame_disposed_ = false;
   render_frame_ = rfh;
-  renderer_api_.reset();
+  TeardownMojoConnection();
+  MaybeSetupMojoConnection();
 }
 
 bool WebFrameMain::CheckRenderFrame() const {
@@ -181,20 +183,30 @@ void WebFrameMain::Send(v8::Isolate* isolate,
 }
 
 const mojo::Remote<mojom::ElectronRenderer>& WebFrameMain::GetRendererApi() {
+  MaybeSetupMojoConnection();
+  return renderer_api_;
+}
+
+void WebFrameMain::MaybeSetupMojoConnection() {
   if (!renderer_api_) {
     pending_receiver_ = renderer_api_.BindNewPipeAndPassReceiver();
-    if (render_frame_->IsRenderFrameCreated()) {
-      render_frame_->GetRemoteInterfaces()->GetInterface(
-          std::move(pending_receiver_));
-    }
     renderer_api_.set_disconnect_handler(base::BindOnce(
         &WebFrameMain::OnRendererConnectionError, weak_factory_.GetWeakPtr()));
   }
-  return renderer_api_;
+  // Wait for RenderFrame to be created in renderer before accessing remote.
+  if (pending_receiver_ && render_frame_->IsRenderFrameCreated()) {
+    render_frame_->GetRemoteInterfaces()->GetInterface(
+        std::move(pending_receiver_));
+  }
 }
 
-void WebFrameMain::OnRendererConnectionError() {
+void WebFrameMain::TeardownMojoConnection() {
   renderer_api_.reset();
+  pending_receiver_.reset();
+}
+
+void WebFrameMain::OnRendererConnectionError() {
+  TeardownMojoConnection();
 }
 
 void WebFrameMain::PostMessage(v8::Isolate* isolate,
@@ -314,13 +326,6 @@ std::vector<content::RenderFrameHost*> WebFrameMain::FramesInSubtree() const {
   return frame_hosts;
 }
 
-void WebFrameMain::Connect() {
-  if (pending_receiver_) {
-    render_frame_->GetRemoteInterfaces()->GetInterface(
-        std::move(pending_receiver_));
-  }
-}
-
 void WebFrameMain::DOMContentLoaded() {
   Emit("dom-ready");
 }
diff --git a/shell/browser/api/electron_api_web_frame_main.h b/shell/browser/api/electron_api_web_frame_main.h
index 23a3b657fd45c..b45264db59627 100644
--- a/shell/browser/api/electron_api_web_frame_main.h
+++ b/shell/browser/api/electron_api_web_frame_main.h
@@ -82,6 +82,9 @@ class WebFrameMain : public gin::Wrappable<WebFrameMain>,
   void UpdateRenderFrameHost(content::RenderFrameHost* rfh);
 
   const mojo::Remote<mojom::ElectronRenderer>& GetRendererApi();
+  void MaybeSetupMojoConnection();
+  void TeardownMojoConnection();
+  void OnRendererConnectionError();
 
   // WebFrameMain can outlive its RenderFrameHost pointer so we need to check
   // whether its been disposed of prior to accessing it.
@@ -112,8 +115,6 @@ class WebFrameMain : public gin::Wrappable<WebFrameMain>,
   std::vector<content::RenderFrameHost*> Frames() const;
   std::vector<content::RenderFrameHost*> FramesInSubtree() const;
 
-  void OnRendererConnectionError();
-  void Connect();
   void DOMContentLoaded();
 
   mojo::Remote<mojom::ElectronRenderer> renderer_api_;


Test patch: