QScxmlStateMachine: adjust the test and fix binding loops

Adjust to the changes in QTestPrivate that allow to catch binding loops
when testing bindable properties.
Fix the binding loops identified by the updated tests.

After fixing all obvious binding loops, it was discovered that the
call to QObject::objectName() in the tableData property setter creates
a binding loop on its own.
Fix it by accessing QObjectPrivate's internals instead of calling
a public getter.
Also add a test to verify that the logic of updating the objectName
did not change.

Task-number: QTBUG-116542
Change-Id: Idc849dd9841dc70df2d36524cb5bb0a331255f39
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
(cherry picked from commit 77b44612e8898657bd235156b9872c12ca4aea80)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit b23811e6afd92ca1411d3cc508d68381c88b3318)

3 files changed, 63 insertions, 19 deletions

diff --git a/src/scxml/qscxmldatamodel.cpp b/src/scxml/qscxmldatamodel.cpp
index 298981e..eaa15e8 100644
--- a/src/scxml/qscxmldatamodel.cpp
+++ b/src/scxml/qscxmldatamodel.cpp
@@ -95,10 +95,11 @@ void QScxmlDataModel::setStateMachine(QScxmlStateMachine *stateMachine)
 {
     Q_D(QScxmlDataModel);
 
-    if (d->m_stateMachine.value() == nullptr && stateMachine != nullptr) {
+    if (d->m_stateMachine.valueBypassingBindings() == nullptr && stateMachine != nullptr) {
         // the binding is removed only on the first valid set
-        // as the later attempts are ignored (removed when value is set below)
-        d->m_stateMachine = stateMachine;
+        // as the later attempts are ignored
+        d->m_stateMachine.removeBindingUnlessInWrapper();
+        d->m_stateMachine.setValueBypassingBindings(stateMachine);
         stateMachine->setDataModel(this);
         d->m_stateMachine.notify();
     }
diff --git a/src/scxml/qscxmlstatemachine.cpp b/src/scxml/qscxmlstatemachine.cpp
index ba7eece..0105a6a 100644
--- a/src/scxml/qscxmlstatemachine.cpp
+++ b/src/scxml/qscxmlstatemachine.cpp
@@ -735,7 +735,8 @@ void QScxmlStateMachinePrivate::updateMetaCache()
     m_stateIndexToSignalIndex.clear();
     m_stateNameToSignalIndex.clear();
 
-    if (!m_tableData.value())
+    const QScxmlTableData *tableData = m_tableData.valueBypassingBindings();
+    if (!tableData)
         return;
 
     if (!m_stateTable)
@@ -747,7 +748,7 @@ void QScxmlStateMachinePrivate::updateMetaCache()
         const auto &s = m_stateTable->state(i);
         if (!s.isHistoryState() && s.type != StateTable::State::Invalid) {
             m_stateIndexToSignalIndex.insert(i, signalIndex);
-            m_stateNameToSignalIndex.insert(m_tableData.value()->string(s.name),
+            m_stateNameToSignalIndex.insert(tableData->string(s.name),
                                             signalIndex + methodOffset);
 
             ++signalIndex;
@@ -1710,10 +1711,11 @@ void QScxmlStateMachine::setDataModel(QScxmlDataModel *model)
 {
     Q_D(QScxmlStateMachine);
 
-    if (d->m_dataModel.value() == nullptr && model != nullptr) {
+    if (d->m_dataModel.valueBypassingBindings() == nullptr && model != nullptr) {
         // the binding is removed only on the first valid set
-        // as the later attempts are ignored (removed when value is set below)
-        d->m_dataModel = model;
+        // as the later attempts are ignored
+        d->m_dataModel.removeBindingUnlessInWrapper();
+        d->m_dataModel.setValueBypassingBindings(model);
         model->setStateMachine(this);
         d->m_dataModel.notify();
         emit dataModelChanged(model);
@@ -1766,16 +1768,18 @@ void QScxmlStateMachine::setTableData(QScxmlTableData *tableData)
 {
     Q_D(QScxmlStateMachine);
 
-    if (d->m_tableData.value() == tableData) {
-        d->m_tableData.removeBindingUnlessInWrapper();
+    d->m_tableData.removeBindingUnlessInWrapper();
+    if (d->m_tableData.valueBypassingBindings() == tableData)
         return;
-    }
 
-    d->m_tableData = tableData;
+    d->m_tableData.setValueBypassingBindings(tableData);
     if (tableData) {
         d->m_stateTable = reinterpret_cast<const QScxmlExecutableContent::StateTable *>(
                     tableData->stateMachineTable());
-        if (objectName().isEmpty()) {
+        // cannot use objectName() here, because it creates binding loop
+        const QString currentObjectName = d->extraData
+                ? d->extraData->objectName.valueBypassingBindings() : QString();
+        if (currentObjectName.isEmpty()) {
             setObjectName(tableData->name());
         }
         if (d->m_stateTable->maxServiceId != QScxmlExecutableContent::StateTable::InvalidIndex) {
diff --git a/tests/auto/statemachine/tst_statemachine.cpp b/tests/auto/statemachine/tst_statemachine.cpp
index 5b717e9..6080d42 100644
--- a/tests/auto/statemachine/tst_statemachine.cpp
+++ b/tests/auto/statemachine/tst_statemachine.cpp
@@ -38,6 +38,8 @@ private Q_SLOTS:
     void logWithoutExpr();
 
     void bindings();
+
+    void setTableDataUpdatesObjectNames();
 };
 
 void tst_StateMachine::stateNames_data()
@@ -449,11 +451,16 @@ void tst_StateMachine::bindings()
         return;
     }
 
+    using StateMachinePtr = std::unique_ptr<QScxmlStateMachine>;
+
     // -- QScxmlStateMachine::initialValues
     QVariantMap map1{{"map", 1}};
     QVariantMap map2{{"map", 2}};
     QTestPrivate::testReadWritePropertyBasics<QScxmlStateMachine, QVariantMap>(
-                *stateMachine1, map1, map2, "initialValues");
+            *stateMachine1, map1, map2, "initialValues",
+            []() {
+                return StateMachinePtr{QScxmlStateMachine::fromFile(QString("not_a_real_file"))};
+            });
     if (QTest::currentTestFailed()) {
         qWarning() << "QScxmlStateMachine::initialValues bindable test failed.";
         return;
@@ -468,7 +475,10 @@ void tst_StateMachine::bindings()
     MockLoader loader1;
     MockLoader loader2;
     QTestPrivate::testReadWritePropertyBasics<QScxmlStateMachine, QScxmlCompiler::Loader*>(
-                *stateMachine1, &loader1, &loader2, "loader");
+            *stateMachine1, &loader1, &loader2, "loader",
+            []() {
+                return StateMachinePtr{QScxmlStateMachine::fromFile(QString("not_a_real_file"))};
+            });
     if (QTest::currentTestFailed()) {
         qWarning() << "QScxmlStateMachine::loader bindable test failed.";
         return;
@@ -483,18 +493,25 @@ void tst_StateMachine::bindings()
     QScxmlNullDataModel model1;
     // data can only change once
     QTestPrivate::testWriteOncePropertyBasics<QScxmlStateMachine, QScxmlDataModel*>(
-                *stateMachine2, nullptr, &model1, "dataModel");
+            *stateMachine2, nullptr, &model1, "dataModel", true,
+            []() {
+                return StateMachinePtr{QScxmlStateMachine::fromFile(QString("not_a_real_file"))};
+            });
     if (QTest::currentTestFailed()) {
         qWarning() << "QScxmlStateMachine::dataModel bindable test failed.";
         return;
     }
 
     // -- QScxmlStateMachine::tableData
-    // Use the statemachine to generate the tabledDatas for testing
+    // Use the statemachine to generate the tableData for testing
     std::unique_ptr<QScxmlStateMachine> stateMachine4(
                 QScxmlStateMachine::fromFile(QString(":/tst_statemachine/invoke.scxml")));
     QTestPrivate::testReadWritePropertyBasics<QScxmlStateMachine, QScxmlTableData*>(
-                *stateMachine2, stateMachine1.get()->tableData(), stateMachine4.get()->tableData(), "tableData");
+            *stateMachine2, stateMachine1.get()->tableData(), stateMachine4.get()->tableData(),
+            "tableData",
+            []() {
+                return StateMachinePtr{QScxmlStateMachine::fromFile(QString("not_a_real_file"))};
+            });
     if (QTest::currentTestFailed()) {
         qWarning() << "QScxmlStateMachine::tableData bindable test failed.";
         return;
@@ -528,7 +545,7 @@ void tst_StateMachine::bindings()
     std::unique_ptr<QScxmlStateMachine> stateMachine5(
                 QScxmlStateMachine::fromFile(QString("not_a_real_file")));
     // data can only change once
-    QTestPrivate::testWriteOncePropertyBasics<QScxmlDataModel, QScxmlStateMachine*>(
+    QTestPrivate::testWriteOncePropertyBasics<QScxmlNullDataModel, QScxmlStateMachine*>(
                 dataModel1, nullptr, stateMachine5.get(), "stateMachine");
     if (QTest::currentTestFailed()) {
         qWarning() << "QScxmlDataModel::stateMachine bindable test failed.";
@@ -536,6 +553,28 @@ void tst_StateMachine::bindings()
     }
 }
 
+void tst_StateMachine::setTableDataUpdatesObjectNames()
+{
+    std::unique_ptr<QScxmlStateMachine> stateMachine1(
+            QScxmlStateMachine::fromFile(QString(":/tst_statemachine/emptylog.scxml")));
+    const QString sm1ObjectName = stateMachine1->objectName();
+    std::unique_ptr<QScxmlStateMachine> stateMachine2(
+            QScxmlStateMachine::fromFile(QString(":/tst_statemachine/eventoccurred.scxml")));
+    const QString sm2ObjectName = stateMachine2->objectName();
+    QCOMPARE_NE(sm1ObjectName, sm2ObjectName);
+
+    std::unique_ptr<QScxmlStateMachine> sm(
+            QScxmlStateMachine::fromFile(QString("not_a_real_file")));
+    QVERIFY(sm->objectName().isEmpty());
+    // no name set, so update object name
+    sm->setTableData(stateMachine1->tableData());
+    QCOMPARE_EQ(sm->objectName(), sm1ObjectName);
+
+    // object name already set, so do not update
+    sm->setTableData(stateMachine2->tableData());
+    QCOMPARE_EQ(sm->objectName(), sm1ObjectName); // did not change
+}
+
 QTEST_MAIN(tst_StateMachine)
 
 #include "tst_statemachine.moc"