QScriptEngine::pushContext(): Don't inherit parent context's scope

This was a regression introduced in Qt 4.6 (JavaScriptCore-based
backend). pushContext() should always create a context with a "clean"
scope (only the Global Object and the context's own activation object
should be in the scope chain).

The scope chain API is internal, but the wrong behavior could still
be observed e.g. through QScriptEngine::evaluate().

Task-number: QTBUG-18188
Change-Id: I138dabc665d7275fb85d3b5e1b473d56096a989e
Reviewed-by: Olivier Goffart <ogoffart@woboq.com>

4 files changed, 36 insertions, 22 deletions

diff --git a/src/script/api/qscriptengine.cpp b/src/script/api/qscriptengine.cpp
index 3b1cb9d..8f8c86d 100644
--- a/src/script/api/qscriptengine.cpp
+++ b/src/script/api/qscriptengine.cpp
@@ -2733,8 +2733,7 @@ QScriptContext *QScriptEngine::pushContext()
    return the new top frame. (might be the same as exec if a new stackframe was not needed) or 0 if stack overflow
 */
 JSC::CallFrame *QScriptEnginePrivate::pushContext(JSC::CallFrame *exec, JSC::JSValue _thisObject,
-                                                  const JSC::ArgList& args, JSC::JSObject *callee, bool calledAsConstructor,
-                                                  bool clearScopeChain)
+                                                  const JSC::ArgList& args, JSC::JSObject *callee, bool calledAsConstructor)
 {
     JSC::JSValue thisObject = _thisObject;
     if (!callee) {
@@ -2777,11 +2776,7 @@ JSC::CallFrame *QScriptEnginePrivate::pushContext(JSC::CallFrame *exec, JSC::JSV
             newCallFrame[++dst] = *it;
         newCallFrame += argc + JSC::RegisterFile::CallFrameHeaderSize;
 
-        if (!clearScopeChain) {
-            newCallFrame->init(0, /*vPC=*/0, exec->scopeChain(), exec, flags | ShouldRestoreCallFrame, argc, callee);
-        } else {
-            newCallFrame->init(0, /*vPC=*/0, globalExec()->scopeChain(), exec, flags | ShouldRestoreCallFrame, argc, callee);
-        }
+        newCallFrame->init(0, /*vPC=*/0, globalExec()->scopeChain(), exec, flags | ShouldRestoreCallFrame, argc, callee);
     } else {
         setContextFlags(newCallFrame, flags);
 #if ENABLE(JIT)
diff --git a/src/script/api/qscriptengine_p.h b/src/script/api/qscriptengine_p.h
index 6c82522..09ecaf2 100644
--- a/src/script/api/qscriptengine_p.h
+++ b/src/script/api/qscriptengine_p.h
@@ -248,7 +248,7 @@ public:
     static JSC::Register *thisRegisterForFrame(JSC::ExecState *frame);
 
     JSC::CallFrame *pushContext(JSC::CallFrame *exec, JSC::JSValue thisObject, const JSC::ArgList& args,
-                                JSC::JSObject *callee, bool calledAsConstructor = false, bool clearScopeChain = false);
+                                JSC::JSObject *callee, bool calledAsConstructor = false);
     void popContext();
 
     void mark(JSC::MarkStack& markStack);
diff --git a/src/script/bridge/qscriptdeclarativeclass.cpp b/src/script/bridge/qscriptdeclarativeclass.cpp
index 3fa8943..01886e0 100644
--- a/src/script/bridge/qscriptdeclarativeclass.cpp
+++ b/src/script/bridge/qscriptdeclarativeclass.cpp
@@ -397,27 +397,19 @@ QScriptValue QScriptDeclarativeClass::scopeChainValue(QScriptContext *context, i
   By default, the `this' object of the new context is the Global Object.
   The context's \l{QScriptContext::callee()}{callee}() will be invalid.
 
-  Unlike pushContext(), the default scope chain is reset to include
-  only the global object and the QScriptContext's activation object.
+  The context's scope chain initially contains only the Global Object
+  and the QScriptContext's activation object.
 
-  \sa QScriptEngine::popContext()
+  This function behaves exactly like QScriptEngine::popContext();
+  it exists because QScriptEngine::popContext() used to have a bug
+  that caused the scope chain of the new context to be incorrect.
 */
 QScriptContext * QScriptDeclarativeClass::pushCleanContext(QScriptEngine *engine)
 {
     if (!engine)
         return 0;
 
-    QScriptEnginePrivate *d = QScriptEnginePrivate::get(engine);
-    QScript::APIShim shim(d);
-
-    JSC::CallFrame* newFrame = d->pushContext(d->currentFrame, 
-                                              d->currentFrame->globalData().dynamicGlobalObject,
-                                              JSC::ArgList(), /*callee = */0, false, true);
-
-    if (engine->agent())
-        engine->agent()->contextPush();
-
-    return d->contextForFrame(newFrame);
+    return engine->pushContext();
 }
 
 QScriptDeclarativeClass::~QScriptDeclarativeClass()
diff --git a/tests/auto/qscriptengine/tst_qscriptengine.cpp b/tests/auto/qscriptengine/tst_qscriptengine.cpp
index b1b416c..514d29a 100644
--- a/tests/auto/qscriptengine/tst_qscriptengine.cpp
+++ b/tests/auto/qscriptengine/tst_qscriptengine.cpp
@@ -250,6 +250,7 @@ private slots:
     void stringListFromArrayWithEmptyElement();
     void collectQObjectWithCachedWrapper_data();
     void collectQObjectWithCachedWrapper();
+    void pushContext_noInheritedScope();
 };
 
 tst_QScriptEngine::tst_QScriptEngine()
@@ -6218,5 +6219,31 @@ void tst_QScriptEngine::collectQObjectWithCachedWrapper()
     QCOMPARE(ptr == 0, shouldBeCollected);
 }
 
+// QTBUG-18188
+void tst_QScriptEngine::pushContext_noInheritedScope()
+{
+    QScriptEngine eng;
+    eng.globalObject().setProperty("foo", 123);
+
+    QScriptContext *ctx1 = eng.pushContext();
+    QCOMPARE(ctx1->scopeChain().size(), 2);
+    ctx1->activationObject().setProperty("foo", 456);
+    QCOMPARE(eng.evaluate("foo").toInt32(), 456);
+
+    QScriptContext *ctx2 = eng.pushContext();
+    // The parent context's scope should not be inherited.
+    QCOMPARE(ctx2->scopeChain().size(), 2);
+    QCOMPARE(eng.evaluate("foo").toInt32(), 123);
+
+    ctx2->activationObject().setProperty("foo", 789);
+    QCOMPARE(eng.evaluate("foo").toInt32(), 789);
+
+    eng.popContext();
+    QCOMPARE(eng.evaluate("foo").toInt32(), 456);
+
+    eng.popContext();
+    QCOMPARE(eng.evaluate("foo").toInt32(), 123);
+}
+
 QTEST_MAIN(tst_QScriptEngine)
 #include "tst_qscriptengine.moc"