qt: Handle exceptions instead of crash

hebasto commented at 12:01 pm on May 6, 2020: member

Related issue #18643.

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

Screenshot from 2020-05-06 18-16-51

and, if the exception is a non-fatal error, leaves the main window running.

fanquake added the label GUI on May 6, 2020

DrahtBot commented at 12:08 pm on May 6, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17908 (qt: Remove QFont warnings with QT_QPA_PLATFORM=minimal by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke commented at 1:11 pm on May 6, 2020: member

Do I understand correctly that every gui element needs to have this handler if a crash wants to be avoided?

I.e. every dialog or window that could possibly pop up and just putting this catch in the “main gui” won’t work?

MarcoFalke commented at 1:11 pm on May 6, 2020: member

Concept ACK either way

hebasto commented at 1:14 pm on May 6, 2020: member

Do I understand correctly that every gui element needs to have this handler if a crash wants to be avoided?

I.e. every dialog or window that could possibly pop up and just putting this catch in the “main gui” won’t work?

Every slot.

See: https://doc.qt.io/qt-5/exceptionsafety.html#signals-and-slots

MarcoFalke commented at 1:19 pm on May 6, 2020: member

So currently it is undefined behaviour. Wow, qt is scary.

Slightly stronger concept ACK, but this also makes we want to remove qt.

hebasto commented at 1:21 pm on May 6, 2020: member

So currently it is undefined behaviour. Wow, qt is scary.

Slightly stronger concept ACK, but this also makes we want to remove qt.

Unrelated, but do we have a long-term plan of moving from Qt?

in src/qt/transactionview.cpp:455 in 40a622dd93 outdated

448+{
449+    try {
450+        bumpFeeHelper();
451+    } catch (const NonFatalCheckError& e) {
452+        Q_EMIT nonFatalCheckError(QString::fromStdString(e.what()));
453+    }

ryanofsky commented at 2:16 pm on May 6, 2020:

No reason to tie this to to the CHECK_NONFATAL implementation or to have exceptions pass through QT code which by design does not use them. Would change this to not be tied to a specific exception type:

0try {
1    bumpFeeHelper();
2} catch (const std::exception& e) {
3    Q_EMIT runawayException(QString::fromStdString(e.what()));
4} catch (...) {
5    Q_EMIT runawayException("Unknown failure occurred");
6}

It looks like other code could stay the same except to renaming nonFatalCheckError to runawayException

hebasto commented at 3:14 pm on May 6, 2020:

Updated.

in src/qt/bitcoingui.cpp:1106 in 40a622dd93 outdated

1100@@ -1101,6 +1101,11 @@ void BitcoinGUI::message(const QString& title, QString message, unsigned int sty
1101     }
1102 }
1103 
1104+void BitcoinGUI::handleNonFatalCheckError(const QString& error)
1105+{
1106+    QMessageBox::warning(nullptr, "NonFatalCheckError exception", error);

ryanofsky commented at 2:23 pm on May 6, 2020:

Would change dialog title to something more understandable like “Internal error”, since if this triggers something’s gone wrong like memory corruption or a bug triggered in the code

hebasto commented at 3:15 pm on May 6, 2020:

Updated.

hebasto force-pushed on May 6, 2020

hebasto commented at 3:14 pm on May 6, 2020: member

Updated 40a622dd9374e8cba80c6150f56f9d43004d953a -> 44f7f519e5fc7aef6df12474ce3c9b8583e6f20b (pr18897.01 -> pr18897.02, diff):

addressed @ryanofsky’s comments

The OP and the screenshot have been updated.

hebasto renamed this:
~~gui: Handle NonFatalCheckError instead of crash~~
gui: Handle exceptions instead of crash
on May 6, 2020

hebasto marked this as a draft on May 6, 2020

ryanofsky commented at 4:34 pm on May 6, 2020: member

This looks pretty reasonable, and I’d ack this if not a draft PR.

One thing that might be nice is a generic exception catcher QObject that emits a handleRunawayException signal. This way the signal doesn’t need to be declared multiple places and repeated boilerplate for catching exceptions could go away. So something like:

0m_exception_catcher->exec([&]{ sendButtonClickedHelper(); });

could replace:

0try {
1    sendButtonClickedHelper();
2} catch (const std::exception& e) {
3    Q_EMIT runawayException(QString::fromStdString(e.what()));
4} catch (...) {
5    Q_EMIT runawayException(QString("Unknown failure occurred."));
6}

in src/qt/sendcoinsdialog.cpp:470 in 44f7f519e5 outdated

461@@ -461,6 +462,18 @@ void SendCoinsDialog::on_sendButton_clicked()
462     m_current_transaction.reset();
463 }
464 
465+void SendCoinsDialog::on_sendButton_clicked()
466+{
467+    try {
468+        sendButtonClickedHelper();
469+    } catch (const std::exception& e) {
470+        Q_EMIT runawayException(QString::fromStdString(e.what()));

promag commented at 9:39 pm on May 6, 2020:

Why emit a signal? No one is interested in this signal right? Could just call another slot in a generic catcher, like:

0template <typename F> void shieldException(F&& f) {
1    try {
2        f();
3    } catch (...) {
4        QMetaObject::invokeMethod(qApp, "messageException", Qt::QueuedConnection,
5            Q_ARG(QString, exception_message), ...
6        );
7    }
8}

hebasto commented at 11:26 am on May 8, 2020:

Updated.

promag commented at 9:40 pm on May 6, 2020: member

There’s this approach:

 0bool BitcoinApplication::notify(QObject* receiver, QEvent* event)
 1{
 2    try {
 3        return QApplication::notify(receiver, event);
 4    } catch (const std::exception& ex) {
 5        // ..
 6    } catch (...) {
 7        // ..
 8    }
 9    return false;
10}

But the documentation says this will be deprecated.

So currently it is undefined behaviour. Wow, qt is scary.

Slightly stronger concept ACK, but this also makes we want to remove qt.

Why? This is by design, Qt doesn’t want to handle exceptions, what should it do anyway?

hebasto force-pushed on May 8, 2020

hebasto commented at 11:23 am on May 8, 2020: member

@ryanofsky @promag Thank you for your reviews.

Updated 44f7f519e5fc7aef6df12474ce3c9b8583e6f20b -> 24a90d54ed31fd3da787b95e29e2fca581235836 (pr18897.02 -> pr18897.03, diff).

Ready for review now :)

hebasto marked this as ready for review on May 8, 2020

hebasto renamed this:
~~gui: Handle exceptions instead of crash~~
qt: Handle exceptions instead of crash
on May 8, 2020

in src/qt/shieldexception.h:17 in 24a90d54ed outdated

12+#include <QMetaObject>
13+#include <QString>
14+
15+#include <cassert>
16+
17+template <typename FunctionType> void shieldException(FunctionType&& f) {

promag commented at 1:25 pm on May 8, 2020:

Move to src/qt/guiutil.h?

hebasto commented at 2:32 pm on May 8, 2020:

Updated.

hebasto force-pushed on May 8, 2020

hebasto commented at 2:32 pm on May 8, 2020: member

Updated 24a90d54ed31fd3da787b95e29e2fca581235836 -> 82529bc4ed82bcdf42428afef63c12ff37dd1dd4 (pr18897.03 -> pr18897.04, diff):

addressed @promag’s comment:

Move to src/qt/guiutil.h?

hebasto commented at 11:39 am on May 10, 2020: member

Also #18435 could be related.

DrahtBot added the label Needs rebase on May 11, 2020

hebasto force-pushed on May 11, 2020

hebasto commented at 7:15 pm on May 11, 2020: member

Rebased 82529bc4ed82bcdf42428afef63c12ff37dd1dd4 -> 1fb68b3c6cd3d153a5701b5e82f18b1f6bf96586 (pr18897.04 -> pr18897.05) due to the conflict with #18914.

DrahtBot removed the label Needs rebase on May 11, 2020

in src/qt/guiutil.h:295 in 1fb68b3c6c outdated

290+        bool ok = true;
291+        try {
292+            f();
293+        } catch (const NonFatalCheckError& e) {
294+            PrintExceptionContinue(&e, "Internal error");
295+            ok = QMetaObject::invokeMethod(qApp, "handleNonFatalException", Qt::DirectConnection, Q_ARG(QString, QString::fromStdString(e.what())));

promag commented at 11:15 pm on May 11, 2020:

Why can’t it be AutoConnection? That way this could be used in a “worker” slot.

Otherwise I’d assert the current thread is the GUI thread at the function begin.

hebasto commented at 5:37 am on May 12, 2020:

I’d keep Qt::DirectConnection to be sure the exception is handled in the same thread where it was thrown.

As QMessageBox in BitcoinApplication::handleNonFatalException() has its own event loop, I think it is safe to call shieldException() from a non-GUI thread, no?

promag commented at 9:35 pm on May 12, 2020:

has its own event loop, I think it is safe to call shieldException() from a non-GUI thread, no?

I don’t think so, only the main (GUI) thread should deal with widgets.

I’d keep Qt::DirectConnection to be sure the exception is handled in the same thread where it was thrown.

What you mean by handled? This is just catching the exception (which happens in the right thread) and then showing the message. If you want to block until the user presses OK then try GUIUtil::blockingGUIThreadConnection.

hebasto commented at 4:39 am on May 13, 2020:

I don’t think so, only the main (GUI) thread should deal with widgets.

You are right. Fixed.

jonasschnelli commented at 7:23 am on May 12, 2020: contributor

Concept ACK

qt: Add shieldException() function

Throwing an exception from a slot invoked by Qt's signal-slot connection
mechanism is considered undefined behavior, unless it is handled within
the slot. The shieldException() function should be used for exception
handling within slots.

553128b919

qt: Handle exceptions in SendCoinsDialog::on_sendButton_clicked slot a65c2f7eef

qt: Handle exceptions in TransactionView::bumpFee slot 8f9424329f

hebasto force-pushed on May 13, 2020

hebasto commented at 4:38 am on May 13, 2020: member

Updated 1fb68b3c6cd3d153a5701b5e82f18b1f6bf96586 -> 8f9424329f05ffa92a7421990c5c7320c7556a09 (pr18897.05 -> pr18897.06, diff):

addressed @promag’s comment:

I don’t think so, only the main (GUI) thread should deal with widgets.

in src/qt/guiutil.h:293 in 553128b919 outdated

288+     */
289+    template <typename FunctionType> void shieldException(FunctionType&& f) {
290+        bool ok = true;
291+        try {
292+            f();
293+        } catch (const NonFatalCheckError& e) {

ryanofsky commented at 9:49 pm on May 26, 2020:

In commit “qt: Add shieldException() function” (553128b91934a5e62c9827cd6260fec9562b5ec9)

Would drop this special case for NonFatalCheckError and simply catch std::exception below. NonFatalCheckError is really an internal implementation of the CHECK macro and most useful to unit tests. Exceptions aren’t valid in qt callbacks so NonFatalCheckError is no different here than any other exception. Just another unexpected condition that should never happen

in src/qt/guiutil.h:297 in 553128b919 outdated

292+            f();
293+        } catch (const NonFatalCheckError& e) {
294+            PrintExceptionContinue(&e, "Internal error");
295+            ok = QMetaObject::invokeMethod(qApp, "handleNonFatalException", blockingGUIThreadConnection(), Q_ARG(QString, QString::fromStdString(e.what())));
296+        } catch (const std::exception& e) {
297+            PrintExceptionContinue(&e, "Runaway exception");

ryanofsky commented at 9:53 pm on May 26, 2020:

In commit “qt: Add shieldException() function” (553128b91934a5e62c9827cd6260fec9562b5ec9)

Here and below would replace “Runaway exception” with “Internal error.” No reason to confuse users with implementation details and obscure jargon when it’s not providing useful information

ryanofsky approved

ryanofsky commented at 10:08 pm on May 26, 2020: member

Code review ACK 8f9424329f05ffa92a7421990c5c7320c7556a09. This looks very good and much simpler than before!

IMO it would be nice to have a followup PR that eliminated the one-line forwarding methods and just used shieldException directly at the call and signal connect sites. Alternately renaming s/Helper/Unsafe/ would be clearer than status quo. But best to just get rid of the indirections.

DrahtBot commented at 9:49 am on May 29, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on May 29, 2020

fanquake commented at 8:36 am on September 15, 2020: member

Given this only had the 1 ACK (with suggested improvements), and now needs a rebase, lets move this over the GUI repo. I’ll move #18643 over there as well.

fanquake closed this on Sep 15, 2020

ryanofsky commented at 4:21 pm on October 13, 2020: member

lets move this over the GUI repo

Reopening this not to lose track, since it doesn’t appear this PR was moved to the GUI repo (or I can’t find it). Can close this again if it is actually moved or no longer applicable.

ryanofsky reopened this on Oct 13, 2020

fanquake commented at 7:10 am on March 26, 2021: member

@hebasto can you either move this over to the GUI repo, or close and we’ll mark it up for grabs? The interested re-opener can open it in the gui repo.

hebasto commented at 6:16 pm on March 26, 2021: member

@fanquake

@hebasto can you either move this over to the GUI repo, or close and we’ll mark it up for grabs? The interested re-opener can open it in the gui repo.

https://github.com/bitcoin-core/gui/pull/260

hebasto closed this on Mar 26, 2021

laanwj referenced this in commit 03ecceedf6 on Apr 14, 2021

fanquake removed the label Needs rebase on May 31, 2021

DrahtBot locked this on Aug 18, 2022

qt: Handle exceptions instead of crash #18897

Conflicts