Don't crash if contexts are deleted during refreshExpressions
authorAaron Kennedy <aaron.kennedy@nokia.com>
Wed, 2 Nov 2011 17:08:16 +0000 (17:08 +0000)
committerQt by Nokia <qt-info@nokia.com>
Fri, 4 Nov 2011 12:00:47 +0000 (13:00 +0100)
Change-Id: I23b59d33c07b017ef7355a7fe4a728d84c5d7eaa
Reviewed-by: Roberto Raggi <roberto.raggi@nokia.com>

13 files changed:
src/declarative/qml/ftw/ftw.pri
src/declarative/qml/ftw/qdeletewatcher_p.h [new file with mode: 0644]
src/declarative/qml/qdeclarativebinding.cpp
src/declarative/qml/qdeclarativecontext.cpp
src/declarative/qml/qdeclarativecontext_p.h
src/declarative/qml/qdeclarativeexpression.cpp
src/declarative/qml/qdeclarativeexpression_p.h
src/declarative/qml/qdeclarativeproperty.cpp
src/declarative/qml/v8/qv8bindings.cpp
tests/auto/declarative/qdeclarativecontext/data/RefreshExpressionsType.qml [new file with mode: 0644]
tests/auto/declarative/qdeclarativecontext/data/refreshExpressions.qml [new file with mode: 0644]
tests/auto/declarative/qdeclarativecontext/qdeclarativecontext.pro
tests/auto/declarative/qdeclarativecontext/tst_qdeclarativecontext.cpp

index 6339992..9d114e2 100644 (file)
@@ -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 (file)
index 0000000..34ef46b
--- /dev/null
@@ -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 <QtCore/qglobal.h>
+
+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
index 531842a..9a3a7e0 100644 (file)
@@ -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<QDeclarativeBinding *>()) {
 
index c4fc764..d7a6424 100644 (file)
@@ -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();
+
     }
 }
 
index a6e83a0..6676a06 100644 (file)
@@ -216,6 +216,8 @@ public:
     }
 
 private:
+    void refreshExpressionsRecursive();
+    void refreshExpressionsRecursive(QDeclarativeAbstractExpression *);
     ~QDeclarativeContextData() {}
 };
 
index fa899cf..b178045 100644 (file)
@@ -434,7 +434,7 @@ v8::Local<v8::Value> QDeclarativeJavaScriptExpression::evaluate(v8::Handle<v8::F
 
     // All code that follows must check with watcher before it accesses data members 
     // incase we have been deleted.
-    QDeclarativeDeleteWatcher watcher(this);
+    QDeleteWatcher watcher(this);
 
     if (sharedContext) {
         lastSharedContext = ep->sharedContext;
index 65b07ca..bfe031f 100644 (file)
 
 #include <private/qv8engine_p.h>
 #include <private/qfieldlist_p.h>
+#include <private/qdeletewatcher_p.h>
 #include <private/qdeclarativeguard_p.h>
 #include <private/qdeclarativeengine_p.h>
 
 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; 
index c630de4..d6a2776 100644 (file)
@@ -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();
index 57411ae..271267d 100644 (file)
@@ -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 (file)
index 0000000..b7c3427
--- /dev/null
@@ -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 (file)
index 0000000..01e503f
--- /dev/null
@@ -0,0 +1,6 @@
+import QtQuick 2.0
+
+QtObject {
+    property variant v1: RefreshExpressionsType {}
+    property variant v2: RefreshExpressionsType {}
+}
index 65af53a..32eea59 100644 (file)
@@ -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
index c241aca..dee2cd9 100644 (file)
 #include <QDeclarativeContext>
 #include <QDeclarativeComponent>
 #include <QDeclarativeExpression>
+#include <private/qdeclarativecontext_p.h>
+#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"