qt: Call setParent() in the parent's context #18948

hebasto commented at 2:39 PM on May 11, 2020: member

The setParent(parent) internally calls QCoreApplication::sendEvent(parent, QChildEvent) that implies running in the thread which created the parent object. That is not the case always, and an internal assertion fails in the debug mode.

Steps to reproduce this issue on master (007e15dcd7f8b42501e31cc36343655c53027077) on Linux Mint 20 (x86_64):

$ make -C depends DEBUG=1
$ CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure
$ make
$ QT_FATAL_WARNINGS=1 lldb src/qt/bitcoin-qt -- --regtest -debug=qt
(lldb) target create "src/qt/bitcoin-qt"
Current executable set to '/home/hebasto/GitHub/bitcoin/src/qt/bitcoin-qt' (x86_64).
(lldb) settings set -- target.run-args  "--regtest" "-debug=qt"
(lldb) run
Process 431562 launched: '/home/hebasto/GitHub/bitcoin/src/qt/bitcoin-qt' (x86_64)
# load wallet via GUI
Process 431562 stopped
* thread [#24](/bitcoin-bitcoin/24/), name = 'QThread', stop reason = signal SIGABRT
    frame [#0](/bitcoin-bitcoin/0/): 0x00007ffff794518b libc.so.6`__GI_raise(sig=2) at raise.c:51:1
(lldb) bt
* thread [#24](/bitcoin-bitcoin/24/), name = 'QThread', stop reason = signal SIGABRT
  * frame [#0](/bitcoin-bitcoin/0/): 0x00007ffff794518b libc.so.6`__GI_raise(sig=2) at raise.c:51:1
    frame [#1](/bitcoin-bitcoin/1/): 0x00007ffff7924859 libc.so.6`__GI_abort at abort.c:79:7
    frame [#2](/bitcoin-bitcoin/2/): 0x0000555556508ec4 bitcoin-qt`::qt_message_fatal((null)=<unavailable>, context=<unavailable>, message=<unavailable>) at qlogging.cpp:1690:15
    frame [#3](/bitcoin-bitcoin/3/): 0x00005555565099cf bitcoin-qt`QMessageLogger::fatal(this=<unavailable>, msg=<unavailable>) const at qlogging.cpp:796:21
    frame [#4](/bitcoin-bitcoin/4/): 0x000055555650479d bitcoin-qt`qt_assert_x(where=<unavailable>, what=<unavailable>, file=<unavailable>, line=<unavailable>) at qglobal.cpp:3088:46
    frame [#5](/bitcoin-bitcoin/5/): 0x0000555556685733 bitcoin-qt`QCoreApplicationPrivate::checkReceiverThread(receiver=0x0000555557b27510) at qcoreapplication.cpp:557:5
    frame [#6](/bitcoin-bitcoin/6/): 0x00005555567ced86 bitcoin-qt`QApplication::notify(this=0x00007fffffffd4a0, receiver=0x0000555557b27510, e=0x00007fff9a7f8ce0) at qapplication.cpp:2956:27
    frame [#7](/bitcoin-bitcoin/7/): 0x0000555556685d31 bitcoin-qt`QCoreApplication::notifyInternal2(receiver=0x0000555557b27510, event=0x00007fff9a7f8ce0) at qcoreapplication.cpp:1024:24
    frame [#8](/bitcoin-bitcoin/8/): 0x00005555566c9224 bitcoin-qt`QObjectPrivate::setParent_helper(QObject*) [inlined] QCoreApplication::sendEvent(event=<unavailable>, receiver=<unavailable>) at qcoreapplication.h:233:59
    frame [#9](/bitcoin-bitcoin/9/): 0x00005555566c9210 bitcoin-qt`QObjectPrivate::setParent_helper(this=0x00007fff85855260, o=0x0000555557b27510) at qobject.cpp:2036
    frame [#10](/bitcoin-bitcoin/10/): 0x00005555566c9b41 bitcoin-qt`QObject::setParent(this=<unavailable>, parent=<unavailable>) at qobject.cpp:1980:24
    frame [#11](/bitcoin-bitcoin/11/): 0x0000555555710be8 bitcoin-qt`WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> >) + 2534

...

Fixes #18835.

hebasto commented at 2:46 PM on May 11, 2020: member

The needless of setParent() was pointed already:

@ryanofsky's comment:

Curious: Is this setParent call actually necessary for anything? Maybe consider dropping it or adding a comment that says why it's useful.

@promag's comment:

setParent is not necessary at the moment because (afaict) the wallet is always unloaded here...

Comments are adjusted according to @ryanofsky's suggestions:

promag commented at 2:51 PM on May 11, 2020: member

Concept ACK, but now it must destroy wallet models on quit, or am I wrong?

hebasto commented at 2:54 PM on May 11, 2020: member

Concept ACK, but now it must destroy wallet models on quit, or am I wrong?

Correct. Aren't they destroyed now?

promag commented at 3:04 PM on May 11, 2020: member

Concept ACK, but now it must destroy wallet models on quit, or am I wrong?

Correct. Aren't they destroyed now?

Where?

hebasto commented at 3:57 PM on May 11, 2020: member

Concept ACK, but now it must destroy wallet models on quit, or am I wrong?

Correct. Aren't they destroyed now?

Where?

It seems the procedure starts in this destructor: https://github.com/bitcoin/bitcoin/blob/f71a3a8829746cb17fc42c7475cf03229ad6c08e/src/interfaces/wallet.cpp#L507

and through the WalletModel::unload() signal https://github.com/bitcoin/bitcoin/blob/f71a3a8829746cb17fc42c7475cf03229ad6c08e/src/qt/walletcontroller.cpp#L122-L134

it ends here: https://github.com/bitcoin/bitcoin/blob/f71a3a8829746cb17fc42c7475cf03229ad6c08e/src/qt/walletcontroller.cpp#L145-L156

DrahtBot added the label GUI on May 11, 2020

promag commented at 5:23 PM on May 11, 2020: member

Yes, but it should also delete when wallet controller is deleted.

hebasto commented at 5:39 PM on May 11, 2020: member

Yes, but it should also delete when wallet controller is deleted.

From the WalletController interface does not follow that its instance takes the ownership of WalletModel objects. What are cases when WalletModel objects could live until WalletController destructor is called?

promag commented at 6:35 PM on May 11, 2020: member

What are cases when WalletModel objects could live until WalletController destructor is called?

None. But before this change wallet models were owned and managed by wallet controller. I just think that it shouldn't depend on whether or not wallets are unloaded before destroying the window on exit. Suppose that with multiprocess you want to just restart the GUI process, in that case you want to just teardown the GUI, which includes wallet models.

tarboss commented at 9:27 AM on May 12, 2020: none

seems to work(quick check), but i still like the solution from branch 18. (auto delete of walletmodel from walletcontroller if u forget to delete & if u want send msgs from walletcontroller to three open walletmodels for example (future). Just on my mind atm. Checking l8ter a bit more.

DrahtBot commented at 10:09 PM on May 12, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

hebasto commented at 6:14 AM on May 13, 2020: member

@promag

Yes, but it should also delete when wallet controller is deleted.

I agree this is important for robustness and safety. Now I can see the following ways to move on:

drop this PR in favor of #18961
use wrapped setParent()

--- a/src/qt/walletcontroller.cpp
+++ b/src/qt/walletcontroller.cpp
@@ -110,7 +110,7 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
     WalletModel* wallet_model = new WalletModel(std::move(wallet), m_client_model, m_platform_style, nullptr);
     // Handler callback runs in a different thread so fix wallet model thread affinity.
     wallet_model->moveToThread(thread());
-    wallet_model->setParent(this);
+    QTimer::singleShot(0, this, [wallet_model, this]() { wallet_model->setParent(this); });
     m_wallets.push_back(wallet_model);
 
     // WalletModel::startPollBalance needs to be called in a thread managed by

enhance ~WalletController() by adding code to delete WalletModel objects, or use smth like deleteLater()

Which way is preferable?

tarboss commented at 3:21 PM on May 13, 2020: none

let promag decide what he prefers.

sidenote: if u breakpoint after: QTimer::singleShot(0, this, [wallet_model, this] () { wallet_model->setParent(this); }); wallet_model parent is "null"!!!

walletmodel's parent is set later - i hope - Queued Connection

tryphe commented at 6:08 AM on May 17, 2020: contributor

Concept ACK

hebasto force-pushed on May 20, 2020

hebasto commented at 6:34 AM on May 20, 2020: member

Updated d6afd1ca6477a1d2f80e0ead6015af67c5b62a9b -> 679b548504d2a541eecb41986629967c51650f61 (pr18948.01 -> pr18948.02, diff):

addressed @promag's comment:

Concept ACK, but now it must destroy wallet models on quit, or am I wrong?

promag commented at 10:47 AM on June 11, 2020: member

With lambda slots we could make this more elegant, something like:

WalletModel* wallet_model;
QMetaObject::invokeMethod(this, [this, &wallet_model] {
    wallet_model = new WalletModel(this);
    // ...
}, GUIUtil::blockingGUIThreadConnection());

hebasto commented at 6:15 PM on July 4, 2020: member

@promag

With lambda slots we could make this more elegant, something like:

WalletModel* wallet_model;
QMetaObject::invokeMethod(this, [this, &wallet_model] {
    wallet_model = new WalletModel(this);
    // ...
}, GUIUtil::blockingGUIThreadConnection());

Unfortunately, QMetaObject::invokeMethod(QObject*, Functor, Qt::ConnectionType, FunctorReturnType*) was introduced in Qt 5.10 only.

hebasto renamed this:
~~qt: Do not call setParent() for WalletModel instances~~
qt: Call setParent() in the parent's context
on Jul 4, 2020

promag commented at 10:31 PM on July 5, 2020: member

Ah yes, too soon then 😅

ryanofsky commented at 10:46 AM on July 22, 2020: member

Unfortunately, QMetaObject::invokeMethod(QObject*, Functor, Qt::ConnectionType, FunctorReturnType*) was introduced in Qt 5.10 only.

Not sure about the context, but it's trivial to replace this method. You can write a 2-line function that just connects the lambda to a temporary object slot and triggers the object. e.g. ObjectInvoke from #16874 (comment)

hebasto force-pushed on Jul 24, 2020

hebasto commented at 12:00 PM on July 24, 2020: member

Reworked.

The previous implementation was broken due to the fact the QTimer requires an event loop, that is not the case when a wallet is loaded in the RPC thread. @ryanofsky

Unfortunately, QMetaObject::invokeMethod(QObject*, Functor, Qt::ConnectionType, FunctorReturnType*) was introduced in Qt 5.10 only.

Not sure about the context, but it's trivial to replace this method. You can write a 2-line function that just connects the lambda to a temporary object slot and triggers the object. e.g. ObjectInvoke from #16874 (comment)

As this place is the only in the code base, I've chosen a different approach, that would be more convenient when migration to Qt 5.10+ occurs.

in src/qt/walletcontroller.cpp:130 in 3200c4492e outdated

 126 | @@ -127,7 +127,19 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
 127 |      WalletModel* wallet_model = new WalletModel(std::move(wallet), m_client_model, m_platform_style, nullptr);
 128 |      // Handler callback runs in a different thread so fix wallet model thread affinity.
 129 |      wallet_model->moveToThread(thread());
 130 | -    wallet_model->setParent(this);
 131 | +#if (QT_VERSION >= QT_VERSION_CHECK(5, 10, 0))

ryanofsky commented at 9:57 PM on August 12, 2020:

In commit "qt: Call setParent() in the parent's context" (3200c4492eafa462a0195c2320ba6b7acfc0b6ae)

I don't think we can toggle behavior with a macro based on qt version without test coverage, or without even comment explaining how to test the disabled case. Otherwise it seems too easy for code to be broken without anybody noticing.

Does the code in the #else block below not work in newer versions of Qt? Assuming it is forward compatible, it would seem safer to just always use the compatible code and have a TODO comment saying it can be upgraded if the minimum Qt version changes.

hebasto commented at 6:36 PM on August 14, 2020:

Updated.

in src/qt/walletcontroller.cpp:136 in 3200c4492e outdated

 132 | +    QMetaObject::invokeMethod(this, [wallet_model, this] {
 133 | +        wallet_model->setParent(this);
 134 | +    }, GUIUtil::blockingGUIThreadConnection());
 135 | +#else
 136 | +    {
 137 | +        QEventLoop event_loop;

ryanofsky commented at 10:07 PM on August 12, 2020:

In commit "qt: Call setParent() in the parent's context" (3200c4492eafa462a0195c2320ba6b7acfc0b6ae)

This workaround is confusing and I think should have some kind of comment explaining what it is doing and how it works. I think the goal is to get setParent to run in the the GUI thread rather than the calling thread, and to have the calling thread block until setParent returns. But it's unclear why setParent needs to run on the gui thread, or why setting a single shot timer would block until after the timer runs (instead of scheduling the timer asynchronously), or how declaring an event_loop variable that is never referenced would have any effect.

Honestly I think the stack overflow approach from https://github.com/ryanofsky/bitcoin/blob/eacd34144c1ee989c9ab43eb439fecca76addc76/src/qt/async_update.h#L18-L23 #18948 (comment) seems simpler and more lightweight, but this subjective. Any workaround seems fine if it has an explanation.

hebasto commented at 6:36 PM on August 14, 2020:

Updated.

in src/qt/walletcontroller.cpp:132 in 2d6672ebd3 outdated

 123 | @@ -124,8 +124,14 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
 124 |      }
 125 |  
 126 |      // Instantiate model and register it.
 127 | -    WalletModel* wallet_model = new WalletModel(std::move(wallet), m_client_model, m_platform_style, nullptr);
 128 | -    // Handler callback runs in a different thread so fix wallet model thread affinity.
 129 | +    WalletModel* wallet_model = new WalletModel(std::move(wallet), m_client_model, m_platform_style,
 130 | +                                                nullptr /* required for the following moveToThread() call */);

ryanofsky commented at 10:12 PM on August 12, 2020:

In commit "qt: Improve comments in WalletController::getOrCreateWallet()" (2d6672ebd3095fb99e8b0fe0227697916665bb95)

Nice comments, these do a good job of explaining the context

hebasto force-pushed on Aug 14, 2020

hebasto commented at 6:36 PM on August 14, 2020: member

Updated 2d6672ebd3095fb99e8b0fe0227697916665bb95 -> 34b83a1d9015ceea97d0f46376256710cb23fe0a (pr18948.03 -> pr18948.04, diff):

addressed @ryanofsky's comments
updated OP

ryanofsky approved

ryanofsky commented at 8:52 PM on August 24, 2020: member

Code review ACK 34b83a1d9015ceea97d0f46376256710cb23fe0a. Just suggested changes since review (thanks!) avoiding using preprocessor and timer.

fanquake commented at 1:45 AM on October 3, 2020: member

@promag can you take a look here again?

jonasschnelli commented at 10:08 AM on November 10, 2020: contributor

utACK 34b83a1d9015ceea97d0f46376256710cb23fe0a

DrahtBot added the label Needs rebase on Nov 19, 2020

qt: Add ObjectInvoke template function

This is a replacement of the QMetaObject::invokeMethod functor overload
which is available in Qt 5.10+.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

5659e73493

qt: Call setParent() in the parent's context

setParent(parent) calls sendEvent(parent, QChildEvent) that implies
running in the thread which created the parent object.

5fcfee68af

qt: Improve comments in WalletController::getOrCreateWallet() 8963b2c71f

hebasto force-pushed on Nov 25, 2020

hebasto commented at 2:19 PM on November 25, 2020: member

Rebased 34b83a1d9015ceea97d0f46376256710cb23fe0a -> 8963b2c71f120b2746396c4987392f0105c8dd60 (pr18948.04 -> pr18948.05) due to the conflict with https://github.com/bitcoin-core/gui/pull/46.

DrahtBot removed the label Needs rebase on Nov 25, 2020

ryanofsky approved

ryanofsky commented at 1:16 AM on December 2, 2020: member

Code review ACK 8963b2c71f120b2746396c4987392f0105c8dd60. No changes since last review, just rebase because of conflict on some adjacent lines

fanquake requested review from promag on Dec 2, 2020

fanquake requested review from jonasschnelli on Dec 2, 2020

jonasschnelli approved

jonasschnelli commented at 9:51 AM on December 2, 2020: contributor

utACK 8963b2c71f120b2746396c4987392f0105c8dd60

jonasschnelli merged this on Dec 2, 2020

jonasschnelli closed this on Dec 2, 2020

hebasto deleted the branch on Dec 2, 2020

sidhujag referenced this in commit 49825c4526 on Dec 2, 2020

DrahtBot locked this on Feb 15, 2022

hebasto commented at 5:30 PM on April 16, 2022: member

5659e73493fcdfb5d0cb9d686c24c4fbe1c217ed has been reverted in bitcoin-core/gui#587.