From 8623dd4c6ff029281e6bdedbf2689855439dbc07 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Thu, 17 Sep 2026 23:19:59 +1200 Subject: [PATCH] Refuse to close an editor while a modal dialog is running (#904) openAndAddProject() shows BackupDialog as a stack object parented to the editor and exec()s it; every QET::QetMessageBox does the same. exec() runs a nested event loop, and closing the editor during it turns WA_DeleteOnClose into a deleteLater() that the nested loop processes: ~QWidget() deletes the editor's children, the stack-allocated dialog among them, and the process aborts. Reported on macOS, where File > Quit lives in the application menu and stays usable while the backup question is up. The close is now refused while any modal widget is active, and the dialog is raised so the refused quit is not silent. It is done in QETMainWindow::event() rather than in closeEvent(), because QETDiagramEditor::closeEvent() starts closing projects before it decides whether to accept. That covers the diagram and title-block editors; QETElementEditor is a plain QMainWindow, so its closeEvent() calls the same helper before canClose(), which itself opens a modal. QETApp::quitQET() needs nothing: closeEveryEditor() goes through each editor's close(), and quitQET() already only quits when every close succeeded. Rejected alternatives, both suggested on the issue: - Giving the dialog no parent stops the abort but not the deletion. One caller of openAndAddProject() is the editor's own constructor, which goes on to open the next file and call slot_updateActions() on this -- a loud abort would become a silent use-after-free. - Guarding only QETApp::closeEveryEditor(), which I first recommended on the issue, misses the reported route entirely: File > Quit is connected to QETDiagramEditor::close(), not to quitQET(). Verified on Linux, where there is nothing to click (the menu bar belongs to the blocked window, and Qt ignores window-manager close requests for it), by calling close() from gdb while the dialog's loop was running -- both QETApp::quitQET() and QWidget::close() on the editor. Unfixed, both abort with "free(): invalid size" in QObjectPrivate::deleteChildren() under ~QETDiagramEditor(), matching the report frame for frame; fixed, close() returns false, the editor and the dialog stay up, and after answering the dialog Ctrl+Q exits normally. The element-editor guard is the same helper but was not exercised separately. tests/modal-quit-regression/ turns that into a gate: it breaks on QDialog::exec(), interrupts inside the nested loop, calls quitQET() and checks the process survives. It matches no window titles (translated) and no window ids, runs on the offscreen platform, and needs only gdb with Python. Checked both ways: exit 1 with the backtrace above on a build without this change, exit 0 with it. ctest 8/8. Co-Authored-By: Claude Opus 5 --- sources/editor/ui/qetelementeditor.cpp | 8 ++ sources/qetmainwindow.cpp | 42 +++++++ sources/qetmainwindow.h | 2 + tests/modal-quit-regression/README.md | 42 +++++++ tests/modal-quit-regression/run.sh | 153 +++++++++++++++++++++++++ 5 files changed, 247 insertions(+) create mode 100644 tests/modal-quit-regression/README.md create mode 100755 tests/modal-quit-regression/run.sh diff --git a/sources/editor/ui/qetelementeditor.cpp b/sources/editor/ui/qetelementeditor.cpp index 2453f04f0..9ae36def3 100644 --- a/sources/editor/ui/qetelementeditor.cpp +++ b/sources/editor/ui/qetelementeditor.cpp @@ -23,6 +23,7 @@ #include "../elementview.h" #include "../../qetmessagebox.h" #include "../../qetapp.h" +#include "../../qetmainwindow.h" #include "../../recentfiles.h" #include "../graphicspart/customelementpart.h" #include "../elementitemeditor.h" @@ -887,6 +888,13 @@ void QETElementEditor::openElement(const QString &filepath) */ void QETElementEditor::closeEvent(QCloseEvent *qce) { + //This editor is a plain QMainWindow, not a QETMainWindow, so the + //guard QETMainWindow::event() applies to the other editors is + //applied here instead -- before canClose(), which itself opens a + //modal dialog. + if (QETMainWindow::refuseCloseWhileModal(qce)) { + return; + } if (canClose()) { writeSettings(); setAttribute(Qt::WA_DeleteOnClose); diff --git a/sources/qetmainwindow.cpp b/sources/qetmainwindow.cpp index efc9f0e83..b3553d9d6 100644 --- a/sources/qetmainwindow.cpp +++ b/sources/qetmainwindow.cpp @@ -16,6 +16,7 @@ along with QElectroTech. If not, see . */ #include +#include #include #include #include @@ -290,6 +291,9 @@ void QETMainWindow::activateMenuBar() { } bool QETMainWindow::event(QEvent *e) { + if (e -> type() == QEvent::Close && refuseCloseWhileModal(e)) { + return(true); + } if (e -> type() == QEvent::WindowStateChange) { updateFullScreenAction(); } else if (first_activation_ && e -> type() == QEvent::WindowActivate) { @@ -299,6 +303,44 @@ bool QETMainWindow::event(QEvent *e) { return(QMainWindow::event(e)); } +/** + @brief QETMainWindow::refuseCloseWhileModal + Refuse to close an editor window while any modal dialog is running. + + A modal dialog's exec() runs a nested event loop. If a window is closed + during it, the window's WA_DeleteOnClose turns into a deleteLater() that + the *nested* loop processes: the window is destroyed while code that + belongs to it -- often the very function that opened the dialog -- is + still on the stack. Most of QET's dialogs are stack objects parented to + the window (BackupDialog, and every QET::QetMessageBox), so ~QWidget() + then deletes a stack object and the process aborts (issue #904). Even a + dialog without a parent would only trade that abort for a silent + use-after-free in the caller. + + Qt already ignores window-manager close requests for a window blocked by + a modal, so this is only reachable through close() called directly: the + File > Quit action, which macOS moves into the application menu where it + stays usable during a modal, and QETApp::quitQET() from the system tray. + + Handled in event(), before closeEvent() runs, because the editors' + closeEvent() starts closing projects before it decides whether to accept. + The dialog is raised so a refused quit is not silent. + + @param e : the QEvent::Close being delivered + @return true if the close was refused and must not be processed further +*/ +bool QETMainWindow::refuseCloseWhileModal(QEvent *e) +{ + QWidget *modal = QApplication::activeModalWidget(); + if (!modal) { + return(false); + } + modal -> raise(); + modal -> activateWindow(); + e -> ignore(); + return(true); +} + /** Base implementation of firstActivation (does nothing). */ diff --git a/sources/qetmainwindow.h b/sources/qetmainwindow.h index 3d90cee4a..9ad1862f0 100644 --- a/sources/qetmainwindow.h +++ b/sources/qetmainwindow.h @@ -30,6 +30,8 @@ class QETMainWindow : public QMainWindow { public: QETMainWindow(QWidget * = nullptr, Qt::WindowFlags = Qt::Widget); ~QETMainWindow() override; + + static bool refuseCloseWhileModal(QEvent *e); // methods protected: diff --git a/tests/modal-quit-regression/README.md b/tests/modal-quit-regression/README.md new file mode 100644 index 000000000..7ed261982 --- /dev/null +++ b/tests/modal-quit-regression/README.md @@ -0,0 +1,42 @@ +# Quit-during-modal regression test + +Guards the abort reported in #904. + +`QETDiagramEditor::openAndAddProject()` shows `BackupDialog` as a stack object +parented to the editor and `exec()`s it, and `QET::QetMessageBox` does the same +for every message box. `exec()` runs a nested event loop. Closing the editor +during that loop turns `WA_DeleteOnClose` into a `deleteLater()` that the nested +loop processes, so `~QWidget()` deletes the stack-allocated dialog and the +process aborts. `QETMainWindow::refuseCloseWhileModal()` refuses such a close. + +## Running it + +```bash +tests/modal-quit-regression/run.sh --binary build/qelectrotech +``` + +Needs `gdb` with Python. It runs on the offscreen platform, so no X server or +window manager is required. Takes about ten seconds. + +Exit codes: `0` survived, `1` crashed, `2` the scenario did not happen. + +## How it works, and why this way + +On Linux there is nothing to click: the menu bar belongs to the window the +dialog blocks, and Qt ignores window-manager close requests for a blocked +window. The reported route is macOS, where File > Quit lives in the application +menu and stays usable during a modal — and what it does is call `close()` while +the dialog's loop is running. The test does the same thing through gdb: + +1. break on `QDialog::exec()`; +2. let the dialog's loop run, then interrupt it, so the main thread is inside + the nested loop — the only place the bug exists, because a `deleteLater()` + posted *before* `exec()` started is not processed by that loop; +3. call `QETApp::quitQET()`, which closes every editor; +4. let it run: an unfixed build aborts within a second. + +It matches no window titles and no window ids. Titles are translated, and +`tests/ipc-regression` once shipped a pass that could not fail because of that. + +Checked both ways before being committed: it fails on a build without the fix +(signal 6, `free(): invalid size`) and passes with it. diff --git a/tests/modal-quit-regression/run.sh b/tests/modal-quit-regression/run.sh new file mode 100755 index 000000000..9ca956c26 --- /dev/null +++ b/tests/modal-quit-regression/run.sh @@ -0,0 +1,153 @@ +#!/usr/bin/env bash +# +# Quit-during-modal regression gate -- issue #904. +# +# tests/modal-quit-regression/run.sh --binary build/qelectrotech +# +# WHAT IT GUARDS +# +# QETDiagramEditor::openAndAddProject() shows BackupDialog as a stack object +# parented to the editor and exec()s it; QET::QetMessageBox does the same for +# every message box. exec() runs a NESTED event loop. If the editor is closed +# during that loop, WA_DeleteOnClose becomes a deleteLater() that the nested +# loop processes: ~QWidget() deletes the editor's children, the stack-allocated +# dialog among them, and the process aborts ("free(): invalid size"). +# QETMainWindow::refuseCloseWhileModal() refuses such a close. +# +# HOW IT TRIGGERS THE BUG WITHOUT A MOUSE +# +# On Linux the menu bar belongs to the window the modal blocks, and Qt ignores +# window-manager close requests for a blocked window, so there is nothing to +# click. The reported route is macOS, where File > Quit moves to the +# application menu and stays usable. What that route does is call close() +# programmatically while the dialog's loop is spinning, and that is what this +# test does, through gdb: +# +# 1. break on QDialog::exec(), i.e. the moment the first dialog is shown; +# 2. let it run for a moment, then interrupt it -- the main thread is now +# inside the dialog's nested loop, which is the only place the bug lives +# (a deleteLater() posted before exec() started is not processed by it); +# 3. call QETApp::quitQET(), which closes every editor; +# 4. let it run again: the unfixed build aborts within a second, the fixed +# one keeps running until the test interrupts it. +# +# Nothing here matches a window title or a window id. A title is a locale: +# tests/ipc-regression once shipped a pass that could not fail because the +# dialog it filtered by name was translated differently in Docker. +# +# It runs on the offscreen platform, so no X server or window manager is +# needed -- only gdb with Python. +# +# RESULT +# exit 0 PASS quitQET() ran inside the modal loop and the process survived +# exit 1 FAIL the process died of a signal after quitQET() +# exit 2 ERROR the scenario never happened (no dialog, gdb problem, ...) +# +set -uo pipefail + +BINARY="" +PROJECT="" +SETTLE=2 # seconds inside the dialog loop before interrupting +OBSERVE=5 # seconds to wait for a crash after quitQET() + +while [ $# -gt 0 ]; do + case "$1" in + --binary) BINARY="$2"; shift 2 ;; + --project) PROJECT="$2"; shift 2 ;; + *) echo "unknown argument: $1" >&2; exit 2 ;; + esac +done + +[ -n "$BINARY" ] || { echo "usage: $0 --binary [--project ]" >&2; exit 2; } +[ -x "$BINARY" ] || { echo "not executable: $BINARY" >&2; exit 2; } +BINARY="$(readlink -f "$BINARY")" + +if [ -z "$PROJECT" ]; then + SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" + PROJECT="$(ls "$SCRIPT_DIR"/../../examples/*.qet 2>/dev/null | head -1)" +fi +[ -f "$PROJECT" ] || { echo "no project found; pass --project" >&2; exit 2; } + +command -v gdb >/dev/null || { echo "missing required tool: gdb" >&2; exit 2; } +gdb -batch -ex 'python import gdb' >/dev/null 2>&1 \ + || { echo "gdb has no Python support" >&2; exit 2; } + +SANDBOX="$(mktemp -d /tmp/qet-modal-quit.XXXXXX)" +cleanup() { [ "${KEEP_LOGS:-0}" = "1" ] || rm -rf "$SANDBOX"; } +trap cleanup EXIT + +# A unique binary path gives this run its own SingleApplication socket, so it +# can neither be captured by nor capture a QElectroTech already running. A +# symlink would not do: applicationFilePath() resolves it back to the real path. +TEST_BINARY="$SANDBOX/qelectrotech-modalquit" +cp "$BINARY" "$TEST_BINARY" || { echo "could not copy binary" >&2; exit 2; } + +cp "$PROJECT" "$SANDBOX/project.qet" || { echo "could not copy project" >&2; exit 2; } + +export HOME="$SANDBOX/home" +export XDG_CONFIG_HOME="$HOME/.config" +export XDG_DATA_HOME="$HOME/.local/share" +mkdir -p "$XDG_CONFIG_HOME" "$XDG_DATA_HOME" +export QT_QPA_PLATFORM=offscreen + +cat > "$SANDBOX/scenario.gdb" < "$SANDBOX/gdb.log" 2>&1 & +GDB_PID=$! +( sleep 120; kill -9 "$GDB_PID" 2>/dev/null ) & +WATCHDOG_PID=$! +wait "$GDB_PID" +kill "$WATCHDOG_PID" 2>/dev/null + +log="$SANDBOX/gdb.log" +if ! grep -q "QET_TEST: dialog exec() entered" "$log"; then + echo "ERROR: no dialog was ever shown -- the scenario did not happen" + KEEP_LOGS=1; echo "log kept: $log"; exit 2 +fi +if ! grep -q "QET_TEST: quitQET() returned" "$log"; then + echo "ERROR: quitQET() was not called inside the dialog loop" + KEEP_LOGS=1; echo "log kept: $log"; exit 2 +fi + +sig="$(sed -n 's/^QET_TEST: final signal \([0-9]*\)$/\1/p' "$log" | tail -1)" +case "$sig" in + 2) + echo "PASS: closing the editor during a modal dialog was refused; the process survived" + exit 0 ;; + "") + echo "ERROR: could not tell how the run ended" + KEEP_LOGS=1; echo "log kept: $log"; exit 2 ;; + *) + echo "FAIL: the process died of signal $sig after quitQET() ran inside the dialog loop (issue #904)" + grep -m1 -E "free\(\)|double free|corrupted" "$log" | sed 's/^/ /' + sed -n '/^QET_TEST: final signal/,$p' "$log" | grep -E '^#[0-9]+ ' | sed 's/^/ /' + KEEP_LOGS=1; echo "log kept: $log"; exit 1 ;; +esac