From cfdbfcebbda5f26b89c70df6b191b17ef242e9d7 Mon Sep 17 00:00:00 2001 From: Erik Verbruggen Date: Tue, 16 Apr 2019 13:30:31 +0200 Subject: [PATCH] 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 --- src/corelib/statemachine/qstatemachine.cpp | 42 +++++++++++++++++-- src/corelib/statemachine/qstatemachine.h | 3 +- src/corelib/statemachine/qstatemachine_p.h | 6 +-- .../qstatemachine/tst_qstatemachine.cpp | 14 +++++-- 4 files changed, 53 insertions(+), 12 deletions(-) diff --git a/src/corelib/statemachine/qstatemachine.cpp b/src/corelib/statemachine/qstatemachine.cpp index be48dbc92f1..e6dfacc0f57 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 &states, bool onlyCompound) const +QState *QStateMachinePrivate::findLCA(const QList &states, bool onlyCompound) { if (states.isEmpty()) return 0; @@ -538,10 +542,16 @@ QState *QStateMachinePrivate::findLCA(const QList &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 &states) const +QState *QStateMachinePrivate::findLCCA(const QList &states) { return findLCA(states, true); } @@ -903,7 +913,7 @@ function getTransitionDomain(t) */ QAbstractState *QStateMachinePrivate::getTransitionDomain(QAbstractTransition *t, const QList &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 e5202854375..07781d09a4c 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 d6fdd72dc15..f140023e31f 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 &states, bool onlyCompound = false) const; - QState *findLCCA(const QList &states) const; + QState *findLCA(const QList &states, bool onlyCompound = false); + QState *findLCCA(const QList &states); static bool transitionStateEntryLessThan(QAbstractTransition *t1, QAbstractTransition *t2); static bool stateEntryLessThan(QAbstractState *s1, QAbstractState *s2); @@ -160,7 +160,7 @@ public: QSet &statesForDefaultEntry, CalculationCache *cache); QAbstractState *getTransitionDomain(QAbstractTransition *t, const QList &effectiveTargetStates, - CalculationCache *cache) const; + CalculationCache *cache); void addDescendantStatesToEnter(QAbstractState *state, QSet &statesToEnter, QSet &statesForDefaultEntry); diff --git a/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp b/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp index 55a672aae1b..0312cff5daa 100644 --- a/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp +++ b/tests/auto/corelib/statemachine/qstatemachine/tst_qstatemachine.cpp @@ -336,7 +336,9 @@ void tst_QStateMachine::transitionToRootState() TEST_ACTIVE_CHANGED(initialState, 1); machine.postEvent(new QEvent(QEvent::User)); - QTest::ignoreMessage(QtWarningMsg, "Unrecoverable error detected in running state machine: No common ancestor for targets and source of transition from state 'initial'"); + QTest::ignoreMessage(QtWarningMsg, + "Unrecoverable error detected in running state machine: " + "Child mode of state machine 'machine' is not 'ExclusiveStates'!"); QCoreApplication::processEvents(); QVERIFY(machine.configuration().isEmpty()); QVERIFY(!machine.isRunning()); @@ -1061,7 +1063,8 @@ void tst_QStateMachine::transitionToStateNotInGraph() initialState->addTransition(&independentState); machine.start(); - QTest::ignoreMessage(QtWarningMsg, "Unrecoverable error detected in running state machine: No common ancestor for targets and source of transition from state 'initialState'"); + QTest::ignoreMessage(QtWarningMsg, "Unrecoverable error detected in running state machine: " + "Child mode of state machine '' is not 'ExclusiveStates'!"); QCoreApplication::processEvents(); QCOMPARE(machine.isRunning(), false); @@ -2099,6 +2102,8 @@ void tst_QStateMachine::parallelRootState() QSignalSpy finishedSpy(&machine, &QStateMachine::finished); QVERIFY(finishedSpy.isValid()); machine.start(); + QTest::ignoreMessage(QtWarningMsg, "Unrecoverable error detected in running state machine: " + "Child mode of state machine '' is not 'ExclusiveStates'!"); QTRY_COMPARE(startedSpy.count(), 1); QCOMPARE(machine.configuration().size(), 4); QVERIFY(machine.configuration().contains(s1)); @@ -3310,14 +3315,15 @@ void tst_QStateMachine::targetStateWithNoParent() QVERIFY(runningSpy.isValid()); machine.start(); - QTest::ignoreMessage(QtWarningMsg, "Unrecoverable error detected in running state machine: No common ancestor for targets and source of transition from state 's1'"); + QTest::ignoreMessage(QtWarningMsg, "Unrecoverable error detected in running state machine: " + "Child mode of state machine '' is not 'ExclusiveStates'!"); TEST_ACTIVE_CHANGED(s1, 2); QTRY_COMPARE(startedSpy.count(), 1); QCOMPARE(machine.isRunning(), false); QCOMPARE(stoppedSpy.count(), 1); QCOMPARE(finishedSpy.count(), 0); TEST_RUNNING_CHANGED_STARTED_STOPPED; - QCOMPARE(machine.error(), QStateMachine::NoCommonAncestorForTransitionError); + QCOMPARE(machine.error(), QStateMachine::StateMachineChildModeSetToParallelError); } void tst_QStateMachine::targetStateDeleted()