Fix segfault when shutdown during wallet open #693

1. 
   john-moffett commented at 0:16 am on January 2, 2023: contributor

   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:

   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_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.

2. 
   DrahtBot commented at 0:16 am on January 2, 2023: 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	hebasto, furszy

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

3. jarolrod added the label Bug on Jan 3, 2023
4. john-moffett force-pushed on Jan 3, 2023
5. 
   john-moffett commented at 3:23 pm on January 3, 2023: contributor

   Pushed an update to address the failing tests.

   They failed because the TransactionTableModel gets updated on a QueuedConnection:

   https://github.com/bitcoin-core/gui/blob/d8bdee0fc889def7c5f5076da13db4fce0a3728a/src/qt/transactiontablemodel.cpp#L73-L76

   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.

6. in src/qt/sendcoinsdialog.cpp:611 in 114d738830 outdated

   606-        bar->setSliderPosition(bar->maximum());
   607+    QMetaObject::invokeMethod(ui->scrollArea, [this] {
   608+        if (ui->scrollArea->verticalScrollBar()) {
   609+            ui->scrollArea->verticalScrollBar()->setValue(ui->scrollArea->verticalScrollBar()->maximum());
   610+        }
   611+    }, Qt::QueuedConnection);
   

   ---

   ---

   

   furszy commented at 1:22 pm on January 5, 2023:

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

   Try this:

    0diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp
    1--- a/src/qt/sendcoinsdialog.cpp	(revision 114d738830368dc155f27a72128adf91b4fffd37)
    2+++ b/src/qt/sendcoinsdialog.cpp	(date 1672923728012)
    3@@ -600,11 +600,9 @@
    4     entry->clear();
    5     entry->setFocus();
    6     ui->scrollAreaWidgetContents->resize(ui->scrollAreaWidgetContents->sizeHint());
    7-    QMetaObject::invokeMethod(ui->scrollArea, [this] {
    8-        if (ui->scrollArea->verticalScrollBar()) {
    9-            ui->scrollArea->verticalScrollBar()->setValue(ui->scrollArea->verticalScrollBar()->maximum());
   10-        }
   11-    }, Qt::QueuedConnection);
   12+    if (ui->scrollArea->verticalScrollBar()) {
   13+        ui->scrollArea->verticalScrollBar()->setValue(ui->scrollArea->verticalScrollBar()->maximum());
   14+    }
   15 
   16     updateTabsAndLabels();
   17     return entry;
   18@@ -697,6 +695,13 @@
   19     return true;
   20 }
   21 
   22+void SendCoinsDialog::showEvent(QShowEvent* event)
   23+{
   24+    if (ui->scrollArea->verticalScrollBar()) {
   25+        ui->scrollArea->verticalScrollBar()->setValue(ui->scrollArea->verticalScrollBar()->maximum());
   26+    }
   27+}
   28+
   29 void SendCoinsDialog::setBalance(const interfaces::WalletBalances& balances)
   30 {
   31     if(model && model->getOptionsModel())
   32Index: src/qt/sendcoinsdialog.h
   33IDEA additional info:
   34Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
   35<+>UTF-8
   36===================================================================
   37diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h
   38--- a/src/qt/sendcoinsdialog.h	(revision 114d738830368dc155f27a72128adf91b4fffd37)
   39+++ b/src/qt/sendcoinsdialog.h	(date 1672923728023)
   40@@ -49,6 +49,8 @@
   41     void pasteEntry(const SendCoinsRecipient &rv);
   42     bool handlePaymentRequest(const SendCoinsRecipient &recipient);
   43 
   44+    void showEvent(QShowEvent* event) override;
   45+
   46 public Q_SLOTS:
   47     void clear();
   48     void reject() override;
   

   ---

   

   john-moffett commented at 2:08 pm on January 5, 2023:

   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 commented at 12:11 pm on January 6, 2023:

   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 commented at 3:04 pm on January 6, 2023:

   Good call. Adding.
7. 
   furszy commented at 12:37 pm on January 6, 2023: member

   Code ACK 114d7388 with the nuance of not being able to reproduce the crash on master on my system.
8. 
   furszy commented at 10:50 pm on January 8, 2023: member

   Thanks for the added comment. Should squash the commits, check https://github.com/bitcoin-core/gui/blob/master/CONTRIBUTING.md#squashing-commits
9. Fix segfault when shutdown during wallet open

   If you open a wallet and send a shutdown signal during
   that process, the GUI will segfault due to some queued
   wallet events happening after the wallet controller
   is deleted. This is a minimal fix for those issues.

   9a1d73fdff
10. john-moffett force-pushed on Jan 9, 2023
11. 
    hebasto commented at 4:47 pm on January 16, 2023: member

    cc @promag
12. 
    hebasto commented at 4:09 pm on January 17, 2023: member

    Concept ACK.
13. hebasto approved
14. 
    hebasto commented at 10:46 am on March 27, 2023: member

    ACK 9a1d73fdffa4f35e33bc187ac9b3afbba28b1950, I have reviewed the code and it looks OK.
15. DrahtBot requested review from furszy on Mar 27, 2023
16. 
    furszy commented at 2:44 pm on March 27, 2023: member

    ACK https://github.com/bitcoin-core/gui/commit/9a1d73fdffa4f35e33bc187ac9b3afbba28b1950
17. hebasto merged this on Mar 27, 2023
18. hebasto closed this on Mar 27, 2023

    ---

19. sidhujag referenced this in commit 3618cbd691 on Mar 27, 2023
20. bitcoin-core locked this on Mar 26, 2024

Contributors
john-moffett DrahtBot furszy hebasto

Labels
Bug