Merge #801: Fix nullptr clientModel access during shutdown

b7aa717 refactor: gui, simplify boost signals disconnection (furszy)
f3a612f gui: guard accessing a nullptr 'clientModel' (furszy)

Pull request description:

  Fixing #800.

  During shutdown, already queue events dispatched from the backend such
  'numConnectionsChanged' and 0networkActiveChanged' could try to access
  the clientModel object, which might not exist because we manually delete
  it inside 'BitcoinApplication::requestShutdown()'.

  This happen because boost does not clears the queued events when they arise
  concurrently with the signal disconnection (see https://www.boost.org/doc/libs/1_55_0/doc/html/signals2/thread-safety.html).
  From the docs:
  1) "Note that since we unlock the connection's mutex before executing its associated slot, it is possible a slot will still be executing after it has been disconnected by a [connection::disconnect](https://www.boost.org/doc/libs/1_55_0/doc/html/boost/signals2/connection.html#idp89761576-bb)(), if the disconnect was called concurrently with signal invocation."
  2)  "The fact that concurrent signal invocations use the same combiner object means you need to insure any custom combiner you write is thread-safe"

  So, we need to guard `clientModel` before accessing it at the handler side.

ACKs for top commit:
  hebasto:
    re-ACK b7aa717

Tree-SHA512: f1a21d69248628f6a13556a9438c9e4ea9f0a3678aab09ddfe836e78e4eee405a6730d37d39f1445068ada3a110b655b619cf0e090fc2d0cdf99bed061364aeb

Author: hebasto

src/qt/bitcoin.cpp

@@ -372,6 +372,11 @@ void BitcoinApplication::requestShutdown()
     // Request node shutdown, which can interrupt long operations, like
     // rescanning a wallet.
     node().startShutdown();
+    // Prior to unsetting the client model, stop listening backend signals
+    if (clientModel) {
+        clientModel->stop();
+    }
+
     // Unsetting the client model can cause the current thread to wait for node
     // to complete an operation, like wait for a RPC execution to complete.
     window->setClientModel(nullptr);

src/qt/bitcoingui.cpp

@@ -989,6 +989,7 @@ void BitcoinGUI::gotoLoadPSBT(bool from_clipboard)
 
 void BitcoinGUI::updateNetworkState()
 {
+    if (!clientModel) return;
     int count = clientModel->getNumConnections();
     QString icon;
     switch(count)

src/qt/clientmodel.cpp

@@ -70,14 +70,19 @@ ClientModel::ClientModel(interfaces::Node& node, OptionsModel *_optionsModel, QO
     subscribeToCoreSignals();
 }
 
-ClientModel::~ClientModel()
+void ClientModel::stop()
 {
     unsubscribeFromCoreSignals();
 
     m_thread->quit();
     m_thread->wait();
 }
 
+ClientModel::~ClientModel()
+{
+    stop();
+}
+
 int ClientModel::getNumConnections(unsigned int flags) const
 {
     ConnectionDirection connections = ConnectionDirection::None;
@@ -238,47 +243,41 @@ void ClientModel::TipChanged(SynchronizationState sync_state, interfaces::BlockT
 
 void ClientModel::subscribeToCoreSignals()
 {
-    m_handler_show_progress = m_node.handleShowProgress(
+    m_event_handlers.emplace_back(m_node.handleShowProgress(
         [this](const std::string& title, int progress, [[maybe_unused]] bool resume_possible) {
             Q_EMIT showProgress(QString::fromStdString(title), progress);
-        });
-    m_handler_notify_num_connections_changed = m_node.handleNotifyNumConnectionsChanged(
+        }));
+    m_event_handlers.emplace_back(m_node.handleNotifyNumConnectionsChanged(
         [this](int new_num_connections) {
             Q_EMIT numConnectionsChanged(new_num_connections);
-        });
-    m_handler_notify_network_active_changed = m_node.handleNotifyNetworkActiveChanged(
+        }));
+    m_event_handlers.emplace_back(m_node.handleNotifyNetworkActiveChanged(
         [this](bool network_active) {
             Q_EMIT networkActiveChanged(network_active);
-        });
-    m_handler_notify_alert_changed = m_node.handleNotifyAlertChanged(
+        }));
+    m_event_handlers.emplace_back(m_node.handleNotifyAlertChanged(
         [this]() {
             qDebug() << "ClientModel: NotifyAlertChanged";
             Q_EMIT alertsChanged(getStatusBarWarnings());
-        });
-    m_handler_banned_list_changed = m_node.handleBannedListChanged(
+        }));
+    m_event_handlers.emplace_back(m_node.handleBannedListChanged(
         [this]() {
             qDebug() << "ClienModel: Requesting update for peer banlist";
             QMetaObject::invokeMethod(banTableModel, [this] { banTableModel->refresh(); });
-        });
-    m_handler_notify_block_tip = m_node.handleNotifyBlockTip(
+        }));
+    m_event_handlers.emplace_back(m_node.handleNotifyBlockTip(
         [this](SynchronizationState sync_state, interfaces::BlockTip tip, double verification_progress) {
             TipChanged(sync_state, tip, verification_progress, SyncType::BLOCK_SYNC);
-        });
-    m_handler_notify_header_tip = m_node.handleNotifyHeaderTip(
+        }));
+    m_event_handlers.emplace_back(m_node.handleNotifyHeaderTip(
         [this](SynchronizationState sync_state, interfaces::BlockTip tip, bool presync) {
             TipChanged(sync_state, tip, /*verification_progress=*/0.0, presync ? SyncType::HEADER_PRESYNC : SyncType::HEADER_SYNC);
-        });
+        }));
 }
 
 void ClientModel::unsubscribeFromCoreSignals()
 {
-    m_handler_show_progress->disconnect();
-    m_handler_notify_num_connections_changed->disconnect();
-    m_handler_notify_network_active_changed->disconnect();
-    m_handler_notify_alert_changed->disconnect();
-    m_handler_banned_list_changed->disconnect();
-    m_handler_notify_block_tip->disconnect();
-    m_handler_notify_header_tip->disconnect();
+    m_event_handlers.clear();
 }
 
 bool ClientModel::getProxyInfo(std::string& ip_port) const

src/qt/clientmodel.h

@@ -58,6 +58,8 @@ class ClientModel : public QObject
     explicit ClientModel(interfaces::Node& node, OptionsModel *optionsModel, QObject *parent = nullptr);
     ~ClientModel();
 
+    void stop();
+
     interfaces::Node& node() const { return m_node; }
     OptionsModel *getOptionsModel();
     PeerTableModel *getPeerTableModel();
@@ -95,13 +97,7 @@ class ClientModel : public QObject
 
 private:
     interfaces::Node& m_node;
-    std::unique_ptr<interfaces::Handler> m_handler_show_progress;
-    std::unique_ptr<interfaces::Handler> m_handler_notify_num_connections_changed;
-    std::unique_ptr<interfaces::Handler> m_handler_notify_network_active_changed;
-    std::unique_ptr<interfaces::Handler> m_handler_notify_alert_changed;
-    std::unique_ptr<interfaces::Handler> m_handler_banned_list_changed;
-    std::unique_ptr<interfaces::Handler> m_handler_notify_block_tip;
-    std::unique_ptr<interfaces::Handler> m_handler_notify_header_tip;
+    std::vector<std::unique_ptr<interfaces::Handler>> m_event_handlers;
     OptionsModel *optionsModel;
     PeerTableModel* peerTableModel{nullptr};
     PeerTableSortProxy* m_peer_table_sort_proxy{nullptr};

src/qt/rpcconsole.cpp

@@ -966,6 +966,7 @@ void RPCConsole::message(int category, const QString &message, bool html)
 
 void RPCConsole::updateNetworkState()
 {
+    if (!clientModel) return;
     QString connections = QString::number(clientModel->getNumConnections()) + " (";
     connections += tr("In:") + " " + QString::number(clientModel->getNumConnections(CONNECTIONS_IN)) + " / ";
     connections += tr("Out:") + " " + QString::number(clientModel->getNumConnections(CONNECTIONS_OUT)) + ")";