From 4a8871d3fe6c9fe16752697abf95d3a7b8fba7b7 Mon Sep 17 00:00:00 2001 From: Aaron Kennedy Date: Wed, 2 Nov 2011 17:08:16 +0000 Subject: [PATCH] Don't crash if contexts are deleted during refreshExpressions Change-Id: I23b59d33c07b017ef7355a7fe4a728d84c5d7eaa Reviewed-by: Roberto Raggi --- src/declarative/qml/ftw/ftw.pri | 1 + src/declarative/qml/ftw/qdeletewatcher_p.h | 113 +++++++++++++++++++ src/declarative/qml/qdeclarativebinding.cpp | 2 +- src/declarative/qml/qdeclarativecontext.cpp | 87 ++++++++++++--- src/declarative/qml/qdeclarativecontext_p.h | 2 + src/declarative/qml/qdeclarativeexpression.cpp | 2 +- src/declarative/qml/qdeclarativeexpression_p.h | 57 +--------- src/declarative/qml/qdeclarativeproperty.cpp | 2 +- src/declarative/qml/v8/qv8bindings.cpp | 2 +- .../data/RefreshExpressionsType.qml | 5 + .../data/refreshExpressions.qml | 6 ++ .../qdeclarativecontext/qdeclarativecontext.pro | 2 +- .../tst_qdeclarativecontext.cpp | 120 +++++++++++++++++++++ 13 files changed, 328 insertions(+), 73 deletions(-) create mode 100644 src/declarative/qml/ftw/qdeletewatcher_p.h create mode 100644 tests/auto/declarative/qdeclarativecontext/data/RefreshExpressionsType.qml create mode 100644 tests/auto/declarative/qdeclarativecontext/data/refreshExpressions.qml diff --git a/src/declarative/qml/ftw/ftw.pri b/src/declarative/qml/ftw/ftw.pri index 6339992..9d114e2 100644 --- a/src/declarative/qml/ftw/ftw.pri +++ b/src/declarative/qml/ftw/ftw.pri @@ -11,6 +11,7 @@ HEADERS += \ $$PWD/qdeclarativethread_p.h \ $$PWD/qfinitestack_p.h \ $$PWD/qrecursionwatcher_p.h \ + $$PWD/qdeletewatcher_p.h \ $$PWD/qrecyclepool_p.h \ SOURCES += \ diff --git a/src/declarative/qml/ftw/qdeletewatcher_p.h b/src/declarative/qml/ftw/qdeletewatcher_p.h new file mode 100644 index 0000000..34ef46b --- /dev/null +++ b/src/declarative/qml/ftw/qdeletewatcher_p.h @@ -0,0 +1,113 @@ +/**************************************************************************** +** +** Copyright (C) 2011 Nokia Corporation and/or its subsidiary(-ies). +** All rights reserved. +** Contact: Nokia Corporation (qt-info@nokia.com) +** +** This file is part of the QtDeclarative module of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:LGPL$ +** GNU Lesser General Public License Usage +** This file may be used under the terms of the GNU Lesser General Public +** License version 2.1 as published by the Free Software Foundation and +** appearing in the file LICENSE.LGPL included in the packaging of this +** file. Please review the following information to ensure the GNU Lesser +** General Public License version 2.1 requirements will be met: +** http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. +** +** In addition, as a special exception, Nokia gives you certain additional +** rights. These rights are described in the Nokia Qt LGPL Exception +** version 1.1, included in the file LGPL_EXCEPTION.txt in this package. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU General +** Public License version 3.0 as published by the Free Software Foundation +** and appearing in the file LICENSE.GPL included in the packaging of this +** file. Please review the following information to ensure the GNU General +** Public License version 3.0 requirements will be met: +** http://www.gnu.org/copyleft/gpl.html. +** +** Other Usage +** Alternatively, this file may be used in accordance with the terms and +** conditions contained in a signed written agreement between you and Nokia. +** +** +** +** +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#ifndef QDELETEWATCHER_P_H +#define QDELETEWATCHER_P_H + +// +// W A R N I N G +// ------------- +// +// This file is not part of the Qt API. It exists purely as an +// implementation detail. This header file may change from version to +// version without notice, or even be removed. +// +// We mean it. +// + +#include + +QT_BEGIN_NAMESPACE + +class QDeleteWatchable +{ +public: + inline QDeleteWatchable(); + inline ~QDeleteWatchable(); +private: + friend class QDeleteWatcher; + bool *_w; +}; + +class QDeleteWatcher { +public: + inline QDeleteWatcher(QDeleteWatchable *data); + inline ~QDeleteWatcher(); + inline bool wasDeleted() const; +private: + void *operator new(size_t); + bool *_w; + bool _s; + QDeleteWatchable *m_d; +}; + +QDeleteWatchable::QDeleteWatchable() +: _w(0) +{ +} + +QDeleteWatchable::~QDeleteWatchable() +{ + if (_w) *_w = true; +} + +QDeleteWatcher::QDeleteWatcher(QDeleteWatchable *data) +: _s(false), m_d(data) +{ + if (!m_d->_w) + m_d->_w = &_s; + _w = m_d->_w; +} + +QDeleteWatcher::~QDeleteWatcher() +{ + if (false == *_w && &_s == m_d->_w) + m_d->_w = 0; +} + +bool QDeleteWatcher::wasDeleted() const +{ + return *_w; +} + +QT_END_NAMESPACE + +#endif // QDELETEWATCHER_P_H diff --git a/src/declarative/qml/qdeclarativebinding.cpp b/src/declarative/qml/qdeclarativebinding.cpp index 531842a..9a3a7e0 100644 --- a/src/declarative/qml/qdeclarativebinding.cpp +++ b/src/declarative/qml/qdeclarativebinding.cpp @@ -345,7 +345,7 @@ void QDeclarativeBinding::update(QDeclarativePropertyPrivate::WriteFlags flags) QDeclarativeBindingProfiler prof(this); d->updating = true; - QDeclarativeDeleteWatcher watcher(d); + QDeleteWatcher watcher(d); if (d->property.propertyType() == qMetaTypeId()) { diff --git a/src/declarative/qml/qdeclarativecontext.cpp b/src/declarative/qml/qdeclarativecontext.cpp index c4fc764..d7a6424 100644 --- a/src/declarative/qml/qdeclarativecontext.cpp +++ b/src/declarative/qml/qdeclarativecontext.cpp @@ -653,23 +653,82 @@ void QDeclarativeContextData::setParent(QDeclarativeContextData *p, bool parentT } } -/* -Refreshes all expressions that could possibly depend on this context. Refreshing flushes all -context-tree dependent caches in the expressions, and should occur every time the context tree - *structure* (not values) changes. -*/ -void QDeclarativeContextData::refreshExpressions() +void QDeclarativeContextData::refreshExpressionsRecursive(QDeclarativeAbstractExpression *expression) { - QDeclarativeContextData *child = childContexts; - while (child) { - child->refreshExpressions(); - child = child->nextChild; - } + QDeleteWatcher w(expression); - QDeclarativeAbstractExpression *expression = expressions; - while (expression) { + if (expression->m_nextExpression) + refreshExpressionsRecursive(expression->m_nextExpression); + + if (!w.wasDeleted()) expression->refresh(); - expression = expression->m_nextExpression; +} + +void QDeclarativeContextData::refreshExpressionsRecursive() +{ + // For efficiency, we try and minimize the number of guards we have to create + if (expressions && (nextChild || childContexts)) { + QDeclarativeGuardedContextData guard(this); + + if (childContexts) + childContexts->refreshExpressionsRecursive(); + + if (guard.isNull()) return; + + if (nextChild) + nextChild->refreshExpressionsRecursive(); + + if (guard.isNull()) return; + + if (expressions) + refreshExpressionsRecursive(expressions); + + } else if (expressions) { + + refreshExpressionsRecursive(expressions); + + } else if (nextChild && childContexts) { + + QDeclarativeGuardedContextData guard(this); + + childContexts->refreshExpressionsRecursive(); + + if (!guard.isNull() && nextChild) + nextChild->refreshExpressionsRecursive(); + + } else if (nextChild) { + + nextChild->refreshExpressionsRecursive(); + + } else if (childContexts) { + + childContexts->refreshExpressionsRecursive(); + + } +} + +// Refreshes all expressions that could possibly depend on this context. Refreshing flushes all +// context-tree dependent caches in the expressions, and should occur every time the context tree +// *structure* (not values) changes. +void QDeclarativeContextData::refreshExpressions() +{ + // For efficiency, we try and minimize the number of guards we have to create + if (expressions && childContexts) { + QDeclarativeGuardedContextData guard(this); + + childContexts->refreshExpressionsRecursive(); + + if (!guard.isNull() && expressions) + refreshExpressionsRecursive(expressions); + + } else if (expressions) { + + refreshExpressionsRecursive(expressions); + + } else if (childContexts) { + + childContexts->refreshExpressionsRecursive(); + } } diff --git a/src/declarative/qml/qdeclarativecontext_p.h b/src/declarative/qml/qdeclarativecontext_p.h index a6e83a0..6676a06 100644 --- a/src/declarative/qml/qdeclarativecontext_p.h +++ b/src/declarative/qml/qdeclarativecontext_p.h @@ -216,6 +216,8 @@ public: } private: + void refreshExpressionsRecursive(); + void refreshExpressionsRecursive(QDeclarativeAbstractExpression *); ~QDeclarativeContextData() {} }; diff --git a/src/declarative/qml/qdeclarativeexpression.cpp b/src/declarative/qml/qdeclarativeexpression.cpp index fa899cf..b178045 100644 --- a/src/declarative/qml/qdeclarativeexpression.cpp +++ b/src/declarative/qml/qdeclarativeexpression.cpp @@ -434,7 +434,7 @@ v8::Local QDeclarativeJavaScriptExpression::evaluate(v8::HandlesharedContext; diff --git a/src/declarative/qml/qdeclarativeexpression_p.h b/src/declarative/qml/qdeclarativeexpression_p.h index 65b07ca..bfe031f 100644 --- a/src/declarative/qml/qdeclarativeexpression_p.h +++ b/src/declarative/qml/qdeclarativeexpression_p.h @@ -57,12 +57,13 @@ #include #include +#include #include #include QT_BEGIN_NAMESPACE -class QDeclarativeAbstractExpression +class QDeclarativeAbstractExpression : public QDeleteWatchable { public: QDeclarativeAbstractExpression(); @@ -107,31 +108,8 @@ private: QDeclarativeDelayedError **prevError; }; -class QDeclarativeDeleteWatchable -{ -public: - inline QDeclarativeDeleteWatchable(); - inline ~QDeclarativeDeleteWatchable(); -private: - friend class QDeclarativeDeleteWatcher; - bool *m_wasDeleted; -}; - -class QDeclarativeDeleteWatcher { -public: - inline QDeclarativeDeleteWatcher(QDeclarativeDeleteWatchable *data); - inline ~QDeclarativeDeleteWatcher(); - inline bool wasDeleted() const; -private: - void *operator new(size_t); - bool *m_wasDeleted; - bool m_wasDeletedStorage; - QDeclarativeDeleteWatchable *m_d; -}; - class QDeclarativeJavaScriptExpression : public QDeclarativeAbstractExpression, - public QDeclarativeDelayedError, - public QDeclarativeDeleteWatchable + public QDeclarativeDelayedError { public: QDeclarativeJavaScriptExpression(); @@ -233,35 +211,6 @@ public: QDeclarativeRefCount *dataRef; }; -QDeclarativeDeleteWatchable::QDeclarativeDeleteWatchable() -: m_wasDeleted(0) -{ -} - -QDeclarativeDeleteWatchable::~QDeclarativeDeleteWatchable() -{ - if (m_wasDeleted) *m_wasDeleted = true; -} - -QDeclarativeDeleteWatcher::QDeclarativeDeleteWatcher(QDeclarativeDeleteWatchable *data) -: m_wasDeletedStorage(false), m_d(data) -{ - if (!m_d->m_wasDeleted) - m_d->m_wasDeleted = &m_wasDeletedStorage; - m_wasDeleted = m_d->m_wasDeleted; -} - -QDeclarativeDeleteWatcher::~QDeclarativeDeleteWatcher() -{ - if (false == *m_wasDeleted && m_wasDeleted == m_d->m_wasDeleted) - m_d->m_wasDeleted = 0; -} - -bool QDeclarativeDeleteWatcher::wasDeleted() const -{ - return *m_wasDeleted; -} - bool QDeclarativeJavaScriptExpression::requiresThisObject() const { return m_requiresThisObject; diff --git a/src/declarative/qml/qdeclarativeproperty.cpp b/src/declarative/qml/qdeclarativeproperty.cpp index c630de4..d6a2776 100644 --- a/src/declarative/qml/qdeclarativeproperty.cpp +++ b/src/declarative/qml/qdeclarativeproperty.cpp @@ -1348,7 +1348,7 @@ bool QDeclarativePropertyPrivate::writeBinding(QObject *object, int type = core.isValueTypeVirtual()?core.valueTypePropType:core.propType; - QDeclarativeDeleteWatcher watcher(expression); + QDeleteWatcher watcher(expression); QVariant value; bool isVmeProperty = core.isVMEProperty(); diff --git a/src/declarative/qml/v8/qv8bindings.cpp b/src/declarative/qml/v8/qv8bindings.cpp index 57411ae..271267d 100644 --- a/src/declarative/qml/v8/qv8bindings.cpp +++ b/src/declarative/qml/v8/qv8bindings.cpp @@ -85,7 +85,7 @@ void QV8Bindings::Binding::update(QDeclarativePropertyPrivate::WriteFlags flags) bool isUndefined = false; - QDeclarativeDeleteWatcher watcher(this); + QDeleteWatcher watcher(this); ep->referenceScarceResources(); v8::HandleScope handle_scope; diff --git a/tests/auto/declarative/qdeclarativecontext/data/RefreshExpressionsType.qml b/tests/auto/declarative/qdeclarativecontext/data/RefreshExpressionsType.qml new file mode 100644 index 0000000..b7c3427 --- /dev/null +++ b/tests/auto/declarative/qdeclarativecontext/data/RefreshExpressionsType.qml @@ -0,0 +1,5 @@ +import QtQuick 2.0 + +QtObject { + property var dummy: countCommand.doCommand(); +} diff --git a/tests/auto/declarative/qdeclarativecontext/data/refreshExpressions.qml b/tests/auto/declarative/qdeclarativecontext/data/refreshExpressions.qml new file mode 100644 index 0000000..01e503f --- /dev/null +++ b/tests/auto/declarative/qdeclarativecontext/data/refreshExpressions.qml @@ -0,0 +1,6 @@ +import QtQuick 2.0 + +QtObject { + property variant v1: RefreshExpressionsType {} + property variant v2: RefreshExpressionsType {} +} diff --git a/tests/auto/declarative/qdeclarativecontext/qdeclarativecontext.pro b/tests/auto/declarative/qdeclarativecontext/qdeclarativecontext.pro index 65af53a..32eea59 100644 --- a/tests/auto/declarative/qdeclarativecontext/qdeclarativecontext.pro +++ b/tests/auto/declarative/qdeclarativecontext/qdeclarativecontext.pro @@ -5,4 +5,4 @@ macx:CONFIG -= app_bundle CONFIG += parallel_test -QT += core-private gui-private declarative-private testlib +QT += core-private gui-private declarative-private testlib v8-private diff --git a/tests/auto/declarative/qdeclarativecontext/tst_qdeclarativecontext.cpp b/tests/auto/declarative/qdeclarativecontext/tst_qdeclarativecontext.cpp index c241aca..dee2cd9 100644 --- a/tests/auto/declarative/qdeclarativecontext/tst_qdeclarativecontext.cpp +++ b/tests/auto/declarative/qdeclarativecontext/tst_qdeclarativecontext.cpp @@ -45,6 +45,18 @@ #include #include #include +#include +#include "../shared/util.h" + +inline QUrl TEST_FILE(const QString &filename) +{ + return QUrl::fromLocalFile(TESTDATA(filename)); +} + +inline QUrl TEST_FILE(const char *filename) +{ + return TEST_FILE(QLatin1String(filename)); +} class tst_qdeclarativecontext : public QObject { @@ -64,6 +76,9 @@ private slots: void readOnlyContexts(); void nameForObject(); + void refreshExpressions(); + void refreshExpressionsCrash(); + private: QDeclarativeEngine engine; }; @@ -490,6 +505,111 @@ void tst_qdeclarativecontext::nameForObject() delete o; } +class DeleteCommand : public QObject +{ +Q_OBJECT +public: + DeleteCommand() : object(0) {} + + QObject *object; + +public slots: + void doCommand() { if (object) delete object; object = 0; } +}; + +// Calling refresh expressions would crash if an expression or context was deleted during +// the refreshing +void tst_qdeclarativecontext::refreshExpressionsCrash() +{ + { + QDeclarativeEngine engine; + + DeleteCommand command; + engine.rootContext()->setContextProperty("deleteCommand", &command); + // We use a fresh context here to bypass any root-context optimizations in + // the engine + QDeclarativeContext ctxt(engine.rootContext()); + + QDeclarativeComponent component(&engine); + component.setData("import QtQuick 2.0; QtObject { property var binding: deleteCommand.doCommand() }", QUrl()); + QVERIFY(component.isReady()); + + QObject *o1 = component.create(&ctxt); + QObject *o2 = component.create(&ctxt); + + command.object = o2; + + QDeclarativeContextData::get(&ctxt)->refreshExpressions(); + + delete o1; + } + { + QDeclarativeEngine engine; + + DeleteCommand command; + engine.rootContext()->setContextProperty("deleteCommand", &command); + // We use a fresh context here to bypass any root-context optimizations in + // the engine + QDeclarativeContext ctxt(engine.rootContext()); + + QDeclarativeComponent component(&engine); + component.setData("import QtQuick 2.0; QtObject { property var binding: deleteCommand.doCommand() }", QUrl()); + QVERIFY(component.isReady()); + + QObject *o1 = component.create(&ctxt); + QObject *o2 = component.create(&ctxt); + + command.object = o1; + + QDeclarativeContextData::get(&ctxt)->refreshExpressions(); + + delete o2; + } +} + +class CountCommand : public QObject +{ +Q_OBJECT +public: + CountCommand() : count(0) {} + + int count; + +public slots: + void doCommand() { ++count; } +}; + + +// Test that calling refresh expressions causes all the expressions to refresh +void tst_qdeclarativecontext::refreshExpressions() +{ + QDeclarativeEngine engine; + QDeclarativeComponent component(&engine, TEST_FILE("refreshExpressions.qml")); + QDeclarativeComponent component2(&engine, TEST_FILE("RefreshExpressionsType.qml")); + + CountCommand command; + engine.rootContext()->setContextProperty("countCommand", &command); + + // We use a fresh context here to bypass any root-context optimizations in + // the engine + QDeclarativeContext context(engine.rootContext()); + QDeclarativeContext context2(&context); + + QObject *o1 = component.create(&context); + QObject *o2 = component.create(&context2); + QObject *o3 = component2.create(&context); + + QCOMPARE(command.count, 5); + + QDeclarativeContextData::get(&context)->refreshExpressions(); + + QCOMPARE(command.count, 10); + + delete o3; + delete o2; + delete o1; +} + QTEST_MAIN(tst_qdeclarativecontext) #include "tst_qdeclarativecontext.moc" -- 2.7.4