fix(tci): prevent crash on quit when TciServer outlives RadioModel (#2385)#2386
Conversation
|
Ian, thanks for the PR. You have some cruft from your existing spots work in here. Can you stash it and rebase this crash fix on main so I can approve? |
…en9876#2385) TciServer is a QObject child of MainWindow, so Qt deletes it during ~QWidget::deleteChildren() — which runs *after* MainWindow's value members (including m_radioModel) have already been destroyed. This meant stop() → releaseDaxForTci() → m_model->isConnected() dereferenced a destroyed object on every clean exit when a radio was connected, causing EXC_BAD_ACCESS at 0x38 on the main thread. Primary fix: explicitly delete m_tciServer in ~MainWindow() after the audio thread is stopped (no more daxAudioReady cross-thread signals) but while m_radioModel is still fully alive (DAX stream-remove commands reach the radio). TciApplet's raw back-reference is nulled first via setTciServer(nullptr) to prevent a dangling pointer surviving in the widget tree between the explicit delete and ~QWidget::deleteChildren(). Belt-and-braces: promote TciServer::m_model from RadioModel* to QPointer<RadioModel>. QPointer auto-clears when the pointee is destroyed, so the existing `if (!m_model) return;` guard in releaseDaxForTci() now covers the post-destruct case — a future refactor cannot silently re-introduce the crash.
M7HNF-Ian
force-pushed
the
fix/tciserver-crash-on-quit
branch
from
59282c8 to
0aae49d
Compare
|
Thanks jensenpat, missed that was still there |
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the careful writeup, @M7HNF-Ian — the diagnosis and fix both check out.
Diagnosis verified. m_radioModel is a value member of MainWindow (MainWindow.h:253), so it destructs as part of ~MainWindow's implicit member teardown — before ~QObject walks children(). TciServer was added as a QObject child at MainWindow.cpp:3594 (new TciServer(&m_radioModel, this)), so by the time Qt deletes it, m_radioModel is already gone, and ~TciServer() → stop() → releaseDaxForTci() (TciServer.cpp:188, 231, 1573) dereferences the dead object. Crash matches.
Primary fix — placement is right. The explicit teardown sits after the audio-worker-thread join (m_audio = nullptr at MainWindow.cpp:3907) and before any value-member destruction. Critically, m_radioModel and m_radioModel.panStream() are still alive, so both branches in releaseDaxForTci() succeed: panStream()->unregisterDaxStream() (1571) and m_model->sendCommand("stream remove …") (1574) reach the radio. Nulling TciApplet's back-pointer first is the right call — TciApplet is also a child and would otherwise outlive m_tciServer with a stale TciServer*.
Belt-and-braces — QPointer is appropriate. RadioModel derives from QObject (RadioModel.h:41), so QPointer is well-defined. When ~RadioModel() runs as a value-member destruction, ~QObject emits destroyed synchronously and QPointer clears, so the existing if (!m_model) return; early-out at TciServer.cpp:1561 becomes a real safety net. Other call sites (m_model->slice(...) at line 1586, m_model->panStream(), etc.) inside releaseDaxForTci() are reached only after that guard, so they're covered too.
One nit, take it or leave it: the comment says "audio is stopped (no more daxAudioReady cross-thread signals)" but daxAudioReady is emitted by PanadapterStream (owned by m_radioModel), not by AudioEngine. The audio teardown above doesn't directly silence that signal. It's harmless because Qt disconnects all of m_tciServer's connections in ~QObject before releaseDaxForTci() runs, but a future reader might be misled by the rationale. Could be reworded to "TciServer disconnects from all senders during ~QObject before releaseDaxForTci runs, so no daxAudioReady slot can re-enter," or just trimmed.
Not a blocker. Fix is solid and the reasoning travels well — appreciate the thorough PR description and the call stack from VU2CPL.
…2380) (#2394) SpotHub (DX cluster / RBN) did not automatically reconnect after a Wi-Fi drop or any other failed connection attempt. The app would try once when the network came back, fail, and stop retrying — requiring a manual reconnect. Root cause: Qt's QAbstractSocket only emits disconnected() on a Connected → Unconnected transition. When a socket fails during ConnectingState (host blocked, refused, or timed out), Qt emits errorOccurred but NOT disconnected. The reconnect timer was only armed inside onDisconnected(), so a single failed connect attempt permanently halted the backoff chain. Two additional failure paths had the same gap: onSocketError() emitted the UI error only and never re-armed the timer, and the 10-second connect timeout lambda called abort() without scheduling a retry. Fix: extracts a scheduleReconnect() helper called from all three failure paths, with two guards — m_intentionalDisconnect (don't override user-initiated disconnect) and m_reconnectTimer->isActive() (prevents double-scheduling when errorOccurred and the connect timeout both fire for the same failed attempt). A per-call epoch counter (m_connectEpoch) captured by the timeout lambda ensures a stale timeout from attempt N cannot abort a later attempt N+1 that has since succeeded. connectToCluster() also now rejects calls when the socket is already in ConnectingState to prevent overlapping attempts during rapid retries. Backoff sequence is unchanged: 5 s → 10 s → 20 s → 40 s → 60 s (cap). Cherry-picked from PR #2388 (M7HNF-Ian). The rest of that branch contained already-merged commits (#2349 Spot Lines toggle, #2386 TCI crash fix), the rejected #2387 spectrum fix, and two unrelated spot-lines naming-refinement commits that belong in their own focused PR. Co-authored-by: Ian M7HNF <[email protected]> Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
3 participants
Summary
Fixes #2385
TciServeris a QObject child ofMainWindow, so Qt deletes it during~QWidget::deleteChildren()— which runs afterMainWindow's own value members have destructed, includingm_radioModel. This meantstop()→releaseDaxForTci()→m_model->isConnected()dereferenced a destroyed object on every clean exit when a radio was connected.EXC_BAD_ACCESSat0x38, 100% reproducible on quit.Fix
Primary — explicit early teardown in
~MainWindow()(MainWindow.cpp):Teardown is placed after the audio thread is stopped (no more
daxAudioReadycross-thread signals), but whilem_radioModelis still fully alive (DAXstream removecommands reach the radio).TciApplet's raw back-reference toTciServeris nulled first to prevent a dangling pointer surviving in the widget tree:Belt-and-braces —
QPointer<RadioModel>inTciServer.h:Promotes
m_modeltoQPointer<RadioModel>.QPointerauto-clears when the pointee is destroyed, so the existingif (!m_model) return;guard inreleaseDaxForTci()covers the post-destruct case — a future refactor cannot silently re-introduce the crash.Files modified
src/core/TciServer.h— add#include <QPointer>,RadioModel* m_model→QPointer<RadioModel> m_modelsrc/gui/MainWindow.cpp— explicitdelete m_tciServerin~MainWindow()after audio teardown, with TciApplet back-reference cleared firstTest plan
HAVE_WEBSOCKETSdefined — no compile errorsReported by VU2CPL (Manoj) with full crash report and call stack.