From b33b615f4a4ac4d61bbce320783d5cca6edd91d1 Mon Sep 17 00:00:00 2001
From: Paul Lemire <paul.lemire@kdab.com>
Date: Thu, 31 Oct 2019 14:21:05 +0100
Subject: QNode: stop using hash<node,connection> for bookkeeping

It is totally valid to have actually the same node used for 2 distinct
connections (e.g setting 2 different node properties to the same node).
With the hash, the second setter call would overwrite the first connection
resulting in leaving a dangling connection around potentially resulting in
crashes.

Instead use a QVector<pair<node, connection>> and adjust code accordingly.

Change-Id: I49870c409c3f7b629c8f1bdfcb8757a904db2490
Reviewed-by: Mike Krus <mike.krus@kdab.com>
(cherry picked from commit 906f8a62f89a7ce2343a155e6db62616e66dc14b)
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>
---
 src/core/nodes/qnode.cpp |  5 ++---
 src/core/nodes/qnode_p.h | 21 +++++++++++++++------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index 900c3f8ce..c418e03d5 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -795,10 +795,9 @@ QNode::~QNode()
 {
     Q_D(QNode);
     // Disconnect each connection that was stored
-    for (auto it = d->m_destructionConnections.begin(), end = d->m_destructionConnections.end(); it != end; ++it)
-        QObject::disconnect(it.value());
+    for (const auto &nodeConnectionPair : qAsConst(d->m_destructionConnections))
+        QObject::disconnect(nodeConnectionPair.second);
     d->m_destructionConnections.clear();
-
     Q_EMIT nodeDestroyed();
 
     // Notify the backend that the parent lost this node as a child and
diff --git a/src/core/nodes/qnode_p.h b/src/core/nodes/qnode_p.h
index 511a0e562..86bfab85a 100644
--- a/src/core/nodes/qnode_p.h
+++ b/src/core/nodes/qnode_p.h
@@ -120,7 +120,7 @@ public:
         // If the node is destoyed, we make sure not to keep a dangling pointer to it
         Q_Q(QNode);
         auto f = [q, func]() { (static_cast<Caller *>(q)->*func)(nullptr); };
-        m_destructionConnections.insert(node, QObject::connect(node, &QNode::nodeDestroyed, f));
+        m_destructionConnections.push_back({node, QObject::connect(node, &QNode::nodeDestroyed, f)});
     }
 
     template<typename Caller, typename NodeType>
@@ -129,7 +129,7 @@ public:
         // If the node is destoyed, we make sure not to keep a dangling pointer to it
         Q_Q(QNode);
         auto f = [q, func, node]() { (static_cast<Caller *>(q)->*func)(node); };
-        m_destructionConnections.insert(node, QObject::connect(node, &QNode::nodeDestroyed, f));
+        m_destructionConnections.push_back({node, QObject::connect(node, &QNode::nodeDestroyed, f)});
     }
 
     template<typename Caller, typename ValueType>
@@ -142,7 +142,7 @@ public:
         // If the node is destoyed, we make sure not to keep a dangling pointer to it
         Q_Q(QNode);
         auto f = [q, func, resetValue]() { (static_cast<Caller *>(q)->*func)(resetValue); };
-        m_destructionConnections.insert(node, QObject::connect(node, &QNode::nodeDestroyed, f));
+        m_destructionConnections.push_back({node, QObject::connect(node, &QNode::nodeDestroyed, f)});
     }
 
     template<typename Caller, typename NodeType>
@@ -150,12 +150,21 @@ public:
     {
         // If the node is destoyed, we make sure not to keep a dangling pointer to it
         auto f = [this, func, node]() { (static_cast<Caller *>(this)->*func)(node); };
-        m_destructionConnections.insert(node, QObject::connect(node, &QNode::nodeDestroyed, f));
+        m_destructionConnections.push_back({node, QObject::connect(node, &QNode::nodeDestroyed, f)});
     }
 
     void unregisterDestructionHelper(QNode *node)
     {
-        QObject::disconnect(m_destructionConnections.take(node));
+        m_destructionConnections.erase(std::remove_if(m_destructionConnections.begin(),
+                                                      m_destructionConnections.end(),
+                                                      [this, node] (const QPair<QNode *, QMetaObject::Connection> &nodeConnectionPair) {
+                                                          if (nodeConnectionPair.first == node) {
+                                                              QObject::disconnect(nodeConnectionPair.second);
+                                                              return true;
+                                                          }
+                                                          return false;
+                                                      }),
+                                       m_destructionConnections.end());
     }
 
     static const QMetaObject *findStaticMetaObject(const QMetaObject *metaObject);
@@ -180,7 +189,7 @@ private:
     friend class PropertyChangeHandler<QNodePrivate>;
     bool m_propertyChangesSetup;
     PropertyChangeHandler<QNodePrivate> m_signals;
-    QHash<QNode *, QMetaObject::Connection> m_destructionConnections;
+    QVector<QPair<QNode *, QMetaObject::Connection>> m_destructionConnections;
 };
 
 class NodePostConstructorInit : public QObject
-- 
cgit v1.2.3