qt: Fix missing qRegisterMetaType for size_t #17427

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:20191109-fix-signal-argument-type changing 1 files +9 −6

hebasto commented at 11:06 AM on November 9, 2019: member

On master (a7aec7ad97949a82f870c033d8fd8b65d772eacb) 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

$ QT_FATAL_WARNINGS=1 lldb src/qt/bitcoin-qt -- -debug=qt
...
(lldb) bt
* thread [#17](/bitcoin-bitcoin/17/), name = 'QThread', stop reason = signal SIGABRT
  * frame [#0](/bitcoin-bitcoin/0/): 0x00007ffff35fce97 libc.so.6`__GI_raise(sig=2) at raise.c:51
    frame [#1](/bitcoin-bitcoin/1/): 0x00007ffff35fe801 libc.so.6`__GI_abort at abort.c:79
    frame [#2](/bitcoin-bitcoin/2/): 0x00007ffff5901352 libQt5Core.so.5`QMessageLogger::warning(char const*, ...) const + 354
    frame [#3](/bitcoin-bitcoin/3/): 0x00007ffff5b216fe libQt5Core.so.5`___lldb_unnamed_symbol2329$$libQt5Core.so.5 + 334
    frame [#4](/bitcoin-bitcoin/4/): 0x00007ffff5b2456d libQt5Core.so.5`QMetaObject::activate(QObject*, int, int, void**) + 1933
    frame [#5](/bitcoin-bitcoin/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 https://github.com/bitcoin/bitcoin/blob/a7aec7ad97949a82f870c033d8fd8b65d772eacb/src/qt/bitcoin.cpp#L63 is redundant since long CAmount is a typedef.

hebasto commented at 11:07 AM on November 9, 2019: member

friendly ping @promag
DrahtBot added the label GUI on Nov 9, 2019
fanquake requested review from promag on Nov 9, 2019
fanquake added this to the milestone 0.19.1 on Nov 9, 2019
fanquake added the label Needs backport (0.19) on Nov 9, 2019
fanquake commented at 5:05 PM on November 9, 2019: member

Tested that 40d2256d1037a1c6c2499b052807463377f44431 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(a7aec7ad97949a82f870c033d8fd8b65d772eacb):

pr(40d2256d1037a1c6c2499b052807463377f44431):
hebasto commented at 5:29 PM on November 9, 2019: member

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 commented at 5:33 PM on November 9, 2019: member

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

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

in src/qt/bitcoin.cpp:434 in 40d2256d10 outdated

 429 | @@ -430,16 +430,19 @@ int GuiMain(int argc, char* argv[])
 430 |  
 431 |      BitcoinApplication app(*node);
 432 |  
 433 | -    // Register meta types used for QMetaObject::invokeMethod
 434 | -    qRegisterMetaType< bool* >();

luke-jr commented at 9:41 PM on November 9, 2019:

No need to touch formatting.

in src/qt/bitcoin.cpp:439 in 40d2256d10 outdated

 440 | -    //   Need to pass name here as CAmount is a typedef (see http://qt-project.org/doc/qt-5/qmetatype.html#qRegisterMetaType)
 441 | -    //   IMPORTANT if it is no longer a typedef use the normal variant above
 442 | -    qRegisterMetaType< CAmount >("CAmount");
 443 | -    qRegisterMetaType< std::function<void()> >("std::function<void()>");
 444 | +    // Register typedefs (see http://qt-project.org/doc/qt-5/qmetatype.html#qRegisterMetaType)
 445 | +    // IMPORTANT: if CAmount is no longer a typedef use the normal variant above (see https://doc.qt.io/qt-5/qmetatype.html#qRegisterMetaType-1)

luke-jr commented at 9:41 PM on November 9, 2019:

size_t is also using the typedef variant here now too?

hebasto commented at 9:51 PM on November 9, 2019:

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

luke-jr changes_requested
laanwj commented at 9:14 AM on November 10, 2019: member

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 commented at 9:47 AM on November 10, 2019: member

I think the warning is on emit, not on connect.
hebasto commented at 9:51 AM on November 10, 2019: member

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

True.
laanwj commented at 9:54 AM on November 10, 2019: member

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.
in src/qt/bitcoin.cpp:433 in 40d2256d10 outdated
```
 429 | @@ -430,16 +430,19 @@ int GuiMain(int argc, char* argv[])
 430 |  
 431 |      BitcoinApplication app(*node);
 432 |  
 433 | -    // Register meta types used for QMetaObject::invokeMethod
```
laanwj commented at 9:56 AM on November 10, 2019:

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.

promag commented at 10:08 AM on November 10, 2019:

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

hebasto commented at 10:12 AM on November 10, 2019:

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

Done.

qt: Fix missing qRegisterMetaType for size_t

It is required in order to use size_t in QueuedConnections.

88a94f7bb8

refactor: Styling w/ clang-format, comment update 1828c6f05f
hebasto force-pushed on Nov 10, 2019
hebasto commented at 10:17 AM on November 10, 2019: member

Split in two commits as requested by @laanwj.
laanwj commented at 10:36 AM on November 10, 2019: member

Thanks! Tested ACK 1828c6f05fcbed9ed432b042cc36eee0d80b113d
laanwj referenced this in commit 89e93135ae on Nov 10, 2019
laanwj merged this on Nov 10, 2019
laanwj closed this on Nov 10, 2019
hebasto deleted the branch on Nov 10, 2019
MarcoFalke commented at 8:30 PM on November 11, 2019: member

Confirmed that this was introduced in 6b6be41c36e4fe9a74bed50e7f0a06532ab1260b. Confirmed that this was fixed by 88a94f7bb8ba2b0257315d70717f9af928ca6561.

Didn't look at the code
promag commented at 11:27 PM on November 12, 2019: member
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 https://github.com/bitcoin/bitcoin/pull/17457/commits/52b09b3103727982e6a3e046c2537b5e8fe3e8c3
luke-jr referenced this in commit 989d94ed65 on Nov 15, 2019
MarkLTZ referenced this in commit 15353a96a8 on Nov 17, 2019
laanwj referenced this in commit 2aba76ce02 on Nov 25, 2019
laanwj referenced this in commit a284bbbee8 on Nov 25, 2019
laanwj removed the label Needs backport (0.19) on Nov 25, 2019
fanquake deleted a comment on Mar 31, 2020
fanquake deleted a comment on Mar 31, 2020
fanquake deleted a comment on Mar 31, 2020
fanquake locked this on Mar 31, 2020

Contributors

Labels

Milestone
0.19.1

Linked (view graph)

#1 JSON-RPC support for mobile devices ("ultra-lightweight" clients)#2 Long-term, safe, store-of-value #3 Encrypt wallet #4 Export/Import wallet in a human readable, future-proof format #5 Make the version number the protocol version and not the client version #17 listaccounts method #16348 qt: Assert QMetaObject::invokeMethod result