fix: cross-origin navigation disposing WebFrameMain instances (#30599)

* refactor: call methods directly from electron:WebContents

Writing static method boilerplate isn't fun

* fix: cross-origin navigation disposing WebFrameMain instances

* fix: move call to WFM::Connect

* refactor: use base::NoDestructor<T>

* Update shell/browser/api/electron_api_web_frame_main.cc

Co-authored-by: Jeremy Rose <[email protected]>

Co-authored-by: samuelmaddock <[email protected]>
Co-authored-by: Jeremy Rose <[email protected]>

shell/browser/api/electron_api_web_contents.cc

@@ -1387,7 +1387,9 @@ void WebContents::HandleNewRenderFrame(
   if (rwh_impl)
     rwh_impl->disable_hidden_ = !background_throttling_;
 
-  WebFrameMain::RenderFrameCreated(render_frame_host);
+  auto* web_frame = WebFrameMain::FromRenderFrameHost(render_frame_host);
+  if (web_frame)
+    web_frame->Connect();
 }
 
 void WebContents::RenderFrameCreated(
@@ -1395,6 +1397,46 @@ void WebContents::RenderFrameCreated(
   HandleNewRenderFrame(render_frame_host);
 }
 
+void WebContents::RenderFrameDeleted(
+    content::RenderFrameHost* render_frame_host) {
+  // A RenderFrameHost can be deleted when:
+  // - A WebContents is removed and its containing frames are disposed.
+  // - An <iframe> is removed from the DOM.
+  // - Cross-origin navigation creates a new RFH in a separate process which
+  //   is swapped by content::RenderFrameHostManager.
+  //
+  // WebFrameMain::FromRenderFrameHost(rfh) will use the RFH's FrameTreeNode ID
+  // to find an existing instance of WebFrameMain. During a cross-origin
+  // navigation, the deleted RFH will be the old host which was swapped out. In
+  // this special case, we need to also ensure that WebFrameMain's internal RFH
+  // matches before marking it as disposed.
+  auto* web_frame = WebFrameMain::FromRenderFrameHost(render_frame_host);
+  if (web_frame && web_frame->render_frame_host() == render_frame_host)
+    web_frame->MarkRenderFrameDisposed();
+}
+
+void WebContents::RenderFrameHostChanged(content::RenderFrameHost* old_host,
+                                         content::RenderFrameHost* new_host) {
+  // During cross-origin navigation, a FrameTreeNode will swap out its RFH.
+  // If an instance of WebFrameMain exists, it will need to have its RFH
+  // swapped as well.
+  //
+  // |old_host| can be a nullptr in so we use |new_host| for looking up the
+  // WebFrameMain instance.
+  auto* web_frame =
+      WebFrameMain::FromFrameTreeNodeId(new_host->GetFrameTreeNodeId());
+  if (web_frame) {
+    CHECK_EQ(web_frame->render_frame_host(), old_host);
+    web_frame->UpdateRenderFrameHost(new_host);
+  }
+}
+
+void WebContents::FrameDeleted(int frame_tree_node_id) {
+  auto* web_frame = WebFrameMain::FromFrameTreeNodeId(frame_tree_node_id);
+  if (web_frame)
+    web_frame->Destroyed();
+}
+
 void WebContents::RenderViewDeleted(content::RenderViewHost* render_view_host) {
   // This event is necessary for tracking any states with respect to
   // intermediate render view hosts aka speculative render view hosts. Currently
@@ -1635,13 +1677,6 @@ void WebContents::UpdateDraggableRegions(
     observer.OnDraggableRegionsUpdated(regions);
 }
 
-void WebContents::RenderFrameDeleted(
-    content::RenderFrameHost* render_frame_host) {
-  // A WebFrameMain can outlive its RenderFrameHost so we need to mark it as
-  // disposed to prevent access to it.
-  WebFrameMain::RenderFrameDeleted(render_frame_host);
-}
-
 void WebContents::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
   EmitNavigationEvent("did-start-navigation", navigation_handle);

shell/browser/api/electron_api_web_contents.h

@@ -544,9 +544,12 @@ class WebContents : public gin::Wrappable<WebContents>,
   void BeforeUnloadFired(bool proceed,
                          const base::TimeTicks& proceed_time) override;
   void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
+  void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
+  void RenderFrameHostChanged(content::RenderFrameHost* old_host,
+                              content::RenderFrameHost* new_host) override;
+  void FrameDeleted(int frame_tree_node_id) override;
   void RenderViewDeleted(content::RenderViewHost*) override;
   void RenderProcessGone(base::TerminationStatus status) override;
-  void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
   void DOMContentLoaded(content::RenderFrameHost* render_frame_host) override;
   void DidFinishLoad(content::RenderFrameHost* render_frame_host,
                      const GURL& validated_url) override;

shell/browser/api/electron_api_web_frame_main.cc

@@ -9,8 +9,8 @@
 #include <utility>
 #include <vector>
 
-#include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/no_destructor.h"
 #include "content/browser/renderer_host/frame_tree_node.h"  // nogncheck
 #include "content/public/browser/render_frame_host.h"
 #include "electron/shell/common/api/api.mojom.h"
@@ -56,36 +56,54 @@ namespace electron {
 
 namespace api {
 
-typedef std::unordered_map<content::RenderFrameHost*, WebFrameMain*>
-    RenderFrameMap;
-base::LazyInstance<RenderFrameMap>::DestructorAtExit g_render_frame_map =
-    LAZY_INSTANCE_INITIALIZER;
+typedef std::unordered_map<int, WebFrameMain*> WebFrameMainIdMap;
 
-WebFrameMain* FromRenderFrameHost(content::RenderFrameHost* rfh) {
-  auto frame_map = g_render_frame_map.Get();
-  auto iter = frame_map.find(rfh);
+WebFrameMainIdMap& GetWebFrameMainMap() {
+  static base::NoDestructor<WebFrameMainIdMap> instance;
+  return *instance;
+}
+
+// static
+WebFrameMain* WebFrameMain::FromFrameTreeNodeId(int frame_tree_node_id) {
+  WebFrameMainIdMap& frame_map = GetWebFrameMainMap();
+  auto iter = frame_map.find(frame_tree_node_id);
   auto* web_frame = iter == frame_map.end() ? nullptr : iter->second;
   return web_frame;
 }
 
+// static
+WebFrameMain* WebFrameMain::FromRenderFrameHost(content::RenderFrameHost* rfh) {
+  return rfh ? FromFrameTreeNodeId(rfh->GetFrameTreeNodeId()) : nullptr;
+}
+
 gin::WrapperInfo WebFrameMain::kWrapperInfo = {gin::kEmbedderNativeGin};
 
-WebFrameMain::WebFrameMain(content::RenderFrameHost* rfh) : render_frame_(rfh) {
-  g_render_frame_map.Get().emplace(rfh, this);
+WebFrameMain::WebFrameMain(content::RenderFrameHost* rfh)
+    : frame_tree_node_id_(rfh->GetFrameTreeNodeId()), render_frame_(rfh) {
+  GetWebFrameMainMap().emplace(frame_tree_node_id_, this);
 }
 
 WebFrameMain::~WebFrameMain() {
+  Destroyed();
+}
+
+void WebFrameMain::Destroyed() {
   MarkRenderFrameDisposed();
+  GetWebFrameMainMap().erase(frame_tree_node_id_);
+  Unpin();
 }
 
 void WebFrameMain::MarkRenderFrameDisposed() {
-  if (render_frame_disposed_)
-    return;
-  Unpin();
-  g_render_frame_map.Get().erase(render_frame_);
+  render_frame_ = nullptr;
   render_frame_disposed_ = true;
 }
 
+void WebFrameMain::UpdateRenderFrameHost(content::RenderFrameHost* rfh) {
+  // Should only be called when swapping frames.
+  DCHECK(render_frame_);
+  render_frame_ = rfh;
+}
+
 bool WebFrameMain::CheckRenderFrame() const {
   if (render_frame_disposed_) {
     v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
@@ -213,9 +231,7 @@ void WebFrameMain::PostMessage(v8::Isolate* isolate,
 }
 
 int WebFrameMain::FrameTreeNodeID() const {
-  if (!CheckRenderFrame())
-    return -1;
-  return render_frame_->GetFrameTreeNodeId();
+  return frame_tree_node_id_;
 }
 
 std::string WebFrameMain::Name() const {
@@ -293,6 +309,13 @@ std::vector<content::RenderFrameHost*> WebFrameMain::FramesInSubtree() const {
   return frame_hosts;
 }
 
+void WebFrameMain::Connect() {
+  if (pending_receiver_) {
+    render_frame_->GetRemoteInterfaces()->GetInterface(
+        std::move(pending_receiver_));
+  }
+}
+
 // static
 gin::Handle<WebFrameMain> WebFrameMain::New(v8::Isolate* isolate) {
   return gin::Handle<WebFrameMain>();
@@ -315,35 +338,6 @@ gin::Handle<WebFrameMain> WebFrameMain::From(v8::Isolate* isolate,
   return handle;
 }
 
-// static
-gin::Handle<WebFrameMain> WebFrameMain::FromID(v8::Isolate* isolate,
-                                               int render_process_id,
-                                               int render_frame_id) {
-  auto* rfh =
-      content::RenderFrameHost::FromID(render_process_id, render_frame_id);
-  return From(isolate, rfh);
-}
-
-// static
-void WebFrameMain::RenderFrameDeleted(content::RenderFrameHost* rfh) {
-  auto* web_frame = FromRenderFrameHost(rfh);
-  if (web_frame)
-    web_frame->MarkRenderFrameDisposed();
-}
-
-void WebFrameMain::RenderFrameCreated(content::RenderFrameHost* rfh) {
-  auto* web_frame = FromRenderFrameHost(rfh);
-  if (web_frame)
-    web_frame->Connect();
-}
-
-void WebFrameMain::Connect() {
-  if (pending_receiver_) {
-    render_frame_->GetRemoteInterfaces()->GetInterface(
-        std::move(pending_receiver_));
-  }
-}
-
 // static
 v8::Local<v8::ObjectTemplate> WebFrameMain::FillObjectTemplate(
     v8::Isolate* isolate,
@@ -387,9 +381,10 @@ v8::Local<v8::Value> FromID(gin_helper::ErrorThrower thrower,
     return v8::Null(thrower.isolate());
   }
 
-  return WebFrameMain::FromID(thrower.isolate(), render_process_id,
-                              render_frame_id)
-      .ToV8();
+  auto* rfh =
+      content::RenderFrameHost::FromID(render_process_id, render_frame_id);
+
+  return WebFrameMain::From(thrower.isolate(), rfh).ToV8();
 }
 
 void Initialize(v8::Local<v8::Object> exports,

shell/browser/api/electron_api_web_frame_main.h

@@ -36,6 +36,8 @@ namespace electron {
 
 namespace api {
 
+class WebContents;
+
 // Bindings for accessing frames from the main process.
 class WebFrameMain : public gin::Wrappable<WebFrameMain>,
                      public gin_helper::Pinnable<WebFrameMain>,
@@ -44,23 +46,12 @@ class WebFrameMain : public gin::Wrappable<WebFrameMain>,
   // Create a new WebFrameMain and return the V8 wrapper of it.
   static gin::Handle<WebFrameMain> New(v8::Isolate* isolate);
 
-  static gin::Handle<WebFrameMain> FromID(v8::Isolate* isolate,
-                                          int render_process_id,
-                                          int render_frame_id);
   static gin::Handle<WebFrameMain> From(
       v8::Isolate* isolate,
       content::RenderFrameHost* render_frame_host);
-
-  // Called to mark any RenderFrameHost as disposed by any WebFrameMain that
-  // may be holding a weak reference.
-  static void RenderFrameDeleted(content::RenderFrameHost* rfh);
-  static void RenderFrameCreated(content::RenderFrameHost* rfh);
-
-  // Mark RenderFrameHost as disposed and to no longer access it. This can
-  // occur upon frame navigation.
-  void MarkRenderFrameDisposed();
-
-  const mojo::Remote<mojom::ElectronRenderer>& GetRendererApi();
+  static WebFrameMain* FromFrameTreeNodeId(int frame_tree_node_id);
+  static WebFrameMain* FromRenderFrameHost(
+      content::RenderFrameHost* render_frame_host);
 
   // gin::Wrappable
   static gin::WrapperInfo kWrapperInfo;
@@ -69,11 +60,28 @@ class WebFrameMain : public gin::Wrappable<WebFrameMain>,
       v8::Local<v8::ObjectTemplate>);
   const char* GetTypeName() override;
 
+  content::RenderFrameHost* render_frame_host() const { return render_frame_; }
+
  protected:
   explicit WebFrameMain(content::RenderFrameHost* render_frame);
   ~WebFrameMain() override;
 
  private:
+  friend class WebContents;
+
+  // Called when FrameTreeNode is deleted.
+  void Destroyed();
+
+  // Mark RenderFrameHost as disposed and to no longer access it. This can
+  // happen when the WebFrameMain v8 handle is GC'd or when a FrameTreeNode
+  // is removed.
+  void MarkRenderFrameDisposed();
+
+  // Swap out the internal RFH when cross-origin navigation occurs.
+  void UpdateRenderFrameHost(content::RenderFrameHost* rfh);
+
+  const mojo::Remote<mojom::ElectronRenderer>& GetRendererApi();
+
   // WebFrameMain can outlive its RenderFrameHost pointer so we need to check
   // whether its been disposed of prior to accessing it.
   bool CheckRenderFrame() const;
@@ -109,6 +117,8 @@ class WebFrameMain : public gin::Wrappable<WebFrameMain>,
   mojo::Remote<mojom::ElectronRenderer> renderer_api_;
   mojo::PendingReceiver<mojom::ElectronRenderer> pending_receiver_;
 
+  int frame_tree_node_id_;
+
   content::RenderFrameHost* render_frame_ = nullptr;
 
   // Whether the RenderFrameHost has been removed and that it should no longer

spec-main/api-web-frame-main-spec.ts

@@ -14,6 +14,24 @@ describe('webFrameMain module', () => {
 
   const fileUrl = (filename: string) => url.pathToFileURL(path.join(subframesPath, filename)).href;
 
+  type Server = { server: http.Server, url: string }
+
+  /** Creates an HTTP server whose handler embeds the given iframe src. */
+  const createServer = () => new Promise<Server>(resolve => {
+    const server = http.createServer((req, res) => {
+      const params = new URLSearchParams(url.parse(req.url || '').search || '');
+      if (params.has('frameSrc')) {
+        res.end(`<iframe src="${params.get('frameSrc')}"></iframe>`);
+      } else {
+        res.end('');
+      }
+    });
+    server.listen(0, '127.0.0.1', () => {
+      const url = `http://127.0.0.1:${(server.address() as AddressInfo).port}/`;
+      resolve({ server, url });
+    });
+  });
+
   afterEach(closeAllWindows);
 
   describe('WebFrame traversal APIs', () => {
@@ -70,24 +88,6 @@ describe('webFrameMain module', () => {
     });
 
     describe('cross-origin', () => {
-      type Server = { server: http.Server, url: string }
-
-      /** Creates an HTTP server whose handler embeds the given iframe src. */
-      const createServer = () => new Promise<Server>(resolve => {
-        const server = http.createServer((req, res) => {
-          const params = new URLSearchParams(url.parse(req.url || '').search || '');
-          if (params.has('frameSrc')) {
-            res.end(`<iframe src="${params.get('frameSrc')}"></iframe>`);
-          } else {
-            res.end('');
-          }
-        });
-        server.listen(0, '127.0.0.1', () => {
-          const url = `http://127.0.0.1:${(server.address() as AddressInfo).port}/`;
-          resolve({ server, url });
-        });
-      });
-
       let serverA = null as unknown as Server;
       let serverB = null as unknown as Server;
 
@@ -194,21 +194,32 @@ describe('webFrameMain module', () => {
     });
   });
 
-  describe('disposed WebFrames', () => {
+  describe('RenderFrame lifespan', () => {
     let w: BrowserWindow;
-    let webFrame: WebFrameMain;
 
-    before(async () => {
+    beforeEach(async () => {
       w = new BrowserWindow({ show: false, webPreferences: { contextIsolation: true } });
+    });
+
+    it('throws upon accessing properties when disposed', async () => {
       await w.loadFile(path.join(subframesPath, 'frame-with-frame-container.html'));
-      webFrame = w.webContents.mainFrame;
+      const { mainFrame } = w.webContents;
       w.destroy();
       // Wait for WebContents, and thus RenderFrameHost, to be destroyed.
       await new Promise(resolve => setTimeout(resolve, 0));
-    });
-
-    it('throws upon accessing properties', () => {
-      expect(() => webFrame.url).to.throw();
+      expect(() => mainFrame.url).to.throw();
+    });
+
+    it('persists through cross-origin navigation', async () => {
+      const server = await createServer();
+      // 'localhost' is treated as a separate origin.
+      const crossOriginUrl = server.url.replace('127.0.0.1', 'localhost');
+      await w.loadURL(server.url);
+      const { mainFrame } = w.webContents;
+      expect(mainFrame.url).to.equal(server.url);
+      await w.loadURL(crossOriginUrl);
+      expect(w.webContents.mainFrame).to.equal(mainFrame);
+      expect(mainFrame.url).to.equal(crossOriginUrl);
     });
   });