From 833336fa963b3cd9f1996d7d255098f523dfa580 Mon Sep 17 00:00:00 2001 From: Aaron Kennedy Date: Fri, 9 Mar 2012 11:40:44 +0100 Subject: [PATCH] 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 Reviewed-by: Michael Brasser --- src/qml/qml/qqmlexpression.cpp | 12 ++-------- src/qml/qml/qqmlexpression_p.h | 3 --- src/qml/qml/qqmlrewrite.cpp | 2 ++ src/qml/qml/qqmlrewrite_p.h | 28 +++++++++++++++++++--- src/qml/qml/v8/qv8bindings.cpp | 13 +++++----- src/qml/qml/v8/qv8bindings_p.h | 9 ++++--- .../singleV8BindingDestroyedDuringEvaluation.qml | 12 ++++++++++ .../auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp | 12 +++++++++- .../auto/qml/qqmlproperty/data/invalidBinding.qml | 2 +- 9 files changed, 66 insertions(+), 27 deletions(-) create mode 100644 tests/auto/qml/qqmlecmascript/data/singleV8BindingDestroyedDuringEvaluation.qml diff --git a/src/qml/qml/qqmlexpression.cpp b/src/qml/qml/qqmlexpression.cpp index 6e20047..940b3c8 100644 --- 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(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 --- 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 --- 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 --- 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 --- 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 --- 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 target; QFlagPointer 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 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 --- 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 --- 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; } -- 2.7.4