Improve the QIfSimulationEngine recursion guard

When onIsInitialized is called we are already in the initialize
function. At this point calling another function in the simulation
is prevented by the recursion guard.
The same happened when trying to trigger an property update in
the onPropertyChanged handler.

The new recursion guard is part of the QIF_SIMULATION_TRY_CALL_FUNC
macro and makes sure direct recursions are prevented within the
simulation engine, but still allows updates from within the
signal or property changed handlers in the UI QML code.

Change-Id: I3f8dcaf09030eaf58bad53246357e6be96fcc55d
Reviewed-by: Robert Griebl <robert.griebl@qt.io>
(cherry picked from commit 6c1a2e54d97c01a99faf181ce0bdc70dc9a83dd9)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

15 files changed, 300 insertions, 16 deletions

diff --git a/.gitignore b/.gitignore
index ee83038d..c089d900 100644
--- a/.gitignore
+++ b/.gitignore
@@ -23,6 +23,7 @@ tst_*
 !tst_*.h
 !tst_*.tpl
 !tst_*.qface
+!tst_*.qml
 *Makefile.tst_*
 
 # Directories to ignore
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 64002817..81e353c1 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -15,7 +15,7 @@ set(QT_REPO_NOT_WARNINGS_CLEAN TRUE)
 set(QT_NO_INTERNAL_COMPATIBILITY_FUNCTIONS TRUE)
 
 find_package(Qt6 ${PROJECT_VERSION} CONFIG REQUIRED COMPONENTS BuildInternals Core)
-find_package(Qt6 ${PROJECT_VERSION} CONFIG OPTIONAL_COMPONENTS Gui Qml Quick RemoteObjects Multimedia Sql DBus Widgets)
+find_package(Qt6 ${PROJECT_VERSION} CONFIG OPTIONAL_COMPONENTS Gui Qml Quick QuickTest RemoteObjects Multimedia Sql DBus Widgets)
 
 include(src/interfaceframework/Qt6InterfaceFrameworkMacros.cmake)
 
diff --git a/src/interfaceframework/qifsimulationproxy.cpp b/src/interfaceframework/qifsimulationproxy.cpp
index b2b7be95..10c0ad0c 100644
--- a/src/interfaceframework/qifsimulationproxy.cpp
+++ b/src/interfaceframework/qifsimulationproxy.cpp
@@ -12,7 +12,9 @@
 #include <private/qmetaobjectbuilder_p.h>
 
 QT_BEGIN_NAMESPACE
-Q_LOGGING_CATEGORY(qLcIfSimulationEngine, "qt.if.simulationengine");
+
+Q_LOGGING_CATEGORY(qLcIfSimulationEngine, "qt.if.simulationengine")
+Q_LOGGING_CATEGORY(qLcIfRecGuard, "qt.if.simulationengine.recursionguard")
 
 namespace qtif_private {
 
@@ -172,13 +174,6 @@ bool QIfSimulationProxyBase::callQmlMethod(const char *function, QGenericReturnA
     if (m_noSimulationEngine)
         return false;
 
-    //Prevent recursion
-    static bool recursionGuard = false;
-    if (recursionGuard)
-        return false;
-
-    recursionGuard = true;
-
     bool functionExecuted = false;
     const QMetaObject *mo = metaObject();
 
@@ -195,7 +190,6 @@ bool QIfSimulationProxyBase::callQmlMethod(const char *function, QGenericReturnA
             break;
         }
     }
-    recursionGuard = false;
     return functionExecuted;
 }
 
diff --git a/src/interfaceframework/qifsimulationproxy.h b/src/interfaceframework/qifsimulationproxy.h
index d9e5d9ba..16b152b2 100644
--- a/src/interfaceframework/qifsimulationproxy.h
+++ b/src/interfaceframework/qifsimulationproxy.h
@@ -17,6 +17,7 @@
 QT_BEGIN_NAMESPACE
 
 Q_DECLARE_LOGGING_CATEGORY(qLcIfSimulationEngine)
+Q_QTINTERFACEFRAMEWORK_EXPORT Q_DECLARE_LOGGING_CATEGORY(qLcIfRecGuard)
 
 class QIfSimulationEngine;
 
@@ -147,6 +148,7 @@ namespace qtif_private {
 
         static QMetaObject staticMetaObject;
         static QList<QIfSimulationProxy<T> *> proxies;
+        static QByteArray recursionGuard;
 
     private:
         static QIfSimulationEngine *m_engine;
@@ -157,14 +159,64 @@ namespace qtif_private {
     template <typename T> T *QIfSimulationProxy<T>::m_instance = nullptr;
     template <typename T> QIfSimulationEngine *QIfSimulationProxy<T>::m_engine = nullptr;
     template <typename T> QList<QIfSimulationProxy<T> *> QIfSimulationProxy<T>::proxies =  QList<QIfSimulationProxy<T> *>();
+    template <typename T> QByteArray QIfSimulationProxy<T>::recursionGuard = QByteArray();
+
+    template <class T> class RecursionGuard
+    {
+    public:
+        RecursionGuard() {
+            m_savedValue = qtif_private::QIfSimulationProxy<T>::recursionGuard;
+        }
+
+        ~RecursionGuard() {
+            qCDebug(qLcIfRecGuard, "Reset recursion guard to: %s", m_savedValue.constData());
+            // When the recursion guards gets destroyed we want to restore the previous value.
+            // This helps to prevent recursions also for the following script call.
+            qtif_private::QIfSimulationProxy<T>::recursionGuard = m_savedValue;
+        }
+
+        bool trySet(const QByteArray &value) {
+            // Try to set the recursion guard to a new value
+            // If the guard is already at that value, this calls fails and QIF_SIMULATION_TRY_CALL_FUNC
+            // can act accordingly and not call the QML function again.
+            if (qtif_private::QIfSimulationProxy<T>::recursionGuard == value) {
+                qCDebug(qLcIfRecGuard, "Prevent recursion calling: %s", value.constData());
+                return false;
+            }
+
+            qCDebug(qLcIfRecGuard, "Update recursion guard to: %s", value.constData());
+            qtif_private::QIfSimulationProxy<T>::recursionGuard = value;
+            return true;
+        }
+
+        void saveAndRelease() {
+            // If the recursion is prevented this function can be used to save the current
+            // guard for later and reset the guard.
+            // Reseting the guard is important as a signal might be emitted after this call.
+            // If the slots for those signals are executed directly, new calls to the simulation
+            // (via the QtIF frontend API) are allowed.
+            qCDebug(qLcIfRecGuard, "Disable recursion guard and saving current value");
+            m_savedValue = qtif_private::QIfSimulationProxy<T>::recursionGuard;
+            qtif_private::QIfSimulationProxy<T>::recursionGuard.clear();
+        }
+
+        private:
+            QByteArray m_savedValue;
+    };
 }
 
+
 #define QIF_SIMULATION_TRY_CALL_FUNC(instance_type, function, ret_func, ...) \
-for (auto _qif_instance : qtif_private::QIfSimulationProxy<instance_type>::proxies) { \
-    QVariant return_value; \
-    if (_qif_instance->callQmlMethod(function, return_value, ##__VA_ARGS__)) { \
-        ret_func; \
+qtif_private::RecursionGuard<instance_type> _guard; \
+if (_guard.trySet(function)) { \
+    for (auto _qif_instance : qtif_private::QIfSimulationProxy<instance_type>::proxies) { \
+        QVariant return_value; \
+        if (_qif_instance->callQmlMethod(function, return_value, ##__VA_ARGS__)) { \
+            ret_func; \
+        } \
     } \
+} else { \
+    _guard.saveAndRelease(); \
 } \
 
 
diff --git a/src/tools/ifcodegen/templates/common/backend_simulation.qml.tpl b/src/tools/ifcodegen/templates/common/backend_simulation.qml.tpl
index 17958abb..26a01324 100644
--- a/src/tools/ifcodegen/templates/common/backend_simulation.qml.tpl
+++ b/src/tools/ifcodegen/templates/common/backend_simulation.qml.tpl
@@ -12,12 +12,12 @@ import {{module|qml_type}}.simulation
 {% set interface_zoned = interface.tags.config and interface.tags.config.zoned %}
 
 QtObject {
-    property var settings : IfSimulator.findData(IfSimulator.simulationData, "{{interface}}")
+    property var settings: IfSimulator.findData(IfSimulator.simulationData, "{{interface}}")
     property bool defaultInitialized: false
     property LoggingCategory qLc{{interface|upperfirst}}: LoggingCategory {
         name: "{{module|qml_type|lower}}.simulation.{{interface|lower}}backend"
     }
-    property var backend : {{interface|upperfirst}}Backend {
+    property var backend: {{interface|upperfirst}}Backend {
 
         function initialize() {
             console.log(qLc{{interface|upperfirst}}, "INITIALIZE")
diff --git a/tests/auto/core/ifcodegen/CMakeLists.txt b/tests/auto/core/ifcodegen/CMakeLists.txt
index 3e816f04..4a171934 100644
--- a/tests/auto/core/ifcodegen/CMakeLists.txt
+++ b/tests/auto/core/ifcodegen/CMakeLists.txt
@@ -6,6 +6,7 @@ add_subdirectory(flat-cmake-hierarchy-test)
 add_subdirectory(org-example-echo-noprivate)
 add_subdirectory(org-example-echo-noanno)
 add_subdirectory(include-test)
+add_subdirectory(simulation-behavior)
 if(QT_FEATURE_remoteobjects)
     add_subdirectory(org-example-echo-qtro)
 endif()
diff --git a/tests/auto/core/ifcodegen/custom-template/frontend.yaml b/tests/auto/core/ifcodegen/custom-template/frontend.yaml
new file mode 100644
index 00000000..e69de29b
--- /dev/null
+++ b/tests/auto/core/ifcodegen/custom-template/frontend.yaml
diff --git a/tests/auto/core/ifcodegen/simulation-behavior/CMakeLists.txt b/tests/auto/core/ifcodegen/simulation-behavior/CMakeLists.txt
new file mode 100644
index 00000000..62eb8f06
--- /dev/null
+++ b/tests/auto/core/ifcodegen/simulation-behavior/CMakeLists.txt
@@ -0,0 +1,42 @@
+set(CMAKE_AUTOMOC ON)
+
+add_subdirectory(frontend)
+add_subdirectory(qmlplugin)
+add_subdirectory(simulator)
+
+#####################################################################
+## tst_simulation-behavior Test:
+#####################################################################
+
+# Collect test data
+file(GLOB_RECURSE test_data_glob
+    RELATIVE ${CMAKE_CURRENT_SOURCE_DIR}
+    ${CMAKE_CURRENT_SOURCE_DIR}/data/*.qml)
+list(APPEND test_data ${test_data_glob})
+
+qt_internal_add_test(tst_simulation-behavior
+    GUI
+    QMLTEST
+    SOURCES
+        tst_simulation-behavior.cpp
+    PUBLIC_LIBRARIES
+        Qt::Gui
+
+    TESTDATA ${test_data}
+    QML_IMPORTPATH
+            ${CMAKE_CURRENT_BINARY_DIR}/imports/
+)
+
+#####################################################################
+## Scopes:
+#####################################################################
+
+qt_internal_extend_target(tst_simulation-behavior CONDITION ANDROID OR IOS
+    DEFINES
+        QT_QMLTEST_DATADIR=\\\":/data\\\"
+)
+
+qt_internal_extend_target(tst_simulation-behavior  CONDITION NOT ANDROID AND NOT IOS
+    DEFINES
+        QT_QMLTEST_DATADIR=\\\"${CMAKE_CURRENT_SOURCE_DIR}/data\\\"
+)
diff --git a/tests/auto/core/ifcodegen/simulation-behavior/data/tst_simulation-behavior.qml b/tests/auto/core/ifcodegen/simulation-behavior/data/tst_simulation-behavior.qml
new file mode 100644
index 00000000..144ed8f6
--- /dev/null
+++ b/tests/auto/core/ifcodegen/simulation-behavior/data/tst_simulation-behavior.qml
@@ -0,0 +1,74 @@
+// Copyright (C) 2022 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+import QtTest
+import QtQml
+import Simu
+
+TestCase {
+    SomeInterface {
+        id: someInterface
+        onIsInitializedChanged: (val) => {
+            verify(val);
+            addOne(41).then(function(result) {
+                compare(result, 42);
+            }, function() {
+                verify(false);
+            });
+        }
+    }
+
+    SignalSpy {
+        id: isInitializedChangedSpy
+        target: someInterface
+        signalName: "isInitializedChanged"
+    }
+
+    // Calling a function directly from the
+    // onIsInitializedChanged handler should
+    // be possible and not prevented by the
+    // recursion guard in the simulation engine.
+    function test_immediateCall() {
+        isInitializedChangedSpy.wait();
+        wait(200);
+    }
+
+    // The property value is hardcoded in the
+    // simulation. The setter causes a recursion
+    // which should be detected by the recursion guard
+    // The onTestProeprtyChanged handler is setting it
+    // again, causing the same behavior to be executed
+    // and should cause another (wanted) recursion
+    // until the property is settled as it's already correct.
+    Connections {
+        id: hpConnections
+        target: someInterface
+        enabled: false
+        property bool changeSignalCalled: false
+        function onHardcodedPropertyChanged() {
+            changeSignalCalled = true;
+            console.log("setHardcodedProperty 10")
+            someInterface.hardcodedProperty = 10;
+        }
+    }
+
+    function test_allowCallsInOnChangedHandler() {
+        hpConnections.enabled = true
+        console.log("init hardcodedProperty 10")
+        someInterface.hardcodedProperty = 10;
+        // This is the value hardcoded in the backend
+        // If the recursion guard doesn't work correctly,
+        // the second call doesn't hit the simulation and
+        // would cause the backend to change the value to 10.
+        compare(someInterface.hardcodedProperty, 1);
+        compare(hpConnections.changeSignalCalled, true);
+    }
+
+    // The simulation updates the same property multiple times
+    // all those calls would mean a recursion and need to be prevented
+    function test_preventRecursionOnMultipleSimulationCalls() {
+        console.log("init otherHardcodedProperty 10")
+        someInterface.otherHardcodedProperty = 10;
+        compare(someInterface.otherHardcodedProperty, 2);
+    }
+}
diff --git a/tests/auto/core/ifcodegen/simulation-behavior/frontend/CMakeLists.txt b/tests/auto/core/ifcodegen/simulation-behavior/frontend/CMakeLists.txt
new file mode 100644
index 00000000..56a937a5
--- /dev/null
+++ b/tests/auto/core/ifcodegen/simulation-behavior/frontend/CMakeLists.txt
@@ -0,0 +1,19 @@
+qt_add_library(simu)
+
+qt_ifcodegen_extend_target(
+    simu
+    IDL_FILES ../simu.qface
+    TEMPLATE frontend
+)
+
+target_link_libraries(
+    simu
+    PRIVATE Qt::InterfaceFrameworkPrivate Qt::Quick
+)
+
+set_target_properties(
+    simu
+    PROPERTIES RUNTIME_OUTPUT_DIRECTORY ..
+               VERSION "1.0.0"
+               SOVERSION "1"
+)
diff --git a/tests/auto/core/ifcodegen/simulation-behavior/qmlplugin/CMakeLists.txt b/tests/auto/core/ifcodegen/simulation-behavior/qmlplugin/CMakeLists.txt
new file mode 100644
index 00000000..92a03876
--- /dev/null
+++ b/tests/auto/core/ifcodegen/simulation-behavior/qmlplugin/CMakeLists.txt
@@ -0,0 +1,21 @@
+qt_ifcodegen_import_variables(
+    SIMU
+    IDL_FILES ../simu.qface
+    TEMPLATE qmlplugin
+)
+
+qt_add_qml_module(
+    simuplugin
+    OUTPUT_DIRECTORY "../imports/${SIMU_URI_PATH}"
+    URI ${SIMU_URI}
+    VERSION ${SIMU_VERSION}
+    PLUGIN_TARGET simuplugin
+    NO_PLUGIN_OPTIONAL
+    NO_GENERATE_PLUGIN_SOURCE
+    SOURCES ${SIMU_SOURCES}
+)
+
+target_link_libraries(
+    simuplugin
+    PRIVATE Qt::InterfaceFramework simu
+)
diff --git a/tests/auto/core/ifcodegen/simulation-behavior/simu.qface b/tests/auto/core/ifcodegen/simulation-behavior/simu.qface
new file mode 100644
index 00000000..571d68d2
--- /dev/null
+++ b/tests/auto/core/ifcodegen/simulation-behavior/simu.qface
@@ -0,0 +1,10 @@
+@config_simulator: { simulationFile: "qrc:/simulation.qml" }
+module Simu 1.0
+
+interface SomeInterface {
+    int simpleProperty;
+    int hardcodedProperty;
+    int otherHardcodedProperty;
+
+    int addOne(int value);
+}
diff --git a/tests/auto/core/ifcodegen/simulation-behavior/simulator/CMakeLists.txt b/tests/auto/core/ifcodegen/simulation-behavior/simulator/CMakeLists.txt
new file mode 100644
index 00000000..48a84af6
--- /dev/null
+++ b/tests/auto/core/ifcodegen/simulation-behavior/simulator/CMakeLists.txt
@@ -0,0 +1,24 @@
+qt_add_plugin(simu_simulator)
+
+qt_ifcodegen_extend_target(
+    simu_simulator
+    IDL_FILES ../simu.qface
+    TEMPLATE backend_simulator
+)
+
+target_link_libraries(
+    simu_simulator
+    PRIVATE Qt::InterfaceFramework simu
+)
+
+set_target_properties(
+    simu_simulator
+    PROPERTIES LIBRARY_OUTPUT_DIRECTORY ../interfaceframework
+)
+
+qt_add_resources(
+    simu_simulator
+    "simulation"
+    PREFIX "/"
+    FILES simulation.qml
+)
diff --git a/tests/auto/core/ifcodegen/simulation-behavior/simulator/simulation.qml b/tests/auto/core/ifcodegen/simulation-behavior/simulator/simulation.qml
new file mode 100644
index 00000000..df1c20f9
--- /dev/null
+++ b/tests/auto/core/ifcodegen/simulation-behavior/simulator/simulation.qml
@@ -0,0 +1,39 @@
+// Copyright (C) 2022 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+import QtQuick
+import Simu.simulation
+
+QtObject {
+    property bool defaultInitialized: false
+    property var backend: SomeInterfaceBackend {
+        function initialize() {
+            if (!defaultInitialized) {
+                IfSimulator.initializeDefault(IfSimulator.findData(IfSimulator.simulationData, "SomeInterface"),
+                                              backend);
+                defaultInitialized = true;
+            }
+            Base.initialize();
+        }
+
+        function addOne(reply, value) {
+            reply.setSuccess(++value);
+        }
+
+        function setHardcodedProperty(value) {
+            console.log("simu setHardcodedProperty(" + value + ") called")
+            console.log("simu hardcoding value to 1")
+            Base.setHardcodedProperty(1)
+        }
+
+        function setOtherHardcodedProperty(value) {
+            console.log("simu setOtherHardcodedProperty(" + value + ") called")
+            console.log("simu hardcoding value to 1")
+            Base.setOtherHardcodedProperty(1)
+            console.log("update other unrelated property")
+            simpleProperty = 5;
+            console.log("simu hardcoding value to 2")
+            Base.setOtherHardcodedProperty(2)
+        }
+    }
+}
diff --git a/tests/auto/core/ifcodegen/simulation-behavior/tst_simulation-behavior.cpp b/tests/auto/core/ifcodegen/simulation-behavior/tst_simulation-behavior.cpp
new file mode 100644
index 00000000..a34977a4
--- /dev/null
+++ b/tests/auto/core/ifcodegen/simulation-behavior/tst_simulation-behavior.cpp
@@ -0,0 +1,7 @@
+// Copyright (C) 2022 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+#include <QtTest/qtest.h>
+#include <QtQuickTest/quicktest.h>
+
+QUICK_TEST_MAIN(data)