qt: Revert changes of pr17943 #17965

hebasto · 2020-01-19T08:04:46Z

The code, the bool* ret = nullptr parameter in the BitcoinGUI::message() slot, removed in #17943 is not dead actually. It is used in ThreadSafeMessageBox() function:

bitcoin/src/qt/bitcoingui.cpp

Lines 1363 to 1368 in a654626

    
           bool invoked = QMetaObject::invokeMethod(gui, "message", 
        
                                      modal ? GUIUtil::blockingGUIThreadConnection() : Qt::QueuedConnection, 
        
                                      Q_ARG(QString, QString::fromStdString(caption)), 
        
                                      Q_ARG(QString, QString::fromStdString(message)), 
        
                                      Q_ARG(unsigned int, style), 
        
                                      Q_ARG(bool*, &ret));

Now in master (a654626):

$ ./src/qt/bitcoin-qt -prune=-1
Error: Prune cannot be configured with a negative value.
bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed.
Aborted (core dumped)

This PR reverts all commits of #17943

Additional notes: the bug was missed due to dynamic function call QMetaObject::invokeMethod() which cannot be checked at compile time. See #16348 for more discussion.

Sorry for introducing a bug.

This reverts commit 1a53b0d.

This reverts commit 7d0a8f4.

Sjors · 2020-01-19T10:36:38Z

ACK 70e4706

From #16348:

Once we bump Qt to at least 5.10 these can be refactored to use the invokeMethod overload that allows connecting to lambdas or member pointers, which are compile checked.

From #13478:

Qt Versions:

5.12 (LTS) Supported for 3 years post release.
5.11 Supported until May 2019
5.10 Supported until Dec 2018 (Archived).
5.9 (LTS) Supported until May 31, 2020.
5.6 (LTS) Supported until Mar. 16, 2019 (will end around the same time as the v0.18.0 release).

Any other releases older than 5.10 are no longer supported (by Qt).

Qt releases seem to be getting more frequent, and in some cases more aggressive about dropping support for OS versions. i.e macOS >10.12 is required for Qt 5.12.

It might be a good idea to bump the minimum version to 5.12 after the 0.20 branch-off. Alternatively we could duplicate the code with #IF QT_VERSION >= 0x051000 with the modern syntax, so that the compiler will catch an accidental refactor bug like this.

promag · 2020-01-19T10:55:46Z

I think we should fix ThreadSafeMessageBox instead. Note that if modal == false Qt::QueuedConnection is used and a pointer to the local ret is passed, which goes out of scope. So by the time slot message is called it dereferences an invalid pointer. Please correct me if I'm wrong.

hebasto · 2020-01-19T10:59:42Z

I think we should fix ThreadSafeMessageBox instead. Note that if modal == false Qt::QueuedConnection is used and a pointer to the local ret is passed, which goes out of scope. So by the time slot message is called it dereferences an invalid pointer. Please correct me if I'm wrong.

QMetaObject::invokeMethod() passes 4 arguments to BitcoinGUI::message() with 3 parameters signature.

promag · 2020-01-19T11:01:53Z

@hebasto I mean that if we merge this then the problem above exists.

hebasto · 2020-01-19T11:06:14Z

@promag

Note that if modal == false Qt::QueuedConnection is used and a pointer to the local ret is passed, which goes out of scope. So by the time slot message is called it dereferences an invalid pointer.

Indeed.

hebasto · 2020-01-19T11:41:49Z

I think we should fix ThreadSafeMessageBox instead.

I found the only place where return value is used:

bitcoin/src/init.cpp

Lines 1614 to 1617 in a654626

    
           bool fRet = uiInterface.ThreadSafeQuestion( 
        
               strLoadError + ".\n\n" + _("Do you want to rebuild the block database now?").translated, 
        
               strLoadError + ".\nPlease restart with -reindex or -reindex-chainstate to recover.", 
        
               "", CClientUIInterface::MSG_ERROR | CClientUIInterface::BTN_ABORT);

~~Could we replace our custom ThreadSafeMessageBox with standard QMessageBox here?~~

DrahtBot · 2020-01-19T12:13: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:

#17966 (qt, refactor: Optimize signal-slot connections logic by hebasto)
#17937 (gui: Remove WalletView and BitcoinGUI circular dependency by promag)
#16224 (gui: Bilingual GUI error messages 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.

laanwj · 2020-01-19T14:13:52Z

I'm happy this was discovered before a release! Please do be careful with cleanup changes like this.

hebasto · 2020-01-19T20:51:12Z

@promag how about this patch to master:

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index 6043e93f9..ea13b60c2 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -1025,7 +1025,7 @@ void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, double nVer
     progressBar->setToolTip(tooltip);
 }
 
-void BitcoinGUI::message(const QString& title, QString message, unsigned int style)
+bool BitcoinGUI::message(const QString& title, QString message, unsigned int style)
 {
     // Default title. On macOS, the window title is ignored (as required by the macOS Guidelines).
     QString strTitle{PACKAGE_NAME};
@@ -1079,9 +1079,10 @@ void BitcoinGUI::message(const QString& title, QString message, unsigned int sty
         showNormalIfMinimized();
         QMessageBox mBox(static_cast<QMessageBox::Icon>(nMBoxIcon), strTitle, message, buttons, this);
         mBox.setTextFormat(Qt::PlainText);
-        mBox.exec();
+        return mBox.exec() == QMessageBox::Ok;
     } else {
         notificator->notify(static_cast<Notificator::Class>(nNotifyIcon), strTitle, message);
+        return false;
     }
 }
 
@@ -1362,10 +1363,10 @@ static bool ThreadSafeMessageBox(BitcoinGUI* gui, const std::string& message, co
     // In case of modal message, use blocking connection to wait for user to click a button
     bool invoked = QMetaObject::invokeMethod(gui, "message",
                                modal ? GUIUtil::blockingGUIThreadConnection() : Qt::QueuedConnection,
+                               Q_RETURN_ARG(bool, ret),
                                Q_ARG(QString, QString::fromStdString(caption)),
                                Q_ARG(QString, QString::fromStdString(message)),
-                               Q_ARG(unsigned int, style),
-                               Q_ARG(bool*, &ret));
+                               Q_ARG(unsigned int, style));
     assert(invoked);
     return ret;
 }
diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h
index 45fbb03aa..53c9e750b 100644
--- a/src/qt/bitcoingui.h
+++ b/src/qt/bitcoingui.h
@@ -219,8 +219,9 @@ public Q_SLOTS:
        @param[in] message   the displayed text
        @param[in] style     modality and style definitions (icon and used buttons - buttons only for message boxes)
                             @see CClientUIInterface::MessageBoxFlags
+       @return True if Ok was clicked (modal only)
     */
-    void message(const QString& title, QString message, unsigned int style);
+    bool message(const QString& title, QString message, unsigned int style);
 
 #ifdef ENABLE_WALLET
     void setCurrentWallet(WalletModel* wallet_model);

From Qt docs:

If the invocation is asynchronous, the return value cannot be evaluated.

hebasto · 2020-01-19T21:19:07Z

Also I did not find any ThreadSafeMessageBox() invocation without CClientUIInterface::MODAL set.

jonasschnelli · 2020-01-20T04:38:59Z

Thanks for fixing.
Sorry for not testing #17943.
I think GUI refactoring should not be taken too easy since there are little tests that would detect possible new issues.

laanwj · 2020-01-20T15:45:00Z

I think GUI refactoring should not be taken too easy since there are little tests that would detect possible new issues.

Agree.

laanwj · 2020-01-22T15:12:05Z

I reverted the commits from this PR and got exactly the same.
ACK 70e4706

70e4706 Revert "refactor: Remove never used default parameter" (Hennadii Stepanov) 219417b Revert "refactor: Simplify connection syntax" (Hennadii Stepanov) Pull request description: The code, the `bool* ret = nullptr` parameter in the `BitcoinGUI::message()` slot, removed in #17943 is not dead actually. It is used in `ThreadSafeMessageBox()` function: https://github.com/bitcoin/bitcoin/blob/a654626f076a72416a3d354218d7107571d6caaf/src/qt/bitcoingui.cpp#L1363-L1368 Now in master (a654626): ``` $ ./src/qt/bitcoin-qt -prune=-1 Error: Prune cannot be configured with a negative value. bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed. Aborted (core dumped) ``` This PR reverts all commits of #17943 Additional notes: the bug was missed due to dynamic function call `QMetaObject::invokeMethod()` which cannot be checked at compile time. See #16348 for more discussion. Sorry for introducing a bug. ACKs for top commit: Sjors: ACK 70e4706 laanwj: ACK 70e4706 Tree-SHA512: b968a026eaa4f5f39fd36ddc715d8e233f3c6420e6580f11d4ca422a5ff5d1d9d3df9ac11b353c3d4f434d67d6a69e37d2e26b8248d72bedd14ecba0a545a327

Empact · 2020-01-22T19:02:20Z

Post-hoc ACK 70e4706

And partial mea culpa: if I had looked more closely at 35ecf85 which I cited here I would have seen that it had been moved and retained and continued to follow it to its current location.

70e4706 Revert "refactor: Remove never used default parameter" (Hennadii Stepanov) 219417b Revert "refactor: Simplify connection syntax" (Hennadii Stepanov) Pull request description: The code, the `bool* ret = nullptr` parameter in the `BitcoinGUI::message()` slot, removed in bitcoin#17943 is not dead actually. It is used in `ThreadSafeMessageBox()` function: https://github.com/bitcoin/bitcoin/blob/a654626f076a72416a3d354218d7107571d6caaf/src/qt/bitcoingui.cpp#L1363-L1368 Now in master (a654626): ``` $ ./src/qt/bitcoin-qt -prune=-1 Error: Prune cannot be configured with a negative value. bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed. Aborted (core dumped) ``` This PR reverts all commits of bitcoin#17943 Additional notes: the bug was missed due to dynamic function call `QMetaObject::invokeMethod()` which cannot be checked at compile time. See bitcoin#16348 for more discussion. Sorry for introducing a bug. ACKs for top commit: Sjors: ACK 70e4706 laanwj: ACK 70e4706 Tree-SHA512: b968a026eaa4f5f39fd36ddc715d8e233f3c6420e6580f11d4ca422a5ff5d1d9d3df9ac11b353c3d4f434d67d6a69e37d2e26b8248d72bedd14ecba0a545a327

hebasto added 2 commits January 19, 2020 09:37

Revert "refactor: Simplify connection syntax"

219417b

This reverts commit 1a53b0d.

Revert "refactor: Remove never used default parameter"

70e4706

This reverts commit 7d0a8f4.

fanquake added the GUI label Jan 19, 2020

fanquake requested review from Empact, promag and Sjors January 19, 2020 08:05

laanwj merged commit 70e4706 into bitcoin:master Jan 22, 2020

hebasto mentioned this pull request Jan 23, 2020

gui: Bilingual GUI error messages #16224

Merged

promag mentioned this pull request Jan 30, 2020

gui: Remove WalletView and BitcoinGUI circular dependency #17937

Merged

hebasto deleted the 20200119-revert17943 branch January 31, 2020 16:37

Sjors mentioned this pull request Feb 1, 2020

QT Core wallet crash when starting with "-txindex" #12406

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

qt: Revert changes of pr17943 #17965

qt: Revert changes of pr17943 #17965

Uh oh!

hebasto commented Jan 19, 2020

Uh oh!

Sjors commented Jan 19, 2020

Qt Versions:

Uh oh!

promag commented Jan 19, 2020

Uh oh!

hebasto commented Jan 19, 2020

Uh oh!

promag commented Jan 19, 2020

Uh oh!

hebasto commented Jan 19, 2020

Uh oh!

hebasto commented Jan 19, 2020 •

edited

Loading

Uh oh!

DrahtBot commented Jan 19, 2020 •

edited

Loading

Uh oh!

laanwj commented Jan 19, 2020

Uh oh!

hebasto commented Jan 19, 2020 •

edited

Loading

Uh oh!

hebasto commented Jan 19, 2020

Uh oh!

jonasschnelli commented Jan 20, 2020

Uh oh!

laanwj commented Jan 20, 2020

Uh oh!

laanwj commented Jan 22, 2020

Uh oh!

Empact commented Jan 22, 2020

Uh oh!

Uh oh!

	bool invoked = QMetaObject::invokeMethod(gui, "message",
	modal ? GUIUtil::blockingGUIThreadConnection() : Qt::QueuedConnection,
	Q_ARG(QString, QString::fromStdString(caption)),
	Q_ARG(QString, QString::fromStdString(message)),
	Q_ARG(unsigned int, style),
	Q_ARG(bool*, &ret));

qt: Revert changes of pr17943 #17965

qt: Revert changes of pr17943 #17965

Uh oh!

Conversation

hebasto commented Jan 19, 2020

Uh oh!

Sjors commented Jan 19, 2020

Qt Versions:

Uh oh!

promag commented Jan 19, 2020

Uh oh!

hebasto commented Jan 19, 2020

Uh oh!

promag commented Jan 19, 2020

Uh oh!

hebasto commented Jan 19, 2020

Uh oh!

hebasto commented Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Jan 19, 2020

Uh oh!

hebasto commented Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 19, 2020

Uh oh!

jonasschnelli commented Jan 20, 2020

Uh oh!

laanwj commented Jan 20, 2020

Uh oh!

laanwj commented Jan 22, 2020

Uh oh!

Empact commented Jan 22, 2020

Uh oh!

Uh oh!

hebasto commented Jan 19, 2020 •

edited

Loading

DrahtBot commented Jan 19, 2020 •

edited

Loading

hebasto commented Jan 19, 2020 •

edited

Loading