Aiming to fix #862.
Attempting to update views during shutdown can crash the app due to accessing backend models that may no longer exist.
The shutdown procedure begins by unsetting clientModel from all views.
Therefore, it’s safe to ignore events when clientModel is nullptr.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | pablomartin4btc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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 —
Attempting to update views during shutdown can crash the app due to
accessing backend models that may no longer exist.
The shutdown procedure begins by unsetting clientModel from all views.
Therefore, it’s safe to ignore events when clientModel is nullptr.
