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

hebasto · 2020-05-11T14:39:55Z

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 (007e15d) 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, name = 'QThread', stop reason = signal SIGABRT
    frame #0: 0x00007ffff794518b libc.so.6`__GI_raise(sig=2) at raise.c:51:1
(lldb) bt
* thread #24, name = 'QThread', stop reason = signal SIGABRT
  * frame #0: 0x00007ffff794518b libc.so.6`__GI_raise(sig=2) at raise.c:51:1
    frame #1: 0x00007ffff7924859 libc.so.6`__GI_abort at abort.c:79:7
    frame #2: 0x0000555556508ec4 bitcoin-qt`::qt_message_fatal((null)=<unavailable>, context=<unavailable>, message=<unavailable>) at qlogging.cpp:1690:15
    frame #3: 0x00005555565099cf bitcoin-qt`QMessageLogger::fatal(this=<unavailable>, msg=<unavailable>) const at qlogging.cpp:796:21
    frame #4: 0x000055555650479d bitcoin-qt`qt_assert_x(where=<unavailable>, what=<unavailable>, file=<unavailable>, line=<unavailable>) at qglobal.cpp:3088:46
    frame #5: 0x0000555556685733 bitcoin-qt`QCoreApplicationPrivate::checkReceiverThread(receiver=0x0000555557b27510) at qcoreapplication.cpp:557:5
    frame #6: 0x00005555567ced86 bitcoin-qt`QApplication::notify(this=0x00007fffffffd4a0, receiver=0x0000555557b27510, e=0x00007fff9a7f8ce0) at qapplication.cpp:2956:27
    frame #7: 0x0000555556685d31 bitcoin-qt`QCoreApplication::notifyInternal2(receiver=0x0000555557b27510, event=0x00007fff9a7f8ce0) at qcoreapplication.cpp:1024:24
    frame #8: 0x00005555566c9224 bitcoin-qt`QObjectPrivate::setParent_helper(QObject*) [inlined] QCoreApplication::sendEvent(event=<unavailable>, receiver=<unavailable>) at qcoreapplication.h:233:59
    frame #9: 0x00005555566c9210 bitcoin-qt`QObjectPrivate::setParent_helper(this=0x00007fff85855260, o=0x0000555557b27510) at qobject.cpp:2036
    frame #10: 0x00005555566c9b41 bitcoin-qt`QObject::setParent(this=<unavailable>, parent=<unavailable>) at qobject.cpp:1980:24
    frame #11: 0x0000555555710be8 bitcoin-qt`WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> >) + 2534

...

Fixes #18835.

hebasto · 2020-05-11T14:46:14Z

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 · 2020-05-11T14:51:12Z

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

hebasto · 2020-05-11T14:54:40Z

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

Correct. Aren't they destroyed now?

promag · 2020-05-11T15:04:55Z

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

Correct. Aren't they destroyed now?

Where?

hebasto · 2020-05-11T15:57:29Z

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:

bitcoin/src/interfaces/wallet.cpp

Line 507 in f71a3a8

~WalletClientImpl() override { UnloadWallets(); }

and through the WalletModel::unload() signal

bitcoin/src/qt/walletcontroller.cpp

Lines 122 to 134 in f71a3a8

    
           connect(wallet_model, &WalletModel::unload, this, [this, wallet_model] { 
        
               // Defer removeAndDeleteWallet when no modal widget is active. 
        
               // TODO: remove this workaround by removing usage of QDiallog::exec. 
        
               if (QApplication::activeModalWidget()) { 
        
                   connect(qApp, &QApplication::focusWindowChanged, wallet_model, [this, wallet_model]() { 
        
                       if (!QApplication::activeModalWidget()) { 
        
                           removeAndDeleteWallet(wallet_model); 
        
                       } 
        
                   }, Qt::QueuedConnection); 
        
               } else { 
        
                   removeAndDeleteWallet(wallet_model); 
        
               } 
        
           }, Qt::QueuedConnection);

it ends here:

bitcoin/src/qt/walletcontroller.cpp

Lines 145 to 156 in f71a3a8

    
           void WalletController::removeAndDeleteWallet(WalletModel* wallet_model) 
        
           { 
        
               // Unregister wallet model. 
        
               { 
        
                   QMutexLocker locker(&m_mutex); 
        
                   m_wallets.erase(std::remove(m_wallets.begin(), m_wallets.end(), wallet_model)); 
        
               } 
        
               Q_EMIT walletRemoved(wallet_model); 
        
               // Currently this can trigger the unload since the model can hold the last 
        
               // CWallet shared pointer. 
        
               delete wallet_model; 
        
           }

promag · 2020-05-11T17:23:57Z

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

hebasto · 2020-05-11T17:39:42Z

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 · 2020-05-11T18:35:36Z

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 · 2020-05-12T09:27:18Z

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 · 2020-05-12T22:09:37Z

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

Conflicts

No conflicts as of last run.

hebasto · 2020-05-13T06:14:56Z

@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 gui: remove assert in walletcontroller & run setparent in gui-qt main thread #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 · 2020-05-13T15:21:07Z

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 · 2020-05-17T06:08:33Z

Concept ACK

hebasto · 2020-05-20T06:34:37Z

Updated d6afd1c -> 679b548 (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

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 · 2020-07-04T18:15:28Z

@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.

promag · 2020-07-05T22:31:42Z

Ah yes, too soon then 😅

ryanofsky · 2020-07-22T10:46:44Z

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 · 2020-07-24T12:00:49Z

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.

src/qt/walletcontroller.cpp

ryanofsky · 2020-08-12T22:07:20Z

src/qt/walletcontroller.cpp

+    }, GUIUtil::blockingGUIThreadConnection());
+#else
+    {
+        QEventLoop event_loop;


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

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.

ryanofsky · 2020-08-12T22:12:54Z

src/qt/walletcontroller.cpp

-    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.
+    WalletModel* wallet_model = new WalletModel(std::move(wallet), m_client_model, m_platform_style,
+                                                nullptr /* required for the following moveToThread() call */);


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

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

hebasto · 2020-08-14T18:36:02Z

Updated 2d6672e -> 34b83a1 (pr18948.03 -> pr18948.04, diff):

addressed @ryanofsky's comments
updated OP

ryanofsky

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

fanquake · 2020-10-03T01:45:31Z

@promag can you take a look here again?

jonasschnelli · 2020-11-10T10:08:54Z

utACK 34b83a1

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>

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

hebasto · 2020-11-25T14:19:37Z

Rebased 34b83a1 -> 8963b2c (pr18948.04 -> pr18948.05) due to the conflict with bitcoin-core/gui#46.

ryanofsky

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

jonasschnelli

utACK 8963b2c

8963b2c qt: Improve comments in WalletController::getOrCreateWallet() (Hennadii Stepanov) 5fcfee6 qt: Call setParent() in the parent's context (Hennadii Stepanov) 5659e73 qt: Add ObjectInvoke template function (Hennadii Stepanov) Pull request description: 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 (007e15d) 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, name = 'QThread', stop reason = signal SIGABRT frame #0: 0x00007ffff794518b libc.so.6`__GI_raise(sig=2) at raise.c:51:1 (lldb) bt * thread #24, name = 'QThread', stop reason = signal SIGABRT * frame #0: 0x00007ffff794518b libc.so.6`__GI_raise(sig=2) at raise.c:51:1 frame #1: 0x00007ffff7924859 libc.so.6`__GI_abort at abort.c:79:7 frame #2: 0x0000555556508ec4 bitcoin-qt`::qt_message_fatal((null)=<unavailable>, context=<unavailable>, message=<unavailable>) at qlogging.cpp:1690:15 frame #3: 0x00005555565099cf bitcoin-qt`QMessageLogger::fatal(this=<unavailable>, msg=<unavailable>) const at qlogging.cpp:796:21 frame #4: 0x000055555650479d bitcoin-qt`qt_assert_x(where=<unavailable>, what=<unavailable>, file=<unavailable>, line=<unavailable>) at qglobal.cpp:3088:46 frame #5: 0x0000555556685733 bitcoin-qt`QCoreApplicationPrivate::checkReceiverThread(receiver=0x0000555557b27510) at qcoreapplication.cpp:557:5 frame #6: 0x00005555567ced86 bitcoin-qt`QApplication::notify(this=0x00007fffffffd4a0, receiver=0x0000555557b27510, e=0x00007fff9a7f8ce0) at qapplication.cpp:2956:27 frame #7: 0x0000555556685d31 bitcoin-qt`QCoreApplication::notifyInternal2(receiver=0x0000555557b27510, event=0x00007fff9a7f8ce0) at qcoreapplication.cpp:1024:24 frame #8: 0x00005555566c9224 bitcoin-qt`QObjectPrivate::setParent_helper(QObject*) [inlined] QCoreApplication::sendEvent(event=<unavailable>, receiver=<unavailable>) at qcoreapplication.h:233:59 frame #9: 0x00005555566c9210 bitcoin-qt`QObjectPrivate::setParent_helper(this=0x00007fff85855260, o=0x0000555557b27510) at qobject.cpp:2036 frame #10: 0x00005555566c9b41 bitcoin-qt`QObject::setParent(this=<unavailable>, parent=<unavailable>) at qobject.cpp:1980:24 frame #11: 0x0000555555710be8 bitcoin-qt`WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> >) + 2534 ... ``` Fixes bitcoin#18835. ACKs for top commit: ryanofsky: Code review ACK 8963b2c. No changes since last review, just rebase because of conflict on some adjacent lines jonasschnelli: utACK 8963b2c Tree-SHA512: fef615904168717df3d8a0bd85eccc3eef990cc3e66c9fa280c8ef08ea009a7cb5a2a4f868ed0be3c0fe5bf683e8465850b5958deb896fdadd22d296186c9586

hebasto · 2022-04-16T17:30:15Z

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

hebasto mentioned this pull request May 11, 2020

qt: Fix OpenWalletActivity::open() #18883

Closed

DrahtBot added the GUI label May 11, 2020

DrahtBot mentioned this pull request May 12, 2020

gui: remove assert in walletcontroller & run setparent in gui-qt main thread #18961

Closed

hebasto force-pushed the 200511-qt-assert branch from d6afd1c to 679b548 Compare May 20, 2020 06:30

promag reviewed Jun 11, 2020

View reviewed changes

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

hebasto force-pushed the 200511-qt-assert branch from 679b548 to 2d6672e Compare July 24, 2020 11:53

ryanofsky reviewed Aug 12, 2020

View reviewed changes

hebasto force-pushed the 200511-qt-assert branch from 2d6672e to 34b83a1 Compare August 14, 2020 18:33

ryanofsky approved these changes Aug 24, 2020

View reviewed changes

DrahtBot added the Needs rebase label Nov 19, 2020

hebasto and others added 3 commits November 25, 2020 16:12

qt: Add ObjectInvoke template function

5659e73

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>

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

5fcfee6

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

qt: Improve comments in WalletController::getOrCreateWallet()

8963b2c

hebasto force-pushed the 200511-qt-assert branch from 34b83a1 to 8963b2c Compare November 25, 2020 14:17

DrahtBot removed the Needs rebase label Nov 25, 2020

ryanofsky approved these changes Dec 2, 2020

View reviewed changes

fanquake requested review from promag and jonasschnelli December 2, 2020 01:19

jonasschnelli approved these changes Dec 2, 2020

View reviewed changes

jonasschnelli merged commit c33662a into bitcoin:master Dec 2, 2020

hebasto deleted the 200511-qt-assert branch December 2, 2020 09:59

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hebasto commented May 11, 2020 •

edited

Loading

hebasto commented May 11, 2020

promag commented May 11, 2020

hebasto commented May 11, 2020

promag commented May 11, 2020

hebasto commented May 11, 2020

promag commented May 11, 2020

hebasto commented May 11, 2020

promag commented May 11, 2020

tarboss commented May 12, 2020

DrahtBot commented May 12, 2020 •

edited

Loading

hebasto commented May 13, 2020

tarboss commented May 13, 2020 •

edited

Loading

tryphe commented May 17, 2020

hebasto commented May 20, 2020

promag left a comment

hebasto commented Jul 4, 2020

promag commented Jul 5, 2020

ryanofsky commented Jul 22, 2020

hebasto commented Jul 24, 2020

ryanofsky Aug 12, 2020

hebasto Aug 14, 2020

ryanofsky Aug 12, 2020

hebasto commented Aug 14, 2020

ryanofsky left a comment

fanquake commented Oct 3, 2020

jonasschnelli commented Nov 10, 2020

hebasto commented Nov 25, 2020 •

edited

Loading

ryanofsky left a comment

jonasschnelli left a comment

hebasto commented Apr 16, 2022

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

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

Conversation

hebasto commented May 11, 2020 • edited Loading

hebasto commented May 11, 2020

promag commented May 11, 2020

hebasto commented May 11, 2020

promag commented May 11, 2020

hebasto commented May 11, 2020

promag commented May 11, 2020

hebasto commented May 11, 2020

promag commented May 11, 2020

tarboss commented May 12, 2020

DrahtBot commented May 12, 2020 • edited Loading

Conflicts

hebasto commented May 13, 2020

tarboss commented May 13, 2020 • edited Loading

tryphe commented May 17, 2020

hebasto commented May 20, 2020

promag left a comment

Choose a reason for hiding this comment

hebasto commented Jul 4, 2020

promag commented Jul 5, 2020

ryanofsky commented Jul 22, 2020

hebasto commented Jul 24, 2020

ryanofsky Aug 12, 2020

Choose a reason for hiding this comment

hebasto Aug 14, 2020

Choose a reason for hiding this comment

ryanofsky Aug 12, 2020

Choose a reason for hiding this comment

hebasto commented Aug 14, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

fanquake commented Oct 3, 2020

jonasschnelli commented Nov 10, 2020

hebasto commented Nov 25, 2020 • edited Loading

ryanofsky left a comment

Choose a reason for hiding this comment

jonasschnelli left a comment

Choose a reason for hiding this comment

hebasto commented Apr 16, 2022

hebasto commented May 11, 2020 •

edited

Loading

DrahtBot commented May 12, 2020 •

edited

Loading

tarboss commented May 13, 2020 •

edited

Loading

hebasto commented Nov 25, 2020 •

edited

Loading