Merge bitcoin-core/gui#693: Fix segfault when shutdown during wallet open

9a1d73fdffa4f35e33bc187ac9b3afbba28b1950 Fix segfault when shutdown during wallet open (John Moffett)

Pull request description:

  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](https://github.com/bitcoin-core/gui/blob/e9262ea32a6e1d364fb7974844fadc36f931f8c6/src/qt/sendcoinsdialog.cpp#L603), which causes the main event loop to detect the shutdown (now that the modal window has been dismissed). The `WalletController` and all the `WalletModel`s are [deleted](https://github.com/bitcoin-core/gui/blob/65de8eeeca29e71378aa34602b287ab921b040e4/src/qt/bitcoin.cpp#L394-L401).
  6. Control returns to the `finish` method, which eventually tries to send a [signal](https://github.com/bitcoin-core/gui/blob/e9262ea32a6e1d364fb7974844fadc36f931f8c6/src/qt/sendcoinsdialog.cpp#L167) from a wallet model, but it's been deleted already (and the signal is sent from a now-[dangling](https://github.com/bitcoin-core/gui/blob/d8bdee0fc889def7c5f5076da13db4fce0a3728a/src/qt/walletview.cpp#L65) pointer).

  The simplest fix for that is to change the `qApp->processEvents()` into a `QueuedConnection` call. (The `qApp->processEvents() was a [workaround](https://github.com/bitcoin/bitcoin/pull/593#issuecomment-3050699) 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:

  https://github.com/bitcoin-core/gui/blob/65de8eeeca29e71378aa34602b287ab921b040e4/src/qt/bitcoin.cpp#L394-L401

  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:

  https://github.com/bitcoin-core/gui/blob/65de8eeeca29e71378aa34602b287ab921b040e4/src/qt/bitcoingui.cpp#L413

  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_ptr`s 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`.

ACKs for top commit:
  hebasto:
    ACK 9a1d73fdffa4f35e33bc187ac9b3afbba28b1950, I have reviewed the code and it looks OK.
  furszy:
    ACK https://github.com/bitcoin-core/gui/commit/9a1d73fdffa4f35e33bc187ac9b3afbba28b1950

Tree-SHA512: a1b94676eb2fcb7606e68fab443b1565b4122aab93c35382b561842a049f4b43fecc459535370d67a64d6ebc4bcec0ebcda981fff633ebd41bdba6f7093ea540

3 files changed, 17 insertions, 6 deletions

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index 0eae7d3e79..d26ef52eb4 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -689,6 +689,10 @@ void BitcoinGUI::setWalletController(WalletController* wallet_controller)
 
     GUIUtil::ExceptionSafeConnect(wallet_controller, &WalletController::walletAdded, this, &BitcoinGUI::addWallet);
     connect(wallet_controller, &WalletController::walletRemoved, this, &BitcoinGUI::removeWallet);
+    connect(wallet_controller, &WalletController::destroyed, this, [this] {
+        // wallet_controller gets destroyed manually, but it leaves our member copy dangling
+        m_wallet_controller = nullptr;
+    });
 
     auto activity = new LoadWalletsActivity(m_wallet_controller, this);
     activity->load();
@@ -701,7 +705,7 @@ WalletController* BitcoinGUI::getWalletController()
 
 void BitcoinGUI::addWallet(WalletModel* walletModel)
 {
-    if (!walletFrame) return;
+    if (!walletFrame || !m_wallet_controller) return;
 
     WalletView* wallet_view = new WalletView(walletModel, platformStyle, walletFrame);
     if (!walletFrame->addView(wallet_view)) return;
@@ -753,7 +757,7 @@ void BitcoinGUI::removeWallet(WalletModel* walletModel)
 
 void BitcoinGUI::setCurrentWallet(WalletModel* wallet_model)
 {
-    if (!walletFrame) return;
+    if (!walletFrame || !m_wallet_controller) return;
     walletFrame->setCurrentWallet(wallet_model);
     for (int index = 0; index < m_wallet_selector->count(); ++index) {
         if (m_wallet_selector->itemData(index).value<WalletModel*>() == wallet_model) {
diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp
index 89dd0ada62..1eba4d1b71 100644
--- a/src/qt/sendcoinsdialog.cpp
+++ b/src/qt/sendcoinsdialog.cpp
@@ -596,10 +596,15 @@ SendCoinsEntry *SendCoinsDialog::addEntry()
     entry->clear();
     entry->setFocus();
     ui->scrollAreaWidgetContents->resize(ui->scrollAreaWidgetContents->sizeHint());
-    qApp->processEvents();
-    QScrollBar* bar = ui->scrollArea->verticalScrollBar();
-    if(bar)
-        bar->setSliderPosition(bar->maximum());
+
+    // Scroll to the newly added entry on a QueuedConnection because Qt doesn't
+    // adjust the scroll area and scrollbar immediately when the widget is added.
+    // Invoking on a DirectConnection will only scroll to the second-to-last entry.
+    QMetaObject::invokeMethod(ui->scrollArea, [this] {
+        if (ui->scrollArea->verticalScrollBar()) {
+            ui->scrollArea->verticalScrollBar()->setValue(ui->scrollArea->verticalScrollBar()->maximum());
+        }
+    }, Qt::QueuedConnection);
 
     updateTabsAndLabels();
     return entry;
diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
index 62f2019438..be5bcbbd54 100644
--- a/src/qt/test/wallettests.cpp
+++ b/src/qt/test/wallettests.cpp
@@ -212,6 +212,8 @@ void TestGUI(interfaces::Node& node)
     QCOMPARE(transactionTableModel->rowCount({}), 105);
     uint256 txid1 = SendCoins(*wallet.get(), sendCoinsDialog, PKHash(), 5 * COIN, /*rbf=*/false);
     uint256 txid2 = SendCoins(*wallet.get(), sendCoinsDialog, PKHash(), 10 * COIN, /*rbf=*/true);
+    // Transaction table model updates on a QueuedConnection, so process events to ensure it's updated.
+    qApp->processEvents();
     QCOMPARE(transactionTableModel->rowCount({}), 107);
     QVERIFY(FindTx(*transactionTableModel, txid1).isValid());
     QVERIFY(FindTx(*transactionTableModel, txid2).isValid());