Crash fix, disconnect numBlocksChanged() signal during shutdown

furszy commented at 3:49 pm on April 7, 2025: member

Aiming to fix bitcoin-core/gui#862.

The crash stems from the order of the shutdown procedure: We first unset the client model, then destroy the wallet controller—but we leave the internal wallet models (m_wallets) untouched for a brief period. As a result, there’s a point in time where views still have connected signals and access to wallet models that are not connected to any wallet controller. Now.. since the clientModel is only replaced with nullptr locally and not destroyed yet, signals like numBlocksChanged can still emit. Thus, when wallet views receive them, they see a non-null wallet model ptr, and proceed to call backend functions from a model that is being torn down.

As the shutdown procedure begins by unsetting clientModel from all views. It’s safe to ignore events when clientModel is nullptr.

DrahtBot commented at 3:49 pm on April 7, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, maflcko, hebasto
Stale ACK	laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

yeyeypro approved

luke-jr commented at 5:05 am on April 10, 2025: member

I think Qt is supposed to automatically disconnect signals when objects are destroyed? Seems like we should just ensure the model gets destroyed before the underlying interfaces…?

luke-jr commented at 5:07 am on April 10, 2025: member

OTOH, it seems like we should unconditionally disconnect the old clientmodel before setting a new one, so this makes sense up at the top of the method.

furszy commented at 2:53 pm on April 10, 2025: member

OTOH, it seems like we should unconditionally disconnect the old clientmodel before setting a new one, so this makes sense up at the top of the method.

Yeah. We could do what the rest of the code is doing too.. just check for the local clientModel before consuming the event:

 0diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp
 1--- a/src/qt/sendcoinsdialog.cpp
 2+++ b/src/qt/sendcoinsdialog.cpp
 3@@ -860,7 +860,7 @@
 4 }
 5 
 6 void SendCoinsDialog::updateNumberOfBlocks(int count, const QDateTime& blockDate, double nVerificationProgress, SyncType synctype, SynchronizationState sync_state) {
 7-    if (sync_state == SynchronizationState::POST_INIT) {
 8+    if (sync_state == SynchronizationState::POST_INIT && clientModel) {
 9         updateSmartFeeLabel();
10     }
11 }

Just to be clear about this issue, it happens because we first unset the client model, then destroy the wallet controller but we do not destroy the internal wallet models (m_wallets). So, there is a point in time just after these two calls where our views still have the signals connected (because the clientModel has not been destroyed yet, just replaced by a nullptr locally) and the view still has access to the wallet model. So they receive the clientModel new block signal, check if the wallet model is there and try to call some backend functions. So, the simplest solution would be to check for a local nullptr client model when handling the signal.

A more general solution would be to close all wallets before destroying the wallet controller, but… I’m not sure I want to go down that rabbit hole. Better to fix this crash first, then change shutdown behavior.

— Will do some additional testing next week —

furszy force-pushed on Apr 10, 2025

pablomartin4btc commented at 4:42 pm on April 14, 2025: contributor

tACK c6f4b0d79606b525064ad702de06bcde11582dd1

laanwj commented at 12:08 pm on April 24, 2025: member

My preference would be, instead of this fairly fragile check, to symmetrically disconnect the numBlocksChanged signal in setClientModel when the clientModel is unset.

laanwj renamed this:
~~gui: crash fix, disconnect numBlocksChanged() signal during shutdown~~
Crash fix, disconnect numBlocksChanged() signal during shutdown
on Apr 24, 2025

furszy force-pushed on Apr 24, 2025

laanwj approved

laanwj commented at 2:46 pm on April 24, 2025: member

Code review ACK 0ee5376fb302ebb21fb6192ed708800ee8035eb5 Nice solution, also handles the (hypothethical) case where a client model is replaced with a new one.

DrahtBot requested review from pablomartin4btc on Apr 24, 2025

maflcko commented at 4:57 pm on April 24, 2025: contributor

I tried 0ee5376fb302ebb21fb6192ed708800ee8035eb5 with my steps to reproduce and still got:

0==301067== Process terminating with default action of signal 11 (SIGSEGV): dumping core

laanwj commented at 5:05 pm on April 24, 2025: member

That’s really strange. Is there some thread race maybe?

Does the old solution (to check clientModel in the callback) work better?

furszy commented at 6:16 pm on April 24, 2025: member

That’s really strange. Is there some thread race maybe? Does the old solution (to check clientModel in the callback) work better?

Hmm, I should check, but I have the hunch that already queued events are still delivered after disconnection. Which, if that is the case, the previous clientModel check would work better.

laanwj commented at 6:22 pm on April 24, 2025: member

Hmm, I should check, but I have the hunch that events already queued are still delivered after disconnection. Which, if that is the case, the previous clientModel check would work better.

It’s worth checking but imo it’s unlikely it works that way. The signals are queued, but if no handler is connected, they sould just be dropped (next time the event loop runs).

furszy commented at 7:00 pm on April 24, 2025: member

Hmm, I should check, but I have the hunch that events already queued are still delivered after disconnection. Which, if that is the case, the previous clientModel check would work better.

It’s worth checking but imo it’s unlikely it works that way. The signals are queued, but if no handler is connected, they sould just be dropped (next time the event loop runs).

It seems to be the case: https://stackoverflow.com/questions/2532341/problem-with-qtqueuedconnection-signal-delivered-after-disconnect

So.. it seems that reverting to the null clientModel check is the simplest fix path. We could discuss the general solution of closing all wallets before destroying the wallet controller in another PR too (if/when we ever want to do that). Agree?

laanwj commented at 7:05 pm on April 24, 2025: member

Okay, that’s really awful (doubt this is the only such case), but yes, better to revert to previous solution then. (to be clear, i’m just disappointed in how Qt handles this, don’t have any better proposal right now)

gui: crash fix, disconnect numBlocksChanged() signal during shutdown

The crash stems from the order of the shutdown procedure:
We first unset the client model, then destroy the wallet controller—but we leave
the internal wallet models ('m_wallets') untouched for a brief period. As a result,
there’s a point in time where views still have connected signals and access to
wallet models that are not connected to any wallet controller.
Now.. since the clientModel is only replaced with nullptr locally and not destroyed
yet, signals like numBlocksChanged can still emit. Thus, when wallet views receive
them, they see a non-null wallet model ptr, and proceed to call backend functions
from a model that is being torn down.

As the shutdown procedure begins by unsetting clientModel from all views. It’s safe
to ignore events when clientModel is nullptr.

71656bdfaa

furszy force-pushed on Apr 24, 2025

furszy commented at 7:40 pm on April 24, 2025: member

Updated. Back into the clientModel nullptr check approach.

(doubt this is the only such case)

For sure. We could create a global connection function that checks if the app is tearing down before processing the event too. Still, I think I’d rather rework the shutdown process than go that route. Yet, not something for this PR anyway.

pablomartin4btc approved

pablomartin4btc commented at 7:11 am on April 25, 2025: contributor

re-ACK 71656bdfaa6bfe08ce9651246a3ef606f923351b

DrahtBot requested review from laanwj on Apr 25, 2025

maflcko commented at 7:27 am on April 25, 2025: contributor

lgtm ACK 71656bdfaa6bfe08ce9651246a3ef606f923351b

I have not reviewed the code, but I tried to reproduce the segfault with my steps to reproduce and it seemed to pass with about 15 restarts.

pablomartin4btc commented at 11:45 am on April 25, 2025: contributor

I’ve tested https://github.com/bitcoin-core/gui/commit/71656bdfaa6bfe08ce9651246a3ef606f923351b on Ubuntu 22.04, trying several times as well with the steps mentioned in #862 and I can’t reproduce the bug anymore.

hebasto approved

hebasto commented at 12:44 pm on April 26, 2025: member

ACK 71656bdfaa6bfe08ce9651246a3ef606f923351b, I have reviewed the code and it looks OK.

hebasto merged this on Apr 26, 2025

hebasto closed this on Apr 26, 2025

hebasto added the label Needs backport (29.x) on Apr 26, 2025

furszy deleted the branch on Apr 26, 2025

fanquake removed the label Needs backport (29.x) on Apr 28, 2025

fanquake commented at 9:00 am on April 28, 2025: member

Backported to 29.x in #32292.

Crash fix, disconnect numBlocksChanged() signal during shutdown #864

Reviews