qt: Fix missing qRegisterMetaType for size_t #17427

hebasto · 2019-11-09T11:06:27Z

On master (a7aec7a) this connection

Line 587 in a7aec7a

    
           connect(model, &ClientModel::mempoolSizeChanged, this, &RPCConsole::setMempoolSize);

fails due to ClientModel::mempoolSizeChanged() signal has unregistered parameter type size_t:

bitcoin/src/qt/clientmodel.h

Line 102 in a7aec7a

void mempoolSizeChanged(long count, size_t mempoolSizeInBytes);

More:

$ QT_FATAL_WARNINGS=1 lldb src/qt/bitcoin-qt -- -debug=qt
...
(lldb) bt
* thread #17, name = 'QThread', stop reason = signal SIGABRT
  * frame #0: 0x00007ffff35fce97 libc.so.6`__GI_raise(sig=2) at raise.c:51
    frame #1: 0x00007ffff35fe801 libc.so.6`__GI_abort at abort.c:79
    frame #2: 0x00007ffff5901352 libQt5Core.so.5`QMessageLogger::warning(char const*, ...) const + 354
    frame #3: 0x00007ffff5b216fe libQt5Core.so.5`___lldb_unnamed_symbol2329$$libQt5Core.so.5 + 334
    frame #4: 0x00007ffff5b2456d libQt5Core.so.5`QMetaObject::activate(QObject*, int, int, void**) + 1933
    frame #5: 0x000055555566872e bitcoin-qt`ClientModel::mempoolSizeChanged(this=<unavailable>, _t1=<unavailable>, _t2=<unavailable>) at moc_clientmodel.cpp:260
...

debug.log:

[] GUI: QObject::connect: Cannot queue arguments of type 'size_t'
(Make sure 'size_t' is registered using qRegisterMetaType().)

This PR fixes it.

Refs:

Side NOTE: Also I believe this line

bitcoin/src/qt/bitcoin.cpp

Line 63 in a7aec7a

Q_DECLARE_METATYPE(CAmount)

is redundant since long CAmount is a typedef.

hebasto · 2019-11-09T11:07:44Z

friendly ping @promag

fanquake · 2019-11-09T17:05:30Z

Tested that 40d2256 fixes the issue. Looks like the mempool info in the debug window has been broken since #17135 was merged, so is part of 0.19.0. We can backport this into 0.19.1.

master(a7aec7a):

pr(40d2256):

hebasto · 2019-11-09T17:29:16Z

Looks like the mempool info in the debug window has been broken since #16348 was merged

It seems there was another commit (not from #16348) that changed the GUI thread model and, as consequence, the connection type from Qt::DirectConnection to Qt::QueuedConnection.

promag

Not sure but I think the bug was introduced in #17135.

I'd just add the new qRegisterMetaType considering this is a fix.

luke-jr · 2019-11-09T21:41:29Z

src/qt/bitcoin.cpp

@@ -430,16 +430,19 @@ int GuiMain(int argc, char* argv[])

    BitcoinApplication app(*node);

-    // Register meta types used for QMetaObject::invokeMethod
-    qRegisterMetaType< bool* >();


No need to touch formatting.

luke-jr · 2019-11-09T21:41:50Z

src/qt/bitcoin.cpp

-    qRegisterMetaType< CAmount >("CAmount");
-    qRegisterMetaType< std::function<void()> >("std::function<void()>");
+    // Register typedefs (see http://qt-project.org/doc/qt-5/qmetatype.html#qRegisterMetaType)
+    // IMPORTANT: if CAmount is no longer a typedef use the normal variant above (see https://doc.qt.io/qt-5/qmetatype.html#qRegisterMetaType-1)


size_t is also using the typedef variant here now too?

size_t is missed intentionally in this comment, as it is C++'s typedef, not ours.

laanwj · 2019-11-10T09:14:19Z

ACK.

It's kind of annoying that this can't be a compile-time error (as it's essentially static). Would it help to add checks to the return values of connect everywhere, to prevent something like this from silently happening again? (not necessarily in this PR but in general)

promag · 2019-11-10T09:47:53Z

I think the warning is on emit, not on connect.

hebasto · 2019-11-10T09:51:56Z

I think the warning is on emit, not on connect.

True.

laanwj · 2019-11-10T09:54:18Z

Wait, so there's no way to do static or dynamic checking on this at connect time? That's kind of sucky.

It would be good to add a developer note on this, that Qt's signal/slot deceptively look like functions, but the arguments are marshalled internally. This restrict the set of types that can be passed that way.
And to be really careful around this.

laanwj · 2019-11-10T09:56:24Z

src/qt/bitcoin.cpp

@@ -430,16 +430,19 @@ int GuiMain(int argc, char* argv[])

    BitcoinApplication app(*node);

-    // Register meta types used for QMetaObject::invokeMethod


I don't like how this commit rewrites and reformats this entire section to add a statement. I'd prefer to split this into two commits: one that adds the missing line, and one that's purely restyling / comment updates.

Like I said, I also prefer a single line addition.

I'd prefer to split this into two commits: one that adds the missing line, and one that's purely restyling / comment updates.

Done.

It is required in order to use size_t in QueuedConnections.

hebasto · 2019-11-10T10:17:00Z

Split in two commits as requested by @laanwj.

laanwj · 2019-11-10T10:36:53Z

Thanks!
Tested ACK 1828c6f

1828c6f refactor: Styling w/ clang-format, comment update (Hennadii Stepanov) 88a94f7 qt: Fix missing qRegisterMetaType for size_t (Hennadii Stepanov) Pull request description: On master (a7aec7a) this connection https://github.com/bitcoin/bitcoin/blob/a7aec7ad97949a82f870c033d8fd8b65d772eacb/src/qt/rpcconsole.cpp#L587 fails due to `ClientModel::mempoolSizeChanged()` signal has unregistered parameter type `size_t`: https://github.com/bitcoin/bitcoin/blob/a7aec7ad97949a82f870c033d8fd8b65d772eacb/src/qt/clientmodel.h#L102 More: ``` $ QT_FATAL_WARNINGS=1 lldb src/qt/bitcoin-qt -- -debug=qt ... (lldb) bt * thread #17, name = 'QThread', stop reason = signal SIGABRT * frame #0: 0x00007ffff35fce97 libc.so.6`__GI_raise(sig=2) at raise.c:51 frame #1: 0x00007ffff35fe801 libc.so.6`__GI_abort at abort.c:79 frame #2: 0x00007ffff5901352 libQt5Core.so.5`QMessageLogger::warning(char const*, ...) const + 354 frame #3: 0x00007ffff5b216fe libQt5Core.so.5`___lldb_unnamed_symbol2329$$libQt5Core.so.5 + 334 frame #4: 0x00007ffff5b2456d libQt5Core.so.5`QMetaObject::activate(QObject*, int, int, void**) + 1933 frame #5: 0x000055555566872e bitcoin-qt`ClientModel::mempoolSizeChanged(this=<unavailable>, _t1=<unavailable>, _t2=<unavailable>) at moc_clientmodel.cpp:260 ... ``` `debug.log`: ``` [] GUI: QObject::connect: Cannot queue arguments of type 'size_t' (Make sure 'size_t' is registered using qRegisterMetaType().) ``` This PR fixes it. Refs: - [Qt docs: qRegisterMetaType](https://doc.qt.io/qt-5/qmetatype.html#qRegisterMetaType) - #16348 --- Side NOTE: Also I believe this line https://github.com/bitcoin/bitcoin/blob/a7aec7ad97949a82f870c033d8fd8b65d772eacb/src/qt/bitcoin.cpp#L63 is redundant since long `CAmount` is a `typedef`. ACKs for top commit: laanwj: Tested ACK 1828c6f Tree-SHA512: 2c7f9fe6a5ae70f2e1dd86b07f95d4b00c85c5706a9d722f063f80beb71880d012ec46556963fb1544c2af53d006936c2f7612eae60d9193f67db62ba3d86129

maflcko · 2019-11-11T20:30:10Z

Confirmed that this was introduced in 6b6be41.
Confirmed that this was fixed by 88a94f7.

Didn't look at the code

promag · 2019-11-12T23:27:58Z

qt/test/test_bitcoin-qt gives

QWARN  : AppTests::appTests() QObject::connect: Cannot queue arguments of type 'size_t'
(Make sure 'size_t' is registered using qRegisterMetaType().)

Fixed in 52b09b3

It is required in order to use size_t in QueuedConnections. Github-Pull: bitcoin#17427 Rebased-From: 88a94f7

It is required in order to use size_t in QueuedConnections. Github-Pull: #17427 Rebased-From: 88a94f7 Tree-SHA512: 55accd997209c559dfc8e88b0db189ba314ac31e265ba2f94fa2009f1aba6b96213e2aa8cbad492b1230078f2e6cf1cca7a233dc6f54e9bc449f4e5438330b4d

Github-Pull: #17427 Rebased-From: 1828c6f Tree-SHA512: 140b723bd9c8d8823ecb84d7f774d213a9e9dac45fb0de82c43521cf8276d946feaa1b7c0b7d40f51dbab38c88c53b4680da1965b1e0ce7f858eb35b2c4537da

DrahtBot added the GUI label Nov 9, 2019

fanquake requested a review from promag November 9, 2019 14:56

fanquake added this to the 0.19.1 milestone Nov 9, 2019

fanquake added the Needs backport (0.19) label Nov 9, 2019

promag reviewed Nov 9, 2019

View reviewed changes

luke-jr suggested changes Nov 9, 2019

View reviewed changes

laanwj reviewed Nov 10, 2019

View reviewed changes

hebasto added 2 commits November 10, 2019 12:03

qt: Fix missing qRegisterMetaType for size_t

88a94f7

It is required in order to use size_t in QueuedConnections.

refactor: Styling w/ clang-format, comment update

1828c6f

hebasto force-pushed the 20191109-fix-signal-argument-type branch from 40d2256 to 1828c6f Compare November 10, 2019 10:11

laanwj merged commit 1828c6f into bitcoin:master Nov 10, 2019

hebasto deleted the 20191109-fix-signal-argument-type branch November 10, 2019 10:53

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 15, 2019

qt: Fix missing qRegisterMetaType for size_t

989d94e

It is required in order to use size_t in QueuedConnections. Github-Pull: bitcoin#17427 Rebased-From: 88a94f7

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 17, 2019

[qt] Fix missing qRegisterMetaType for size_t (bitcoin#17427)

15353a9

laanwj mentioned this pull request Nov 24, 2019

Mempool fields in Debug Windows show "N/A" #17576

Closed

laanwj removed the Needs backport (0.19) label Nov 25, 2019

zeven74 mentioned this pull request Nov 26, 2019

Memory pool information update does not work #17610

Closed

hebasto mentioned this pull request Jan 12, 2020

"Export Transaction History" dialog not showing folders correctly #17909

Closed

bitcoin deleted a comment from Domel1985 Mar 31, 2020

bitcoin locked and limited conversation to collaborators Mar 31, 2020

		@@ -430,16 +430,19 @@ int GuiMain(int argc, char* argv[])

		BitcoinApplication app(*node);

		// Register meta types used for QMetaObject::invokeMethod

qt: Fix missing qRegisterMetaType for size_t #17427

qt: Fix missing qRegisterMetaType for size_t #17427

Uh oh!

Conversation

hebasto commented Nov 9, 2019

Uh oh!

hebasto commented Nov 9, 2019

Uh oh!

fanquake commented Nov 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Nov 9, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr Nov 9, 2019

Choose a reason for hiding this comment

Uh oh!

luke-jr Nov 9, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 9, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 10, 2019

Uh oh!

hebasto commented Nov 10, 2019

Uh oh!

laanwj commented Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

promag Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto commented Nov 10, 2019

Uh oh!

laanwj commented Nov 10, 2019

Uh oh!

maflcko commented Nov 11, 2019

Uh oh!

promag commented Nov 12, 2019

Uh oh!

Uh oh!

fanquake commented Nov 9, 2019 •

edited

Loading

laanwj commented Nov 10, 2019 •

edited

Loading

laanwj commented Nov 10, 2019 •

edited

Loading