obs-browser: fix panel use-after-destruction due to data race

panel/browser-panel-client.cpp

@@ -71,6 +71,8 @@ CefRefPtr<CefJSDialogHandler> QCefBrowserClient::GetJSDialogHandler()
 /* CefDisplayHandler */
 void QCefBrowserClient::OnTitleChange(CefRefPtr<CefBrowser> browser, const CefString &title)
 {
+	std::lock_guard<std::recursive_mutex> 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<CefBrowser> browser, CefRefPtr<
 		}
 	}
 
+	std::lock_guard<std::recursive_mutex> 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<CefBrowser>, CefRefPtr<CefFrame>
 				      CefWindowInfo &windowInfo, CefRefPtr<CefClient> &, CefBrowserSettings &,
 				      CefRefPtr<CefDictionaryValue> &, bool *)
 {
+	std::lock_guard<std::recursive_mutex> 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<CefBrowser>, CefRefPtr<CefFrame>
 
 		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<CefBrowser>, CefRefPtr<CefFrame>
 
 void QCefBrowserClient::OnBeforeClose(CefRefPtr<CefBrowser>)
 {
+	std::lock_guard<std::recursive_mutex> widgetLock(widgetMutex);
+
 	if (widget) {
 		widget->finishCloseBrowser();
 	}
@@ -332,11 +340,16 @@ bool QCefBrowserClient::OnContextMenuCommand(CefRefPtr<CefBrowser> browser, CefR
 	CefRefPtr<CefBrowserHost> host = browser->GetHost();
 	CefWindowInfo windowInfo;
 	QPoint pos;
+
+	std::lock_guard<std::recursive_mutex> 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<CefBrowser> 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<CefBrowser>, CefRefPtr<CefFrame> fra
 	if (!frame->IsMain())
 		return;
 
+	std::lock_guard<std::recursive_mutex> 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<CefBrowser>, const CefString &,
 				   const CefString &default_prompt_text, CefRefPtr<CefJSDialogCallback> callback,
 				   bool &)
 {
+	std::lock_guard<std::recursive_mutex> 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<CefBrowser> 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;
 }

panel/browser-panel-client.hpp

@@ -3,6 +3,7 @@
 #include "cef-headers.hpp"
 #include "browser-panel-internal.hpp"
 
+#include <mutex>
 #include <string>
 
 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<std::recursive_mutex> 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<CefBrowser> browser)
+	{
+		{
+			std::lock_guard<std::recursive_mutex> lock(widgetMutex);
+			if (widget) {
+				widget->cefBrowser = browser;
+				return;
+			}
+		}
+
+		if (browser)
+			browser->GetHost()->CloseBrowser(true);
+	}
+
+#ifdef __linux__
+	void unsetToplevelXdndProxy()
+	{
+		std::lock_guard<std::recursive_mutex> lock(widgetMutex);
+		if (widget)
+			widget->unsetToplevelXdndProxy();
+	}
+#endif
+
 	/* CefClient */
 	virtual CefRefPtr<CefLoadHandler> GetLoadHandler() override;
 	virtual CefRefPtr<CefDisplayHandler> GetDisplayHandler() override;
@@ -96,9 +132,15 @@ class QCefBrowserClient : public CefClient,
 				const CefString &default_prompt_text, CefRefPtr<CefJSDialogCallback> 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);
 };

panel/browser-panel-internal.hpp

@@ -8,6 +8,8 @@
 #include <vector>
 #include <mutex>
 
+class QCefBrowserClient;
+
 struct PopupWhitelistInfo {
 	std::string url;
 	QPointer<QObject> obj;
@@ -29,6 +31,13 @@ class QCefWidgetInternal : public QCefWidget {
 	~QCefWidgetInternal();
 
 	CefRefPtr<CefBrowser> 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<QCefBrowserClient> browserClient;
 	std::string url;
 	std::string script;
 	CefRefPtr<CefRequestContext> rqc;

panel/browser-panel.cpp

@@ -4,6 +4,7 @@
 #include "browser-app.hpp"
 
 #include <QWindow>
+#include <QScopeGuard>
 #include <QApplication>
 
 #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<CefClient> client{host->GetClient()};
-
-	if (client) {
-		QCefBrowserClient *browserClient{static_cast<QCefBrowserClient *>(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<QCefBrowserClient> client = browserClient;
+	const std::string initUrl = url;
+	CefRefPtr<CefRequestContext> 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<QCefBrowserClient> 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<CefDictionaryValue>(), rqc);
+		CefBrowserSettings cefBrowserSettings;
+		client->attachBrowser(CefBrowserHost::CreateBrowserSync(windowInfo, client, initUrl, cefBrowserSettings,
+									CefRefPtr<CefDictionaryValue>(), 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)