QScriptEngine: Fix reentrency involving creation and desctructions of QScriptEngines

the currentIdentifierTable table, which is a static thread local variable, could be corrupted.

The main change is to fix the QScriptEngine constructor not to alter the currentIdentifierTable

This showed a lot of cases where APIShim guards where missings.

The problem was seen with creator, related to QTBUG-9426

Reviewed-by: Jedrzej Nowacki

4 files changed, 101 insertions, 24 deletions

diff --git a/src/script/api/qscriptengine.cpp b/src/script/api/qscriptengine.cpp
index b322523..70f798e 100644
--- a/src/script/api/qscriptengine.cpp
+++ b/src/script/api/qscriptengine.cpp
@@ -875,7 +875,7 @@ QScriptEnginePrivate::QScriptEnginePrivate()
         return;
     }
     JSC::initializeThreading();
-
+    JSC::IdentifierTable *oldTable = JSC::currentIdentifierTable();
     globalData = JSC::JSGlobalData::create().releaseRef();
     globalData->clientData = new QScript::GlobalClientData(this);
     JSC::JSGlobalObject *globalObject = new (globalData)QScript::GlobalObject();
@@ -911,11 +911,12 @@ QScriptEnginePrivate::QScriptEnginePrivate()
     activeAgent = 0;
     agentLineNumber = -1;
     processEventsInterval = -1;
+    JSC::setCurrentIdentifierTable(oldTable);
 }
 
 QScriptEnginePrivate::~QScriptEnginePrivate()
 {
-    JSC::setCurrentIdentifierTable(globalData->identifierTable);
+    QScript::APIShim shim(this);
 
     //disconnect all loadedScripts and generate all jsc::debugger::scriptUnload events
     QHash<intptr_t,QScript::UStringSourceProviderWithFeedback*>::const_iterator it;
@@ -3277,6 +3278,7 @@ bool QScriptEnginePrivate::hasDemarshalFunction(int type) const
 bool QScriptEngine::convert(const QScriptValue &value, int type, void *ptr)
 {
     Q_D(QScriptEngine);
+    QScript::APIShim shim(d);
     return QScriptEnginePrivate::convertValue(d->currentFrame, d->scriptValueToJSCValue(value), type, ptr);
 }
 
@@ -3289,8 +3291,12 @@ bool QScriptEngine::convertV2(const QScriptValue &value, int type, void *ptr)
     if (vp) {
         switch (vp->type) {
         case QScriptValuePrivate::JavaScriptCore: {
-            JSC::ExecState *exec = vp->engine ? vp->engine->currentFrame : 0;
-            return QScriptEnginePrivate::convertValue(exec, vp->jscValue, type, ptr);
+            if (vp->engine) {
+                QScript::APIShim shim(vp->engine);
+                return QScriptEnginePrivate::convertValue(vp->engine->currentFrame, vp->jscValue, type, ptr);
+            } else {
+                return QScriptEnginePrivate::convertValue(0, vp->jscValue, type, ptr);
+            }
         }
         case QScriptValuePrivate::Number:
             return QScriptEnginePrivate::convertNumber(vp->numberValue, type, ptr);
@@ -3341,6 +3347,7 @@ void QScriptEngine::registerCustomType(int type, MarshalFunction mf,
 void QScriptEngine::installTranslatorFunctions(const QScriptValue &object)
 {
     Q_D(QScriptEngine);
+    QScript::APIShim shim(d);
     JSC::ExecState* exec = d->currentFrame;
     JSC::JSValue jscObject = d->scriptValueToJSCValue(object);
     JSC::JSGlobalObject *glob = d->originalGlobalObject();
diff --git a/src/script/api/qscriptvalue.cpp b/src/script/api/qscriptvalue.cpp
index 4cd84a4..3aab268 100644
--- a/src/script/api/qscriptvalue.cpp
+++ b/src/script/api/qscriptvalue.cpp
@@ -561,6 +561,7 @@ QScriptValue QScriptValue::scope() const
     Q_D(const QScriptValue);
     if (!d || !d->isObject())
         return QScriptValue();
+    QScript::APIShim shim(d->engine);
     // ### make hidden property
     JSC::JSValue result = d->property("__qt_scope__", QScriptValue::ResolveLocal);
     return d->engine->scriptValueFromJSCValue(result);
@@ -650,11 +651,12 @@ static Type type(const QScriptValue &v)
     return Object;
 }
 
-QScriptValue ToPrimitive(const QScriptValue &object, JSC::PreferredPrimitiveType hint = JSC::NoPreference)
+static QScriptValue ToPrimitive(const QScriptValue &object, JSC::PreferredPrimitiveType hint = JSC::NoPreference)
 {
     Q_ASSERT(object.isObject());
     QScriptValuePrivate *pp = QScriptValuePrivate::get(object);
     Q_ASSERT(pp->engine != 0);
+    QScript::APIShim shim(pp->engine);
     JSC::ExecState *exec = pp->engine->currentFrame;
     JSC::JSValue savedException;
     QScriptEnginePrivate::saveException(exec, &savedException);
@@ -848,6 +850,7 @@ bool QScriptValue::equals(const QScriptValue &other) const
         if (!eng_p)
             eng_p = other.d_ptr->engine;
         if (eng_p) {
+            QScript::APIShim shim(eng_p);
             JSC::ExecState *exec = eng_p->currentFrame;
             JSC::JSValue savedException;
             QScriptEnginePrivate::saveException(exec, &savedException);
@@ -940,9 +943,12 @@ QString QScriptValue::toString() const
         return QString();
     switch (d->type) {
     case QScriptValuePrivate::JavaScriptCore: {
-        JSC::ExecState *exec = d->engine ? d->engine->currentFrame : 0;
-        return QScriptEnginePrivate::toString(exec, d->jscValue);
-    }
+        if (d->engine) {
+            QScript::APIShim shim(d->engine);
+            return QScriptEnginePrivate::toString(d->engine->currentFrame, d->jscValue);
+        } else {
+            return QScriptEnginePrivate::toString(0, d->jscValue);
+        }    }
     case QScriptValuePrivate::Number:
         return QScript::ToString(d->numberValue);
     case QScriptValuePrivate::String:
@@ -970,8 +976,12 @@ qsreal QScriptValue::toNumber() const
         return 0;
     switch (d->type) {
     case QScriptValuePrivate::JavaScriptCore: {
-        JSC::ExecState *exec = d->engine ? d->engine->currentFrame : 0;
-        return QScriptEnginePrivate::toNumber(exec, d->jscValue);
+        if (d->engine) {
+            QScript::APIShim shim(d->engine);
+            return QScriptEnginePrivate::toNumber(d->engine->currentFrame, d->jscValue);
+        } else {
+            return QScriptEnginePrivate::toNumber(0, d->jscValue);
+        }
     }
     case QScriptValuePrivate::Number:
         return d->numberValue;
@@ -993,8 +1003,12 @@ bool QScriptValue::toBoolean() const
         return false;
     switch (d->type) {
     case QScriptValuePrivate::JavaScriptCore: {
-        JSC::ExecState *exec = d->engine ? d->engine->currentFrame : 0;
-        return QScriptEnginePrivate::toBool(exec, d->jscValue);
+        if (d->engine) {
+            QScript::APIShim shim(d->engine);
+            return QScriptEnginePrivate::toBool(d->engine->currentFrame, d->jscValue);
+        } else {
+            return QScriptEnginePrivate::toBool(0, d->jscValue);
+        }
     }
     case QScriptValuePrivate::Number:
         return QScript::ToBool(d->numberValue);
@@ -1025,8 +1039,12 @@ bool QScriptValue::toBool() const
         return false;
     switch (d->type) {
     case QScriptValuePrivate::JavaScriptCore: {
-        JSC::ExecState *exec = d->engine ? d->engine->currentFrame : 0;
-        return QScriptEnginePrivate::toBool(exec, d->jscValue);
+        if (d->engine) {
+            QScript::APIShim shim(d->engine);
+            return QScriptEnginePrivate::toBool(d->engine->currentFrame, d->jscValue);
+        } else {
+            return QScriptEnginePrivate::toBool(0, d->jscValue);
+        }
     }
     case QScriptValuePrivate::Number:
         return QScript::ToBool(d->numberValue);
@@ -1055,8 +1073,12 @@ qint32 QScriptValue::toInt32() const
         return 0;
     switch (d->type) {
     case QScriptValuePrivate::JavaScriptCore: {
-        JSC::ExecState *exec = d->engine ? d->engine->currentFrame : 0;
-        return QScriptEnginePrivate::toInt32(exec, d->jscValue);
+        if (d->engine) {
+            QScript::APIShim shim(d->engine);
+            return QScriptEnginePrivate::toInt32(d->engine->currentFrame, d->jscValue);
+        } else {
+            return QScriptEnginePrivate::toInt32(0, d->jscValue);
+        }
     }
     case QScriptValuePrivate::Number:
         return QScript::ToInt32(d->numberValue);
@@ -1085,8 +1107,12 @@ quint32 QScriptValue::toUInt32() const
         return 0;
     switch (d->type) {
     case QScriptValuePrivate::JavaScriptCore: {
-        JSC::ExecState *exec = d->engine ? d->engine->currentFrame : 0;
-        return QScriptEnginePrivate::toUInt32(exec, d->jscValue);
+        if (d->engine) {
+            QScript::APIShim shim(d->engine);
+            return QScriptEnginePrivate::toUInt32(d->engine->currentFrame, d->jscValue);
+        } else {
+            return QScriptEnginePrivate::toUInt32(0, d->jscValue);
+        }
     }
     case QScriptValuePrivate::Number:
         return QScript::ToUInt32(d->numberValue);
@@ -1115,8 +1141,12 @@ quint16 QScriptValue::toUInt16() const
         return 0;
     switch (d->type) {
     case QScriptValuePrivate::JavaScriptCore: {
-        JSC::ExecState *exec = d->engine ? d->engine->currentFrame : 0;
-        return QScriptEnginePrivate::toUInt16(exec, d->jscValue);
+        if (d->engine) {
+            QScript::APIShim shim(d->engine);
+            return QScriptEnginePrivate::toUInt16(d->engine->currentFrame, d->jscValue);
+        } else {
+            return QScriptEnginePrivate::toUInt16(0, d->jscValue);
+        }
     }
     case QScriptValuePrivate::Number:
         return QScript::ToUInt16(d->numberValue);
@@ -1145,8 +1175,12 @@ qsreal QScriptValue::toInteger() const
         return 0;
     switch (d->type) {
     case QScriptValuePrivate::JavaScriptCore: {
-        JSC::ExecState *exec = d->engine ? d->engine->currentFrame : 0;
-        return QScriptEnginePrivate::toInteger(exec, d->jscValue);
+        if (d->engine) {
+            QScript::APIShim shim(d->engine);
+            return QScriptEnginePrivate::toInteger(d->engine->currentFrame, d->jscValue);
+        } else {
+            return QScriptEnginePrivate::toInteger(0, d->jscValue);
+        }
     }
     case QScriptValuePrivate::Number:
         return QScript::ToInteger(d->numberValue);
@@ -1185,8 +1219,12 @@ QVariant QScriptValue::toVariant() const
         return QVariant();
     switch (d->type) {
     case QScriptValuePrivate::JavaScriptCore: {
-        JSC::ExecState *exec = d->engine ? d->engine->currentFrame : 0;
-        return QScriptEnginePrivate::toVariant(exec, d->jscValue);
+        if (d->engine) {
+            QScript::APIShim shim(d->engine);
+            return QScriptEnginePrivate::toVariant(d->engine->currentFrame, d->jscValue);
+        } else {
+            return QScriptEnginePrivate::toVariant(0, d->jscValue);
+        }
     }
     case QScriptValuePrivate::Number:
         return QVariant(d->numberValue);
diff --git a/src/script/api/qscriptvalueiterator.cpp b/src/script/api/qscriptvalueiterator.cpp
index 7fd7093..460dddb 100644
--- a/src/script/api/qscriptvalueiterator.cpp
+++ b/src/script/api/qscriptvalueiterator.cpp
@@ -85,6 +85,17 @@ public:
         : initialized(false)
     {}
 
+    ~QScriptValueIteratorPrivate()
+    {
+        if (!initialized)
+            return;
+        QScriptEnginePrivate *eng_p = engine();
+        if (!eng_p)
+            return;
+        QScript::APIShim shim(eng_p);
+        propertyNames.clear(); //destroying the identifiers need to be done under the APIShim guard
+    }
+
     QScriptValuePrivate *object() const
     {
         return QScriptValuePrivate::get(objectValue);
diff --git a/tests/auto/qscriptengine/tst_qscriptengine.cpp b/tests/auto/qscriptengine/tst_qscriptengine.cpp
index 0615b63..3c6c7b2 100644
--- a/tests/auto/qscriptengine/tst_qscriptengine.cpp
+++ b/tests/auto/qscriptengine/tst_qscriptengine.cpp
@@ -161,6 +161,7 @@ private slots:
 
     void qRegExpInport_data();
     void qRegExpInport();
+    void reentrency();
 };
 
 tst_QScriptEngine::tst_QScriptEngine()
@@ -4577,5 +4578,25 @@ void tst_QScriptEngine::qRegExpInport()
     }
 }
 
+static QScriptValue createAnotherEngine(QScriptContext *, QScriptEngine *)
+{
+    QScriptEngine eng;
+    eng.evaluate("function foo(x, y) { return x + y; }" );
+    eng.evaluate("hello = 5; world = 6" );
+    return eng.evaluate("foo(hello,world)").toInt32();
+}
+
+
+void tst_QScriptEngine::reentrency()
+{
+    QScriptEngine eng;
+    eng.globalObject().setProperty("foo", eng.newFunction(createAnotherEngine));
+    eng.evaluate("function bar() { return foo(); }  hello = 9; function getHello() { return hello; }");
+    QCOMPARE(eng.evaluate("foo() + getHello() + foo()").toInt32(), 5+6 + 9 + 5+6);
+    QCOMPARE(eng.evaluate("foo").call().toInt32(), 5+6);
+    QCOMPARE(eng.evaluate("hello").toInt32(), 9);
+    QCOMPARE(eng.evaluate("foo() + hello").toInt32(), 5+6+9);
+}
+
 QTEST_MAIN(tst_QScriptEngine)
 #include "tst_qscriptengine.moc"