fix: dangling speculative frames (#45686)

* fix: dangling speculative frames

Co-authored-by: Samuel Maddock <[email protected]>

* harden lifecycle state checks

Co-authored-by: Samuel Maddock <[email protected]>

* feedback

Co-authored-by: Samuel Maddock <[email protected]>

* add const

Co-authored-by: Samuel Maddock <[email protected]>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Samuel Maddock <[email protected]>

shell/browser/api/electron_api_web_contents.cc

@@ -1792,7 +1792,6 @@ void WebContents::RenderFrameDeleted(
   // - 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
@@ -1800,8 +1799,13 @@ void WebContents::RenderFrameDeleted(
   // 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();
+  if (web_frame) {
+    // Need to directly compare frame tokens as frames pending deletion can no
+    // longer be looked up using content::RenderFrameHost::FromFrameToken().
+    if (web_frame->frame_token_ == render_frame_host->GetGlobalFrameToken()) {
+      web_frame->MarkRenderFrameDisposed();
+    }
+  }
 }
 
 void WebContents::RenderFrameHostChanged(content::RenderFrameHost* old_host,

shell/browser/api/electron_api_web_frame_main.cc

@@ -27,6 +27,7 @@
 #include "shell/common/gin_converters/blink_converter.h"
 #include "shell/common/gin_converters/frame_converter.h"
 #include "shell/common/gin_converters/gurl_converter.h"
+#include "shell/common/gin_converters/std_converter.h"
 #include "shell/common/gin_converters/value_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/error_thrower.h"
@@ -37,31 +38,34 @@
 
 namespace {
 
+using LifecycleState = content::RenderFrameHostImpl::LifecycleStateImpl;
+
+// RenderFrameCreated is called for speculative frames which may not be
+// used in certain cross-origin navigations. Invoking
+// RenderFrameHost::GetLifecycleState currently crashes when called for
+// speculative frames so we need to filter it out for now. Check
+// https://crbug.com/1183639 for details on when this can be removed.
+[[nodiscard]] LifecycleState GetLifecycleState(
+    const content::RenderFrameHost* rfh) {
+  const auto* rfh_impl = static_cast<const content::RenderFrameHostImpl*>(rfh);
+  return rfh_impl->lifecycle_state();
+}
+
 // RenderFrameHost (RFH) exists as a child of a FrameTreeNode. When a
 // cross-origin navigation occurs, the FrameTreeNode swaps RFHs. After
 // swapping, the old RFH will be marked for deletion and run any unload
 // listeners. If an IPC is sent during an unload/beforeunload listener,
 // it's possible that it arrives after the RFH swap and has been
 // detached from the FrameTreeNode.
-bool IsDetachedFrameHost(content::RenderFrameHost* rfh) {
+[[nodiscard]] bool IsDetachedFrameHost(const content::RenderFrameHost* rfh) {
   if (!rfh)
     return true;
 
-  // RenderFrameCreated is called for speculative frames which may not be
-  // used in certain cross-origin navigations. Invoking
-  // RenderFrameHost::GetLifecycleState currently crashes when called for
-  // speculative frames so we need to filter it out for now. Check
-  // https://crbug.com/1183639 for details on when this can be removed.
-  auto* rfh_impl = static_cast<content::RenderFrameHostImpl*>(rfh);
-
   // During cross-origin navigation, a RFH may be swapped out of its
   // FrameTreeNode with a new RFH. In these cases, it's marked for
   // deletion. As this pending deletion RFH won't be following future
-  // swaps, we need to indicate that its been pinned.
-  return (rfh_impl->lifecycle_state() !=
-              content::RenderFrameHostImpl::LifecycleStateImpl::kSpeculative &&
-          rfh->GetLifecycleState() ==
-              content::RenderFrameHost::LifecycleState::kPendingDeletion);
+  // swaps, we need to indicate that its been detached.
+  return GetLifecycleState(rfh) == LifecycleState::kRunningUnloadHandlers;
 }
 
 }  // namespace
@@ -117,8 +121,6 @@ FrameTokenMap& GetFrameTokenMap() {
 // static
 WebFrameMain* WebFrameMain::FromFrameTreeNodeId(
     content::FrameTreeNodeId frame_tree_node_id) {
-  // Pinned frames aren't tracked across navigations so only non-pinned
-  // frames will be retrieved.
   FrameTreeNodeIdMap& frame_map = GetFrameTreeNodeIdMap();
   auto iter = frame_map.find(frame_tree_node_id);
   auto* web_frame = iter == frame_map.end() ? nullptr : iter->second;
@@ -141,15 +143,29 @@ WebFrameMain* WebFrameMain::FromRenderFrameHost(content::RenderFrameHost* rfh) {
   return FromFrameToken(rfh->GetGlobalFrameToken());
 }
 
+content::RenderFrameHost* WebFrameMain::render_frame_host() const {
+  return render_frame_disposed_
+             ? nullptr
+             : content::RenderFrameHost::FromFrameToken(frame_token_);
+}
+
 gin::WrapperInfo WebFrameMain::kWrapperInfo = {gin::kEmbedderNativeGin};
 
 WebFrameMain::WebFrameMain(content::RenderFrameHost* rfh)
     : frame_tree_node_id_(rfh->GetFrameTreeNodeId()),
       frame_token_(rfh->GetGlobalFrameToken()),
-      render_frame_(rfh),
       render_frame_detached_(IsDetachedFrameHost(rfh)) {
-  GetFrameTreeNodeIdMap().emplace(frame_tree_node_id_, this);
+  // Detached RFH should not insert itself in FTN lookup since it has been
+  // swapped already.
+  if (!render_frame_detached_)
+    GetFrameTreeNodeIdMap().emplace(frame_tree_node_id_, this);
+
+  DCHECK(GetFrameTokenMap().find(frame_token_) == GetFrameTokenMap().end());
   GetFrameTokenMap().emplace(frame_token_, this);
+
+  // WebFrameMain should only be created for active or unloading frames.
+  DCHECK(GetLifecycleState(rfh) == LifecycleState::kActive ||
+         GetLifecycleState(rfh) == LifecycleState::kRunningUnloadHandlers);
 }
 
 WebFrameMain::~WebFrameMain() {
@@ -157,14 +173,19 @@ WebFrameMain::~WebFrameMain() {
 }
 
 void WebFrameMain::Destroyed() {
-  MarkRenderFrameDisposed();
-  GetFrameTreeNodeIdMap().erase(frame_tree_node_id_);
+  if (FromFrameTreeNodeId(frame_tree_node_id_) == this) {
+    // WebFrameMain initialized as detached doesn't support FTN lookup and
+    // shouldn't erase the entry.
+    DCHECK(!render_frame_detached_ || render_frame_disposed_);
+    GetFrameTreeNodeIdMap().erase(frame_tree_node_id_);
+  }
+
   GetFrameTokenMap().erase(frame_token_);
+  MarkRenderFrameDisposed();
   Unpin();
 }
 
 void WebFrameMain::MarkRenderFrameDisposed() {
-  render_frame_ = nullptr;
   render_frame_detached_ = true;
   render_frame_disposed_ = true;
   TeardownMojoConnection();
@@ -173,11 +194,14 @@ void WebFrameMain::MarkRenderFrameDisposed() {
 // Should only be called when swapping frames.
 void WebFrameMain::UpdateRenderFrameHost(content::RenderFrameHost* rfh) {
   GetFrameTokenMap().erase(frame_token_);
+
+  // Ensure that RFH being swapped in doesn't already exist as its own
+  // WebFrameMain instance.
   frame_token_ = rfh->GetGlobalFrameToken();
+  DCHECK(GetFrameTokenMap().find(frame_token_) == GetFrameTokenMap().end());
   GetFrameTokenMap().emplace(frame_token_, this);
 
   render_frame_disposed_ = false;
-  render_frame_ = rfh;
   TeardownMojoConnection();
   MaybeSetupMojoConnection();
 }
@@ -218,7 +242,7 @@ v8::Local<v8::Promise> WebFrameMain::ExecuteJavaScript(
     return handle;
   }
 
-  static_cast<content::RenderFrameHostImpl*>(render_frame_)
+  static_cast<content::RenderFrameHostImpl*>(render_frame_host())
       ->ExecuteJavaScriptForTests(
           code, user_gesture, true /* resolve_promises */,
           /*honor_js_content_settings=*/true, content::ISOLATED_WORLD_ID_GLOBAL,
@@ -243,7 +267,7 @@ v8::Local<v8::Promise> WebFrameMain::ExecuteJavaScript(
 bool WebFrameMain::Reload() {
   if (!CheckRenderFrame())
     return false;
-  return render_frame_->Reload();
+  return render_frame_host()->Reload();
 }
 
 bool WebFrameMain::IsDestroyed() const {
@@ -287,13 +311,12 @@ void WebFrameMain::MaybeSetupMojoConnection() {
         &WebFrameMain::OnRendererConnectionError, weak_factory_.GetWeakPtr()));
   }
 
-  DCHECK(render_frame_);
+  content::RenderFrameHost* rfh = render_frame_host();
+  DCHECK(rfh);
 
   // Wait for RenderFrame to be created in renderer before accessing remote.
-  if (pending_receiver_ && render_frame_ &&
-      render_frame_->IsRenderFrameLive()) {
-    render_frame_->GetRemoteInterfaces()->GetInterface(
-        std::move(pending_receiver_));
+  if (pending_receiver_ && rfh && rfh->IsRenderFrameLive()) {
+    rfh->GetRemoteInterfaces()->GetInterface(std::move(pending_receiver_));
   }
 }
 
@@ -306,6 +329,17 @@ void WebFrameMain::OnRendererConnectionError() {
   TeardownMojoConnection();
 }
 
+[[nodiscard]] bool WebFrameMain::HasRenderFrame() const {
+  if (render_frame_disposed_)
+    return false;
+
+  // If RFH is a nullptr, this instance of WebFrameMain is dangling and wasn't
+  // properly deleted.
+  CHECK(render_frame_host());
+
+  return true;
+}
+
 void WebFrameMain::PostMessage(v8::Isolate* isolate,
                                const std::string& channel,
                                v8::Local<v8::Value> message_value,
@@ -350,57 +384,57 @@ content::FrameTreeNodeId WebFrameMain::FrameTreeNodeID() const {
 std::string WebFrameMain::Name() const {
   if (!CheckRenderFrame())
     return {};
-  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);
 }
 
 int32_t WebFrameMain::ProcessID() const {
   if (!CheckRenderFrame())
     return -1;
-  return render_frame_->GetProcess()->GetID().GetUnsafeValue();
+  return render_frame_host()->GetProcess()->GetID().GetUnsafeValue();
 }
 
 int WebFrameMain::RoutingID() const {
   if (!CheckRenderFrame())
     return -1;
-  return render_frame_->GetRoutingID();
+  return render_frame_host()->GetRoutingID();
 }
 
 GURL WebFrameMain::URL() const {
   if (!CheckRenderFrame())
     return {};
-  return render_frame_->GetLastCommittedURL();
+  return render_frame_host()->GetLastCommittedURL();
 }
 
 std::string WebFrameMain::Origin() const {
   if (!CheckRenderFrame())
     return {};
-  return render_frame_->GetLastCommittedOrigin().Serialize();
+  return render_frame_host()->GetLastCommittedOrigin().Serialize();
 }
 
 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 {
@@ -408,9 +442,9 @@ std::vector<content::RenderFrameHost*> WebFrameMain::Frames() const {
   if (!CheckRenderFrame())
     return frame_hosts;
 
-  render_frame_->ForEachRenderFrameHost(
+  render_frame_host()->ForEachRenderFrameHost(
       [&frame_hosts, this](content::RenderFrameHost* rfh) {
-        if (rfh->GetParent() == render_frame_)
+        if (rfh && rfh->GetParent() == render_frame_host())
           frame_hosts.push_back(rfh);
       });
 
@@ -422,7 +456,7 @@ std::vector<content::RenderFrameHost*> WebFrameMain::FramesInSubtree() const {
   if (!CheckRenderFrame())
     return frame_hosts;
 
-  render_frame_->ForEachRenderFrameHost(
+  render_frame_host()->ForEachRenderFrameHost(
       [&frame_hosts](content::RenderFrameHost* rfh) {
         frame_hosts.push_back(rfh);
       });
@@ -430,6 +464,13 @@ std::vector<content::RenderFrameHost*> WebFrameMain::FramesInSubtree() const {
   return frame_hosts;
 }
 
+const char* WebFrameMain::LifecycleStateForTesting() const {
+  if (!HasRenderFrame())
+    return {};
+  return content::RenderFrameHostImpl::LifecycleStateImplToString(
+      GetLifecycleState(render_frame_host()));
+}
+
 v8::Local<v8::Promise> WebFrameMain::CollectDocumentJSCallStack(
     gin::Arguments* args) {
   gin_helper::Promise<base::Value> promise(args->isolate());
@@ -449,7 +490,8 @@ v8::Local<v8::Promise> WebFrameMain::CollectDocumentJSCallStack(
   }
 
   content::RenderProcessHostImpl* rph_impl =
-      static_cast<content::RenderProcessHostImpl*>(render_frame_->GetProcess());
+      static_cast<content::RenderProcessHostImpl*>(
+          render_frame_host()->GetProcess());
 
   rph_impl->GetJavaScriptCallStackGeneratorInterface()
       ->CollectJavaScriptCallStack(
@@ -469,7 +511,8 @@ void WebFrameMain::CollectedJavaScriptCallStack(
     return;
   }
 
-  const blink::LocalFrameToken& frame_token = render_frame_->GetFrameToken();
+  const blink::LocalFrameToken& frame_token =
+      render_frame_host()->GetFrameToken();
   if (remote_frame_token == frame_token) {
     base::Value base_value(untrusted_javascript_call_stack);
     promise.Resolve(base_value);
@@ -500,7 +543,31 @@ gin::Handle<WebFrameMain> WebFrameMain::From(v8::Isolate* isolate,
   if (!rfh)
     return {};
 
-  auto* web_frame = FromRenderFrameHost(rfh);
+  WebFrameMain* web_frame;
+  switch (GetLifecycleState(rfh)) {
+    case LifecycleState::kSpeculative:
+    case LifecycleState::kPendingCommit:
+      // RFH is in the process of being swapped. Need to lookup by FTN to avoid
+      // creating dangling WebFrameMain.
+      web_frame = FromFrameTreeNodeId(rfh->GetFrameTreeNodeId());
+      break;
+    case LifecycleState::kPrerendering:
+    case LifecycleState::kActive:
+    case LifecycleState::kInBackForwardCache:
+      // RFH is already assigned to the FrameTreeNode and can safely be looked
+      // up directly.
+      web_frame = FromRenderFrameHost(rfh);
+      break;
+    case LifecycleState::kRunningUnloadHandlers:
+      // Event/IPC emitted for a frame running unload handlers. Return the exact
+      // RFH so the security origin will be accurate.
+      web_frame = FromRenderFrameHost(rfh);
+      break;
+    case LifecycleState::kReadyToBeDeleted:
+      // RFH is gone
+      return {};
+  }
+
   if (web_frame)
     return gin::CreateHandle(isolate, web_frame);
 
@@ -536,6 +603,8 @@ void WebFrameMain::FillObjectTemplate(v8::Isolate* isolate,
       .SetProperty("parent", &WebFrameMain::Parent)
       .SetProperty("frames", &WebFrameMain::Frames)
       .SetProperty("framesInSubtree", &WebFrameMain::FramesInSubtree)
+      .SetProperty("_lifecycleStateForTesting",
+                   &WebFrameMain::LifecycleStateForTesting)
       .Build();
 }
 

shell/browser/api/electron_api_web_frame_main.h

@@ -9,7 +9,6 @@
 #include <string>
 #include <vector>
 
-#include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/process/process.h"
 #include "content/public/browser/frame_tree_node_id.h"
@@ -72,7 +71,7 @@ class WebFrameMain final : public gin::Wrappable<WebFrameMain>,
   static gin::WrapperInfo kWrapperInfo;
   const char* GetTypeName() override;
 
-  content::RenderFrameHost* render_frame_host() const { return render_frame_; }
+  content::RenderFrameHost* render_frame_host() const;
 
   // disable copy
   WebFrameMain(const WebFrameMain&) = delete;
@@ -101,9 +100,7 @@ class WebFrameMain final : public gin::Wrappable<WebFrameMain>,
   void TeardownMojoConnection();
   void OnRendererConnectionError();
 
-  [[nodiscard]] constexpr bool HasRenderFrame() const {
-    return !render_frame_disposed_ && render_frame_ != nullptr;
-  }
+  [[nodiscard]] bool HasRenderFrame() const;
 
   // Throws a JS error if HasRenderFrame() is false.
   // WebFrameMain can outlive its RenderFrameHost pointer,
@@ -139,6 +136,8 @@ class WebFrameMain final : public gin::Wrappable<WebFrameMain>,
   std::vector<content::RenderFrameHost*> Frames() const;
   std::vector<content::RenderFrameHost*> FramesInSubtree() const;
 
+  const char* LifecycleStateForTesting() const;
+
   v8::Local<v8::Promise> CollectDocumentJSCallStack(gin::Arguments* args);
   void CollectedJavaScriptCallStack(
       gin_helper::Promise<base::Value> promise,
@@ -153,8 +152,6 @@ class WebFrameMain final : public gin::Wrappable<WebFrameMain>,
   content::FrameTreeNodeId frame_tree_node_id_;
   content::GlobalRenderFrameHostToken frame_token_;
 
-  raw_ptr<content::RenderFrameHost> render_frame_ = nullptr;
-
   // Whether the RenderFrameHost has been removed and that it should no longer
   // be accessed.
   bool render_frame_disposed_ = false;

shell/common/gin_converters/std_converter.h

@@ -61,6 +61,15 @@ struct Converter<char[N]> {
   }
 };
 
+template <>
+struct Converter<const char*> {
+  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate, const char* val) {
+    return v8::String::NewFromUtf8(isolate, val ? val : "",
+                                   v8::NewStringType::kNormal)
+        .ToLocalChecked();
+  }
+};
+
 template <>
 struct Converter<v8::Local<v8::Array>> {
   static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,

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

@@ -428,6 +428,47 @@ describe('webFrameMain module', () => {
       crossOriginPromise = w.webContents.loadURL(server.crossOriginUrl);
       await expect(unloadPromise).to.eventually.be.fulfilled();
     });
+
+    // Skip test as we don't have an offline repro yet
+    it.skip('should not crash due to dangling frames', async () => {
+      w = new BrowserWindow({
+        show: false,
+        webPreferences: {
+          preload: path.join(subframesPath, 'preload.js')
+        }
+      });
+
+      // Persist frame references so WebFrameMain is initialized for each
+      const frames: Electron.WebFrameMain[] = [];
+      w.webContents.on('frame-created', (_event, details) => {
+        console.log('frame-created');
+        frames.push(details.frame!);
+      });
+      w.webContents.on('will-frame-navigate', (event) => {
+        console.log('will-frame-navigate', event);
+        frames.push(event.frame!);
+      });
+
+      // Load document with several speculative subframes
+      await w.webContents.loadURL('https://www.ezcater.com/delivery/pizza-catering');
+
+      // Test that no frame will crash due to a dangling render frame host
+      const crashTest = () => {
+        for (const frame of frames) {
+          expect(frame._lifecycleStateForTesting).to.not.equal('Speculative');
+          try {
+            expect(frame.url).to.be.a('string');
+          } catch {
+            // Exceptions from non-dangling frames are expected
+          }
+        }
+      };
+
+      crashTest();
+      w.webContents.destroy();
+      await setTimeout(1);
+      crashTest();
+    });
   });
 
   describe('webFrameMain.fromId', () => {

typings/internal-electron.d.ts

@@ -132,6 +132,7 @@ declare namespace Electron {
     _send(internal: boolean, channel: string, args: any): void;
     _sendInternal(channel: string, ...args: any[]): void;
     _postMessage(channel: string, message: any, transfer?: any[]): void;
+    _lifecycleStateForTesting: string;
   }
 
   interface WebFrame {