From d4f351228c8215eea316e74fca0a2d02cd7c19dc Mon Sep 17 00:00:00 2001
From: John Zimmermann <me@johnnynator.dev>
Date: Mon, 30 Aug 2021 23:02:49 +0200
Subject: [PATCH 2/2] Revert "fix: cross-origin navigation disposing
 WebFrameMain instances (#30598)"

This reverts commit 18de6be607dce5126f18216eb972061be86e3db0.
---
 .../browser/api/electron_api_web_contents.cc  | 51 ++--------
 shell/browser/api/electron_api_web_contents.h |  5 +-
 .../api/electron_api_web_frame_main.cc        | 93 ++++++++++---------
 .../browser/api/electron_api_web_frame_main.h | 38 +++-----
 spec-main/api-web-frame-main-spec.ts          | 63 ++++++-------
 5 files changed, 98 insertions(+), 152 deletions(-)

diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc
index e2fc303..6211b08 100644
--- a/shell/browser/api/electron_api_web_contents.cc
+++ b/shell/browser/api/electron_api_web_contents.cc
@@ -1386,9 +1386,7 @@ void WebContents::HandleNewRenderFrame(
   if (rwh_impl)
     rwh_impl->disable_hidden_ = !background_throttling_;
 
-  auto* web_frame = WebFrameMain::FromRenderFrameHost(render_frame_host);
-  if (web_frame)
-    web_frame->Connect();
+  WebFrameMain::RenderFrameCreated(render_frame_host);
 }
 
 void WebContents::RenderFrameCreated(
@@ -1396,46 +1394,6 @@ 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
@@ -1677,6 +1635,13 @@ 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);
diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h
index 3ed91c9..3f87f8b 100644
--- a/shell/browser/api/electron_api_web_contents.h
+++ b/shell/browser/api/electron_api_web_contents.h
@@ -551,12 +551,9 @@ 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;
diff --git a/shell/browser/api/electron_api_web_frame_main.cc b/shell/browser/api/electron_api_web_frame_main.cc
index 185cdc4..9ef45ea 100644
--- a/shell/browser/api/electron_api_web_frame_main.cc
+++ b/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"
@@ -34,54 +34,36 @@ namespace electron {
 
 namespace api {
 
-typedef std::unordered_map<int, WebFrameMain*> WebFrameMainIdMap;
+typedef std::unordered_map<content::RenderFrameHost*, WebFrameMain*>
+    RenderFrameMap;
+base::LazyInstance<RenderFrameMap>::DestructorAtExit g_render_frame_map =
+    LAZY_INSTANCE_INITIALIZER;
 
-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);
+WebFrameMain* FromRenderFrameHost(content::RenderFrameHost* rfh) {
+  auto frame_map = g_render_frame_map.Get();
+  auto iter = frame_map.find(rfh);
   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)
-    : frame_tree_node_id_(rfh->GetFrameTreeNodeId()), render_frame_(rfh) {
-  GetWebFrameMainMap().emplace(frame_tree_node_id_, this);
+WebFrameMain::WebFrameMain(content::RenderFrameHost* rfh) : render_frame_(rfh) {
+  g_render_frame_map.Get().emplace(rfh, this);
 }
 
 WebFrameMain::~WebFrameMain() {
-  Destroyed();
-}
-
-void WebFrameMain::Destroyed() {
   MarkRenderFrameDisposed();
-  GetWebFrameMainMap().erase(frame_tree_node_id_);
-  Unpin();
 }
 
 void WebFrameMain::MarkRenderFrameDisposed() {
-  render_frame_ = nullptr;
+  if (render_frame_disposed_)
+    return;
+  Unpin();
+  g_render_frame_map.Get().erase(render_frame_);
   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();
@@ -209,7 +191,9 @@ void WebFrameMain::PostMessage(v8::Isolate* isolate,
 }
 
 int WebFrameMain::FrameTreeNodeID() const {
-  return frame_tree_node_id_;
+  if (!CheckRenderFrame())
+    return -1;
+  return render_frame_->GetFrameTreeNodeId();
 }
 
 std::string WebFrameMain::Name() const {
@@ -281,13 +265,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_));
-  }
-}
-
 // static
 gin::Handle<WebFrameMain> WebFrameMain::New(v8::Isolate* isolate) {
   return gin::Handle<WebFrameMain>();
@@ -310,6 +287,35 @@ 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,
@@ -352,10 +358,9 @@ v8::Local<v8::Value> FromID(gin_helper::ErrorThrower thrower,
     return v8::Null(thrower.isolate());
   }
 
-  auto* rfh =
-      content::RenderFrameHost::FromID(render_process_id, render_frame_id);
-
-  return WebFrameMain::From(thrower.isolate(), rfh).ToV8();
+  return WebFrameMain::FromID(thrower.isolate(), render_process_id,
+                              render_frame_id)
+      .ToV8();
 }
 
 void Initialize(v8::Local<v8::Object> exports,
diff --git a/shell/browser/api/electron_api_web_frame_main.h b/shell/browser/api/electron_api_web_frame_main.h
index 188f9c1..4d0dc98 100644
--- a/shell/browser/api/electron_api_web_frame_main.h
+++ b/shell/browser/api/electron_api_web_frame_main.h
@@ -34,8 +34,6 @@ 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,12 +42,23 @@ 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);
-  static WebFrameMain* FromFrameTreeNodeId(int frame_tree_node_id);
-  static WebFrameMain* FromRenderFrameHost(
-      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();
 
   // gin::Wrappable
   static gin::WrapperInfo kWrapperInfo;
@@ -58,28 +67,11 @@ 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;
@@ -114,8 +106,6 @@ 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
diff --git a/spec-main/api-web-frame-main-spec.ts b/spec-main/api-web-frame-main-spec.ts
index 255fcaf..fa178f3 100644
--- a/spec-main/api-web-frame-main-spec.ts
+++ b/spec-main/api-web-frame-main-spec.ts
@@ -13,24 +13,6 @@ 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', () => {
@@ -87,6 +69,24 @@ 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;
 
@@ -179,32 +179,21 @@ describe('webFrameMain module', () => {
     });
   });
 
-  describe('RenderFrame lifespan', () => {
+  describe('disposed WebFrames', () => {
     let w: BrowserWindow;
+    let webFrame: WebFrameMain;
 
-    beforeEach(async () => {
+    before(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'));
-      const { mainFrame } = w.webContents;
+      webFrame = w.webContents.mainFrame;
       w.destroy();
       // Wait for WebContents, and thus RenderFrameHost, to be destroyed.
       await new Promise(resolve => setTimeout(resolve, 0));
-      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);
+    });
+
+    it('throws upon accessing properties', () => {
+      expect(() => webFrame.url).to.throw();
     });
   });
 
-- 
2.33.0

