Fix onBodyContact being called on removed nodes

Since collision callbacks happen in the physx simulation thread we need
to store these callbacks. Otherwise, if an object is deleted in the same
frame a 'onBodyContact' signal is enqueued and a crash will happen.
Therefore we save these contact callbacks and run them at the end of the
physics frame when we know if the objects are deleted or not.

We move the mutex in QPhysicsWorld::deregisterNode to cover also the
decoupling of the physics node since this node can be read
asynchronously in the 'onContact' physx callback.

In the 'onContact' callback we check if the node is removed _before_
dereferencing it and checking 'm_backendObject'.

Fixes: QTBUG-121033
Pick-to: 6.7 6.6 6.5
Change-Id: I2412502baa6ede1897a4d1377e8c52a8339471d2
Reviewed-by: Andy Nichols <andy.nichols@qt.io>

8 files changed, 226 insertions, 5 deletions

diff --git a/src/quick3dphysics/physxnode/qphysxworld.cpp b/src/quick3dphysics/physxnode/qphysxworld.cpp
index 28b8f4b..e1d1a81 100644
--- a/src/quick3dphysics/physxnode/qphysxworld.cpp
+++ b/src/quick3dphysics/physxnode/qphysxworld.cpp
@@ -91,8 +91,9 @@ public:
                 QAbstractPhysicsNode *other =
                         static_cast<QAbstractPhysicsNode *>(pairHeader.actors[1]->userData);
 
-                if (!trigger || !other || !trigger->m_backendObject || !other->m_backendObject
-                    || world->isNodeRemoved(trigger) || world->isNodeRemoved(other))
+                if (!trigger || !other || world->isNodeRemoved(trigger)
+                    || world->isNodeRemoved(other) || !trigger->m_backendObject
+                    || !other->m_backendObject)
                     continue;
 
                 const bool triggerReceive =
@@ -130,9 +131,9 @@ public:
                 }
 
                 if (triggerReceive)
-                    trigger->registerContact(other, positions, impulses, normals);
+                    world->registerContact(other, trigger, positions, impulses, normals);
                 if (otherReceive)
-                    other->registerContact(trigger, positions, impulses, normalsInverted);
+                    world->registerContact(trigger, other, positions, impulses, normalsInverted);
             }
         }
     };
diff --git a/src/quick3dphysics/qphysicsworld.cpp b/src/quick3dphysics/qphysicsworld.cpp
index b07ec55..0790a83 100644
--- a/src/quick3dphysics/qphysicsworld.cpp
+++ b/src/quick3dphysics/qphysicsworld.cpp
@@ -371,18 +371,39 @@ void QPhysicsWorld::deregisterNode(QAbstractPhysicsNode *physicsNode)
 {
     for (auto world : worldManager.worlds) {
         world->m_newPhysicsNodes.removeAll(physicsNode);
+        QMutexLocker locker(&world->m_removedPhysicsNodesMutex);
         if (physicsNode->m_backendObject) {
             Q_ASSERT(physicsNode->m_backendObject->frontendNode == physicsNode);
             physicsNode->m_backendObject->frontendNode = nullptr;
             physicsNode->m_backendObject->isRemoved = true;
             physicsNode->m_backendObject = nullptr;
         }
-        QMutexLocker locker(&world->m_removedPhysicsNodesMutex);
         world->m_removedPhysicsNodes.insert(physicsNode);
     }
     worldManager.orphanNodes.removeAll(physicsNode);
 }
 
+void QPhysicsWorld::registerContact(QAbstractPhysicsNode *sender, QAbstractPhysicsNode *receiver,
+                                    const QVector<QVector3D> &positions,
+                                    const QVector<QVector3D> &impulses,
+                                    const QVector<QVector3D> &normals)
+{
+    // Since collision callbacks happen in the physx simulation thread we need
+    // to store these callbacks. Otherwise, if an object is deleted in the same
+    // frame a 'onBodyContact' signal is enqueued and a crash will happen.
+    // Therefore we save these contact callbacks and run them at the end of the
+    // physics frame when we know if the objects are deleted or not.
+
+    BodyContact contact;
+    contact.sender = sender;
+    contact.receiver = receiver;
+    contact.positions = positions;
+    contact.impulses = impulses;
+    contact.normals = normals;
+
+    m_registeredContacts.push_back(contact);
+}
+
 QPhysicsWorld::QPhysicsWorld(QObject *parent) : QObject(parent)
 {
     m_inDesignStudio = !qEnvironmentVariableIsEmpty("QML_PUPPET_MODE");
@@ -1161,6 +1182,7 @@ void QPhysicsWorld::initPhysics()
 void QPhysicsWorld::frameFinished(float deltaTime)
 {
     matchOrphanNodes();
+    emitContactCallbacks();
     cleanupRemovedNodes();
     for (auto *node : std::as_const(m_newPhysicsNodes)) {
         auto *body = node->createPhysXBackend();
@@ -1192,6 +1214,7 @@ void QPhysicsWorld::frameFinishedDesignStudio()
 {
     // Note sure if this is needed but do it anyway
     matchOrphanNodes();
+    emitContactCallbacks();
     cleanupRemovedNodes();
     // Ignore new physics nodes, we find them from the scene node anyway
     m_newPhysicsNodes.clear();
@@ -1274,6 +1297,19 @@ void QPhysicsWorld::findPhysicsNodes()
     }
 }
 
+void QPhysicsWorld::emitContactCallbacks()
+{
+    for (const QPhysicsWorld::BodyContact &contact : m_registeredContacts) {
+        if (m_removedPhysicsNodes.contains(contact.sender)
+            || m_removedPhysicsNodes.contains(contact.receiver))
+            continue;
+        contact.receiver->registerContact(contact.sender, contact.positions, contact.impulses,
+                                          contact.normals);
+    }
+
+    m_registeredContacts.clear();
+}
+
 physx::PxPhysics *QPhysicsWorld::getPhysics()
 {
     return StaticPhysXObjects::getReference().physics;
diff --git a/src/quick3dphysics/qphysicsworld_p.h b/src/quick3dphysics/qphysicsworld_p.h
index 173a566..56e58c5 100644
--- a/src/quick3dphysics/qphysicsworld_p.h
+++ b/src/quick3dphysics/qphysicsworld_p.h
@@ -112,6 +112,10 @@ public:
     static void registerNode(QAbstractPhysicsNode *physicsNode);
     static void deregisterNode(QAbstractPhysicsNode *physicsNode);
 
+    void registerContact(QAbstractPhysicsNode *sender, QAbstractPhysicsNode *receiver,
+                         const QVector<QVector3D> &positions, const QVector<QVector3D> &impulses,
+                         const QVector<QVector3D> &normals);
+
     Q_REVISION(6, 5) QQuick3DNode *viewport() const;
     void setHasIndividualDebugDraw();
     physx::PxControllerManager *controllerManager();
@@ -167,6 +171,16 @@ private:
     void disableDebugDraw();
     void matchOrphanNodes();
     void findPhysicsNodes();
+    void emitContactCallbacks();
+
+    struct BodyContact
+    {
+        QAbstractPhysicsNode *sender = nullptr;
+        QAbstractPhysicsNode *receiver = nullptr;
+        QVector<QVector3D> positions;
+        QVector<QVector3D> impulses;
+        QVector<QVector3D> normals;
+    };
 
     struct DebugModelHolder
     {
@@ -213,6 +227,7 @@ private:
             m_collisionShapeDebugModels;
     QSet<QAbstractPhysicsNode *> m_removedPhysicsNodes;
     QMutex m_removedPhysicsNodesMutex;
+    QList<BodyContact> m_registeredContacts;
 
     QVector3D m_gravity = QVector3D(0.f, -981.f, 0.f);
     float m_typicalLength = 100.f; // 100 cm
diff --git a/tests/auto/CMakeLists.txt b/tests/auto/CMakeLists.txt
index f605dc5..142cdeb 100644
--- a/tests/auto/CMakeLists.txt
+++ b/tests/auto/CMakeLists.txt
@@ -1,4 +1,5 @@
 add_subdirectory(callback)
+add_subdirectory(callback_create_delete_node)
 add_subdirectory(changescene)
 add_subdirectory(character)
 add_subdirectory(character_remove)
diff --git a/tests/auto/callback_create_delete_node/Box.qml b/tests/auto/callback_create_delete_node/Box.qml
new file mode 100644
index 0000000..a8137b3
--- /dev/null
+++ b/tests/auto/callback_create_delete_node/Box.qml
@@ -0,0 +1,21 @@
+// Copyright (C) 2024 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR BSD-3-Clause
+
+import QtQuick
+import QtQuick3D
+import QtQuick3D.Physics
+
+DynamicRigidBody {
+    Model {
+        source: "#Cube"
+        materials: PrincipledMaterial {
+            baseColor: "red"
+        }
+    }
+
+    sendContactReports: true
+    receiveContactReports: true
+    onBodyContact: (body, positions, impulses, normals) => {}
+
+    collisionShapes: BoxShape {}
+}
diff --git a/tests/auto/callback_create_delete_node/CMakeLists.txt b/tests/auto/callback_create_delete_node/CMakeLists.txt
new file mode 100644
index 0000000..2cc871e
--- /dev/null
+++ b/tests/auto/callback_create_delete_node/CMakeLists.txt
@@ -0,0 +1,22 @@
+# Copyright (C) 2024 The Qt Company Ltd.
+# SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+set(PROJECT_NAME "test_auto_callback_create_delete_node")
+
+qt_internal_add_test(${PROJECT_NAME}
+    GUI
+    QMLTEST
+    SOURCES
+        tst_callback_create_delete_node.cpp
+    LIBRARIES
+        Qt::Core
+        Qt::Qml
+    TESTDATA
+        tst_callback_create_delete_node.qml
+        Box.qml
+    BUILTIN_TESTDATA
+)
+
+if(QT_BUILD_STANDALONE_TESTS)
+    qt_import_qml_plugins(${PROJECT_NAME})
+endif()
diff --git a/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.cpp b/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.cpp
new file mode 100644
index 0000000..1e4324f
--- /dev/null
+++ b/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.cpp
@@ -0,0 +1,21 @@
+// Copyright (C) 2024 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+#include <QtQuickTest/quicktest.h>
+class tst_callback_create_delete_node : public QObject
+{
+    Q_OBJECT
+private slots:
+    void skiptest() { QSKIP("This test will fail, skipping."); };
+};
+int main(int argc, char **argv)
+{
+    if (!qEnvironmentVariableIsEmpty("QEMU_LD_PREFIX")) {
+        qWarning("This test is unstable on QEMU, so it will be skipped.");
+        tst_callback_create_delete_node skip;
+        return QTest::qExec(&skip, argc, argv);
+    }
+    QTEST_SET_MAIN_SOURCE_PATH
+    return quick_test_main(argc, argv, "tst_callback_create_delete_node", QUICK_TEST_SOURCE_DIR);
+}
+#include "tst_callback_create_delete_node.moc"
diff --git a/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.qml b/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.qml
new file mode 100644
index 0000000..c43db93
--- /dev/null
+++ b/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.qml
@@ -0,0 +1,104 @@
+// Copyright (C) 2024 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR BSD-3-Clause
+
+// Tests that removing and adding objects with active contact callbacks
+// does not crash. QTBUG-121033
+
+import QtCore
+import QtTest
+import QtQuick3D
+import QtQuick3D.Physics
+import QtQuick3D.Physics.Helpers
+import QtQuick
+
+Item {
+    width: 800
+    height: 600
+    visible: true
+
+    PhysicsWorld {
+        scene: viewport.scene
+    }
+
+    View3D {
+        id: viewport
+        width: parent.width
+        height: parent.height
+        focus: true
+
+        environment: SceneEnvironment {
+            antialiasingMode: SceneEnvironment.MSAA
+            backgroundMode: SceneEnvironment.Color
+            clearColor: "#f0f0f0"
+        }
+
+        PerspectiveCamera {
+            id: camera
+            position: Qt.vector3d(-400, 500, 1000)
+            eulerRotation: Qt.vector3d(-20, -20, 0)
+            clipFar: 5000
+            clipNear: 1
+        }
+
+        DirectionalLight {
+            eulerRotation: Qt.vector3d(-45, 45, 0)
+        }
+
+        Node {
+            id: shapeSpawner
+            property var instancesBoxes: []
+            property var boxComponent: Qt.createComponent("Box.qml")
+            property int numSpawns: 0
+
+            function createStack() {
+                let size = 10
+
+                for (var i = 0; i < 3; i++) {
+                    for (var j = 0; j < 3; j++) {
+                        let center = Qt.vector3d(j*100, 100*i, 0)
+                        let box = boxComponent.incubateObject(shapeSpawner, {
+                                                                  "position": center,
+                                                              })
+                        instancesBoxes.push(box)
+                    }
+                }
+
+                numSpawns = numSpawns + 1;
+            }
+
+            function reset() {
+                // Only run method if previous stack has been created fully
+                for (var i = 0; i < instancesBoxes.length; i++)
+                    if (!instancesBoxes[i].object)
+                        return
+
+                instancesBoxes.forEach(box => {
+                                           box.object.collisionShapes = []
+                                           box.object.destroy()
+                                       })
+                instancesBoxes = []
+
+                shapeSpawner.createStack()
+            }
+        }
+    }
+
+    FrameAnimation {
+        property int frame: 0
+        running: true
+        onTriggered: {
+            frame = frame + 1;
+            if (frame % 2 == 0) {
+                shapeSpawner.reset()
+            }
+        }
+    }
+
+    TestCase {
+        name: "100 cycles"
+        when: shapeSpawner.numSpawns > 100
+        function triggered() {}
+    }
+
+}
+