From adea52a5fe8530a6260d10246ab7443d143e9790 Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Wed, 14 Apr 2021 10:22:57 +0200 Subject: [PATCH] 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 | 17 ++++++++++- src/qt/bitcoin.h | 6 ++++ src/qt/bitcoingui.cpp | 2 +- src/qt/guiutil.cpp | 20 +++++++++++++ src/qt/guiutil.h | 57 +++++++++++++++++++++++++++++++++++++ src/qt/sendcoinsdialog.cpp | 4 ++- src/qt/sendcoinsdialog.h | 2 +- src/qt/test/wallettests.cpp | 2 +- src/qt/walletmodel.cpp | 6 +++- src/qt/walletmodel.h | 2 ++ 10 files changed, 112 insertions(+), 6 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 8884086013..a9be5c5e30 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -47,11 +47,13 @@ #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -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("

") + 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("

") % 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("

") % GUIUtil::MakeHtmlLink(message, PACKAGE_BUGREPORT)); +} + WId BitcoinApplication::getMainWinId() const { if (!window) diff --git a/src/qt/bitcoin.h b/src/qt/bitcoin.h index 52ec1cc280..76ebdb4486 100644 --- a/src/qt/bitcoin.h +++ b/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); diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index eb0accfdb3..561fefefe1 100644 --- a/src/qt/bitcoingui.cpp +++ b/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()) { diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp index 705e3a952a..e8c13d8906 100644 --- a/src/qt/guiutil.cpp +++ b/src/qt/guiutil.cpp @@ -51,6 +51,7 @@ #include #include #include +#include #include #include #include @@ -63,6 +64,7 @@ #include #include #include +#include #include // for Qt::mightBeRichText #include #include @@ -1858,4 +1860,22 @@ QImage GetImage(const QLabel* label) #endif } +QString MakeHtmlLink(const QString& source, const QString& link) +{ + return QString(source).replace( + link, + QLatin1String("") % link % QLatin1String("")); +} + +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 diff --git a/src/qt/guiutil.h b/src/qt/guiutil.h index b5095becd2..d53ac4522c 100644 --- a/src/qt/guiutil.h +++ b/src/qt/guiutil.h @@ -9,18 +9,23 @@ #include #include #include +#include +#include #include #include #include #include +#include #include #include #include #include #include +#include #include +#include class QValidatedLineEdit; class OptionsModel; @@ -520,6 +525,58 @@ namespace GUIUtil QObject::connect(&source, &QObject::destroyed, object, std::forward(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 + 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(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 6c350a797f..f52b45b17e 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/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& recipients, QString& 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 c8766be9e8..ece928b84b 100644 --- a/src/qt/sendcoinsdialog.h +++ b/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); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 8cae9eaf2d..1da85c9803 100644 --- a/src/qt/test/wallettests.cpp +++ b/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; } diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 08e783ad91..2b33db9a66 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -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); } diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 816a745c6c..8194c141a8 100644 --- a/src/qt/walletmodel.h +++ b/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();