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)