Skip to content

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

Open
zavitax wants to merge 1 commit into
obsproject:masterfrom
zavitax:fix/panel-widget-lifetime
Open

obs-browser: fix panel use-after-destruction due to data race#536
zavitax wants to merge 1 commit into
obsproject:masterfrom
zavitax:fix/panel-widget-lifetime

Conversation

@zavitax

@zavitax zavitax commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Description

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.

Motivation and Context

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'::<lambda_1>::_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.

How Has This Been Tested?

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.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the contributing document.
  • My code has been run through clang-format.
  • My code follows the project's style guidelines
  • My code is not on the master branch.
  • My code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.
  • I have used AI tooling in the creation of this PR

@zavitax
zavitax force-pushed the fix/panel-widget-lifetime branch from c1ea3d0 to 7443ea1 Compare September 5, 2026 20:31
…reation

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'::<lambda_1>::_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 <noreply@anthropic.com>
@zavitax
zavitax force-pushed the fix/panel-widget-lifetime branch from 7443ea1 to 2829061 Compare September 5, 2026 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant