Fix removal of components when they are destroyed

Automatic removal of components when they are destroyed is based on
connecting to the destroyed() signal. This however means that by the
time removeComponent() is called, the pointer is no longer a valid
QComponent (just a QNode). While accessing member data of derived
classes such as nodeId is fine, emitting signals from derived class
does nothing, and in some cases asserts.

Fix this by:
- doing the QComponent clean up from it's destructor
- implementing a separate method on QEntity to simply clear the now
  partly invalid pointer from the list.

Change-Id: Id7632ee2ceaff6548c44c7a43ae40a0372febde9
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>
 
 
 

5 files changed, 44 insertions, 3 deletions

diff --git a/src/core/nodes/qcomponent.cpp b/src/core/nodes/qcomponent.cpp
index 5ca49ff30..f67989b1e 100644
--- a/src/core/nodes/qcomponent.cpp
+++ b/src/core/nodes/qcomponent.cpp
@@ -134,10 +134,13 @@ QComponent::~QComponent()
 {
     Q_D(QComponent);
 
-    for (QEntity *entity : qAsConst(d->m_entities)) {
+    // iterate on copy since removeEntity removes from the list, invalidating the iterator
+    const auto entities = std::move(d->m_entities);
+    for (QEntity *entity : entities) {
         QEntityPrivate *entityPimpl = static_cast<QEntityPrivate *>(QEntityPrivate::get(entity));
         if (entityPimpl)
             entityPimpl->m_components.removeAll(this);
+        d->removeEntity(entity);
     }
 }
 
diff --git a/src/core/nodes/qentity.cpp b/src/core/nodes/qentity.cpp
index 6a24b1956..4c2081680 100644
--- a/src/core/nodes/qentity.cpp
+++ b/src/core/nodes/qentity.cpp
@@ -88,6 +88,26 @@ QEntityPrivate::~QEntityPrivate()
 {
 }
 
+/*! \internal */
+void QEntityPrivate::removeDestroyedComponent(QComponent *comp)
+{
+    // comp is actually no longer a QComponent, just a QObject
+
+    Q_CHECK_PTR(comp);
+    qCDebug(Nodes) << Q_FUNC_INFO << comp;
+    Q_Q(QEntity);
+
+    if (m_changeArbiter) {
+        const auto componentRemovedChange = QComponentRemovedChangePtr::create(q, comp);
+        notifyObservers(componentRemovedChange);
+    }
+
+    m_components.removeOne(comp);
+
+    // Remove bookkeeping connection
+    unregisterDestructionHelper(comp);
+}
+
 /*!
     Constructs a new Qt3DCore::QEntity instance with \a parent as parent.
  */
@@ -157,7 +177,7 @@ void QEntity::addComponent(QComponent *comp)
     d->m_components.append(comp);
 
     // Ensures proper bookkeeping
-    d->registerDestructionHelper(comp, &QEntity::removeComponent, d->m_components);
+    d->registerPrivateDestructionHelper(comp, &QEntityPrivate::removeDestroyedComponent);
 
     if (d->m_changeArbiter) {
         const auto componentAddedChange = QComponentAddedChangePtr::create(this, comp);
diff --git a/src/core/nodes/qentity_p.h b/src/core/nodes/qentity_p.h
index 8fe03cd6b..65c9278da 100644
--- a/src/core/nodes/qentity_p.h
+++ b/src/core/nodes/qentity_p.h
@@ -82,6 +82,7 @@ public :
         return typedComponents;
     }
 
+    void removeDestroyedComponent(QComponent *comp);
 
     QComponentVector m_components;
     mutable QNodeId m_parentEntityId;
diff --git a/src/core/nodes/qnode_p.h b/src/core/nodes/qnode_p.h
index 8b43c2695..361a3b75b 100644
--- a/src/core/nodes/qnode_p.h
+++ b/src/core/nodes/qnode_p.h
@@ -145,6 +145,14 @@ public:
         m_destructionConnections.insert(node, QObject::connect(node, &QNode::nodeDestroyed, f));
     }
 
+    template<typename Caller, typename NodeType>
+    void registerPrivateDestructionHelper(NodeType *node, DestructionFunctionPointer<Caller, NodeType> func)
+    {
+        // 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));
+    }
+
     void unregisterDestructionHelper(QNode *node)
     {
         QObject::disconnect(m_destructionConnections.take(node));
diff --git a/tests/auto/core/qentity/tst_qentity.cpp b/tests/auto/core/qentity/tst_qentity.cpp
index ef520d7f2..bc0707f05 100644
--- a/tests/auto/core/qentity/tst_qentity.cpp
+++ b/tests/auto/core/qentity/tst_qentity.cpp
@@ -655,10 +655,19 @@ void tst_Entity::checkComponentBookkeeping()
         QCOMPARE(rootEntity->components().size(), 1);
 
         // WHEN
-        rootEntity.reset();
+        int sigCount = 0;
+        QObject *sigSender = comp.data();
+        connect(comp.data(), &QComponent::removedFromEntity, [&sigCount, sigSender](QEntity *) {
+            QComponent *c = qobject_cast<QComponent *>(sigSender);
+            if (sigSender && c)
+                sigCount++; // test the sender is still a QComponent when signal is emitted
+        });
+
         comp.reset();
+        rootEntity.reset();
 
         // THEN (Should not crash when the comp is destroyed (tests for failed removal of destruction helper)
+        QCOMPARE(sigCount, 1);
     }
 }