From 2829061e9e9eaa0dba678b6aee48801e099ce8d9 Mon Sep 17 00:00:00 2001 From: Ilya Melamed Date: Sat, 5 Sep 2026 20:51:42 +0300 Subject: [PATCH] panel: fix use-after-destruction when a widget outlives its browser creation QCefWidgetInternal::Init() posts a CEF task that captures `this` raw. The task assigns cefBrowser and constructs a QCefBrowserClient holding a back-pointer to the widget. Nothing cancels or guards it, so a widget destroyed before that task runs is read and written after it is freed. closeBrowser() is the only place that clears the client's back-pointer, and it reaches the client through host->GetClient() -- which needs a live browser. So it sits after `if (!cefBrowser) return;`, and the one case that needs clearing, "the browser does not exist yet", is exactly the case that returns early. Closing OBS a few seconds after launch reproduces it: CreateBrowserSync faults on the freed widget. Symbolized from a live capture: BrowserManagerThread -> task_execute -> QCefWidgetInternal::Init'::::_Do_call -> CefBrowserHost::CreateBrowserSync -> cef_browser_host_create_browser_sync -> KiUserExceptionDispatch The visible symptom is not always a crash. When the host's unhandled-exception filter puts a message box up it does so on this thread, nothing dismisses it, and obs_module_unload's Thrd_join then never returns -- the process hangs at shutdown with no window the user can find. The widget now holds a reference to the client from the moment it REQUESTS a browser rather than from the moment one exists, which is what makes the back-pointer clearable in that window: * browserClient is created on the Qt thread in Init() and stored on the widget. * closeBrowser() detaches through a qScopeGuard, so it happens on every path out including both early returns. * The queued task captures the refcounted client plus copies of url, rqc and size instead of `this`, and publishes its result with attachBrowser(), which closes the browser rather than leaking it if the widget went away meanwhile. * The double-init guard moves to the Qt thread. Guarding on the queue side also drops the assumption that two queued tasks run in an order that lets the second see the first one's result. `widget` becomes private behind a recursive_mutex so the compiler enforces that the eight callbacks using it go through the lock. Five of those call sites dereferenced it with no null check at all -- OnBeforePopup (twice), OnContextMenuCommand, OnJSDialog and OnPreKeyEvent -- which is already reachable today, since closeBrowser() nulls the pointer while the client stays alive. QSize is now read on the Qt thread on macOS too. It was calling QWidget::size() from a CEF thread. The nested event loop in closeBrowser() is deliberately left alone. Removing it was tried and it turned an intermittent shutdown hang into one that reproduced in four runs out of four; external_message_pump means CEF only advances while Qt keeps pumping, so that wait is load-bearing. Measured with a harness that launches OBS and posts WM_CLOSE as soon as a main window exists, 16 runs per configuration, every hang classified from its stack: this crash accounted for 3 hangs before the patch and 0 after. Co-Authored-By: Claude Opus 5 --- panel/browser-panel-client.cpp | 50 ++++++++++++--- panel/browser-panel-client.hpp | 44 ++++++++++++- panel/browser-panel-internal.hpp | 9 +++ panel/browser-panel.cpp | 105 +++++++++++++++++++------------ 4 files changed, 157 insertions(+), 51 deletions(-) diff --git a/panel/browser-panel-client.cpp b/panel/browser-panel-client.cpp index 91b4ee0b7..0109e6389 100644 --- a/panel/browser-panel-client.cpp +++ b/panel/browser-panel-client.cpp @@ -71,6 +71,8 @@ CefRefPtr QCefBrowserClient::GetJSDialogHandler() /* CefDisplayHandler */ void QCefBrowserClient::OnTitleChange(CefRefPtr browser, const CefString &title) { + std::lock_guard widgetLock(widgetMutex); + if (widget && widget->cefBrowser->IsSame(browser)) { std::string str_title = title; QString qt_title = QString::fromUtf8(str_title.c_str()); @@ -118,6 +120,8 @@ bool QCefBrowserClient::OnBeforeBrowse(CefRefPtr browser, CefRefPtr< } } + std::lock_guard widgetLock(widgetMutex); + if (widget) { QString qt_url = QString::fromUtf8(str_url.c_str()); QMetaObject::invokeMethod(widget, "urlChanged", Q_ARG(QString, qt_url)); @@ -181,10 +185,12 @@ bool QCefBrowserClient::OnBeforePopup(CefRefPtr, CefRefPtr CefWindowInfo &windowInfo, CefRefPtr &, CefBrowserSettings &, CefRefPtr &, bool *) { + std::lock_guard widgetLock(widgetMutex); + if (allowAllPopups) { #ifdef _WIN32 - HWND hwnd = (HWND)widget->effectiveWinId(); - windowInfo.parent_window = hwnd; + if (widget) + windowInfo.parent_window = (HWND)widget->effectiveWinId(); #else UNUSED_PARAMETER(windowInfo); #endif @@ -204,8 +210,8 @@ bool QCefBrowserClient::OnBeforePopup(CefRefPtr, CefRefPtr if (astrcmpi(info.url.c_str(), str_url.c_str()) == 0) { #ifdef _WIN32 - HWND hwnd = (HWND)widget->effectiveWinId(); - windowInfo.parent_window = hwnd; + if (widget) + windowInfo.parent_window = (HWND)widget->effectiveWinId(); #endif return false; } @@ -219,6 +225,8 @@ bool QCefBrowserClient::OnBeforePopup(CefRefPtr, CefRefPtr void QCefBrowserClient::OnBeforeClose(CefRefPtr) { + std::lock_guard widgetLock(widgetMutex); + if (widget) { widget->finishCloseBrowser(); } @@ -332,11 +340,16 @@ bool QCefBrowserClient::OnContextMenuCommand(CefRefPtr browser, CefR CefRefPtr host = browser->GetHost(); CefWindowInfo windowInfo; QPoint pos; + + std::lock_guard widgetLock(widgetMutex); + switch (command_id) { case MENU_ITEM_DEVTOOLS: #if defined(_WIN32) && CHROME_VERSION_BUILD < 6533 windowInfo.SetAsPopup(host->GetWindowHandle(), ""); #endif + if (!widget) + return true; pos = widget->mapToGlobal(QPoint(0, 0)); windowInfo.bounds.x = pos.x(); windowInfo.bounds.y = pos.y() + 30; @@ -349,13 +362,16 @@ bool QCefBrowserClient::OnContextMenuCommand(CefRefPtr browser, CefR host->SetAudioMuted(!host->IsAudioMuted()); return true; case MENU_ITEM_ZOOM_IN: - widget->zoomPage(1); + if (widget) + widget->zoomPage(1); return true; case MENU_ITEM_ZOOM_RESET: - widget->zoomPage(0); + if (widget) + widget->zoomPage(0); return true; case MENU_ITEM_ZOOM_OUT: - widget->zoomPage(-1); + if (widget) + widget->zoomPage(-1); return true; case MENU_ITEM_COPY_URL: std::string url = browser->GetMainFrame()->GetURL().ToString(); @@ -392,6 +408,8 @@ void QCefBrowserClient::OnLoadEnd(CefRefPtr, CefRefPtr fra if (!frame->IsMain()) return; + std::lock_guard widgetLock(widgetMutex); + if (widget && !widget->script.empty()) frame->ExecuteJavaScript(widget->script, CefString(), 0); else if (!script.empty()) @@ -403,6 +421,15 @@ bool QCefBrowserClient::OnJSDialog(CefRefPtr, const CefString &, const CefString &default_prompt_text, CefRefPtr callback, bool &) { + std::lock_guard widgetLock(widgetMutex); + + if (!widget) { + /* The browser is on its way out, so there is nothing to parent a + * dialog to. Cancel it rather than let CEF put up its own. */ + callback->Continue(false, CefString()); + return true; + } + QString parentTitle = widget->parentWidget()->windowTitle(); std::string default_value = default_prompt_text; QString msg_raw(message_text.ToString().c_str()); @@ -488,15 +515,18 @@ bool QCefBrowserClient::OnPreKeyEvent(CefRefPtr browser, const CefKe } else if ((event.windows_key_code == 189 || event.windows_key_code == 109) && (event.modifiers & EVENTFLAG_CONTROL_DOWN) != 0) { // Zoom out - return widget->zoomPage(-1); + if (widget) + return widget->zoomPage(-1); } else if ((event.windows_key_code == 187 || event.windows_key_code == 107) && (event.modifiers & EVENTFLAG_CONTROL_DOWN) != 0) { // Zoom in - return widget->zoomPage(1); + if (widget) + return widget->zoomPage(1); } else if ((event.windows_key_code == 48 || event.windows_key_code == 96) && (event.modifiers & EVENTFLAG_CONTROL_DOWN) != 0) { // Reset zoom - return widget->zoomPage(0); + if (widget) + return widget->zoomPage(0); } return false; } diff --git a/panel/browser-panel-client.hpp b/panel/browser-panel-client.hpp index 417021fa9..5517e6dc7 100644 --- a/panel/browser-panel-client.hpp +++ b/panel/browser-panel-client.hpp @@ -3,6 +3,7 @@ #include "cef-headers.hpp" #include "browser-panel-internal.hpp" +#include #include class QCefBrowserClient : public CefClient, @@ -23,6 +24,41 @@ class QCefBrowserClient : public CefClient, { } + /* Called by the widget while it is still alive. Blocks until any callback + * currently holding the back-pointer has finished, so once it returns no + * CEF thread can be inside one. */ + void detachWidget() + { + std::lock_guard lock(widgetMutex); + widget = nullptr; + } + + /* Publish the browser created by the queued task in Init(). If the widget + * went away while the browser was being created then nothing owns it, so + * close it here rather than leak it. */ + void attachBrowser(CefRefPtr browser) + { + { + std::lock_guard lock(widgetMutex); + if (widget) { + widget->cefBrowser = browser; + return; + } + } + + if (browser) + browser->GetHost()->CloseBrowser(true); + } + +#ifdef __linux__ + void unsetToplevelXdndProxy() + { + std::lock_guard lock(widgetMutex); + if (widget) + widget->unsetToplevelXdndProxy(); + } +#endif + /* CefClient */ virtual CefRefPtr GetLoadHandler() override; virtual CefRefPtr GetDisplayHandler() override; @@ -96,9 +132,15 @@ class QCefBrowserClient : public CefClient, const CefString &default_prompt_text, CefRefPtr callback, bool &suppress_message) override; - QCefWidgetInternal *widget = nullptr; std::string script; bool allowAllPopups; +private: + /* Private on purpose. Every use has to go through widgetMutex, and making + * the compiler enforce that is what guarantees no unguarded one is left + * behind. */ + std::recursive_mutex widgetMutex; + QCefWidgetInternal *widget = nullptr; + IMPLEMENT_REFCOUNTING(QCefBrowserClient); }; diff --git a/panel/browser-panel-internal.hpp b/panel/browser-panel-internal.hpp index e689bb2f2..bbc9a5eba 100644 --- a/panel/browser-panel-internal.hpp +++ b/panel/browser-panel-internal.hpp @@ -8,6 +8,8 @@ #include #include +class QCefBrowserClient; + struct PopupWhitelistInfo { std::string url; QPointer obj; @@ -29,6 +31,13 @@ class QCefWidgetInternal : public QCefWidget { ~QCefWidgetInternal(); CefRefPtr cefBrowser; + + /* Held from the moment a browser is requested rather than from the moment + * one exists, so the widget can always detach itself from the client. + * Reaching the client through cefBrowser->GetHost()->GetClient() only works + * once the browser has been created, and that is precisely the window in + * which the widget can be destroyed with the back-pointer still set. */ + CefRefPtr browserClient; std::string url; std::string script; CefRefPtr rqc; diff --git a/panel/browser-panel.cpp b/panel/browser-panel.cpp index b3fbe289d..f74fb83fa 100644 --- a/panel/browser-panel.cpp +++ b/panel/browser-panel.cpp @@ -4,6 +4,7 @@ #include "browser-app.hpp" #include +#include #include #ifdef ENABLE_BROWSER_QT_LOOP @@ -188,6 +189,23 @@ QCefWidgetInternal::~QCefWidgetInternal() void QCefWidgetInternal::closeBrowser() { + /* The client is refcounted and outlives this widget -- CEF holds references + * to it from threads we do not control -- and every one of its callbacks + * dereferences the widget back-pointer. Clearing that pointer is therefore + * the one thing that has to happen on every path out of here, including the + * two early returns below. Before this, a widget destroyed while its browser + * was still being created returned at the !cefBrowser check and left the + * client pointing at freed memory. + * + * It runs at scope exit rather than up front because the close handshake + * below still needs OnBeforeClose to reach finishCloseBrowser(). */ + const auto detach = qScopeGuard([this]() { + if (browserClient) { + browserClient->detachWidget(); + browserClient = nullptr; + } + }); + if (!cefBrowser) { return; } @@ -223,13 +241,6 @@ void QCefWidgetInternal::closeBrowser() browserCloseLoop.exec(); - CefRefPtr client{host->GetClient()}; - - if (client) { - QCefBrowserClient *browserClient{static_cast(client.get())}; - browserClient->widget = nullptr; - } - cefBrowser = nullptr; } @@ -305,58 +316,72 @@ void QCefWidgetInternal::unsetToplevelXdndProxy() void QCefWidgetInternal::Init() { + /* Make sure Init isn't called more than once. Guarding here rather than + * inside the task also removes the reliance on two queued tasks running in + * order to see each other's result. */ + if (browserClient) { + timer.stop(); + return; + } + #ifndef __APPLE__ WId handle = window->winId(); - QSize size = this->size(); - size *= devicePixelRatioF(); - bool success = QueueCEFTask( - [this, handle, size]() + QSize size = this->size() * devicePixelRatioF(); #else WId handle = winId(); - bool success = QueueCEFTask( - [this, handle]() + /* Read on this thread rather than inside the task: QWidget::size() is not + * safe to call from a CEF thread. */ + QSize size = this->size(); #endif - { - CefWindowInfo windowInfo; - /* Make sure Init isn't called more than once. */ - if (cefBrowser) - return; + /* Constructed here, on the Qt thread, so the widget holds a reference to + * the client from the moment it asks for a browser rather than from the + * moment one exists. Nothing about constructing it needs the CEF thread; + * only CreateBrowserSync does. */ + browserClient = new QCefBrowserClient(this, script, allowAllPopups_); -#ifdef __APPLE__ - QSize size = this->size(); -#endif + /* `this` is deliberately not captured. This task outlives the widget + * whenever the widget is destroyed while the browser is still being + * created, and it used to write the result straight into freed memory. */ + CefRefPtr client = browserClient; + const std::string initUrl = url; + CefRefPtr initRqc = rqc; + + bool success = QueueCEFTask([client, handle, size, initUrl, initRqc]() { + CefWindowInfo windowInfo; #if CHROME_VERSION_BUILD >= 6533 - windowInfo.runtime_style = CEF_RUNTIME_STYLE_ALLOY; + windowInfo.runtime_style = CEF_RUNTIME_STYLE_ALLOY; #endif - windowInfo.SetAsChild((CefWindowHandle)handle, CefRect(0, 0, size.width(), size.height())); - - CefRefPtr browserClient = - new QCefBrowserClient(this, script, allowAllPopups_); + windowInfo.SetAsChild((CefWindowHandle)handle, CefRect(0, 0, size.width(), size.height())); - CefBrowserSettings cefBrowserSettings; - cefBrowser = CefBrowserHost::CreateBrowserSync(windowInfo, browserClient, url, - cefBrowserSettings, - CefRefPtr(), rqc); + CefBrowserSettings cefBrowserSettings; + client->attachBrowser(CefBrowserHost::CreateBrowserSync(windowInfo, client, initUrl, cefBrowserSettings, + CefRefPtr(), initRqc)); #ifdef __linux__ - QueueCEFTask([this]() { unsetToplevelXdndProxy(); }); + QueueCEFTask([client]() { client->unsetToplevelXdndProxy(); }); #endif - }); + }); - if (success) { - timer.stop(); + if (!success) { + /* CEF is not up yet; the 500 ms timer will call this again. Drop the + * client so that retry is not turned into a no-op by the guard at the + * top of this function. */ + browserClient = nullptr; + return; + } + + timer.stop(); #ifndef __APPLE__ - if (!container) { - container = QWidget::createWindowContainer(window, this); - container->show(); - } + if (!container) { + container = QWidget::createWindowContainer(window, this); + container->show(); + } - Resize(); + Resize(); #endif - } } void QCefWidgetInternal::resizeEvent(QResizeEvent *event)