Fix segfault when shutdown during wallet open

[john-moffett]

Fixes #689

Summary

If you open a wallet and send a shutdown signal during that process, you'll get a segfault when the wallet finishes opening. That's because the WalletController object gets deleted manually in bitcoin.cpp during shutdown, but copies of the pointer (and pointers to child objects) are dangling in various places and are accessed in queued events after the deletion.

Details

The issue in #689 is caused by the following sequence of events:

1. Wallet open modal dialog is shown and worker thread does the actual work.
2. Every 200ms, the main event loop checks to see if a shutdown has been requested, but only if a modal is not being shown.
3. Request a shutdown while the modal window is shown.
4. The wallet open process completes, the modal window is dismissed, and various finish signals are sent.
5. During handling of one of the finish signals, qApp->processEvents() is called, which causes the main event loop to detect the shutdown (now that the modal window has been dismissed). The WalletController and all the WalletModels are deleted.
6. Control returns to the finish method, which eventually tries to send a signal from a wallet model, but it's been deleted already (and the signal is sent from a now-dangling pointer).

The simplest fix for that is to change the qApp->processEvents() into a QueuedConnection call. (The `qApp->processEvents() was a workaround to get the GUI to scroll to the last item in a list that just got added, and this is just a safer way of doing that).

However, once that segfault is fixed, another segfault occurs due to some queued wallet events happening after the wallet controller object is deleted here:

gui/src/qt/bitcoin.cpp

Lines 394 to 401 in 65de8ee

	// Delete wallet controller here manually, instead of relying on Qt object
	// tracking (https://doc.qt.io/qt-5/objecttrees.html). This makes sure
	// walletmodel m_handle_* notification handlers are deleted before wallets
	// are unloaded, which can simplify wallet implementations. It also avoids
	// these notifications having to be handled while GUI objects are being
	// destroyed, making GUI code less fragile as well.
	delete m_wallet_controller;
	m_wallet_controller = nullptr;

Since m_wallet_controller is a copy of that pointer in bitcoingui.cpp, it's now dangling and if(null) checks won't work correctly. For instance, this line:

gui/src/qt/bitcoingui.cpp

Line 413 in 65de8ee

connect(activity, &OpenWalletActivity::opened, this, &BitcoinGUI::setCurrentWallet, Qt::QueuedConnection);

sets up a QueuedConnection to setCurrentWallet, but by the time control reaches that method (one event cycle after shutdown deleted m_wallet_controller in bitcoin.cpp), the underlying objects have been destroyed (but the pointers are still dangling).

Ideally, we'd use a QPointer or std::shared_ptr / std::weak_ptrs for these, but the changes would be more involved.

This is a minimal fix for the issues. Just set m_wallet_controller to nullptr in bitcoingui.cpp, check its value in a couple places, and avoid a use of qApp->processEvents.

[DrahtBot]

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	hebasto, furszy

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

[john-moffett]

Pushed an update to address the failing tests.

They failed because the TransactionTableModel gets updated on a QueuedConnection:

gui/src/qt/transactiontablemodel.cpp

Lines 73 to 76 in d8bdee0

	bool invoked = QMetaObject::invokeMethod(ttm, "updateTransaction", Qt::QueuedConnection,
	Q_ARG(QString, strHash),
	Q_ARG(int, status),
	Q_ARG(bool, showTransaction));

The reason given is // queue notifications to show a non freezing progress dialog e.g. for rescan.

The qApp->processEvents() workaround in SendCoinsDialog had the (accidental?) side effect of making it act like a DirectConnection when sent from the GUI.

The fix for that is either to change the connection in the TransactionTableModel into AutoConnection (which goes against the comment) or do a qApp->processEvents() in the test so that the model updates in the next event loop.

I chose the latter method.

[furszy]

This is most likely because we are creating the widget in a separate thread.

Try this:

diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp
--- a/src/qt/sendcoinsdialog.cpp	(revision 114d738830368dc155f27a72128adf91b4fffd37)
+++ b/src/qt/sendcoinsdialog.cpp	(date 1672923728012)
@@ -600,11 +600,9 @@
     entry->clear();
     entry->setFocus();
     ui->scrollAreaWidgetContents->resize(ui->scrollAreaWidgetContents->sizeHint());
-    QMetaObject::invokeMethod(ui->scrollArea, [this] {
-        if (ui->scrollArea->verticalScrollBar()) {
-            ui->scrollArea->verticalScrollBar()->setValue(ui->scrollArea->verticalScrollBar()->maximum());
-        }
-    }, Qt::QueuedConnection);
+    if (ui->scrollArea->verticalScrollBar()) {
+        ui->scrollArea->verticalScrollBar()->setValue(ui->scrollArea->verticalScrollBar()->maximum());
+    }
 
     updateTabsAndLabels();
     return entry;
@@ -697,6 +695,13 @@
     return true;
 }
 
+void SendCoinsDialog::showEvent(QShowEvent* event)
+{
+    if (ui->scrollArea->verticalScrollBar()) {
+        ui->scrollArea->verticalScrollBar()->setValue(ui->scrollArea->verticalScrollBar()->maximum());
+    }
+}
+
 void SendCoinsDialog::setBalance(const interfaces::WalletBalances& balances)
 {
     if(model && model->getOptionsModel())
Index: src/qt/sendcoinsdialog.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h
--- a/src/qt/sendcoinsdialog.h	(revision 114d738830368dc155f27a72128adf91b4fffd37)
+++ b/src/qt/sendcoinsdialog.h	(date 1672923728023)
@@ -49,6 +49,8 @@
     void pasteEntry(const SendCoinsRecipient &rv);
     bool handlePaymentRequest(const SendCoinsRecipient &recipient);
 
+    void showEvent(QShowEvent* event) override;
+
 public Q_SLOTS:
     void clear();
     void reject() override;

[john-moffett]

Thank you for the suggestion!

While this (also) fixes the first segfault, it doesn't achieve the intended scrolling behavior (at least on my macOS setup).

Before clicking 'Add Recipient':

After:

Whereas the original workaround (and my workaround of the workaround) gives this "After":

It seems to be a fairly common issue without an ideal solution. There are more involved solutions, like subscribing to rangeChanged signals, but those would require much more logic (eg - ignore those when manually resizing the viewport, etc.).

[furszy]

hmm true, ok. Sounds good to go with the simplest fix here.
Would be good to add a comment above so we don't forget the rationale behind it.

[john-moffett]

Good call. Adding.

[furszy]

Code ACK 114d738 with the nuance of not being able to reproduce the crash on master on my system.

[furszy]

Thanks for the added comment.
Should squash the commits, check https://github.com/bitcoin-core/gui/blob/master/CONTRIBUTING.md#squashing-commits

[hebasto]

cc @promag

[hebasto]

Concept ACK.

[hebasto]

ACK 9a1d73f, I have reviewed the code and it looks OK.

[furszy]

ACK 9a1d73f