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>

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;

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)

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;

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

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();

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(); }

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 }
+    }
+}

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()
 {

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; }