superrob1500
/
mirror-ghostty
mirror of https://github.com/ghostty-org/ghostty
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(audit): pass 4 — symmetric undo on whole-tab close, clipboard cb threading note 

MEDIUM — removeSurface's whole-tab branch now pushes to undo:

  closeTab pushed pushTabUndo before removing a tab; removeSurface's
  "this surface is the whole tab" branch (libghostty's onCloseSurface
  callback path, fires when a child shell exits cleanly) skipped it,
  making undo behavior asymmetric — user-driven tab close was
  undoable, shell-exited tab close wasn't. Push the same snapshot
  with the same gates (>1 tab, !quick-terminal) so both paths
  behave identically.

LOW — onReadClipboard threading clarification:

  Pass 4 reviewer flagged a potential cross-thread QClipboard access.
  Verified by inspecting libghostty: read_clipboard_cb is invoked
  behind ghostty_app_tick, which only runs from our GUI-thread frame
  timer. The "may arrive on a worker thread" comment was stale and
  defensive against a hazard that doesn't exist in this build.
  Updated the comment to document the actual GUI-thread invariant
  so future reviewers don't chase the same false alarm.

Co-Authored-By: claude-flow <ruv@ruv.net>
pull/12846/head
ntomsic 2026-05-21 10:24:01 -05:00
parent 4a7f07402b
commit 7cf505c0f8
1 changed files with 15 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
qt/src/MainWindow.cpp
View File

@ -700,6 +700,13 @@ void MainWindow::removeSurface(GhosttySurface *surface) {
// Otherwise this surface is the whole tab.
const int index = m_tabs->indexOf(parent);
// Push to undo so a shell-exited tab close is symmetric with a
// user-initiated tab close (closeTab pushes too). Skip the last
// tab — its closeEvent runs pushWindowUndo and we don't want to
// double-stack. Also skip the quick terminal (which doesn't push
// to either stack by design).
if (index >= 0 && m_tabs->count() > 1 && !m_quickTerminal)
pushTabUndo(index);
if (index >= 0) m_tabs->removeTab(index);
if (parent) parent->deleteLater(); // page; destroys the surface too
// The surface close was already confirmed; don't re-prompt on the
@ -2812,10 +2819,14 @@ bool MainWindow::onAction(ghostty_app_t, ghostty_target_s target,
bool MainWindow::onReadClipboard(void *ud, ghostty_clipboard_e loc,
void *state) {
// surface userdata. Called synchronously when libghostty needs
// clipboard contents (paste). May arrive on a worker thread, so
// surfaceAlive validates the pointer first — the GhosttySurface
// could be mid-destruction.
// surface userdata. Called synchronously by libghostty when a
// surface needs clipboard contents (paste). This runs on the GUI
// thread by construction: every libghostty entry point that
// surfaces a paste lives behind ghostty_app_tick, which the
// process-wide frame timer drives — and that timer is on the GUI
// thread. QClipboard is GUI-thread-only, so reading directly here
// is safe; surfaceAlive still validates the pointer in case a
// surface is mid-destruction on this same thread.
auto *surface = static_cast<GhosttySurface *>(ud);
if (!surfaceAlive(surface) || !surface->surface()) return false;