Merge bitcoin-core/gui#260: Handle exceptions instead of crash

b8e5d0d3fe3386807d47f50d13ac34fcd2a538fd qt: Handle exceptions in SendCoinsDialog::sendButtonClicked slot (Hennadii Stepanov)
1ac2bc7ac070dfd1df1872d759540b0c92495301 qt: Handle exceptions in TransactionView::bumpFee slot (Hennadii Stepanov)
bc00e13bc800863641b3e1e64732a38418d3022f qt: Handle exceptions in WalletModel::pollBalanceChanged slot (Hennadii Stepanov)
eb6156ba1b4c303eb597e3fc4a9e42ce45e6e78d qt: Handle exceptions in BitcoinGUI::addWallet slot (Hennadii Stepanov)
f7e260a471010e2d656fbc5ea8c310f6d94c26b9 qt: Add GUIUtil::ExceptionSafeConnect function (Hennadii Stepanov)
64a8755af396f1c2791018510e22b58114e68594 qt: Add BitcoinApplication::handleNonFatalException function (Hennadii Stepanov)
af7e365b1516d660d271475fdfe0c20ae09e66a8 qt: Make PACKAGE_BUGREPORT link clickable (Hennadii Stepanov)

Pull request description:

  This PR is an alternative to https://github.com/bitcoin/bitcoin/pull/18897, and is based on Russ' [idea](https://github.com/bitcoin/bitcoin/pull/18897#pullrequestreview-418703664):
  > IMO it would be nice to have a followup PR that eliminated the one-line forwarding methods ...

  Related issues
  - #91
  - https://github.com/bitcoin/bitcoin/issues/18643

  Qt docs: https://doc.qt.io/qt-5.12/exceptionsafety.html#exceptions-in-client-code

  With this PR the GUI handles the wallet-related exception, and:
  - display it to a user:

  ![Screenshot from 2021-04-01 02-55-59](https://user-images.githubusercontent.com/32963518/113226183-33ff8480-9298-11eb-8fe6-2168834ab09a.png)

  - prints a message to `stderr`:
  ```

  ************************
  EXCEPTION: 18NonFatalCheckError
  wallet/wallet.cpp:2677 (IsCurrentForAntiFeeSniping)
  Internal bug detected: '!chain.findBlock(block_hash, FoundBlock().time(block_time))'
  You may report this issue here: https://github.com/bitcoin/bitcoin/issues

  bitcoin in QPushButton->SendCoinsDialog

  ```

  - writes a message to the `debug.log`
  - and, if the exception is a non-fatal error, leaves the main window running.

ACKs for top commit:
  laanwj:
    Code review ACK b8e5d0d3fe3386807d47f50d13ac34fcd2a538fd
  ryanofsky:
    Code review ACK b8e5d0d3fe3386807d47f50d13ac34fcd2a538fd. This is great! I think more improvements are possible but implementation is very clean and I love how targeted each commit is. Changes since last review: adding more explanatory text, making links clickable, reorganizing.

Tree-SHA512: a9f2a2ee8e64b993b0dbc454edcbc39c68c8852abb5dc1feb58f601c0e0e8014dca81c72733aa3fb07b619c6f49b823ed20c7d79cc92088a3abe040ed2149727

src/qt/bitcoin.cpp

@@ -47,11 +47,13 @@
 
 #include <QApplication>
 #include <QDebug>
+#include <QLatin1String>
 #include <QLibraryInfo>
 #include <QLocale>
 #include <QMessageBox>
 #include <QProcess>
 #include <QSettings>
+#include <QStringBuilder>
 #include <QThread>
 #include <QTimer>
 #include <QTranslator>
@@ -491,10 +493,23 @@ void BitcoinApplication::shutdownResult()
 
 void BitcoinApplication::handleRunawayException(const QString &message)
 {
-    QMessageBox::critical(nullptr, "Runaway exception", BitcoinGUI::tr("A fatal error occurred. %1 can no longer continue safely and will quit.").arg(PACKAGE_NAME) + QString("<br><br>") + message);
+    QMessageBox::critical(
+        nullptr, tr("Runaway exception"),
+        tr("A fatal error occurred. %1 can no longer continue safely and will quit.").arg(PACKAGE_NAME) %
+        QLatin1String("<br><br>") % GUIUtil::MakeHtmlLink(message, PACKAGE_BUGREPORT));
     ::exit(EXIT_FAILURE);
 }
 
+void BitcoinApplication::handleNonFatalException(const QString& message)
+{
+    assert(QThread::currentThread() == thread());
+    QMessageBox::warning(
+        nullptr, tr("Internal error"),
+        tr("An internal error occurred. %1 will attempt to continue safely. This is "
+           "an unexpected bug which can be reported as described below.").arg(PACKAGE_NAME) %
+        QLatin1String("<br><br>") % GUIUtil::MakeHtmlLink(message, PACKAGE_BUGREPORT));
+}
+
 WId BitcoinApplication::getMainWinId() const
 {
     if (!window)

src/qt/bitcoin.h

@@ -96,6 +96,12 @@ public Q_SLOTS:
     /// Handle runaway exceptions. Shows a message box with the problem and quits the program.
     void handleRunawayException(const QString &message);
 
+    /**
+     * A helper function that shows a message box
+     * with details about a non-fatal exception.
+     */
+    void handleNonFatalException(const QString& message);
+
 Q_SIGNALS:
     void requestedInitialize();
     void requestedRestart(QStringList args);

src/qt/bitcoingui.cpp

@@ -891,7 +891,7 @@ void BitcoinGUI::setWalletController(WalletController* wallet_controller)
     m_open_wallet_action->setEnabled(true);
     m_open_wallet_action->setMenu(m_open_wallet_menu);
 
-    connect(wallet_controller, &WalletController::walletAdded, this, &BitcoinGUI::addWallet);
+    GUIUtil::ExceptionSafeConnect(wallet_controller, &WalletController::walletAdded, this, &BitcoinGUI::addWallet);
     connect(wallet_controller, &WalletController::walletRemoved, this, &BitcoinGUI::removeWallet);
 
     for (WalletModel* wallet_model : m_wallet_controller->getOpenWallets()) {

src/qt/guiutil.cpp

@@ -51,6 +51,7 @@
 #include <QFontMetrics>
 #include <QGuiApplication>
 #include <QKeyEvent>
+#include <QLatin1String>
 #include <QLineEdit>
 #include <QList>
 #include <QLocale>
@@ -63,6 +64,7 @@
 #include <QShortcut>
 #include <QSize>
 #include <QString>
+#include <QStringBuilder>
 #include <QTextDocument> // for Qt::mightBeRichText
 #include <QThread>
 #include <QTimer>
@@ -1858,4 +1860,22 @@ QImage GetImage(const QLabel* label)
 #endif
 }
 
+QString MakeHtmlLink(const QString& source, const QString& link)
+{
+    return QString(source).replace(
+        link,
+        QLatin1String("<a href=\"") % link % QLatin1String("\">") % link % QLatin1String("</a>"));
+}
+
+void PrintSlotException(
+    const std::exception* exception,
+    const QObject* sender,
+    const QObject* receiver)
+{
+    std::string description = sender->metaObject()->className();
+    description += "->";
+    description += receiver->metaObject()->className();
+    PrintExceptionContinue(std::make_exception_ptr(exception), description.c_str());
+}
+
 } // namespace GUIUtil

src/qt/guiutil.h

@@ -9,18 +9,23 @@
 #include <fs.h>
 #include <qt/guiconstants.h>
 #include <netaddress.h>
+#include <util/check.h>
 
+#include <QApplication>
 #include <QEvent>
 #include <QHeaderView>
 #include <QItemDelegate>
 #include <QMessageBox>
+#include <QMetaObject>
 #include <QObject>
 #include <QProgressBar>
 #include <QString>
 #include <QTableView>
 #include <QLabel>
 
+#include <cassert>
 #include <chrono>
+#include <utility>
 
 class QValidatedLineEdit;
 class OptionsModel;
@@ -520,6 +525,58 @@ namespace GUIUtil
         QObject::connect(&source, &QObject::destroyed, object, std::forward<Fn>(function), connection);
     }
 
+    /**
+     * Replaces a plain text link with an HTML tagged one.
+     */
+    QString MakeHtmlLink(const QString& source, const QString& link);
+
+    void PrintSlotException(
+        const std::exception* exception,
+        const QObject* sender,
+        const QObject* receiver);
+
+    /**
+     * A drop-in replacement of QObject::connect function
+     * (see: https://doc.qt.io/qt-5/qobject.html#connect-3), that
+     * guaranties that all exceptions are handled within the slot.
+     *
+     * NOTE: This function is incompatible with Qt private signals.
+     */
+    template <typename Sender, typename Signal, typename Receiver, typename Slot>
+    auto ExceptionSafeConnect(
+        Sender sender, Signal signal, Receiver receiver, Slot method,
+        Qt::ConnectionType type = Qt::AutoConnection)
+    {
+        return QObject::connect(
+            sender, signal, receiver,
+            [sender, receiver, method](auto&&... args) {
+                bool ok{true};
+                try {
+                    (receiver->*method)(std::forward<decltype(args)>(args)...);
+                } catch (const NonFatalCheckError& e) {
+                    PrintSlotException(&e, sender, receiver);
+                    ok = QMetaObject::invokeMethod(
+                        qApp, "handleNonFatalException",
+                        blockingGUIThreadConnection(),
+                        Q_ARG(QString, QString::fromStdString(e.what())));
+                } catch (const std::exception& e) {
+                    PrintSlotException(&e, sender, receiver);
+                    ok = QMetaObject::invokeMethod(
+                        qApp, "handleRunawayException",
+                        blockingGUIThreadConnection(),
+                        Q_ARG(QString, QString::fromStdString(e.what())));
+                } catch (...) {
+                    PrintSlotException(nullptr, sender, receiver);
+                    ok = QMetaObject::invokeMethod(
+                        qApp, "handleRunawayException",
+                        blockingGUIThreadConnection(),
+                        Q_ARG(QString, "Unknown failure occurred."));
+                }
+                assert(ok);
+            },
+            type);
+    }
+
 } // namespace GUIUtil
 
 #endif // BITCOIN_QT_GUIUTIL_H

src/qt/sendcoinsdialog.cpp

@@ -153,6 +153,8 @@ SendCoinsDialog::SendCoinsDialog(bool _fCoinJoin, QWidget* parent) :
     }
 
     m_coin_control->UseCoinJoin(_fCoinJoin);
+
+    GUIUtil::ExceptionSafeConnect(ui->sendButton, &QPushButton::clicked, this, &SendCoinsDialog::sendButtonClicked);
 }
 
 void SendCoinsDialog::setClientModel(ClientModel *_clientModel)
@@ -459,7 +461,7 @@ bool SendCoinsDialog::send(const QList<SendCoinsRecipient>& recipients, QString&
     return true;
 }
 
-void SendCoinsDialog::on_sendButton_clicked()
+void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
 {
     if(!model || !model->getOptionsModel())
         return;

src/qt/sendcoinsdialog.h

@@ -83,7 +83,7 @@ private:
     void updateCoinControlState(CCoinControl& ctrl);
 
 private Q_SLOTS:
-    void on_sendButton_clicked();
+    void sendButtonClicked(bool checked);
     void on_buttonChooseFee_clicked();
     void on_buttonMinimizeFee_clicked();
     void removeEntry(SendCoinsEntry* entry);

src/qt/test/wallettests.cpp

@@ -68,7 +68,7 @@ uint256 SendCoins(CWallet& wallet, SendCoinsDialog& sendCoinsDialog, const CTxDe
         if (status == CT_NEW) txid = hash;
     }));
     ConfirmSend();
-    bool invoked = QMetaObject::invokeMethod(&sendCoinsDialog, "on_sendButton_clicked");
+    bool invoked = QMetaObject::invokeMethod(&sendCoinsDialog, "sendButtonClicked", Q_ARG(bool, false));
     assert(invoked);
     return txid;
 }

src/qt/walletmodel.cpp

@@ -12,6 +12,7 @@
 #include <qt/addresstablemodel.h>
 #include <qt/clientmodel.h>
 #include <qt/guiconstants.h>
+#include <qt/guiutil.h>
 #include <qt/optionsmodel.h>
 #include <qt/paymentserver.h>
 #include <qt/recentrequeststablemodel.h>
@@ -68,7 +69,10 @@ WalletModel::~WalletModel()
 void WalletModel::startPollBalance()
 {
     // This timer will be fired repeatedly to update the balance
-    connect(timer, &QTimer::timeout, this, &WalletModel::pollBalanceChanged);
+    // Since the QTimer::timeout is a private signal, it cannot be used
+    // in the GUIUtil::ExceptionSafeConnect directly.
+    connect(timer, &QTimer::timeout, this, &WalletModel::timerTimeout);
+    GUIUtil::ExceptionSafeConnect(this, &WalletModel::timerTimeout, this, &WalletModel::pollBalanceChanged);
     timer->start(MODEL_UPDATE_DELAY);
 }
 

src/qt/walletmodel.h

@@ -233,6 +233,8 @@ Q_SIGNALS:
     // Notify that there are now keys in the keypool
     void canGetAddressesChanged();
 
+    void timerTimeout();
+
 public Q_SLOTS:
     /* Starts a timer to periodically update the balance */
     void startPollBalance();