Fix QJSEngine::evaluate using the wrong execution context

In contrary to what the documentation says, QJSEngine in Qt 5.x executes
in the context of the global object (QScriptIsolate always called enter
on the QV8Engine's "root" context, thus making it current). The v4
implementation unfortunately did what the documentation said and used
the current context, which is wrong in many ways. For example it completely
breaks the optimization of stack allocated contexts, because when a C++
callback is called from within a JS function with a stack allocated context
and that C++ code calls QJSEngine::evaluate and creates new closures, the
stack context would become an outter context and cause crashes during GC.

This patch restores the behavior of Qt 5.0/5.1 and fixes the documentation.

Task-number: QTBUG-38530
Change-Id: Ie6481f02e676954cc94b188a1c87c88e7c56dafa
Reviewed-by: Lars Knoll <lars.knoll@digia.com>

diff --git a/src/qml/jsapi/qjsengine.cpp b/src/qml/jsapi/qjsengine.cpp
index d05ada4..9cb727d 100644 (file)
--- a/src/qml/jsapi/qjsengine.cpp
+++ b/src/qml/jsapi/qjsengine.cpp
@@ -235,7 +235,7 @@ void QJSEngine::collectGarbage()
     Evaluates \a program, using \a lineNumber as the base line number,
     and returns the result of the evaluation.
 
-    The script code will be evaluated in the current context.
+    The script code will be evaluated in the context of the global object.
 
     The evaluation of \a program can cause an exception in the
     engine; in this case the return value will be the exception
@@ -261,8 +261,11 @@ void QJSEngine::collectGarbage()
 */
 QJSValue QJSEngine::evaluate(const QString& program, const QString& fileName, int lineNumber)
 {
-    QV4::Scope scope(d->m_v4Engine);
-    QV4::ExecutionContext *ctx = d->m_v4Engine->currentContext();
+    QV4::ExecutionEngine *v4 = d->m_v4Engine;
+    QV4::Scope scope(v4);
+    QV4::ExecutionContext *ctx = v4->currentContext();
+    if (ctx != v4->rootContext)
+        ctx = v4->pushGlobalContext();
     QV4::ScopedValue result(scope);
 
     QV4::Script script(ctx, program, fileName, lineNumber);
@@ -273,7 +276,9 @@ QJSValue QJSEngine::evaluate(const QString& program, const QString& fileName, in
         result = script.run();
     if (scope.engine->hasException)
         result = ctx->catchException();
-    return new QJSValuePrivate(d->m_v4Engine, result);
+    if (ctx != v4->rootContext)
+        v4->popContext();
+    return new QJSValuePrivate(v4, result);
 }
 
 /*!

diff --git a/tests/auto/qml/qjsengine/tst_qjsengine.cpp b/tests/auto/qml/qjsengine/tst_qjsengine.cpp
index b2723b0..17acb4f 100644 (file)
--- a/tests/auto/qml/qjsengine/tst_qjsengine.cpp
+++ b/tests/auto/qml/qjsengine/tst_qjsengine.cpp
@@ -157,6 +157,8 @@ private slots:
 
     void dynamicProperties();
 
+    void scopeOfEvaluate();
+
 signals:
     void testSignal();
 };
@@ -3011,6 +3013,43 @@ void tst_QJSEngine::dynamicProperties()
     }
 }
 
+class EvaluateWrapper : public QObject
+{
+    Q_OBJECT
+public:
+    EvaluateWrapper(QJSEngine *engine)
+        : engine(engine)
+    {}
+
+public slots:
+    QJSValue cppEvaluate(const QString &program)
+    {
+        return engine->evaluate(program);
+    }
+
+private:
+    QJSEngine *engine;
+};
+
+void tst_QJSEngine::scopeOfEvaluate()
+{
+    QJSEngine engine;
+    QJSValue wrapper = engine.newQObject(new EvaluateWrapper(&engine));
+
+    engine.evaluate("testVariable = 42");
+
+    QJSValue function = engine.evaluate("(function(evalWrapper){\n"
+                                        "var testVariable = 100; \n"
+                                        "try { \n"
+                                        "    return evalWrapper.cppEvaluate(\"(function() { return testVariable; })\")\n"
+                                        "           ()\n"
+                                        "} catch (e) {}\n"
+                                        "})");
+    QVERIFY(function.isCallable());
+    QJSValue result = function.call(QJSValueList() << wrapper);
+    QCOMPARE(result.toInt(), 42);
+}
+
 QTEST_MAIN(tst_QJSEngine)
 
 #include "tst_qjsengine.moc"