gui: Fix start timer from non QThread #17120

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-10-fix-timer changing 3 files +17 −7

promag commented at 9:30 PM on October 12, 2019: member

Fixes #16296.
hebasto commented at 9:37 PM on October 12, 2019: member

Concept ACK
fanquake added the label GUI on Oct 12, 2019
emilengler commented at 10:06 PM on October 12, 2019: contributor

Concept ACK
in src/qt/walletmodel.cpp:55 in af0eeba79d outdated
```
  51 | @@ -57,6 +52,14 @@ WalletModel::~WalletModel()
  52 |      unsubscribeFromCoreSignals();
  53 |  }
  54 |  
  55 | +void WalletModel::init()
```
hebasto commented at 10:39 AM on October 13, 2019:

nit: Could slot name be more expressive, as it initiates regular polling?

promag commented at 10:43 AM on October 13, 2019:

Yeah like startPollBalance?

hebasto commented at 11:02 AM on October 13, 2019:

Yup ;)
hebasto approved
hebasto commented at 10:40 AM on October 13, 2019: member
ACK af0eeba79da7b387731a41fb05c382a9e4b41a5f, tested on:
- Linux Mint 19.2, Qt 5.9.5
- macOS 10.13.6, Qt 5.11.2
There is no more QTimer warnings during loadwallet via bitcoin-cli RPC call.
promag force-pushed on Oct 13, 2019
promag commented at 11:19 AM on October 13, 2019: member

Renamed init to startPollBalance and asserted QMetaObject::invokeMethod result for Qt < 5.10..
hebasto commented at 11:35 AM on October 13, 2019: member

@promag

Renamed init to startPollBalance and asserted QMetaObject::invokeMethod result for Qt < 5.10..

Why for Qt < 5.10 only?
promag commented at 11:39 AM on October 13, 2019: member

@hebasto because for Qt >= 5.10 the invokeMethod is compiler checked.
hebasto commented at 12:33 PM on October 13, 2019: member
@promag

because for Qt >= 5.10 the invokeMethod is compiler checked.

To be pedantic, a compiler just checks a lambda body in your code ;)

A compiler checked overloaded invokeMethod function is:
```
QMetaObject::invokeMethod(wallet_model, &WalletModel::startPollBalance, Qt::QueuedConnection);
```
More details:
- Add QMetaObject::invokeMethod() overloads for function pointers
- https://forum.qt.io/topic/83278/qt5-invokemethod/4
As startPollBalance() slot does not accept parameters, the mentioned above invokeMethod() call without lambda seems cleaner, IMO ;)
in src/qt/walletcontroller.cpp:111 in d65437b203 outdated
```
 107 | @@ -108,6 +108,13 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
 108 |      wallet_model->setParent(this);
 109 |      m_wallets.push_back(wallet_model);
 110 |  
 111 | +#if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
```
laanwj commented at 9:34 AM on October 14, 2019:

There's a risk in implementing this twice. It's not strictly necessary because the pre-5.10 code will work fine on newer Qt. Might just as well remove the #if imo.

promag commented at 9:42 AM on October 14, 2019:

Just wanted to make it easier for when we bump Qt. But will remove considering there are other places that would benefit the same approach.

laanwj commented at 10:00 AM on October 14, 2019:

I understand, but I think it's better to change these (if desired) when bumping the minimum supported Qt version. Don't introduce conditional compilation when not necessary.

promag commented at 10:03 AM on October 14, 2019:

Done :+1:

laanwj commented at 12:07 PM on October 15, 2019:

Thanks!
gui: Fix start timer from non QThread a8f5026d6d
promag force-pushed on Oct 14, 2019
promag commented at 9:54 AM on October 14, 2019: member

Dropped Qt::QueuedConnection as the default Qt::AutoConnection is enough. Also added comment.
promag referenced this in commit cff91a8c3b on Oct 14, 2019
MarcoFalke added this to the milestone 0.19.0 on Oct 14, 2019
MarcoFalke added the label Needs backport (0.19) on Oct 14, 2019
MarcoFalke commented at 12:44 PM on October 14, 2019: member

How can this be tested?
promag commented at 1:09 PM on October 14, 2019: member
@MarcoFalke without this change:
- bitcoin-qt -regtest -server
- generate coins
- bitcoin-cli -regtest createwallet test
- bitcoin-cli -regtest -rpcwallet=test getnewaddress
- bitcoin-cli -regtest -rpcwallet="" sendtoaddress with the above address
- check that balance hasn't changed in the test wallet GUI
hebasto commented at 1:13 PM on October 14, 2019: member

Could a functional test be added to prevent regressions in the future?
laanwj added the label Bug on Oct 23, 2019
laanwj commented at 11:04 AM on October 23, 2019: member

A functional test cannot help with testing this GUI functionality.
laanwj commented at 11:44 AM on October 24, 2019: member

code review ACK a8f5026d6d992fd8d72908c848c5028f0f9a8cd1
in src/qt/walletcontroller.cpp:111 in a8f5026d6d
```
 107 | @@ -108,6 +108,12 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
 108 |      wallet_model->setParent(this);
 109 |      m_wallets.push_back(wallet_model);
 110 |  
 111 | +    // WalletModel::startPollBalance needs to be called in a thread managed by
```
laanwj commented at 12:07 PM on October 24, 2019:

Are you sure this comment is correct? Doesn't QTimer(self), as the timer is intended to end up in the event loop of the GUI thread, need to be called from the GUI thread? Any other Qt thread wouldn't do, either.

(the fix is correct in any case)

promag commented at 3:19 PM on October 25, 2019:

as the timer is intended to end up in the event loop of the GUI thread

Yes this is the case at the moment, but probably will be moved to other thread, like #17135 (same thread maybe doesn't matter now).

Either way startTimer can't be called from a RPC thread.
laanwj referenced this in commit 9bd109b78d on Oct 26, 2019
laanwj merged this on Oct 26, 2019
laanwj closed this on Oct 26, 2019
laanwj referenced this in commit e39c9cff1a on Oct 26, 2019
laanwj removed the label Needs backport (0.19) on Oct 26, 2019
sidhujag referenced this in commit bcfa469a4c on Oct 28, 2019
HashUnlimited referenced this in commit a06e3bdbd6 on Nov 17, 2019
fxtc referenced this in commit 12ef9aee09 on Nov 25, 2019
fxtc referenced this in commit e652638603 on Nov 25, 2019
jasonbcox referenced this in commit e03979f898 on Sep 28, 2020
sidhujag referenced this in commit 780a75e801 on Nov 10, 2020
PastaPastaPasta referenced this in commit c904c38931 on Sep 21, 2021
kittywhiskers referenced this in commit a201faacec on Oct 12, 2021
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

Milestone
0.19.0

Linked (view graph)

#16296 gui: crash with loadwallet & QT_FATAL_WARNINGS #17133 0.19: gui: Fix start timer from non QThread