From c33d43bd677bb43dd5b47abae4dedf90a9d3a39d Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Thu, 16 Apr 2026 15:27:08 -0400 Subject: [PATCH] fix: prevent uaf when destroying guest WebContents during event emission (#51081) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 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 --- .../browser/api/electron_api_web_contents.cc | 25 +++++++---- shell/browser/api/electron_api_web_contents.h | 8 +++- .../webview-destroy-in-event/index.js | 41 +++++++++++++++++++ 3 files changed, 64 insertions(+), 10 deletions(-) create mode 100644 spec/fixtures/crash-cases/webview-destroy-in-event/index.js diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index 8868090618..8a612f1508 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/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 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 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 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 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) { diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h index 3c38a33269..c3aaaddb2e 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -871,9 +871,15 @@ class WebContents final : public ExclusiveAccessContext, const scoped_refptr 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 fullscreen_frame_ = nullptr; diff --git a/spec/fixtures/crash-cases/webview-destroy-in-event/index.js b/spec/fixtures/crash-cases/webview-destroy-in-event/index.js new file mode 100644 index 0000000000..2375c8a2ed --- /dev/null +++ b/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( + '' + + '' + )); + + 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); +});