From a5c3225e545c20cbde8806aa5dedcf2c7cabe67d Mon Sep 17 00:00:00 2001 From: Illia Bobyr Date: Sun, 22 Apr 2012 18:45:48 -0700 Subject: [PATCH] Crash on exit when overriding signals handlers in states. It appears that it is still possible to have an exit crash even after QTBUG-21617. See tests/auto/declarative/qdeclarativestates/tst_qdeclarativestates.cpp for an analysis of the issue. The short term solution: use reference counted pointers for the signal handler expressions. Considering all the cases we want to preserve an expression: - State is destroyed but the item is alive an is in this state - State is destroyed but the item has a reverse action in its reverse list uses this expression (not sure it is actually usable) - The original item signal handler expression should be preserved regardless of the state we are in and regardless of what do we do with the states (add new, delete, change current state) It seems like reference counting is the only manageable solution unless the overall model is modified a little bit. In the long run I would advice a more general refactoring with a strict rules of ownership for the expressions, state groups and states. I would say that the reverse list should be moved into the state group and states should own their expressions. And we do not allow destruction of the current state or any of the states current state extends. When a state is destroyed we would delete all the expressions that belong to that state. The reverse list should contain only references to the expressions that were the initial values of the item signal handlers. Thus unless we are trying to remove current state (or a state that the current state extends) we should be fine from the semantic standpoint. --- .../debugger/qdeclarativeenginedebugservice.cpp | 9 ++- src/declarative/qml/qdeclarativeboundsignal.cpp | 18 +++--- src/declarative/qml/qdeclarativeboundsignal_p.h | 7 +- src/declarative/qml/qdeclarativeproperty.cpp | 25 +++---- src/declarative/qml/qdeclarativeproperty_p.h | 10 ++- src/declarative/qml/qdeclarativevme.cpp | 7 +- src/declarative/util/qdeclarativeconnections.cpp | 4 +- .../util/qdeclarativepropertychanges.cpp | 34 +++------ .../tst_qdeclarativeproperty.cpp | 35 +++++----- .../data/signalOverrideCrash4.qml | 25 +++++++ .../qdeclarativestates/tst_qdeclarativestates.cpp | 77 ++++++++++++++++++++ 11 files changed, 174 insertions(+), 77 deletions(-) create mode 100644 tests/auto/declarative/qdeclarativestates/data/signalOverrideCrash4.qml diff --git a/src/declarative/debugger/qdeclarativeenginedebugservice.cpp b/src/declarative/debugger/qdeclarativeenginedebugservice.cpp index 421e399..a0bcfd7 100644 --- a/src/declarative/debugger/qdeclarativeenginedebugservice.cpp +++ b/src/declarative/debugger/qdeclarativeenginedebugservice.cpp @@ -56,6 +56,7 @@ #include #include +#include QT_BEGIN_NAMESPACE @@ -221,7 +222,7 @@ void QDeclarativeEngineDebugService::buildObjectDump(QDataStream &message, QDeclarativeObjectProperty prop; prop.type = QDeclarativeObjectProperty::SignalProperty; prop.hasNotifySignal = false; - QDeclarativeExpression *expr = signal->expression(); + QSharedPointer expr = signal->expression(); if (expr) { prop.value = expr->expression(); QObject *scope = expr->scopeObject(); @@ -587,7 +588,8 @@ void QDeclarativeEngineDebugService::setBinding(int objectId, if (isLiteralValue) { property.write(expression); } else if (hasValidSignal(object, propertyName)) { - QDeclarativeExpression *declarativeExpression = new QDeclarativeExpression(context, object, expression.toString()); + QSharedPointer declarativeExpression + (new QDeclarativeExpression(context, object, expression.toString())); QDeclarativePropertyPrivate::setSignalExpression(property, declarativeExpression); declarativeExpression->setSourceLocation(filename, line); } else if (property.isProperty()) { @@ -655,7 +657,8 @@ void QDeclarativeEngineDebugService::resetBinding(int objectId, const QString &p } } else if (hasValidSignal(object, propertyName)) { QDeclarativeProperty property(object, propertyName, context); - QDeclarativePropertyPrivate::setSignalExpression(property, 0); + QDeclarativePropertyPrivate::setSignalExpression + (property, QSharedPointer()); } else { if (QDeclarativePropertyChanges *propertyChanges = qobject_cast(object)) { propertyChanges->removeProperty(propertyName); diff --git a/src/declarative/qml/qdeclarativeboundsignal.cpp b/src/declarative/qml/qdeclarativeboundsignal.cpp index a8aece9..363a076 100644 --- a/src/declarative/qml/qdeclarativeboundsignal.cpp +++ b/src/declarative/qml/qdeclarativeboundsignal.cpp @@ -97,7 +97,7 @@ QDeclarativeAbstractBoundSignal::~QDeclarativeAbstractBoundSignal() QDeclarativeBoundSignal::QDeclarativeBoundSignal(QObject *scope, const QMetaMethod &signal, QObject *parent) -: m_expression(0), m_signal(signal), m_paramsValid(false), m_isEvaluating(false), m_params(0) +: m_signal(signal), m_paramsValid(false), m_isEvaluating(false), m_params(0) { // This is thread safe. Although it may be updated by two threads, they // will both set it to the same value - so the worst thing that can happen @@ -111,7 +111,7 @@ QDeclarativeBoundSignal::QDeclarativeBoundSignal(QObject *scope, const QMetaMeth QDeclarativeBoundSignal::QDeclarativeBoundSignal(QDeclarativeContext *ctxt, const QString &val, QObject *scope, const QMetaMethod &signal, QObject *parent) -: m_expression(0), m_signal(signal), m_paramsValid(false), m_isEvaluating(false), m_params(0) +: m_signal(signal), m_paramsValid(false), m_isEvaluating(false), m_params(0) { // This is thread safe. Although it may be updated by two threads, they // will both set it to the same value - so the worst thing that can happen @@ -121,13 +121,12 @@ QDeclarativeBoundSignal::QDeclarativeBoundSignal(QDeclarativeContext *ctxt, cons QDeclarative_setParent_noEvent(this, parent); QDeclarativePropertyPrivate::connect(scope, m_signal.methodIndex(), this, evaluateIdx); - m_expression = new QDeclarativeExpression(ctxt, scope, val); + m_expression = + QSharedPointer(new QDeclarativeExpression(ctxt, scope, val)); } QDeclarativeBoundSignal::~QDeclarativeBoundSignal() { - delete m_expression; - m_expression = 0; } int QDeclarativeBoundSignal::index() const @@ -138,7 +137,7 @@ int QDeclarativeBoundSignal::index() const /*! Returns the signal expression. */ -QDeclarativeExpression *QDeclarativeBoundSignal::expression() const +const QSharedPointer & QDeclarativeBoundSignal::expression() const { return m_expression; } @@ -150,9 +149,10 @@ QDeclarativeExpression *QDeclarativeBoundSignal::expression() const The QDeclarativeBoundSignal instance takes ownership of \a e. The caller is assumes ownership of the returned QDeclarativeExpression. */ -QDeclarativeExpression *QDeclarativeBoundSignal::setExpression(QDeclarativeExpression *e) +QSharedPointer + QDeclarativeBoundSignal::setExpression(QSharedPointer e) { - QDeclarativeExpression *rv = m_expression; + QSharedPointer rv = m_expression; m_expression = e; if (m_expression) m_expression->setNotifyOnValueChanged(false); return rv; @@ -183,7 +183,7 @@ int QDeclarativeBoundSignal::qt_metacall(QMetaObject::Call c, int id, void **a) if (m_params) m_params->setValues(a); if (m_expression && m_expression->engine()) { - QDeclarativeExpressionPrivate::get(m_expression)->value(m_params); + QDeclarativeExpressionPrivate::get(m_expression.data())->value(m_params); if (m_expression && m_expression->hasError()) QDeclarativeEnginePrivate::warning(m_expression->engine(), m_expression->error()); } diff --git a/src/declarative/qml/qdeclarativeboundsignal_p.h b/src/declarative/qml/qdeclarativeboundsignal_p.h index 8a46179..4cbd88e 100644 --- a/src/declarative/qml/qdeclarativeboundsignal_p.h +++ b/src/declarative/qml/qdeclarativeboundsignal_p.h @@ -56,6 +56,7 @@ #include "qdeclarativeexpression.h" #include +#include #include @@ -80,8 +81,8 @@ public: int index() const; - QDeclarativeExpression *expression() const; - QDeclarativeExpression *setExpression(QDeclarativeExpression *); + const QSharedPointer & expression() const; + QSharedPointer setExpression(QSharedPointer e); bool isEvaluating() const { return m_isEvaluating; } @@ -91,7 +92,7 @@ protected: virtual int qt_metacall(QMetaObject::Call c, int id, void **a); private: - QDeclarativeExpression *m_expression; + QSharedPointer m_expression; QMetaMethod m_signal; bool m_paramsValid : 1; bool m_isEvaluating : 1; diff --git a/src/declarative/qml/qdeclarativeproperty.cpp b/src/declarative/qml/qdeclarativeproperty.cpp index 05bec63..61961e2 100644 --- a/src/declarative/qml/qdeclarativeproperty.cpp +++ b/src/declarative/qml/qdeclarativeproperty.cpp @@ -56,6 +56,8 @@ #include "private/qdeclarativevmemetaobject_p.h" #include +#include + #include #include @@ -853,11 +855,11 @@ QDeclarativePropertyPrivate::setBindingNoEnable(QObject *object, int coreIndex, Returns the expression associated with this signal property, or 0 if no signal expression exists. */ -QDeclarativeExpression * +QSharedPointer QDeclarativePropertyPrivate::signalExpression(const QDeclarativeProperty &that) { if (!(that.type() & QDeclarativeProperty::SignalProperty)) - return 0; + return QSharedPointer(); const QObjectList &children = that.d->object->children(); @@ -869,27 +871,22 @@ QDeclarativePropertyPrivate::signalExpression(const QDeclarativeProperty &that) return signal->expression(); } - return 0; + return QSharedPointer(); } /*! Set the signal expression associated with this signal property to \a expr. Returns the existing signal expression (if any), otherwise 0. - - Ownership of \a expr transfers to QML. Ownership of the return value is - assumed by the caller. */ -QDeclarativeExpression * +QSharedPointer QDeclarativePropertyPrivate::setSignalExpression(const QDeclarativeProperty &that, - QDeclarativeExpression *expr) + QSharedPointer expr) { - if (!(that.type() & QDeclarativeProperty::SignalProperty)) { - delete expr; - return 0; - } + if (!(that.type() & QDeclarativeProperty::SignalProperty)) + return QSharedPointer(); const QObjectList &children = that.d->object->children(); - + for (int ii = 0; ii < children.count(); ++ii) { QObject *child = children.at(ii); @@ -902,7 +899,7 @@ QDeclarativePropertyPrivate::setSignalExpression(const QDeclarativeProperty &tha QDeclarativeBoundSignal *signal = new QDeclarativeBoundSignal(that.d->object, that.method(), that.d->object); return signal->setExpression(expr); } else { - return 0; + return QSharedPointer(); } } diff --git a/src/declarative/qml/qdeclarativeproperty_p.h b/src/declarative/qml/qdeclarativeproperty_p.h index 40b25ca..d64087c 100644 --- a/src/declarative/qml/qdeclarativeproperty_p.h +++ b/src/declarative/qml/qdeclarativeproperty_p.h @@ -55,6 +55,8 @@ #include "qdeclarativeproperty.h" +#include + #include #include #include @@ -127,9 +129,11 @@ public: static QDeclarativeAbstractBinding *setBinding(const QDeclarativeProperty &that, QDeclarativeAbstractBinding *, WriteFlags flags = DontRemoveBinding); - static QDeclarativeExpression *signalExpression(const QDeclarativeProperty &that); - static QDeclarativeExpression *setSignalExpression(const QDeclarativeProperty &that, - QDeclarativeExpression *) ; + static QSharedPointer + signalExpression(const QDeclarativeProperty &that); + static QSharedPointer + setSignalExpression(const QDeclarativeProperty &that, + QSharedPointer expr); static bool write(const QDeclarativeProperty &that, const QVariant &, WriteFlags); static int valueTypeCoreIndex(const QDeclarativeProperty &that); static int bindingIndex(const QDeclarativeProperty &that); diff --git a/src/declarative/qml/qdeclarativevme.cpp b/src/declarative/qml/qdeclarativevme.cpp index 59c9d2b..77feed2 100644 --- a/src/declarative/qml/qdeclarativevme.cpp +++ b/src/declarative/qml/qdeclarativevme.cpp @@ -71,6 +71,7 @@ #include #include #include +#include QT_BEGIN_NAMESPACE @@ -714,10 +715,10 @@ QObject *QDeclarativeVME::run(QDeclarativeVMEObjectStack &stack, QMetaMethod signal = target->metaObject()->method(instr.storeSignal.signalIndex); QDeclarativeBoundSignal *bs = new QDeclarativeBoundSignal(target, signal, target); - QDeclarativeExpression *expr = - new QDeclarativeExpression(ctxt, context, primitives.at(instr.storeSignal.value)); + QSharedPointer expr + (new QDeclarativeExpression(ctxt, context, primitives.at(instr.storeSignal.value))); expr->setSourceLocation(comp->name, instr.line); - static_cast(QObjectPrivate::get(expr))->name = datas.at(instr.storeSignal.name); + static_cast(QObjectPrivate::get(expr.data()))->name = datas.at(instr.storeSignal.name); bs->setExpression(expr); } break; diff --git a/src/declarative/util/qdeclarativeconnections.cpp b/src/declarative/util/qdeclarativeconnections.cpp index 7c9c933..bbd6648 100644 --- a/src/declarative/util/qdeclarativeconnections.cpp +++ b/src/declarative/util/qdeclarativeconnections.cpp @@ -50,6 +50,7 @@ #include #include +#include #include @@ -260,7 +261,8 @@ void QDeclarativeConnections::connectSignals() if (prop.isValid() && (prop.type() & QDeclarativeProperty::SignalProperty)) { QDeclarativeBoundSignal *signal = new QDeclarativeBoundSignal(target(), prop.method(), this); - QDeclarativeExpression *expression = new QDeclarativeExpression(qmlContext(this), 0, script); + QSharedPointer expression + (new QDeclarativeExpression(qmlContext(this), 0, script)); QDeclarativeData *ddata = QDeclarativeData::get(this); if (ddata && ddata->outerContext && !ddata->outerContext->url.isEmpty()) expression->setSourceLocation(ddata->outerContext->url.toString(), ddata->lineNumber); diff --git a/src/declarative/util/qdeclarativepropertychanges.cpp b/src/declarative/util/qdeclarativepropertychanges.cpp index 97c6a1a..598bc86 100644 --- a/src/declarative/util/qdeclarativepropertychanges.cpp +++ b/src/declarative/util/qdeclarativepropertychanges.cpp @@ -57,6 +57,8 @@ #include #include +#include + #include #include @@ -139,31 +141,20 @@ QT_BEGIN_NAMESPACE class QDeclarativeReplaceSignalHandler : public QDeclarativeActionEvent { public: - QDeclarativeReplaceSignalHandler() : expression(0), reverseExpression(0), - rewindExpression(0), ownedExpression(0) {} - ~QDeclarativeReplaceSignalHandler() { - delete ownedExpression; - } - virtual QString typeName() const { return QLatin1String("ReplaceSignalHandler"); } QDeclarativeProperty property; - QDeclarativeExpression *expression; - QDeclarativeExpression *reverseExpression; - QDeclarativeExpression *rewindExpression; - QDeclarativeGuard ownedExpression; + QSharedPointer expression; + QSharedPointer reverseExpression; + QSharedPointer rewindExpression; virtual void execute(Reason) { - ownedExpression = QDeclarativePropertyPrivate::setSignalExpression(property, expression); - if (ownedExpression == expression) - ownedExpression = 0; + QDeclarativePropertyPrivate::setSignalExpression(property, expression); } virtual bool isReversable() { return true; } virtual void reverse(Reason) { - ownedExpression = QDeclarativePropertyPrivate::setSignalExpression(property, reverseExpression); - if (ownedExpression == reverseExpression) - ownedExpression = 0; + QDeclarativePropertyPrivate::setSignalExpression(property, reverseExpression); } virtual void saveOriginals() { @@ -179,16 +170,10 @@ public: if (rsh == this) return; reverseExpression = rsh->reverseExpression; - if (rsh->ownedExpression == reverseExpression) { - ownedExpression = rsh->ownedExpression; - rsh->ownedExpression = 0; - } } virtual void rewind() { - ownedExpression = QDeclarativePropertyPrivate::setSignalExpression(property, rewindExpression); - if (ownedExpression == rewindExpression) - ownedExpression = 0; + QDeclarativePropertyPrivate::setSignalExpression(property, rewindExpression); } virtual void saveCurrentValues() { rewindExpression = QDeclarativePropertyPrivate::signalExpression(property); @@ -336,7 +321,8 @@ void QDeclarativePropertyChangesPrivate::decode() QDeclarativeProperty prop = property(name); //### better way to check for signal property? if (prop.type() & QDeclarativeProperty::SignalProperty) { - QDeclarativeExpression *expression = new QDeclarativeExpression(qmlContext(q), object, data.toString()); + QSharedPointer expression + (new QDeclarativeExpression(qmlContext(q), object, data.toString())); QDeclarativeData *ddata = QDeclarativeData::get(q); if (ddata && ddata->outerContext && !ddata->outerContext->url.isEmpty()) expression->setSourceLocation(ddata->outerContext->url.toString(), ddata->lineNumber); diff --git a/tests/auto/declarative/qdeclarativeproperty/tst_qdeclarativeproperty.cpp b/tests/auto/declarative/qdeclarativeproperty/tst_qdeclarativeproperty.cpp index 1f464bf..f7177fa 100644 --- a/tests/auto/declarative/qdeclarativeproperty/tst_qdeclarativeproperty.cpp +++ b/tests/auto/declarative/qdeclarativeproperty/tst_qdeclarativeproperty.cpp @@ -45,6 +45,7 @@ #include #include #include +#include #include #include @@ -178,7 +179,7 @@ void tst_qdeclarativeproperty::qmlmetaproperty() QVERIFY(QDeclarativePropertyPrivate::setBinding(prop, binding.data()) == 0); QVERIFY(binding == 0); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression) == 0); QVERIFY(expression == 0); QCOMPARE(prop.index(), -1); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -275,7 +276,7 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object() QVERIFY(QDeclarativePropertyPrivate::setBinding(prop, binding.data()) == 0); QVERIFY(binding == 0); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression) == 0); QVERIFY(expression == 0); QCOMPARE(prop.index(), -1); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -324,7 +325,7 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object() QVERIFY(binding != 0); QVERIFY(QDeclarativePropertyPrivate::binding(prop) == binding.data()); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression)) == 0); QVERIFY(expression == 0); QCOMPARE(prop.index(), dobject.metaObject()->indexOfProperty("defaultProperty")); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -376,7 +377,7 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object_string() QVERIFY(QDeclarativePropertyPrivate::setBinding(prop, binding.data()) == 0); QVERIFY(binding == 0); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression) == 0); QVERIFY(expression == 0); QCOMPARE(prop.index(), -1); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -425,7 +426,7 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object_string() QVERIFY(binding != 0); QVERIFY(QDeclarativePropertyPrivate::binding(prop) == binding.data()); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression) == 0); QVERIFY(expression == 0); QCOMPARE(prop.index(), dobject.metaObject()->indexOfProperty("defaultProperty")); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -472,9 +473,9 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object_string() QVERIFY(QDeclarativePropertyPrivate::setBinding(prop, binding.data()) == 0); QVERIFY(binding == 0); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression) == 0); QVERIFY(expression != 0); - QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == expression.data()); + QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == expression); QCOMPARE(prop.index(), dobject.metaObject()->indexOfMethod("clicked()")); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -520,9 +521,9 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object_string() QVERIFY(QDeclarativePropertyPrivate::setBinding(prop, binding.data()) == 0); QVERIFY(binding == 0); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression) == 0); QVERIFY(expression != 0); - QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == expression.data()); + QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == expression); QCOMPARE(prop.index(), dobject.metaObject()->indexOfMethod("oddlyNamedNotifySignal()")); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -573,7 +574,7 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object_context() QVERIFY(QDeclarativePropertyPrivate::setBinding(prop, binding.data()) == 0); QVERIFY(binding == 0); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression) == 0); QVERIFY(expression == 0); QCOMPARE(prop.index(), -1); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -622,7 +623,7 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object_context() QVERIFY(binding != 0); QVERIFY(QDeclarativePropertyPrivate::binding(prop) == binding.data()); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression) == 0); QVERIFY(expression == 0); QCOMPARE(prop.index(), dobject.metaObject()->indexOfProperty("defaultProperty")); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -674,7 +675,7 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object_string_context() QVERIFY(QDeclarativePropertyPrivate::setBinding(prop, binding.data()) == 0); QVERIFY(binding == 0); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression)) == 0); QVERIFY(expression == 0); QCOMPARE(prop.index(), -1); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -723,7 +724,7 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object_string_context() QVERIFY(binding != 0); QVERIFY(QDeclarativePropertyPrivate::binding(prop) == binding.data()); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression) == 0); QVERIFY(expression == 0); QCOMPARE(prop.index(), dobject.metaObject()->indexOfProperty("defaultProperty")); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -770,9 +771,9 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object_string_context() QVERIFY(QDeclarativePropertyPrivate::setBinding(prop, binding.data()) == 0); QVERIFY(binding == 0); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression)) == 0); QVERIFY(expression != 0); - QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == expression.data()); + QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == expression); QCOMPARE(prop.index(), dobject.metaObject()->indexOfMethod("clicked()")); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); @@ -818,9 +819,9 @@ void tst_qdeclarativeproperty::qmlmetaproperty_object_string_context() QVERIFY(QDeclarativePropertyPrivate::setBinding(prop, binding.data()) == 0); QVERIFY(binding == 0); QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == 0); - QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression.data()) == 0); + QVERIFY(QDeclarativePropertyPrivate::setSignalExpression(prop, expression) == 0); QVERIFY(expression != 0); - QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == expression.data()); + QVERIFY(QDeclarativePropertyPrivate::signalExpression(prop) == expression); QCOMPARE(prop.index(), dobject.metaObject()->indexOfMethod("oddlyNamedNotifySignal()")); QCOMPARE(QDeclarativePropertyPrivate::valueTypeCoreIndex(prop), -1); diff --git a/tests/auto/declarative/qdeclarativestates/data/signalOverrideCrash4.qml b/tests/auto/declarative/qdeclarativestates/data/signalOverrideCrash4.qml new file mode 100644 index 0000000..205eb05 --- /dev/null +++ b/tests/auto/declarative/qdeclarativestates/data/signalOverrideCrash4.qml @@ -0,0 +1,25 @@ +import QtQuick 1.0 + +Rectangle { + id: myRect + width: 400 + height: 400 + + states: [ + State { + name: "state1" + PropertyChanges { + target: myRect + onHeightChanged: console.log("state1") + color: "green" + } + }, + State { + name: "state2"; + PropertyChanges { + target: myRect + onHeightChanged: console.log("state2") + color: "red" + } + }] +} diff --git a/tests/auto/declarative/qdeclarativestates/tst_qdeclarativestates.cpp b/tests/auto/declarative/qdeclarativestates/tst_qdeclarativestates.cpp index eb2558b..d13796a 100644 --- a/tests/auto/declarative/qdeclarativestates/tst_qdeclarativestates.cpp +++ b/tests/auto/declarative/qdeclarativestates/tst_qdeclarativestates.cpp @@ -114,6 +114,7 @@ private slots: void signalOverrideCrash(); void signalOverrideCrash2(); void signalOverrideCrash3(); + void signalOverrideCrash4(); void parentChange(); void parentChangeErrors(); void anchorChanges(); @@ -537,6 +538,82 @@ void tst_qdeclarativestates::signalOverrideCrash3() delete rect; } +/* + * It appears that the old ownership model selected for signal expressions + * attached to states did not account for certain cases. In particular a signal + * owns its current expression. A state owns an expression that the signal had + * just before we entered this state. But when we enter a state an expression + * been set for a signal is the initial expression for this particular state, + * not an expression owned by this state. This would cause all kind of errors + * in a particularly complex case. Here is an explanation of what happens under + * the old ownership model (see data/signalOverrideCrash4.qml): + * + * Lets give our expressions ids: + * 0x1 : console.log("state1") + * 0x2 : console.log("state2") + * + * Initially: + * Rectangle.onHeightChanged: + * Rectangle.state1 + * expression: 0x1 + * owns: + * Rectangle.state2 + * expression: 0x2 + * owns: + * + * Enter state1: + * Rectangle.onHeightChanged: 0x1 + * Rectangle.state1 + * expression: 0x1 + * owns: + * Rectangle.state2 + * expression: 0x2 + * owns: + * + * Enter state2: + * Rectangle.onHeightChanged: 0x2 + * Rectangle.state1 + * expression: 0x1 + * owns: + * Rectangle.state2 + * expression: 0x2 + * owns: 0x1 + * + * state2 now own expression that "conceptually" belongs to state1 and might + * still be set by state1. + * + * Enter state1: + * Rectangle.onHeightChanged: 0x1 + * Rectangle.state1 + * expression: 0x1 + * owns: 0x2 + * Rectangle.state2 + * expression: 0x2 + * owns: 0x1 + * + * Now we have everything mixed up. States owns each others expression and in + * addition to that the rectangle's signal also owns one of the expression. If + * we delete the rectangle the 0x1 expression would be deleted twice. + * + * Note that in addition to the crazy mix ups unless we enter a state at least + * once we would leak all the signal expressions for that state. I am not sure + * how to check for leaks here. + */ +void tst_qdeclarativestates::signalOverrideCrash4() +{ + QDeclarativeEngine engine; + + QDeclarativeComponent rectComponent(&engine, SRCDIR "/data/signalOverrideCrash4.qml"); + QDeclarativeRectangle *rect = qobject_cast(rectComponent.create()); + QVERIFY(rect != 0); + + QDeclarativeItemPrivate::get(rect)->setState("state1"); + QDeclarativeItemPrivate::get(rect)->setState("state2"); + QDeclarativeItemPrivate::get(rect)->setState("state1"); + + delete rect; +} + void tst_qdeclarativestates::parentChange() { QDeclarativeEngine engine; -- 1.7.9