Loosen shared binding tests

Previously we blocked all function calls, but only because we were trying
to prevent "eval" calls from appearing in shared bindings.  Instead this
test allows function calls as long as the identifier "eval" doesn't
appear.

This also exposed a crash with v8 bindings that is also fixed.

Change-Id: I22eefd290c7b82d9c01b2cd2907a46e560ae4db9
Reviewed-by: Chris Adams <christopher.adams@nokia.com>
Reviewed-by: Michael Brasser <michael.brasser@nokia.com>

diff --git a/src/qml/qml/qqmlexpression.cpp b/src/qml/qml/qqmlexpression.cpp
index 6e20047..940b3c8 100644 (file)
--- a/src/qml/qml/qqmlexpression.cpp
+++ b/src/qml/qml/qqmlexpression.cpp
@@ -60,7 +60,7 @@ static QQmlJavaScriptExpression::VTable QQmlExpressionPrivate_jsvtable = {
 QQmlExpressionPrivate::QQmlExpressionPrivate()
 : QQmlJavaScriptExpression(&QQmlExpressionPrivate_jsvtable),
   expressionFunctionValid(true), expressionFunctionRewritten(false),
-  extractExpressionFromFunction(false), line(-1), dataRef(0)
+  line(-1)
 {
 }
 
@@ -68,8 +68,6 @@ QQmlExpressionPrivate::~QQmlExpressionPrivate()
 {
     qPersistentDispose(v8qmlscope);
     qPersistentDispose(v8function);
-    if (dataRef) dataRef->release();
-    dataRef = 0;
 }
 
 void QQmlExpressionPrivate::init(QQmlContextData *ctxt, const QString &expr, QObject *me)
@@ -321,13 +319,7 @@ QQmlContext *QQmlExpression::context() const
 QString QQmlExpression::expression() const
 {
     Q_D(const QQmlExpression);
-    if (d->extractExpressionFromFunction && context()->engine()) {
-        QV8Engine *v8engine = QQmlEnginePrivate::getV8Engine(context()->engine());
-        v8::HandleScope handle_scope;
-        v8::Context::Scope scope(v8engine->context());
-
-        return v8engine->toString(v8::Handle<v8::Value>(d->v8function));
-    } else if (!d->expressionUtf8.isEmpty()) {
+    if (!d->expressionUtf8.isEmpty()) {
         return QString::fromUtf8(d->expressionUtf8);
     } else {
         return d->expression;

diff --git a/src/qml/qml/qqmlexpression_p.h b/src/qml/qml/qqmlexpression_p.h
index d3d27f2..d32e2d3 100644 (file)
--- a/src/qml/qml/qqmlexpression_p.h
+++ b/src/qml/qml/qqmlexpression_p.h
@@ -96,7 +96,6 @@ public:
 
     bool expressionFunctionValid:1;
     bool expressionFunctionRewritten:1;
-    bool extractExpressionFromFunction:1;
 
     // "Inherited" from QQmlJavaScriptExpression
     static QString expressionIdentifier(QQmlJavaScriptExpression *);
@@ -113,8 +112,6 @@ public:
     int line;
     int column;
     QString name; //function name, hint for the debugger
-
-    QQmlRefCount *dataRef;
 };
 
 QQmlExpressionPrivate *QQmlExpressionPrivate::get(QQmlExpression *expr)

diff --git a/src/qml/qml/qqmlrewrite.cpp b/src/qml/qml/qqmlrewrite.cpp
index 0bd8597..bbb17b6 100644 (file)
--- a/src/qml/qml/qqmlrewrite.cpp
+++ b/src/qml/qml/qqmlrewrite.cpp
@@ -51,6 +51,8 @@ DEFINE_BOOL_CONFIG_OPTION(rewriteDump, QML_REWRITE_DUMP);
 
 namespace QQmlRewrite {
 
+QString SharedBindingTester::evalString("eval");
+
 static void rewriteStringLiteral(AST::StringLiteral *ast, const QString *code, int startPosition, TextWriter *writer)
 {
     const unsigned position = ast->firstSourceLocation().begin() - startPosition + 1;

diff --git a/src/qml/qml/qqmlrewrite_p.h b/src/qml/qml/qqmlrewrite_p.h
index 73ff50a..efa10ce 100644 (file)
--- a/src/qml/qml/qqmlrewrite_p.h
+++ b/src/qml/qml/qqmlrewrite_p.h
@@ -70,9 +70,11 @@ public:
     bool isSharable(const QString &code);
     bool isSharable(AST::Node *Node);
     
-    virtual bool visit(AST::FunctionDeclaration *) { _sharable = false; return false; }
-    virtual bool visit(AST::FunctionExpression *) { _sharable = false; return false; }
-    virtual bool visit(AST::CallExpression *) { _sharable = false; return false; }
+    inline virtual bool visit(AST::FunctionDeclaration *);
+    inline virtual bool visit(AST::FunctionExpression *);
+    inline virtual bool visit(AST::IdentifierExpression *);
+
+    static QString evalString;
 };
 
 class RewriteBinding: protected AST::Visitor
@@ -143,6 +145,26 @@ protected:
     virtual bool visit(AST::StringLiteral *ast);
 };
 
+bool SharedBindingTester::visit(AST::FunctionDeclaration *)
+{
+    _sharable = false;
+    return false;
+}
+
+bool SharedBindingTester::visit(AST::FunctionExpression *)
+{
+    _sharable = false;
+    return false;
+}
+
+bool SharedBindingTester::visit(AST::IdentifierExpression *e)
+{
+    if (e->name == evalString)
+        _sharable = false;
+
+    return false; // IdentifierExpression is a leaf node anyway
+}
+
 } // namespace QQmlRewrite
 
 QT_END_NAMESPACE

diff --git a/src/qml/qml/v8/qv8bindings.cpp b/src/qml/qml/v8/qv8bindings.cpp
index 4b96679..487e100 100644 (file)
--- a/src/qml/qml/v8/qv8bindings.cpp
+++ b/src/qml/qml/v8/qv8bindings.cpp
@@ -58,7 +58,7 @@ static QQmlJavaScriptExpression::VTable QV8Bindings_Binding_jsvtable = {
 };
 
 QV8Bindings::Binding::Binding()
-: QQmlJavaScriptExpression(&QV8Bindings_Binding_jsvtable), target(0), parent(0)
+: QQmlJavaScriptExpression(&QV8Bindings_Binding_jsvtable), parent(0)
 {
 }
 
@@ -90,7 +90,7 @@ int QV8Bindings::Binding::propertyIndex() const
 
 QObject *QV8Bindings::Binding::object() const
 {
-    return target;
+    return *target;
 }
 
 void QV8Bindings::Binding::update(QQmlPropertyPrivate::WriteFlags flags)
@@ -127,13 +127,13 @@ void QV8Bindings::Binding::update(QQmlPropertyPrivate::WriteFlags flags)
 
         trace.event("writing V8 result");
         bool needsErrorData = false;
-        if (!watcher.wasDeleted() && !hasError()) {
+        if (!watcher.wasDeleted() && !destroyedFlag() && !hasError()) {
             typedef QQmlPropertyPrivate PP;
-            needsErrorData = !PP::writeBinding(target, instruction->property, context, this, result,
+            needsErrorData = !PP::writeBinding(*target, instruction->property, context, this, result,
                                                isUndefined, flags);
         }
 
-        if (!watcher.wasDeleted()) {
+        if (!watcher.wasDeleted() && !destroyedFlag()) {
 
             if (needsErrorData) {
                 QUrl url = parent->url();
@@ -156,7 +156,7 @@ void QV8Bindings::Binding::update(QQmlPropertyPrivate::WriteFlags flags)
         ep->dereferenceScarceResources(); 
 
     } else {
-        QQmlProperty p = QQmlPropertyPrivate::restore(target, instruction->property, context);
+        QQmlProperty p = QQmlPropertyPrivate::restore(*target, instruction->property, context);
         QQmlAbstractBinding::printBindingLoopError(p);
     }
 }
@@ -177,6 +177,7 @@ void QV8Bindings::Binding::expressionChanged(QQmlJavaScriptExpression *e)
 void QV8Bindings::Binding::destroy()
 {
     setEnabledFlag(false);
+    setDestroyedFlag(true);
     removeFromObject();
     clear();
     clearError();

diff --git a/src/qml/qml/v8/qv8bindings_p.h b/src/qml/qml/v8/qv8bindings_p.h
index ad5b2cb..8d27207 100644 (file)
--- a/src/qml/qml/v8/qv8bindings_p.h
+++ b/src/qml/qml/v8/qv8bindings_p.h
@@ -99,14 +99,17 @@ public:
         virtual int propertyIndex() const;
         virtual QObject *object() const;
 
-        QObject *target;
         QV8Bindings *parent;
 
         // To save memory, we store flags inside the instruction pointer.
-        //    flag1: enabled
-        //    flag2: updating
+        //    target.flag1: destroyed
+        //    instruction.flag1: enabled
+        //    instruction.flag2: updating
+        QFlagPointer<QObject> target;
         QFlagPointer<const QQmlInstruction::instr_assignBinding> instruction;
 
+        inline bool destroyedFlag() const { return target.flag(); }
+        inline void setDestroyedFlag(bool v) { return target.setFlagValue(v); }
         inline bool enabledFlag() const { return instruction.flag(); }
         inline void setEnabledFlag(bool v) { instruction.setFlagValue(v); }
         inline bool updatingFlag() const { return instruction.flag2(); }

diff --git a/tests/auto/qml/qqmlecmascript/data/singleV8BindingDestroyedDuringEvaluation.qml b/tests/auto/qml/qqmlecmascript/data/singleV8BindingDestroyedDuringEvaluation.qml
new file mode 100644 (file)
index 0000000..ae84f02
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/singleV8BindingDestroyedDuringEvaluation.qml
@@ -0,0 +1,12 @@
+import QtQuick 2.0
+import Qt.test 1.0
+
+Item {
+    MyQmlObject {
+        value: if (1) 3
+    }
+
+    MyQmlObject {
+        value: { deleteMe(), 2 }
+    }
+}

diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index eaa6d39..6b10672 100644 (file)
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -183,7 +183,7 @@ private slots:
     void assignSequenceTypes();
     void qtbug_22464();
     void qtbug_21580();
-
+    void singleV8BindingDestroyedDuringEvaluation();
     void bug1();
     void bug2();
     void dynamicCreationCrash();
@@ -1901,6 +1901,16 @@ void tst_qqmlecmascript::qtbug_21580()
     delete object;
 }
 
+// Causes a v8 binding, but not all v8 bindings to be destroyed during evaluation
+void tst_qqmlecmascript::singleV8BindingDestroyedDuringEvaluation()
+{
+    QQmlComponent component(&engine, testFileUrl("singleV8BindingDestroyedDuringEvaluation.qml"));
+
+    QObject *object = component.create();
+    QVERIFY(object != 0);
+    delete object;
+}
+
 // QTBUG-6781
 void tst_qqmlecmascript::bug1()
 {

diff --git a/tests/auto/qml/qqmlproperty/data/invalidBinding.qml b/tests/auto/qml/qqmlproperty/data/invalidBinding.qml
index 58e8307..35dacf1 100644 (file)
--- a/tests/auto/qml/qqmlproperty/data/invalidBinding.qml
+++ b/tests/auto/qml/qqmlproperty/data/invalidBinding.qml
@@ -4,7 +4,7 @@ Item {
     property Text text: myText
 
     property Rectangle rectangle1: myText
-    property Rectangle rectangle2: getMyText()
+    property Rectangle rectangle2: eval('getMyText()') // eval to force non-shared (v8) binding
 
     function getMyText() { return myText; }