fix: prevent uaf when destroying guest WebContents during event emission (#51081)

fix: prevent use-after-free when destroying guest WebContents during event emission

Multiple event emission sites in WebContents destroy the underlying C++
object via a JavaScript event handler calling webContents.destroy(), then
continue to dereference the freed `this` pointer. This is exploitable
through <webview> guest WebContents because Destroy() calls `delete this`
synchronously for guests, unlike non-guests which safely defer deletion.

The fix has two layers:

1. A new `is_emitting_event_` flag is checked in Destroy() — when true,
   guest deletion is deferred to a posted task instead of executing
   synchronously. This is separate from `is_safe_to_delete_` (which
   gates LoadURL re-entrancy) to avoid rejecting legitimate loadURL
   calls from event handlers.

2. AutoReset<bool> guards on `is_emitting_event_` are added to
   CloseContents, RenderViewDeleted, DidFinishNavigation, and
   SetContentsBounds, preventing synchronous destruction while their
   Emit() calls are on the stack.

Destroy() now requires both `is_safe_to_delete_` (navigation re-entrancy)
and `!is_emitting_event_` (event emission) to allow synchronous guest
deletion. The existing AutoReset guards on `is_safe_to_delete_` in
DidStartNavigation, DidRedirectNavigation, and ReadyToCommitNavigation
are also now effective for guests.

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

shell/browser/api/electron_api_web_contents.cc

@@ -1170,7 +1170,9 @@ void WebContents::DeleteThisIfAlive() {
 void WebContents::Destroy() {
   // The content::WebContents should be destroyed asynchronously when possible
   // as user may choose to destroy WebContents during an event of it.
-  if (Browser::Get()->is_shutting_down() || is_guest()) {
+  if (Browser::Get()->is_shutting_down()) {
+    DeleteThisIfAlive();
+  } else if (is_guest() && is_safe_to_delete_ && !is_emitting_event_) {
     DeleteThisIfAlive();
   } else {
     content::GetUIThreadTaskRunner({})->PostTask(
@@ -1427,17 +1429,21 @@ void WebContents::BeforeUnloadFired(content::WebContents* tab,
 
 void WebContents::SetContentsBounds(content::WebContents* source,
                                     const gfx::Rect& rect) {
+  base::AutoReset<bool> resetter(&is_emitting_event_, true);
   if (!Emit("content-bounds-updated", rect))
     observers_.Notify(&ExtendedWebContentsObserver::OnSetContentBounds, rect);
 }
 
 void WebContents::CloseContents(content::WebContents* source) {
-  Emit("close");
+  {
+    base::AutoReset<bool> resetter(&is_emitting_event_, true);
+    Emit("close");
 
-  auto* autofill_driver_factory =
-      AutofillDriverFactory::FromWebContents(web_contents());
-  if (autofill_driver_factory) {
-    autofill_driver_factory->CloseAllPopups();
+    auto* autofill_driver_factory =
+        AutofillDriverFactory::FromWebContents(web_contents());
+    if (autofill_driver_factory) {
+      autofill_driver_factory->CloseAllPopups();
+    }
   }
 
   Destroy();
@@ -1929,6 +1935,7 @@ void WebContents::FrameDeleted(content::FrameTreeNodeId frame_tree_node_id) {
 }
 
 void WebContents::RenderViewDeleted(content::RenderViewHost* render_view_host) {
+  base::AutoReset<bool> resetter(&is_emitting_event_, true);
   const auto id = render_view_host->GetProcess()->GetID().GetUnsafeValue();
   // This event is necessary for tracking any states with respect to
   // intermediate render view hosts aka speculative render view hosts. Currently
@@ -2168,6 +2175,9 @@ void WebContents::DidFinishNavigation(
 
   if (!navigation_handle->HasCommitted())
     return;
+
+  base::AutoReset<bool> resetter(&is_emitting_event_, true);
+
   bool is_main_frame = navigation_handle->IsInMainFrame();
   content::RenderFrameHost* frame_host =
       navigation_handle->GetRenderFrameHost();
@@ -2177,9 +2187,6 @@ void WebContents::DidFinishNavigation(
     frame_routing_id = frame_host->GetRoutingID();
   }
   if (!navigation_handle->IsErrorPage()) {
-    // FIXME: All the Emit() calls below could potentially result in |this|
-    // being destroyed (by JS listening for the event and calling
-    // webContents.destroy()).
     auto url = navigation_handle->GetURL();
     bool is_same_document = navigation_handle->IsSameDocument();
     if (is_same_document) {

shell/browser/api/electron_api_web_contents.h

@@ -871,9 +871,15 @@ class WebContents final : public ExclusiveAccessContext,
   const scoped_refptr<base::TaskRunner> print_task_runner_;
 #endif
 
-  // Track navigation state in order to avoid potential re-entrancy crashes.
+  // Track navigation state in order to avoid potential re-entrancy crashes
+  // in LoadURL. Checked by LoadURL to reject re-entrant navigation attempts.
   bool is_safe_to_delete_ = true;
 
+  // Set to true while dispatching JS events via Emit(). When true, Destroy()
+  // defers guest WebContents deletion to prevent use-after-free when a JS
+  // handler calls webContents.destroy() mid-emission.
+  bool is_emitting_event_ = false;
+
   // Stores the frame that's currently in fullscreen, nullptr if there is none.
   raw_ptr<content::RenderFrameHost> fullscreen_frame_ = nullptr;
 

spec/fixtures/crash-cases/webview-destroy-in-event/index.js

@@ -0,0 +1,41 @@
+const { app, BrowserWindow, webContents } = require('electron');
+
+app.whenReady().then(() => {
+  const w = new BrowserWindow({
+    show: false,
+    webPreferences: {
+      webviewTag: true,
+      nodeIntegration: true,
+      contextIsolation: false
+    }
+  });
+
+  w.loadURL('data:text/html,' + encodeURIComponent(
+    '<webview id="wv" src="about:blank" style="width:100px;height:100px"></webview>' +
+    '<script>document.getElementById("wv").addEventListener("dom-ready", function() {' +
+    '  document.title = "READY:" + this.getWebContentsId();' +
+    '});</script>'
+  ));
+
+  const check = setInterval(() => {
+    if (!w.getTitle().startsWith('READY:')) return;
+    clearInterval(check);
+    const guestId = parseInt(w.getTitle().split(':')[1]);
+    const guest = webContents.fromId(guestId);
+    if (!guest) {
+      app.quit();
+      return;
+    }
+
+    // Destroying a guest webContents inside an event handler previously caused
+    // a use-after-free because Destroy() deleted the C++ object synchronously
+    // while the event emission was still on the call stack.
+    guest.on('did-navigate-in-page', () => {
+      guest.destroy();
+    });
+
+    guest.executeJavaScript('history.pushState({}, "", "#trigger")');
+
+    setTimeout(() => app.quit(), 2000);
+  }, 200);
+});