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); +});