QStateMachine: handle parallel child mode for state machines

Setting the childMode property to ParallelStates will result in an
invalid state machine. This is never checked (worse, we explicitly
allow it and have a constructor to set it), but it results in
findLCCA failing, which then results in a failing assert or crash.

This fix in this patch is to handle this case separately. The proper
fix would be to remove completely the ability to set the childMode
on a QStateMachine, but that will have to wait until Qt6.

Fixes: QTBUG-49975
Change-Id: I43692309c4d438ee1a9bc55fa4f65f8bce8e0a59
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
 
 

3 files changed, 43 insertions, 8 deletions

diff --git a/src/corelib/statemachine/qstatemachine.cpp b/src/corelib/statemachine/qstatemachine.cpp
index be48dbc92f..e6dfacc0f5 100644
--- a/src/corelib/statemachine/qstatemachine.cpp
+++ b/src/corelib/statemachine/qstatemachine.cpp
@@ -140,6 +140,10 @@ QT_BEGIN_NAMESPACE
     no error state applies to the erroneous state, the machine will stop
     executing and an error message will be printed to the console.
 
+    \note Important: setting the \l{ChildMode} of a state machine to parallel (\l{ParallelStates})
+          results in an invalid state machine. It can only be set to (or kept as)
+          \l{ExclusiveStates}.
+
     \sa QAbstractState, QAbstractTransition, QState, {The State Machine Framework}
 */
 
@@ -519,7 +523,7 @@ bool QStateMachinePrivate::stateExitLessThan(QAbstractState *s1, QAbstractState
     }
 }
 
-QState *QStateMachinePrivate::findLCA(const QList<QAbstractState*> &states, bool onlyCompound) const
+QState *QStateMachinePrivate::findLCA(const QList<QAbstractState*> &states, bool onlyCompound)
 {
     if (states.isEmpty())
         return 0;
@@ -538,10 +542,16 @@ QState *QStateMachinePrivate::findLCA(const QList<QAbstractState*> &states, bool
         if (ok)
             return anc;
     }
-    return 0;
+
+    // Oops, this should never happen! The state machine itself is a common ancestor of all states,
+    // no matter what. But, for the onlyCompound case: we probably have a state machine whose
+    // childMode is set to parallel, which is illegal. However, we're stuck with it (and with
+    // exposing this invalid/dangerous API to users), so recover in the least horrible way.
+    setError(QStateMachine::StateMachineChildModeSetToParallelError, q_func());
+    return q_func(); // make the statemachine the LCA/LCCA (which it should have been anyway)
 }
 
-QState *QStateMachinePrivate::findLCCA(const QList<QAbstractState*> &states) const
+QState *QStateMachinePrivate::findLCCA(const QList<QAbstractState*> &states)
 {
     return findLCA(states, true);
 }
@@ -903,7 +913,7 @@ function getTransitionDomain(t)
 */
 QAbstractState *QStateMachinePrivate::getTransitionDomain(QAbstractTransition *t,
                                                           const QList<QAbstractState *> &effectiveTargetStates,
-                                                          CalculationCache *cache) const
+                                                          CalculationCache *cache)
 {
     Q_ASSERT(cache);
 
@@ -1484,6 +1494,14 @@ void QStateMachinePrivate::setError(QStateMachine::Error errorCode, QAbstractSta
         errorString = QStateMachine::tr("No common ancestor for targets and source of transition from state '%1'")
                         .arg(currentContext->objectName());
         break;
+
+    case QStateMachine::StateMachineChildModeSetToParallelError:
+        Q_ASSERT(currentContext != nullptr);
+
+        errorString = QStateMachine::tr("Child mode of state machine '%1' is not 'ExclusiveStates'!")
+                        .arg(currentContext->objectName());
+        break;
+
     default:
         errorString = QStateMachine::tr("Unknown error");
     };
@@ -2445,9 +2463,13 @@ QStateMachine::QStateMachine(QObject *parent)
 
 /*!
   \since 5.0
+  \deprecated
 
   Constructs a new state machine with the given \a childMode
   and \a parent.
+
+  \warning Do not set the \a childMode to anything else than \l{ExclusiveStates}, otherwise the
+           state machine is invalid, and might work incorrectly!
 */
 QStateMachine::QStateMachine(QState::ChildMode childMode, QObject *parent)
     : QState(*new QStateMachinePrivate, /*parentState=*/0)
@@ -2455,6 +2477,18 @@ QStateMachine::QStateMachine(QState::ChildMode childMode, QObject *parent)
     Q_D(QStateMachine);
     d->childMode = childMode;
     setParent(parent); // See comment in constructor above
+
+    if (childMode != ExclusiveStates) {
+        //### FIXME for Qt6: remove this constructor completely, and hide the childMode property.
+        // Yes, the StateMachine itself is conceptually a state, but it should only expose a limited
+        // number of properties. The execution algorithm (in the URL below) treats a state machine
+        // as a state, but from an API point of view, it's questionable if the QStateMachine should
+        // inherit from QState.
+        //
+        // See function findLCCA in https://www.w3.org/TR/2014/WD-scxml-20140529/#AlgorithmforSCXMLInterpretation
+        // to see where setting childMode to parallel will break down.
+        qWarning() << "Invalid childMode for QStateMachine" << this;
+    }
 }
 
 /*!
diff --git a/src/corelib/statemachine/qstatemachine.h b/src/corelib/statemachine/qstatemachine.h
index e520285437..07781d09a4 100644
--- a/src/corelib/statemachine/qstatemachine.h
+++ b/src/corelib/statemachine/qstatemachine.h
@@ -106,7 +106,8 @@ public:
         NoError,
         NoInitialStateError,
         NoDefaultStateInHistoryStateError,
-        NoCommonAncestorForTransitionError
+        NoCommonAncestorForTransitionError,
+        StateMachineChildModeSetToParallelError
     };
 
     explicit QStateMachine(QObject *parent = nullptr);
diff --git a/src/corelib/statemachine/qstatemachine_p.h b/src/corelib/statemachine/qstatemachine_p.h
index d6fdd72dc1..f140023e31 100644
--- a/src/corelib/statemachine/qstatemachine_p.h
+++ b/src/corelib/statemachine/qstatemachine_p.h
@@ -110,8 +110,8 @@ public:
     static QStateMachinePrivate *get(QStateMachine *q)
     { return q ? q->d_func() : nullptr; }
 
-    QState *findLCA(const QList<QAbstractState*> &states, bool onlyCompound = false) const;
-    QState *findLCCA(const QList<QAbstractState*> &states) const;
+    QState *findLCA(const QList<QAbstractState*> &states, bool onlyCompound = false);
+    QState *findLCCA(const QList<QAbstractState*> &states);
 
     static bool transitionStateEntryLessThan(QAbstractTransition *t1, QAbstractTransition *t2);
     static bool stateEntryLessThan(QAbstractState *s1, QAbstractState *s2);
@@ -160,7 +160,7 @@ public:
                                            QSet<QAbstractState*> &statesForDefaultEntry, CalculationCache *cache);
     QAbstractState *getTransitionDomain(QAbstractTransition *t,
                                         const QList<QAbstractState *> &effectiveTargetStates,
-                                        CalculationCache *cache) const;
+                                        CalculationCache *cache);
     void addDescendantStatesToEnter(QAbstractState *state,
                                     QSet<QAbstractState*> &statesToEnter,
                                     QSet<QAbstractState*> &statesForDefaultEntry);