fix(gui): deregister QRhiWidget cleanup callback on all GPU platforms (Windows multi-pan crash, #3714)#3922
Conversation
…, not just macOS (aethersdr#3714) The connect-time multi-panadapter restore crashes on Windows a few seconds after launch. SpectrumWidget::prepareForTopLevelChange() sends QEvent::WindowAboutToChangeInternal so QRhiWidgetPrivate deregisters its cleanup callback from the old backing-store QRhi before a reparent — but the send was gated #ifdef Q_OS_MAC, making it a no-op on Windows. Every reparent site (PanadapterStack::rearrangeLayout, floatPanadapter, dockPanadapter, and the shutdown path) routes through this helper. On Windows the stale callback therefore stayed registered against the old QRhi; when that QRhi was torn down during the splitter rebuild, QRhi::runCleanup() fired against a stale QRhiWidgetPrivate during deferred event delivery and crashed. That matches the reporter's Windows crash dump, whose only resolved frame is QWindowsGuiEventDispatcher::sendPostedEvents. WindowAboutToChangeInternal is cross-platform Qt machinery — QRhiWidget handles it identically on Metal/D3D/Vulkan/GL — so the send belongs on every GPU platform. The macOS gate was historical: the same crash class (aethersdr#2495) was first reproduced and fixed on macOS and the guard was never broadened. Dropping the inner #ifdef closes the whole reparent-race class on Windows in one place, with zero macOS runtime change (it already sent the event). Fixes aethersdr#3714 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the diff against the surrounding code. This is a clean, well-scoped fix — thanks @jensenpat. ✅
Verified the claims:
- The
#ifdef/#endifbalance is correct after dropping the innerQ_OS_MACguard — the outerAETHER_GPU_SPECTRUMblock stays intact, so the only behavioral change is thatWindowAboutToChangeInternalnow fires on every GPU platform. - The new comment's "exactly once, before the reparent" invariant holds:
refreshAfterReparent()(PanadapterStack.cpp:20) only callsresetGpuResources()— it never re-sends the event. Every reparent site (rearrangeLayout, float/dock at :254/:280/:708/:762, plus the shutdown path viaprepareForShutdown) routes the deregistration through this one helper before the reparent, withrefreshAfterReparentrunning after. No double-send. - Root-cause framing matches #2495's crash class, and the macOS gate was indeed historical, not architectural —
QRhiWidgetPrivatehandles the event identically across Metal/D3D/Vulkan/GL.
One observation (not blocking): the PR is framed as a Windows fix, but removing the gate also activates this send on Linux/OpenGL — the app's primary platform, where it was likewise never firing before. That's a good thing (it closes the same stale-callback window on Linux too), but it does mean the change is slightly broader than "Windows only," and the bridge proof covered macOS (a runtime no-op there) rather than the two platforms that actually gain new behavior. The mechanism is sound and CI is green on all three, so I'd treat the Linux side as covered-by-symmetry — just worth noting it's now in the active path there as well.
CI: all 6 checks green — build (Linux), check-windows, check-macos, CodeQL, and Qt Accessibility.
No convention, null-safety, resource-leak, or scope concerns: single file, no settings/RAII surface touched, and the event machinery has no system-boundary error handling to add. Looks good to me.
🤖 aethersdr-agent · cost: $7.6734 · model: claude-opus-4-8
There was a problem hiding this comment.
Reviewed the change against the source. This looks good — clean, well-scoped, and correctly reasoned. Thanks @jensenpat.
Verified:
- The diff does exactly what it says: drops the inner
#ifdef Q_OS_MACsoQEvent::WindowAboutToChangeInternalis sent on every GPU platform underAETHER_GPU_SPECTRUM.WindowAboutToChangeInternalis Qt-internal machinery thatQRhiWidgetPrivatehandles identically across Metal/D3D/Vulkan/GL, so extending it to Windows is the right call and is a runtime no-op on macOS. - The exactly-once claim in the new comment checks out:
refreshAfterReparent()inPanadapterStack.cpponly callsresetGpuResources()(plus the mac-only native-handle dance) and does not re-send the event, so there's no double-fire. - All reparent sites route
setParent(nullptr)/splitter rebuilds throughprepareForTopLevelChange()before the move (PanadapterStack.cpp:254,280,708,762), so this single helper does close the reparent-race class. - CI is green on all platforms (build, check-windows, check-macos, CodeQL, a11y).
One thing worth a note for follow-up (not blocking): the deregistration is now cross-platform, but the re-registration side — refreshAfterReparent()'s windowHandle()->destroy() + WA_NativeWindow native-handle recreation — remains #if defined(Q_OS_MAC). You've already flagged this in the PR description as relying on resetGpuResources() for the Windows rebind via Qt's normal reparent machinery. That's the one part of this fix that's verified by inspection rather than a reproduced Windows run, so it's the natural thing to keep an eye on if any post-reparent render-staleness shows up on Windows. The scoping (deregistration now, native-handle generalization as a separate Windows-tested change) is the right judgment call.
Nice that the whole class collapses to a single-line guard removal because every reparent already funnels through one helper.
🤖 aethersdr-agent · cost: $2.9733 · model: claude-opus-4-8
ten9876
left a comment
There was a problem hiding this comment.
Clean, correctly-reasoned fix. Verified: the diff drops only the inner Q_OS_MAC guard (outer AETHER_GPU_SPECTRUM intact, #ifdef/#endif balanced — CI build green on all 3 platforms confirms), so WindowAboutToChangeInternal now fires on every GPU backend. It's a runtime no-op on macOS (already sent inside the old guard) → zero macOS regression, and it deregisters the stale QRhi cleanup callback before reparent on Windows, closing the #3714 crash class. All reparent sites route through this single helper, so no scattered edits, and the crash-dump frame (QWindowsGuiEventDispatcher::sendPostedEvents) matches the deferred-delivery fault exactly. Mirrors the proven #2495 macOS path. CI green across the board. Approving. Thanks @jensenpat.
#3961) ## Release documentation prep for **v26.7.1** (2026-07-02) 29 commits since v26.6.5. Docs-only + version bump — no code behavior change. ### Version bump 26.6.5 → 26.7.1 The three canonical locations (per `AGENTS.md`): - `CMakeLists.txt:2` — `project(AetherSDR VERSION 26.7.1)` → `AETHERSDR_VERSION` (the app's version string) - `README.md` — Current version line - `AGENTS.md` — Current version line ### `CHANGELOG.md` Promoted `[Unreleased]` → **`## [v26.7.1] — 2026-07-02`** in house style (v-prefix, em-dash, "N commits since…" opener + headline), authored from all merged PRs and categorized: - **Added:** 3D stacked-trace spectrum (#3899), 3D dBm scale (#3937), in-process NVIDIA BNR + AFX download-on-demand (#3902), TX meter mouse-over readouts (#3936), SWR manual sweep range (#3885), CW sidetone capture (#3895), KiwiSDR metadata scaffolding (#3898), bridge verbs (#3920) - **Changed:** BNR container/NIM backend removed, 60 fps + per-pixel GPU FFT trace (#3958), FlexLib-sourced model capabilities (#3954/#2177), RX speaker-latency cut (#3897), Display pane sections (#3935) - **Fixed:** #3922, #3926, #3941, #3940, #3942, #3921, #3903, #3892, #3924, #3891, #3890, #3900, #3947 - Fresh empty `[Unreleased]` restored above the release block. Internal/CI/docs/self-fix PRs (#3931/#3916/#3934/#3929) omitted per house style. ### `ROADMAP.md` - Cycle header → "post-v26.7.1" - NVIDIA BNR removed from **In flight** (shipped this cycle) - Five v26.7.1 highlights prepended to **Recently shipped** ### `README.md` - Highlights: GPU spectrum bullet now notes the **per-pixel FFT trace at up to 60 fps** and the **3D stacked-trace** spectrum mode ### Reviewer notes - Touches protected paths (`*.md` + `AGENTS.md` Tier-1 + `CMakeLists.txt`) — needs `@aethersdr/infrastructure` sign-off. - Tagging `v26.7.1` and cutting the release build are **not** part of this PR (docs prep only). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Fixes the Windows crash in #3714: AetherSDR comes up for a few seconds then crashes on every launch once the radio restores a saved 2-panadapter session (the reporter is on a Flex-6600, Win 11). The workaround that already protects this path on macOS was gated
#ifdef Q_OS_MACand never extended to Windows.Root cause
SpectrumWidget(aQRhiWidgetunderAETHER_GPU_SPECTRUM) registers a cleanup callback on the top-level window's backing-storeQRhi. Before any reparent,SpectrumWidget::prepareForTopLevelChange()sendsQEvent::WindowAboutToChangeInternalsoQRhiWidgetPrivatederegisters that callback from the old QRhi. That send was wrapped in#ifdef Q_OS_MAC, so on Windows it was a no-op.On connect, the radio reports the saved 2 pans and the debounced layout-restore timer fires
PanadapterStack::rearrangeLayout(), which doessetParent(nullptr)on each applet (a momentary top-level change) and rebuilds theQSplitter. On Windows the stale callback stayed registered against the old QRhi; when that QRhi was torn down during the rebuild,QRhi::runCleanup()fired against a now-staleQRhiWidgetPrivateduring deferred event delivery and crashed. This is the same crash class documented for #2495 — just on the one platform where the fix was never applied.This matches the reporter's MS Store crash dump exactly: it is unsymbolized (no shipped PDB), but the one resolved frame is
QWindowsGuiEventDispatcher::sendPostedEventswith ~16 frames of app code beneath it — i.e. the fault is in deferred main-thread event delivery, precisely where a stale QRhi cleanup callback fires.Fix
Drop the inner
#ifdef Q_OS_MACinprepareForTopLevelChange()so the deregistration fires on every GPU platform.WindowAboutToChangeInternalis cross-platform Qt machinery —QRhiWidgethandles it identically on Metal/D3D/Vulkan/GL — so the macOS gate was historical, not architectural (the crash was first reproduced on macOS in #2495 and the guard was never broadened).Because every reparent site already routes through this one helper (
rearrangeLayout,floatPanadapter,dockPanadapter, and the shutdown path), this single change closes the whole reparent-race class on Windows — no scattered edits. It is a runtime no-op on macOS (which already sent the event), so there is zero macOS regression risk.void SpectrumWidget::prepareForTopLevelChange() { #ifdef AETHER_GPU_SPECTRUM -#ifdef Q_OS_MAC - // ... mac-only ... + // ... cross-platform: fire on Metal, D3D/Vulkan and OpenGL alike ... QEvent event(QEvent::WindowAboutToChangeInternal); QCoreApplication::sendEvent(this, &event); -#endif #endif }How the agent automation bridge proved it
The crash is Windows/D3D-specific and cannot be reproduced on the macOS bridge (macOS already had the fix, so it never crashed there). Proof is therefore split:
sendPostedEvents) and the matching Problems with slice reduction and slice reordering "Crash" #2495 history.get pans→ 1 →pan add→ rearrange/reparent →get pans→ 2, both render →pan close→ back to 1). The app stayed responsive (pingok) across both the build-up and teardown reparents, the log showed no crash/runCleanup/stale-callback markers, and the radio was restored to its original 1-pan state.Proof
2-panadapter stacked layout rendering healthy on this branch's build (the exact "stacked top/bottom" arrangement from the report), driven via the bridge — screenshot attached below.
Bridge gaps
Follow-ups (not in this PR)
refreshAfterReparent()'s post-move native-handle recreation (windowHandle()->destroy()+WA_NativeWindow) is still macOS-only. The crash fix doesn't need it (the existingresetGpuResources()handles Windows rebind via Qt's normal reparent machinery), but generalizing it to Windows is a candidate hardening that needs a Windows test before shipping.<GUIClientID>reset recovery step (per the reporter's suggestion in the thread).Fixes #3714
💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat