Merge #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

12 files changed, 114 insertions, 9 deletions

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index fc6d0febc2..de71b7dea7 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -45,10 +45,12 @@
 #include <QApplication>
 #include <QDebug>
 #include <QFontDatabase>
+#include <QLatin1String>
 #include <QLibraryInfo>
 #include <QLocale>
 #include <QMessageBox>
 #include <QSettings>
+#include <QStringBuilder>
 #include <QThread>
 #include <QTimer>
 #include <QTranslator>
@@ -417,10 +419,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)
diff --git a/src/qt/bitcoin.h b/src/qt/bitcoin.h
index 69e0a5921e..5fd6bd607f 100644
--- a/src/qt/bitcoin.h
+++ b/src/qt/bitcoin.h
@@ -99,6 +99,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 requestedShutdown();
diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index 6677c9e3b5..3abca7dcff 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -654,7 +654,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()) {
diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp
index b4afdbcc22..0e91f9f385 100644
--- a/src/qt/guiutil.cpp
+++ b/src/qt/guiutil.cpp
@@ -42,6 +42,7 @@
 #include <QGuiApplication>
 #include <QJsonObject>
 #include <QKeyEvent>
+#include <QLatin1String>
 #include <QLineEdit>
 #include <QList>
 #include <QLocale>
@@ -54,6 +55,7 @@
 #include <QShortcut>
 #include <QSize>
 #include <QString>
+#include <QStringBuilder>
 #include <QTextDocument> // for Qt::mightBeRichText
 #include <QThread>
 #include <QUrlQuery>
@@ -893,4 +895,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(exception, description.c_str());
+}
+
 } // namespace GUIUtil
diff --git a/src/qt/guiutil.h b/src/qt/guiutil.h
index 6395ec6abd..a1cf274354 100644
--- a/src/qt/guiutil.h
+++ b/src/qt/guiutil.h
@@ -9,18 +9,23 @@
 #include <fs.h>
 #include <net.h>
 #include <netaddress.h>
+#include <util/check.h>
 
+#include <QApplication>
 #include <QEvent>
 #include <QHeaderView>
 #include <QItemDelegate>
 #include <QLabel>
 #include <QMessageBox>
+#include <QMetaObject>
 #include <QObject>
 #include <QProgressBar>
 #include <QString>
 #include <QTableView>
 
+#include <cassert>
 #include <chrono>
+#include <utility>
 
 class QValidatedLineEdit;
 class SendCoinsRecipient;
@@ -327,6 +332,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
diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp
index 95e1ce2210..f0e720617c 100644
--- a/src/qt/sendcoinsdialog.cpp
+++ b/src/qt/sendcoinsdialog.cpp
@@ -129,6 +129,8 @@ SendCoinsDialog::SendCoinsDialog(const PlatformStyle *_platformStyle, QWidget *p
     ui->customFee->SetAllowEmpty(false);
     ui->customFee->setValue(settings.value("nTransactionFee").toLongLong());
     minimizeFeeSection(settings.value("fFeeSectionMinimized").toBool());
+
+    GUIUtil::ExceptionSafeConnect(ui->sendButton, &QPushButton::clicked, this, &SendCoinsDialog::sendButtonClicked);
 }
 
 void SendCoinsDialog::setClientModel(ClientModel *_clientModel)
@@ -375,7 +377,7 @@ bool SendCoinsDialog::PrepareSendText(QString& question_string, QString& informa
     return true;
 }
 
-void SendCoinsDialog::on_sendButton_clicked()
+void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
 {
     if(!model || !model->getOptionsModel())
         return;
diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h
index 4fc2f57cd6..3e276201ba 100644
--- a/src/qt/sendcoinsdialog.h
+++ b/src/qt/sendcoinsdialog.h
@@ -80,7 +80,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);
diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
index 1107c44dc9..03460cd6eb 100644
--- a/src/qt/test/wallettests.cpp
+++ b/src/qt/test/wallettests.cpp
@@ -73,7 +73,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;
 }
diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
index 42e08c6af7..1511410129 100644
--- a/src/qt/transactionview.cpp
+++ b/src/qt/transactionview.cpp
@@ -199,7 +199,7 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
     connect(transactionView, &QTableView::doubleClicked, this, &TransactionView::doubleClicked);
     connect(transactionView, &QTableView::customContextMenuRequested, this, &TransactionView::contextualMenu);
 
-    connect(bumpFeeAction, &QAction::triggered, this, &TransactionView::bumpFee);
+    GUIUtil::ExceptionSafeConnect(bumpFeeAction, &QAction::triggered, this, &TransactionView::bumpFee);
     connect(abandonAction, &QAction::triggered, this, &TransactionView::abandonTx);
     connect(copyAddressAction, &QAction::triggered, this, &TransactionView::copyAddress);
     connect(copyLabelAction, &QAction::triggered, this, &TransactionView::copyLabel);
@@ -424,7 +424,7 @@ void TransactionView::abandonTx()
     model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);
 }
 
-void TransactionView::bumpFee()
+void TransactionView::bumpFee([[maybe_unused]] bool checked)
 {
     if(!transactionView || !transactionView->selectionModel())
         return;
diff --git a/src/qt/transactionview.h b/src/qt/transactionview.h
index 35ada4aa7a..66350bdc02 100644
--- a/src/qt/transactionview.h
+++ b/src/qt/transactionview.h
@@ -99,7 +99,7 @@ private Q_SLOTS:
     void openThirdPartyTxUrl(QString url);
     void updateWatchOnlyColumn(bool fHaveWatchOnly);
     void abandonTx();
-    void bumpFee();
+    void bumpFee(bool checked);
 
 Q_SIGNALS:
     void doubleClicked(const QModelIndex&);
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 02254da3ce..c6dd0c2ad6 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -65,7 +65,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);
 }
 
diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h
index 9a3c3f2f66..4ca8643444 100644
--- a/src/qt/walletmodel.h
+++ b/src/qt/walletmodel.h
@@ -223,6 +223,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();