Qt StateMachine: fix binding loops

All bindable properties were not breaking the binding if the new value
was the same as the old one. This is incorrect, so fix the logic.
Also, use (set)ValueBypassingBindings() to access the properties.
Update some helper functions in QStateMachinePrivate to also use these
methods instead of directly accessing the properties.

Some of the QState's properties cannot really be tested for the binding
loops with the current approach, because they require that the new
state belongs to the same object. Explicitly pass a lambda that returns
a nullptr as a helperConstrucor parameter for such cases, meaning that
binding loop checks will be skipped. In practice these property setters
are trivial, so they will not cause binding loops.

Pick-to: 6.5
Task-number: QTBUG-116542
Change-Id: If02c35df2b9c651c7f21f6f85752912e56688c71
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
(cherry picked from commit 205e3da5ec7899f325f88ce0e5e7497bba6a046b)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

5 files changed, 47 insertions, 44 deletions

diff --git a/src/statemachine/qeventtransition.cpp b/src/statemachine/qeventtransition.cpp
index c509415..460010a 100644
--- a/src/statemachine/qeventtransition.cpp
+++ b/src/statemachine/qeventtransition.cpp
@@ -149,12 +149,11 @@ QEvent::Type QEventTransition::eventType() const
 void QEventTransition::setEventType(QEvent::Type type)
 {
     Q_D(QEventTransition);
-    if (d->eventType.value() == type) {
-        d->eventType.removeBindingUnlessInWrapper();
+    d->eventType.removeBindingUnlessInWrapper();
+    if (d->eventType.valueBypassingBindings() == type)
         return;
-    }
     d->unregister();
-    d->eventType = type;
+    d->eventType.setValueBypassingBindings(type);
     d->maybeRegister();
     d->eventType.notify();
 }
@@ -181,12 +180,11 @@ QObject *QEventTransition::eventSource() const
 void QEventTransition::setEventSource(QObject *object)
 {
     Q_D(QEventTransition);
-    if (d->object.value() == object) {
-        d->object.removeBindingUnlessInWrapper();
+    d->object.removeBindingUnlessInWrapper();
+    if (d->object.valueBypassingBindings() == object)
         return;
-    }
     d->unregister();
-    d->object = object;
+    d->object.setValueBypassingBindings(object);
     d->maybeRegister();
     d->object.notify();
 }
diff --git a/src/statemachine/qhistorystate.cpp b/src/statemachine/qhistorystate.cpp
index 90239a6..19eab72 100644
--- a/src/statemachine/qhistorystate.cpp
+++ b/src/statemachine/qhistorystate.cpp
@@ -172,11 +172,10 @@ QAbstractTransition *QHistoryState::defaultTransition() const
 void QHistoryState::setDefaultTransition(QAbstractTransition *transition)
 {
     Q_D(QHistoryState);
-    if (d->defaultTransition.value() == transition) {
-        d->defaultTransition.removeBindingUnlessInWrapper();
+    d->defaultTransition.removeBindingUnlessInWrapper();
+    if (d->defaultTransition.valueBypassingBindings() == transition)
         return;
-    }
-    d->defaultTransition = transition;
+    d->defaultTransition.setValueBypassingBindings(transition);
     if (transition)
         transition->setParent(this);
     d->defaultTransition.notify();
@@ -219,14 +218,13 @@ void QHistoryState::setDefaultState(QAbstractState *state)
                  "to this history state's group (%p)", state, parentState());
         return;
     }
-    if (!d->defaultTransition.value()
-            || !isSoleEntry(d->defaultTransition->targetStates(), state)) {
-        if (!d->defaultTransition.value()
-                || !qobject_cast<DefaultStateTransition*>(d->defaultTransition)) {
+    // evaluate the binding once
+    auto *defaultTransition = d->defaultTransition.value();
+    if (!defaultTransition || !isSoleEntry(defaultTransition->targetStates(), state)) {
+        if (!defaultTransition || !qobject_cast<DefaultStateTransition*>(defaultTransition))
             d->defaultTransition.setValue(new DefaultStateTransition(this, state));
-        } else {
-            d->defaultTransition->setTargetState(state);
-        }
+        else
+            defaultTransition->setTargetState(state);
         emit defaultStateChanged(QHistoryState::QPrivateSignal());
     }
 }
diff --git a/src/statemachine/qsignaltransition.cpp b/src/statemachine/qsignaltransition.cpp
index a4b0770..46e67dc 100644
--- a/src/statemachine/qsignaltransition.cpp
+++ b/src/statemachine/qsignaltransition.cpp
@@ -144,12 +144,11 @@ const QObject *QSignalTransition::senderObject() const
 void QSignalTransition::setSenderObject(const QObject *sender)
 {
     Q_D(QSignalTransition);
-    if (sender == d->senderObject.value()) {
-        d->senderObject.removeBindingUnlessInWrapper();
+    d->senderObject.removeBindingUnlessInWrapper();
+    if (sender == d->senderObject.valueBypassingBindings())
         return;
-    }
     d->unregister();
-    d->senderObject = sender;
+    d->senderObject.setValueBypassingBindings(sender);
     d->maybeRegister();
     d->senderObject.notify();
     emit senderObjectChanged(QPrivateSignal());
@@ -176,12 +175,11 @@ QByteArray QSignalTransition::signal() const
 void QSignalTransition::setSignal(const QByteArray &signal)
 {
     Q_D(QSignalTransition);
-    if (signal == d->signal.value()) {
-        d->signal.removeBindingUnlessInWrapper();
+    d->signal.removeBindingUnlessInWrapper();
+    if (signal == d->signal.valueBypassingBindings())
         return;
-    }
     d->unregister();
-    d->signal = signal;
+    d->signal.setValueBypassingBindings(signal);
     d->maybeRegister();
     d->signal.notify();
     emit signalChanged(QPrivateSignal());
diff --git a/src/statemachine/qstatemachine.cpp b/src/statemachine/qstatemachine.cpp
index 65d8660..acf5b4b 100644
--- a/src/statemachine/qstatemachine.cpp
+++ b/src/statemachine/qstatemachine.cpp
@@ -2199,8 +2199,10 @@ void QStateMachinePrivate::unregisterTransition(QAbstractTransition *transition)
 void QStateMachinePrivate::maybeRegisterSignalTransition(QSignalTransition *transition)
 {
     Q_Q(QStateMachine);
+    const QObject *senderObject =
+            QSignalTransitionPrivate::get(transition)->senderObject.valueBypassingBindings();
     if ((state == Running) && (configuration.contains(transition->sourceState())
-            || (transition->senderObject() && (transition->senderObject()->thread() != q->thread())))) {
+            || (senderObject && (senderObject->thread() != q->thread())))) {
         registerSignalTransition(transition);
     }
 }
@@ -2210,10 +2212,11 @@ void QStateMachinePrivate::registerSignalTransition(QSignalTransition *transitio
     Q_Q(QStateMachine);
     if (QSignalTransitionPrivate::get(transition)->signalIndex != -1)
         return; // already registered
-    const QObject *sender = QSignalTransitionPrivate::get(transition)->senderObject;
+    const QObject *sender =
+            QSignalTransitionPrivate::get(transition)->senderObject.valueBypassingBindings();
     if (!sender)
         return;
-    QByteArray signal = QSignalTransitionPrivate::get(transition)->signal;
+    QByteArray signal = QSignalTransitionPrivate::get(transition)->signal.valueBypassingBindings();
     if (signal.isEmpty())
         return;
     if (signal.startsWith('0'+QSIGNAL_CODE))
@@ -2270,7 +2273,8 @@ void QStateMachinePrivate::unregisterSignalTransition(QSignalTransition *transit
     int signalIndex = QSignalTransitionPrivate::get(transition)->signalIndex;
     if (signalIndex == -1)
         return; // not registered
-    const QObject *sender = QSignalTransitionPrivate::get(transition)->senderObject;
+    const QObject *sender =
+            QSignalTransitionPrivate::get(transition)->senderObject.valueBypassingBindings();
     QSignalTransitionPrivate::get(transition)->signalIndex = -1;
 
     connectionsMutex.lock();
@@ -2325,21 +2329,23 @@ void QStateMachinePrivate::registerEventTransition(QEventTransition *transition)
     Q_Q(QStateMachine);
     if (QEventTransitionPrivate::get(transition)->registered)
         return;
-    if (transition->eventType() >= QEvent::User) {
+    const QEvent::Type eventType =
+            QEventTransitionPrivate::get(transition)->eventType.valueBypassingBindings();
+    if (eventType >= QEvent::User) {
         qWarning("QObject event transitions are not supported for custom types");
         return;
     }
-    QObject *object = QEventTransitionPrivate::get(transition)->object;
+    QObject *object = QEventTransitionPrivate::get(transition)->object.valueBypassingBindings();
     if (!object)
         return;
     QObjectPrivate *od = QObjectPrivate::get(object);
     if (!od->extraData || !od->extraData->eventFilters.contains(q))
         object->installEventFilter(q);
-    ++qobjectEvents[object][transition->eventType()];
+    ++qobjectEvents[object][eventType];
     QEventTransitionPrivate::get(transition)->registered = true;
 #ifdef QSTATEMACHINE_DEBUG
     qDebug() << q << ": added event transition from" << transition->sourceState()
-             << ": ( object =" << object << ", event =" << transition->eventType()
+             << ": ( object =" << object << ", event =" << eventType
              << ", targets =" << transition->targetStates() << ')';
 #endif
 }
@@ -2349,11 +2355,13 @@ void QStateMachinePrivate::unregisterEventTransition(QEventTransition *transitio
     Q_Q(QStateMachine);
     if (!QEventTransitionPrivate::get(transition)->registered)
         return;
-    QObject *object = QEventTransitionPrivate::get(transition)->object;
+    QObject *object = QEventTransitionPrivate::get(transition)->object.valueBypassingBindings();
     QHash<QEvent::Type, int> &events = qobjectEvents[object];
-    Q_ASSERT(events.value(transition->eventType()) > 0);
-    if (--events[transition->eventType()] == 0) {
-        events.remove(transition->eventType());
+    const QEvent::Type eventType =
+            QEventTransitionPrivate::get(transition)->eventType.valueBypassingBindings();
+    Q_ASSERT(events.value(eventType) > 0);
+    if (--events[eventType] == 0) {
+        events.remove(eventType);
         int sum = 0;
         QHash<QEvent::Type, int>::const_iterator it;
         for (it = events.constBegin(); it != events.constEnd(); ++it)
diff --git a/tests/auto/qstatemachine/qstatemachine/tst_qstatemachine.cpp b/tests/auto/qstatemachine/qstatemachine/tst_qstatemachine.cpp
index 2f9793f..e4be3c4 100644
--- a/tests/auto/qstatemachine/qstatemachine/tst_qstatemachine.cpp
+++ b/tests/auto/qstatemachine/qstatemachine/tst_qstatemachine.cpp
@@ -6861,19 +6861,19 @@ void tst_QStateMachine::bindings()
         return;
     }
 
-    // -- QState::initialState
+    // -- QState::initialState. Cannot be tested for bindable loops.
     QAbstractState *is1 = new QState(state1);
     QAbstractState *is2 = new QState(state1);
     QTestPrivate::testReadWritePropertyBasics<QState, QAbstractState*>(
-                *state1, is1, is2, "initialState");
+                *state1, is1, is2, "initialState", []() { return nullptr; });
     if (QTest::currentTestFailed()) {
         qWarning() << "QState::initialState bindable test failed.";
         return;
     }
 
-    // -- QState::errorState
+    // -- QState::errorState. Cannot be tested for bindable loops.
     QTestPrivate::testReadWritePropertyBasics<QState, QAbstractState*>(
-                *state1, is1, is2, "errorState");
+                *state1, is1, is2, "errorState", []() { return nullptr; });
     if (QTest::currentTestFailed()) {
         qWarning() << "QState::errorState bindable test failed.";
         return;
@@ -6935,7 +6935,8 @@ void tst_QStateMachine::bindings()
     auto transitionType1 = QAbstractTransition::InternalTransition;
     auto transitionType2 = QAbstractTransition::ExternalTransition;
     QTestPrivate::testReadWritePropertyBasics<QAbstractTransition, QAbstractTransition::TransitionType>(
-                signalTransition, transitionType1, transitionType2, "transitionType");
+                signalTransition, transitionType1, transitionType2, "transitionType",
+                []() { return std::make_unique<QSignalTransition>(); });
     if (QTest::currentTestFailed()) {
         qWarning() << "QAbstractTransition::transitionType bindable test failed.";
         return;