From ea828164e69eaf16c21468c909a76cd37e9018b5 Mon Sep 17 00:00:00 2001
From: Jonas Schnelli <dev@jonasschnelli.ch>
Date: Wed, 2 Dec 2020 10:51:42 +0100
Subject: [PATCH] Merge #18948: qt: Call setParent() in the parent's context

8963b2c71f120b2746396c4987392f0105c8dd60 qt: Improve comments in WalletController::getOrCreateWallet() (Hennadii Stepanov)
5fcfee68af47d4a891ae9c9964d73886f0f01d7d qt: Call setParent() in the parent's context (Hennadii Stepanov)
5659e73493fcdfb5d0cb9d686c24c4fbe1c217ed 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 (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, 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.

ACKs for top commit:
  ryanofsky:
    Code review ACK 8963b2c71f120b2746396c4987392f0105c8dd60. No changes since last review, just rebase because of conflict on some adjacent lines
  jonasschnelli:
    utACK 8963b2c71f120b2746396c4987392f0105c8dd60

Tree-SHA512: fef615904168717df3d8a0bd85eccc3eef990cc3e66c9fa280c8ef08ea009a7cb5a2a4f868ed0be3c0fe5bf683e8465850b5958deb896fdadd22d296186c9586
---
 src/qt/guiutil.h            | 14 ++++++++++++++
 src/qt/walletcontroller.cpp | 17 +++++++++++++----
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/src/qt/guiutil.h b/src/qt/guiutil.h
index f7625843dfb44..7db77634ea690 100644
--- a/src/qt/guiutil.h
+++ b/src/qt/guiutil.h
@@ -504,6 +504,20 @@ namespace GUIUtil
         return string.split(separator, QString::SkipEmptyParts);
     #endif
     }
+
+    /**
+     * Queue a function to run in an object's event loop. This can be
+     * replaced by a call to the QMetaObject::invokeMethod functor overload after Qt 5.10, but
+     * for now use a QObject::connect for compatibility with older Qt versions, based on
+     * https://stackoverflow.com/questions/21646467/how-to-execute-a-functor-or-a-lambda-in-a-given-thread-in-qt-gcd-style
+     */
+    template <typename Fn>
+    void ObjectInvoke(QObject* object, Fn&& function, Qt::ConnectionType connection = Qt::QueuedConnection)
+    {
+        QObject source;
+        QObject::connect(&source, &QObject::destroyed, object, std::forward<Fn>(function), connection);
+    }
+
 } // namespace GUIUtil
 
 #endif // BITCOIN_QT_GUIUTIL_H
diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp
index bf38880d9804a..28d51783631be 100644
--- a/src/qt/walletcontroller.cpp
+++ b/src/qt/walletcontroller.cpp
@@ -129,10 +129,20 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
     }
 
     // Instantiate model and register it.
-    WalletModel* wallet_model = new WalletModel(std::move(wallet), m_client_model, 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,
+                                                nullptr /* required for the following moveToThread() call */);
+
+    // Move WalletModel object to the thread that created the WalletController
+    // object (GUI main thread), instead of the current thread, which could be
+    // an outside wallet thread or RPC thread sending a LoadWallet notification.
+    // This ensures queued signals sent to the WalletModel object will be
+    // handled on the GUI event loop.
     wallet_model->moveToThread(thread());
-    wallet_model->setParent(this);
+    // setParent(parent) must be called in the thread which created the parent object. More details in #18948.
+    GUIUtil::ObjectInvoke(this, [wallet_model, this] {
+        wallet_model->setParent(this);
+    }, GUIUtil::blockingGUIThreadConnection());
+
     m_wallets.push_back(wallet_model);
 
     // WalletModel::startPollBalance needs to be called in a thread managed by
@@ -158,7 +168,6 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
     // Re-emit coinsSent signal from wallet model.
     connect(wallet_model, &WalletModel::coinsSent, this, &WalletController::coinsSent);
 
-    // Notify walletAdded signal on the GUI thread.
     Q_EMIT walletAdded(wallet_model);
 
     return wallet_model;