fix(automation): defer widget click/toggle out of the socket read callback (crash) by jensenpat · Pull Request #3826 · aethersdr/AetherSDR

jensenpat · 2026-06-26T02:19:50Z

Summary

Follow-up crash fix to #3819. A bridge invoke <target> click could segfault the app.

invoke <target> click (and toggle) ran QAbstractButton::click() synchronously inside the QLocalSocket read callback. When the target is a QToolButton with a dropdown menu, clicking it opens that menu's popup loop (popupTimerDone → QMenu::exec → QMenu::popup) — a nested event loop — right inside the CFSocket read callback. That re-enters socket/window state and crashes:

QWindow::geometry()  ← SIGSEGV (null window, far=0x8)
QCocoaWindow::setVisible(bool)
QMenu::popup → QMenuPrivate::exec
QToolButtonPrivate::popupTimerDone
QAbstractButton::click()
AutomationServer::doInvoke (AutomationServer.cpp:1555)
AutomationServer::handleLine
AutomationServer::onReadyRead
qt_mac_socket_callback → __CFSocketPerformV0

(The app was Role: Background at the time, which makes the popup's native window resolution especially fragile.)

Root cause & fix

This is the latent sibling of the bug #3819 already fixed: that PR deferred the menu-action trigger path out of the socket callback; the widget click/toggle path was left synchronous and opens popups/dialogs the same way.

Fix: defer click/toggle to a clean main-loop turn via QTimer::singleShot(0, qApp, …) (QPointer-guarded), so any nested popup/dialog loop runs on a normal stack — never inside socket I/O. The response returns {deferred:true}; state setters (setValue/setText/setChecked/setCurrentText/setCurrentIndex/submit) stay synchronous and keep their newValue round-trip.

Verification

Built on main, launched a backgrounded automation instance, and clicked the menu-bar overflow popup button (qt_menubar_ext_button) repeatedly:

invoke … click → {deferred:true} in <2 ms (was: nested popup → crash)
bridge stayed responsive (ping immediate)
app survived every click with no crash report

Note

The general principle now holds across both paths: a bridge verb must never run a nested event loop (modal dialog / popup menu) synchronously inside the socket read callback. If a state-setter's slot is ever found to open a modal, the same one-line deferral applies; an alternative is to dispatch all command handling off the socket callback entirely (kept out of scope here to preserve the synchronous newValue model).

💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat

…lback (crash) Follow-up to aethersdr#3819. invoke <target> click (and toggle) ran QAbstractButton::click() synchronously inside the QLocalSocket read callback. A QToolButton with a dropdown opens its popup menu on click (popupTimerDone -> QMenu::exec -> QMenu::popup), a NESTED event loop; running it inside the CFSocket read callback re-enters socket/window state and segfaults — observed EXC_BAD_ACCESS in QWindow::geometry() on a null window while the app was backgrounded. aethersdr#3819 deferred the menu-ACTION trigger path; this is its latent sibling on the widget click/toggle path. Defer both to a clean main-loop turn via QTimer::singleShot(0) so any nested popup/dialog loop runs on a normal stack. The response returns {deferred:true}; state setters (setValue/setText/setChecked/combo/submit) stay synchronous and keep their newValue round-trip. Verified: clicking the menu-bar overflow popup button (qt_menubar_ext_button) repeatedly on a backgrounded instance returns in <2 ms, the bridge stays responsive, and the app survives with no crash report. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

aethersdr-agent

Reviewed the single-file change against the existing code in AutomationServer.cpp. This looks good — clean, minimal, and correct.

What's right:

The fix faithfully mirrors the already-merged #3819 menu-action deferral (AutomationServer.cpp:1458-1462): same QTimer::singleShot(0, qApp, …) + QPointer guard pattern, same deferred:true response contract. QTimer/QPointer/qApp are already in use on that path, so no new includes are needed. The "click/toggle was the latent sibling of the menu-action path" framing is accurate.
Behavior for the toggle verb on a non-checkable button is preserved: useToggle = (action=="toggle" && b->isCheckable()) falls through to click(), matching the old b->isCheckable() ? b->toggle() : b->click().
The safety rails — disabled-control refusal (:1515), TX-keying guard (:1526), and power-ceiling clamp — all run before the deferral, so deferring the click doesn't bypass any of them. Good.
The QPointer capture means a slice/dialog torn down between scheduling and the timer firing is handled safely (if (!bg) return;).

One non-blocking note: the response contract now changes for every click/toggle, not just QToolButton-with-menu cases — a plain push-button that never opens a popup also returns {deferred:true} with no newValue. That's the right call for safety (the callback can't know in advance whether a slot will spin a nested loop, and it matches the menu path exactly), but it does mean any automation script that read newValue off a click reply must now re-read via get/dumpTree. The PR body documents this, so just flagging it for callers.

All six CI checks pass (build, macOS/Windows, CodeQL, a11y). Nice, surgical follow-up — thanks @jensenpat. 🎯

_{🤖 aethersdr-agent · cost: $3.7535 · model: claude-opus-4-8}

… driving popups Headless automation runs the app backgrounded; a native QMenu popped as a side effect of a driven click/trigger can segfault in QWindow::geometry() on a null window (e.g. a QToolButton dropdown, or an AX.25/APRS dialog menu popped from a click handler). Activating the target window first gives Cocoa a valid window to anchor the popup to — verified to foreground a backgrounded instance. Because activateWindow() really does foreground the app, doing it on every driven click would repeatedly steal focus during a sweep. So it is gated behind AETHER_AUTOMATION_RAISE (default OFF): enable it when driving flows that pop native menus from a backgrounded instance; otherwise the deferred click/trigger path runs unchanged with no focus-steal. Applied on the deferred click/toggle and menu-action trigger paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

jensenpat · 2026-06-26T03:23:47Z

Added a second commit: AETHER_AUTOMATION_RAISE (default off) — an opt-in that activates the target window before a deferred click/trigger, so a native menu popped as a side effect (QToolButton dropdown / a dialog menu popped from a click handler) has a valid window instead of segfaulting in QWindow::geometry() on a backgrounded instance. Verified it does foreground a backgrounded app; gated off by default because doing it on every driven click would steal focus during a sweep. The deferral fix (commit 1) is the default-on crash fix; this is the on-demand tool for the rarer backgrounded-popup class.

NF0T · 2026-06-26T14:39:35Z

@NF0T review — PR #3826

Single-file change, 74 additions to AutomationServer.cpp. Clean, correct, and well-scoped.

Root cause and fix are accurate. invoke click/toggle ran QAbstractButton::click() synchronously inside the QLocalSocket::readyRead() callback. A QToolButton with a dropdown calls QMenu::exec on click — a nested event loop — re-entering the CFSocket callback stack and corrupting Cocoa window state. The stack trace in the PR body matches the mechanism exactly. The fix is a one-pattern mirror of the already-merged #3819 menu-action deferral: QTimer::singleShot(0, qApp, …) + QPointer guard, with {deferred:true} in the response. No new includes, no new patterns — the widget click path was literally the latent sibling the PR body describes.

Three correctness points worth noting:

The disabled-control refusal and TX-keying guard both run before the singleShot is scheduled, so deferring the click creates no window where a TX-keying control can slip through.
useToggle = (action == "toggle" && b->isCheckable()) correctly preserves the fall-through-to-click behavior for non-checkable buttons, matching the old b->isCheckable() ? b->toggle() : b->click() exactly.
QPointer<QAbstractButton> in the lambda handles teardown of the button's containing slice or dialog between schedule and fire.

Second commit (AETHER_AUTOMATION_RAISE) is well-judged as opt-in. Caching the env check with static const bool, guarding on null/hidden window, and defaulting off to avoid focus-stealing during headless sweeps are all the right calls.

CI: All six checks green including check-macos — the only leg that exercises the Cocoa/CFSocket path where the crash originates. The live verification in the PR body (backgrounded instance, repeated qt_menubar_ext_button clicks, {deferred:true} in <2 ms, bridge responsive, no crash report) is what CI can't reproduce automatically; it covers the gap.

Constitution: Principle XI (Fixes Are Demonstrated) and Principle VIII (Evidence Over Assertion) both honored — real stack trace diagnosis, live-verified fix.

For future reference (not blocking):

Several inline comments carry (#3646 follow-up) tracker citations in the source. AGENTS.md asks that comments not reference the task/fix/issue — those references rot as the code evolves; they belong in the PR description and commit message, not in the source. The WHY content in those comments is excellent; just drop the tracker suffix in future PRs.
No Fixes #NNNN in the PR body — the crash was found during dogfooding rather than a user-filed issue, so there's nothing to close. The pattern to build toward: file a one-liner issue for every discovered defect before the PR lands so it gets a trackable number. Not a blocker here.

Merge eligible — Tier 3 only (src/core/AutomationServer.cpp → * catch-all). Squash subject will include Principle XI.

NF0T

Root cause confirmed from stack trace; fix mirrors the QTimer::singleShot(0) + QPointer pattern already established by the merged #3819 menu-action path — the widget click/toggle path was its latent sibling. Safety rails (disabled-control refusal, TX-keying guard) verified to run before the deferral. AETHER_AUTOMATION_RAISE companion correctly opt-in and focus-steal-guarded. All six CI checks pass including check-macos. Live verification in the PR body covers what CI cannot reproduce automatically.

Merge when ready. Squash subject: fix(automation): defer widget click/toggle out of the socket read callback — Principle XI. (#3826)

jensenpat marked this pull request as ready for review June 26, 2026 02:21

jensenpat requested a review from a team as a code owner June 26, 2026 02:21

aethersdr-agent Bot reviewed Jun 26, 2026

View reviewed changes

NF0T self-assigned this Jun 26, 2026

NF0T approved these changes Jun 26, 2026

View reviewed changes

NF0T merged commit 9d71f23 into aethersdr:main Jun 26, 2026
6 checks passed

ten9876 mentioned this pull request Jun 26, 2026

Automation bridge Phase-3: remaining drivability/observability gaps (deny-word anchoring, popup/drag verbs, Quindar + overlay snapshots) #3845

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(automation): defer widget click/toggle out of the socket read callback (crash)#3826

fix(automation): defer widget click/toggle out of the socket read callback (crash)#3826
NF0T merged 2 commits into
aethersdr:mainfrom
jensenpat:fix/automation-popup-reentrancy

jensenpat commented Jun 26, 2026

Uh oh!

aethersdr-agent Bot left a comment

Uh oh!

jensenpat commented Jun 26, 2026

Uh oh!

NF0T commented Jun 26, 2026

Uh oh!

NF0T left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jensenpat commented Jun 26, 2026

Summary

Root cause & fix

Verification

Note

Uh oh!

aethersdr-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

jensenpat commented Jun 26, 2026

Uh oh!

NF0T commented Jun 26, 2026

Uh oh!

NF0T left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants