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

9a1d73f 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](bitcoin#593 (comment)) 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 9a1d73f, I have reviewed the code and it looks OK.
  furszy:
    ACK bitcoin-core/gui@9a1d73f

Tree-SHA512: a1b94676eb2fcb7606e68fab443b1565b4122aab93c35382b561842a049f4b43fecc459535370d67a64d6ebc4bcec0ebcda981fff633ebd41bdba6f7093ea540

Author: hebasto

src/qt/bitcoingui.cpp

@@ -899,6 +899,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();
@@ -911,7 +915,7 @@ WalletController* BitcoinGUI::getWalletController()
 
 void BitcoinGUI::addWallet(WalletModel* walletModel)
 {
-    if (!walletFrame) return;
+    if (!walletFrame || !m_wallet_controller) return;
 
     WalletView* wallet_view = new WalletView(walletModel, walletFrame);
     if (!walletFrame->addView(wallet_view)) return;
@@ -959,7 +963,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) {

src/qt/sendcoinsdialog.cpp

@@ -604,10 +604,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;

src/qt/test/wallettests.cpp

@@ -163,6 +163,8 @@ void TestGUI(interfaces::Node& node)
     QCOMPARE(transactionTableModel->rowCount({}), 105);
     uint256 txid1 = SendCoins(*wallet.get(), sendCoinsDialog, PKHash(), 5 * COIN);
     uint256 txid2 = SendCoins(*wallet.get(), sendCoinsDialog, PKHash(), 10 * COIN);
+    // 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());