qt: Revert changes of pr17943

hebasto commented at 8:04 am on January 19, 2020: member

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 (a654626f076a72416a3d354218d7107571d6caaf):

0$ ./src/qt/bitcoin-qt -prune=-1
1Error: Prune cannot be configured with a negative value.
2bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed.
3Aborted (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.

Revert "refactor: Simplify connection syntax"

This reverts commit 1a53b0da60097cd7fd423c519f01ceca0fd0aa14.

219417b388

Revert "refactor: Remove never used default parameter"

This reverts commit 7d0a8f4f530885cbf3870291f10f667326373bd1.

70e4706093

fanquake added the label GUI on Jan 19, 2020

fanquake requested review from Empact on Jan 19, 2020

fanquake requested review from promag on Jan 19, 2020

fanquake requested review from Sjors on Jan 19, 2020

Sjors commented at 10:36 am on January 19, 2020: member

ACK 70e4706093fd7b08a32f9638dace178852a9d249

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 commented at 10:55 am on January 19, 2020: member

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 commented at 10:59 am on January 19, 2020: member

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 commented at 11:01 am on January 19, 2020: member

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

hebasto commented at 11:06 am on January 19, 2020: member

@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 commented at 11:41 am on January 19, 2020: member

I think we should fix ThreadSafeMessageBox instead.

I found the only place where return value is used: https://github.com/bitcoin/bitcoin/blob/a654626f076a72416a3d354218d7107571d6caaf/src/init.cpp#L1614-L1617

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

DrahtBot commented at 12:13 pm on January 19, 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:

#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 commented at 2:13 pm on January 19, 2020: member

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

hebasto commented at 8:51 pm on January 19, 2020: member

@promag how about this patch to master:

 0diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
 1index 6043e93f9..ea13b60c2 100644
 2--- a/src/qt/bitcoingui.cpp
 3+++ b/src/qt/bitcoingui.cpp
 4@@ -1025,7 +1025,7 @@ void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, double nVer
 5     progressBar->setToolTip(tooltip);
 6 }
 7 
 8-void BitcoinGUI::message(const QString& title, QString message, unsigned int style)
 9+bool BitcoinGUI::message(const QString& title, QString message, unsigned int style)
10 {
11     // Default title. On macOS, the window title is ignored (as required by the macOS Guidelines).
12     QString strTitle{PACKAGE_NAME};
13@@ -1079,9 +1079,10 @@ void BitcoinGUI::message(const QString& title, QString message, unsigned int sty
14         showNormalIfMinimized();
15         QMessageBox mBox(static_cast<QMessageBox::Icon>(nMBoxIcon), strTitle, message, buttons, this);
16         mBox.setTextFormat(Qt::PlainText);
17-        mBox.exec();
18+        return mBox.exec() == QMessageBox::Ok;
19     } else {
20         notificator->notify(static_cast<Notificator::Class>(nNotifyIcon), strTitle, message);
21+        return false;
22     }
23 }
24 
25@@ -1362,10 +1363,10 @@ static bool ThreadSafeMessageBox(BitcoinGUI* gui, const std::string& message, co
26     // In case of modal message, use blocking connection to wait for user to click a button
27     bool invoked = QMetaObject::invokeMethod(gui, "message",
28                                modal ? GUIUtil::blockingGUIThreadConnection() : Qt::QueuedConnection,
29+                               Q_RETURN_ARG(bool, ret),
30                                Q_ARG(QString, QString::fromStdString(caption)),
31                                Q_ARG(QString, QString::fromStdString(message)),
32-                               Q_ARG(unsigned int, style),
33-                               Q_ARG(bool*, &ret));
34+                               Q_ARG(unsigned int, style));
35     assert(invoked);
36     return ret;
37 }
38diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h
39index 45fbb03aa..53c9e750b 100644
40--- a/src/qt/bitcoingui.h
41+++ b/src/qt/bitcoingui.h
42@@ -219,8 +219,9 @@ public Q_SLOTS: [@param](/bitcoin-bitcoin/contributor/param/)[in] message   the displayed text [@param](/bitcoin-bitcoin/contributor/param/)[in] style     modality and style definitions (icon and used buttons - buttons only for message boxes) [@see](/bitcoin-bitcoin/contributor/see/) CClientUIInterface::MessageBoxFlags
43+ [@return](/bitcoin-bitcoin/contributor/return/) True if Ok was clicked (modal only)
44     */
45-    void message(const QString& title, QString message, unsigned int style);
46+    bool message(const QString& title, QString message, unsigned int style);
47 
48 #ifdef ENABLE_WALLET
49     void setCurrentWallet(WalletModel* wallet_model);

From Qt docs:

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

hebasto commented at 9:19 pm on January 19, 2020: member

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

jonasschnelli commented at 4:38 am on January 20, 2020: contributor

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 commented at 3:45 pm on January 20, 2020: member

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

Agree.

laanwj commented at 3:12 pm on January 22, 2020: member

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

laanwj referenced this in commit 0038e536de on Jan 22, 2020

laanwj merged this on Jan 22, 2020

laanwj closed this on Jan 22, 2020

Empact commented at 7:02 pm on January 22, 2020: member

Post-hoc ACK https://github.com/bitcoin/bitcoin/pull/17965/commits/70e4706093fd7b08a32f9638dace178852a9d249

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

hebasto deleted the branch on Jan 31, 2020

PastaPastaPasta referenced this in commit 8af1abab0e on Dec 22, 2021

PastaPastaPasta referenced this in commit 70c48b9ca0 on Dec 22, 2021

PastaPastaPasta referenced this in commit 3e28512f96 on Dec 22, 2021

PastaPastaPasta referenced this in commit a60f648ed7 on Dec 28, 2021

DrahtBot locked this on Feb 15, 2022

qt: Revert changes of pr17943 #17965

Qt Versions:

Conflicts