wip

Writing static method boilerplate isn't fun

shell/browser/api/electron_api_web_contents.cc

@@ -1390,13 +1390,39 @@ void WebContents::HandleNewRenderFrame(
       static_cast<content::RenderWidgetHostImpl*>(rwhv->GetRenderWidgetHost());
   if (rwh_impl)
     rwh_impl->disable_hidden_ = !background_throttling_;
-
-  WebFrameMain::RenderFrameCreated(render_frame_host);
 }
 
 void WebContents::RenderFrameCreated(
     content::RenderFrameHost* render_frame_host) {
   HandleNewRenderFrame(render_frame_host);
+
+  auto* web_frame = WebFrameMain::FromRenderFrameHost(render_frame_host);
+  if (web_frame)
+    web_frame->Connect();
+}
+
+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::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) {
@@ -1639,13 +1665,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

@@ -541,9 +541,10 @@ 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 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

@@ -13,6 +13,7 @@
 #include "base/logging.h"
 #include "content/browser/renderer_host/frame_tree_node.h"  // nogncheck
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
 #include "electron/shell/common/api/api.mojom.h"
 #include "gin/object_template_builder.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
@@ -56,36 +57,53 @@ 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);
+base::LazyInstance<WebFrameMainIdMap>::DestructorAtExit
+    g_web_frame_main_id_map = LAZY_INSTANCE_INITIALIZER;
+
+// static
+WebFrameMain* WebFrameMain::FromFrameTreeNodeId(int frame_tree_node_id) {
+  auto frame_map = g_web_frame_main_id_map.Get();
+  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()) {
+  g_web_frame_main_id_map.Get().emplace(frame_tree_node_id_, this);
 }
 
 WebFrameMain::~WebFrameMain() {
+  Destroyed();
+}
+
+void WebFrameMain::Destroyed() {
   MarkRenderFrameDisposed();
+  g_web_frame_main_id_map.Get().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_disposed_ = true;
 }
 
+content::RenderFrameHost* WebFrameMain::render_frame_host() const {
+  content::WebContents* web_contents =
+      content::WebContents::FromFrameTreeNodeId(frame_tree_node_id_);
+  if (!web_contents)
+    return nullptr;
+  return web_contents->UnsafeFindFrameByFrameTreeNodeId(frame_tree_node_id_);
+}
+
 bool WebFrameMain::CheckRenderFrame() const {
   if (render_frame_disposed_) {
     v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
@@ -124,13 +142,13 @@ v8::Local<v8::Promise> WebFrameMain::ExecuteJavaScript(
   }
 
   if (user_gesture) {
-    auto* ftn = content::FrameTreeNode::From(render_frame_);
+    auto* ftn = content::FrameTreeNode::GloballyFindByID(frame_tree_node_id_);
     ftn->UpdateUserActivationState(
         blink::mojom::UserActivationUpdateType::kNotifyActivation,
         blink::mojom::UserActivationNotificationType::kTest);
   }
 
-  render_frame_->ExecuteJavaScriptForTests(
+  render_frame_host()->ExecuteJavaScriptForTests(
       code, base::BindOnce([](gin_helper::Promise<base::Value> promise,
                               base::Value value) { promise.Resolve(value); },
                            std::move(promise)));
@@ -141,7 +159,7 @@ v8::Local<v8::Promise> WebFrameMain::ExecuteJavaScript(
 bool WebFrameMain::Reload() {
   if (!CheckRenderFrame())
     return false;
-  return render_frame_->Reload();
+  return render_frame_host()->Reload();
 }
 
 void WebFrameMain::Send(v8::Isolate* isolate,
@@ -165,8 +183,8 @@ void WebFrameMain::Send(v8::Isolate* isolate,
 const mojo::Remote<mojom::ElectronRenderer>& WebFrameMain::GetRendererApi() {
   if (!renderer_api_) {
     pending_receiver_ = renderer_api_.BindNewPipeAndPassReceiver();
-    if (render_frame_->IsRenderFrameCreated()) {
-      render_frame_->GetRemoteInterfaces()->GetInterface(
+    if (render_frame_host()->IsRenderFrameCreated()) {
+      render_frame_host()->GetRemoteInterfaces()->GetInterface(
           std::move(pending_receiver_));
     }
     renderer_api_.set_disconnect_handler(base::BindOnce(
@@ -213,59 +231,57 @@ 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 {
   if (!CheckRenderFrame())
     return std::string();
-  return render_frame_->GetFrameName();
+  return render_frame_host()->GetFrameName();
 }
 
 base::ProcessId WebFrameMain::OSProcessID() const {
   if (!CheckRenderFrame())
     return -1;
   base::ProcessHandle process_handle =
-      render_frame_->GetProcess()->GetProcess().Handle();
+      render_frame_host()->GetProcess()->GetProcess().Handle();
   return base::GetProcId(process_handle);
 }
 
 int WebFrameMain::ProcessID() const {
   if (!CheckRenderFrame())
     return -1;
-  return render_frame_->GetProcess()->GetID();
+  return render_frame_host()->GetProcess()->GetID();
 }
 
 int WebFrameMain::RoutingID() const {
   if (!CheckRenderFrame())
     return -1;
-  return render_frame_->GetRoutingID();
+  return render_frame_host()->GetRoutingID();
 }
 
 GURL WebFrameMain::URL() const {
   if (!CheckRenderFrame())
     return GURL::EmptyGURL();
-  return render_frame_->GetLastCommittedURL();
+  return render_frame_host()->GetLastCommittedURL();
 }
 
 blink::mojom::PageVisibilityState WebFrameMain::VisibilityState() const {
   if (!CheckRenderFrame())
     return blink::mojom::PageVisibilityState::kHidden;
-  return render_frame_->GetVisibilityState();
+  return render_frame_host()->GetVisibilityState();
 }
 
 content::RenderFrameHost* WebFrameMain::Top() const {
   if (!CheckRenderFrame())
     return nullptr;
-  return render_frame_->GetMainFrame();
+  return render_frame_host()->GetMainFrame();
 }
 
 content::RenderFrameHost* WebFrameMain::Parent() const {
   if (!CheckRenderFrame())
     return nullptr;
-  return render_frame_->GetParent();
+  return render_frame_host()->GetParent();
 }
 
 std::vector<content::RenderFrameHost*> WebFrameMain::Frames() const {
@@ -273,8 +289,8 @@ std::vector<content::RenderFrameHost*> WebFrameMain::Frames() const {
   if (!CheckRenderFrame())
     return frame_hosts;
 
-  for (auto* rfh : render_frame_->GetFramesInSubtree()) {
-    if (rfh->GetParent() == render_frame_)
+  for (auto* rfh : render_frame_host()->GetFramesInSubtree()) {
+    if (rfh->GetParent() == render_frame_host())
       frame_hosts.push_back(rfh);
   }
 
@@ -286,13 +302,20 @@ std::vector<content::RenderFrameHost*> WebFrameMain::FramesInSubtree() const {
   if (!CheckRenderFrame())
     return frame_hosts;
 
-  for (auto* rfh : render_frame_->GetFramesInSubtree()) {
+  for (auto* rfh : render_frame_host()->GetFramesInSubtree()) {
     frame_hosts.push_back(rfh);
   }
 
   return frame_hosts;
 }
 
+void WebFrameMain::Connect() {
+  if (pending_receiver_) {
+    render_frame_host()->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

@@ -31,6 +31,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>,
@@ -39,23 +41,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;
@@ -64,11 +55,25 @@ class WebFrameMain : public gin::Wrappable<WebFrameMain>,
       v8::Local<v8::ObjectTemplate>);
   const char* GetTypeName() override;
 
+  content::RenderFrameHost* render_frame_host() const;
+
  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();
+
+  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;
@@ -104,7 +109,7 @@ class WebFrameMain : public gin::Wrappable<WebFrameMain>,
   mojo::Remote<mojom::ElectronRenderer> renderer_api_;
   mojo::PendingReceiver<mojom::ElectronRenderer> pending_receiver_;
 
-  content::RenderFrameHost* render_frame_ = nullptr;
+  int frame_tree_node_id_;
 
   // Whether the RenderFrameHost has been removed and that it should no longer
   // be accessed.

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));
+      expect(() => mainFrame.url).to.throw();
     });
 
-    it('throws upon accessing properties', () => {
-      expect(() => webFrame.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);
     });
   });