qt: Handle exceptions instead of crash #18897

hebasto · 2020-05-06T12:01:36Z

Related issue #18643.

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

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

DrahtBot · 2020-05-06T12:08:18Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

qt: Remove QFont warnings with QT_QPA_PLATFORM=minimal #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.

maflcko · 2020-05-06T13:11:37Z

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?

maflcko · 2020-05-06T13:11:49Z

Concept ACK either way

hebasto · 2020-05-06T13:14:22Z

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

maflcko · 2020-05-06T13:19:37Z

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

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

hebasto · 2020-05-06T13:21:44Z

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?

src/qt/transactionview.cpp

src/qt/bitcoingui.cpp

hebasto · 2020-05-06T15:14:27Z

Updated 40a622d -> 44f7f51 (pr18897.01 -> pr18897.02, diff):

addressed @ryanofsky's comments

The OP and the screenshot have been updated.

ryanofsky · 2020-05-06T16:34:33Z

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:

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

could replace:

try {
    sendButtonClickedHelper();
} catch (const std::exception& e) {
    Q_EMIT runawayException(QString::fromStdString(e.what()));
} catch (...) {
    Q_EMIT runawayException(QString("Unknown failure occurred."));
}

promag

There's this approach:

bool BitcoinApplication::notify(QObject* receiver, QEvent* event)
{
    try {
        return QApplication::notify(receiver, event);
    } catch (const std::exception& ex) {
        // ..
    } catch (...) {
        // ..
    }
    return false;
}

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?

src/qt/sendcoinsdialog.cpp

hebasto · 2020-05-08T11:23:20Z

@ryanofsky @promag Thank you for your reviews.

Updated 44f7f51 -> 24a90d5 (pr18897.02 -> pr18897.03, diff).

Ready for review now :)

src/qt/shieldexception.h

hebasto · 2020-05-08T14:32:16Z

Updated 24a90d5 -> 82529bc (pr18897.03 -> pr18897.04, diff):

addressed @promag's comment:

Move to src/qt/guiutil.h?

hebasto · 2020-05-10T11:39:57Z

Also #18435 could be related.

hebasto · 2020-05-11T19:15:07Z

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

src/qt/guiutil.h

jonasschnelli · 2020-05-12T07:23:33Z

Concept ACK

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.

hebasto · 2020-05-13T04:38:38Z

Updated 1fb68b3 -> 8f94243 (pr18897.05 -> pr18897.06, diff):

addressed @promag's comment:

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

ryanofsky

Code review ACK 8f94243. 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.

ryanofsky · 2020-05-26T21:49:10Z

src/qt/guiutil.h

+        bool ok = true;
+        try {
+            f();
+        } catch (const NonFatalCheckError& e) {


In commit "qt: Add shieldException() function" (553128b)

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

ryanofsky · 2020-05-26T21:53:48Z

src/qt/guiutil.h

+            PrintExceptionContinue(&e, "Internal error");
+            ok = QMetaObject::invokeMethod(qApp, "handleNonFatalException", blockingGUIThreadConnection(), Q_ARG(QString, QString::fromStdString(e.what())));
+        } catch (const std::exception& e) {
+            PrintExceptionContinue(&e, "Runaway exception");


In commit "qt: Add shieldException() function" (553128b)

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

DrahtBot · 2020-05-29T09:49:21Z

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

fanquake · 2020-09-15T08:36:36Z

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.

ryanofsky · 2020-10-13T16:21:21Z

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.

fanquake · 2021-03-26T07:10:44Z

@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 · 2021-03-26T18:16:54Z

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

bitcoin-core/gui#260

b8e5d0d qt: Handle exceptions in SendCoinsDialog::sendButtonClicked slot (Hennadii Stepanov) 1ac2bc7 qt: Handle exceptions in TransactionView::bumpFee slot (Hennadii Stepanov) bc00e13 qt: Handle exceptions in WalletModel::pollBalanceChanged slot (Hennadii Stepanov) eb6156b qt: Handle exceptions in BitcoinGUI::addWallet slot (Hennadii Stepanov) f7e260a qt: Add GUIUtil::ExceptionSafeConnect function (Hennadii Stepanov) 64a8755 qt: Add BitcoinApplication::handleNonFatalException function (Hennadii Stepanov) af7e365 qt: Make PACKAGE_BUGREPORT link clickable (Hennadii Stepanov) Pull request description: This PR is an alternative to bitcoin/bitcoin#18897, and is based on Russ' [idea](bitcoin/bitcoin#18897 (review)): > IMO it would be nice to have a followup PR that eliminated the one-line forwarding methods ... Related issues - #91 - bitcoin/bitcoin#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 b8e5d0d ryanofsky: Code review ACK b8e5d0d. 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

fanquake added the GUI label May 6, 2020

hebasto mentioned this pull request May 6, 2020

The gui doesn't handle exceptions very well #18643

Closed

ryanofsky reviewed May 6, 2020

View reviewed changes

src/qt/transactionview.cpp Outdated Show resolved Hide resolved

ryanofsky reviewed May 6, 2020

View reviewed changes

src/qt/bitcoingui.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 200506-slot-ex branch from 40a622d to 44f7f51 Compare May 6, 2020 15:10

hebasto changed the title ~~gui: Handle NonFatalCheckError instead of crash~~ gui: Handle exceptions instead of crash May 6, 2020

hebasto marked this pull request as draft May 6, 2020 15:34

This was referenced May 6, 2020

qt, refactor: Optimize signal-slot connections logic #17966

Closed

qt: Add privacy to the Overview page #16432

Merged

promag reviewed May 6, 2020

View reviewed changes

src/qt/sendcoinsdialog.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 200506-slot-ex branch 2 times, most recently from 7c8178b to 24a90d5 Compare May 8, 2020 11:19

hebasto marked this pull request as ready for review May 8, 2020 11:23

hebasto changed the title ~~gui: Handle exceptions instead of crash~~ qt: Handle exceptions instead of crash May 8, 2020

promag reviewed May 8, 2020

View reviewed changes

src/qt/shieldexception.h Outdated Show resolved Hide resolved

hebasto force-pushed the 200506-slot-ex branch from 24a90d5 to 82529bc Compare May 8, 2020 14:29

This was referenced May 8, 2020

build: Enable -Wsuggest-override if available #16710

Merged

refactor: Apply override specifier consistently #18914

Merged

qt: Remove QFont warnings with QT_QPA_PLATFORM=minimal #17908

Merged

DrahtBot added the Needs rebase label May 11, 2020

hebasto force-pushed the 200506-slot-ex branch from 82529bc to 1fb68b3 Compare May 11, 2020 19:00

DrahtBot removed the Needs rebase label May 11, 2020

promag reviewed May 11, 2020

View reviewed changes

src/qt/guiutil.h Outdated Show resolved Hide resolved

hebasto added 3 commits May 13, 2020 07:28

qt: Add shieldException() function

553128b

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.

qt: Handle exceptions in SendCoinsDialog::on_sendButton_clicked slot

a65c2f7

qt: Handle exceptions in TransactionView::bumpFee slot

8f94243

hebasto force-pushed the 200506-slot-ex branch from 1fb68b3 to 8f94243 Compare May 13, 2020 04:35

ryanofsky approved these changes May 26, 2020

View reviewed changes

DrahtBot added the Needs rebase label May 29, 2020

fanquake closed this Sep 15, 2020

ryanofsky reopened this Oct 13, 2020

adamjonas mentioned this pull request Dec 23, 2020

The gui doesn't handle exceptions very well bitcoin-core/gui#91

Closed

hebasto mentioned this pull request Mar 26, 2021

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

Merged

hebasto closed this Mar 26, 2021

fanquake removed the Needs rebase label May 31, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qt: Handle exceptions instead of crash #18897

qt: Handle exceptions instead of crash #18897

hebasto commented May 6, 2020 •

edited

DrahtBot commented May 6, 2020 •

edited

maflcko commented May 6, 2020

maflcko commented May 6, 2020

hebasto commented May 6, 2020

maflcko commented May 6, 2020

hebasto commented May 6, 2020

hebasto commented May 6, 2020 •

edited

ryanofsky commented May 6, 2020

promag left a comment

hebasto commented May 8, 2020

hebasto commented May 8, 2020

hebasto commented May 10, 2020

hebasto commented May 11, 2020

jonasschnelli commented May 12, 2020

hebasto commented May 13, 2020

ryanofsky left a comment

ryanofsky May 26, 2020

ryanofsky May 26, 2020

DrahtBot commented May 29, 2020

fanquake commented Sep 15, 2020

ryanofsky commented Oct 13, 2020

fanquake commented Mar 26, 2021

hebasto commented Mar 26, 2021

qt: Handle exceptions instead of crash #18897

qt: Handle exceptions instead of crash #18897

Conversation

hebasto commented May 6, 2020 • edited

DrahtBot commented May 6, 2020 • edited

Conflicts

maflcko commented May 6, 2020

maflcko commented May 6, 2020

hebasto commented May 6, 2020

maflcko commented May 6, 2020

hebasto commented May 6, 2020

hebasto commented May 6, 2020 • edited

ryanofsky commented May 6, 2020

promag left a comment

Choose a reason for hiding this comment

hebasto commented May 8, 2020

hebasto commented May 8, 2020

hebasto commented May 10, 2020

hebasto commented May 11, 2020

jonasschnelli commented May 12, 2020

hebasto commented May 13, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky May 26, 2020

Choose a reason for hiding this comment

ryanofsky May 26, 2020

Choose a reason for hiding this comment

DrahtBot commented May 29, 2020

fanquake commented Sep 15, 2020

ryanofsky commented Oct 13, 2020

fanquake commented Mar 26, 2021

hebasto commented Mar 26, 2021

hebasto commented May 6, 2020 •

edited

DrahtBot commented May 6, 2020 •

edited

hebasto commented May 6, 2020 •

edited