make it possible to pop all items in a scope chain and then push to it

A lot of the JSC::ExecState functions rely on scopeChain() not
being 0. This means we shouldn't pop the scope chain if doing so
would make it empty; otherwise when you call e.g. pushScope() or
evaluate(), you will crash. So instead of popping the chain
completely, we now set the sole scope chain item's object pointer to
0 and add appropriate checks elsewhere.

A second issue, not solved in this commit, is that JSC expects
the Global Object to always be the last item in every scope
chain. If it's not, you will crash.

2 files changed, 28 insertions, 14 deletions

diff --git a/src/script/api/qscriptcontext.cpp b/src/script/api/qscriptcontext.cpp
index f4b8af6..25763f9 100644
--- a/src/script/api/qscriptcontext.cpp
+++ b/src/script/api/qscriptcontext.cpp
@@ -333,7 +333,7 @@ bool QScriptContext::isCalledAsConstructor() const
     JSC::ScopeChainNode *node = frame->scopeChain();
     JSC::ScopeChainIterator it(node);
     for (it = node->begin(); it != node->end(); ++it) {
-        if ((*it)->isVariableObject()) {
+        if ((*it) && (*it)->isVariableObject()) {
             if ((*it)->inherits(&QScript::QScriptActivationObject::info)) {
                 return static_cast<QScript::QScriptActivationObject *>(*it)->d_ptr()->calledAsConstructor;
             }
@@ -436,7 +436,7 @@ QScriptValue QScriptContext::activationObject() const
         JSC::ScopeChainNode *node = frame->scopeChain();
         JSC::ScopeChainIterator it(node);
         for (it = node->begin(); it != node->end(); ++it) {
-            if ((*it)->isVariableObject()) {
+            if ((*it) && (*it)->isVariableObject()) {
                 result = *it;
                 break;
             }
@@ -484,7 +484,7 @@ void QScriptContext::setActivationObject(const QScriptValue &activation)
     // replace the first activation object in the scope chain
     JSC::ScopeChainNode *node = frame->scopeChain();
     while (node != 0) {
-        if (node->object->isVariableObject()) {
+        if (node->object && node->object->isVariableObject()) {
             node->object = object;
             break;
         }
@@ -631,11 +631,14 @@ QString QScriptContext::toString() const
 QScriptValueList QScriptContext::scopeChain() const
 {
     const JSC::CallFrame *frame = QScriptEnginePrivate::frameForContext(this);
+    QScriptEnginePrivate *engine = QScript::scriptEngineFromExec(frame);
     QScriptValueList result;
     JSC::ScopeChainNode *node = frame->scopeChain();
     JSC::ScopeChainIterator it(node);
-    for (it = node->begin(); it != node->end(); ++it)
-        result.append(QScript::scriptEngineFromExec(frame)->scriptValueFromJSCValue(*it));
+    for (it = node->begin(); it != node->end(); ++it) {
+        if (*it)
+            result.append(engine->scriptValueFromJSCValue(*it));
+    }
     return result;
 }
 
@@ -659,7 +662,12 @@ void QScriptContext::pushScope(const QScriptValue &object)
     }
     JSC::CallFrame *frame = QScriptEnginePrivate::frameForContext(this);
     JSC::JSValue jscObject = QScript::scriptEngineFromExec(frame)->scriptValueToJSCValue(object);
-    frame->setScopeChain(frame->scopeChain()->push(JSC::asObject(jscObject)));
+    JSC::ScopeChainNode *scope = frame->scopeChain();
+    Q_ASSERT(scope != 0);
+    if (!scope->object) // pushing to an "empty" chain
+        scope->object = JSC::asObject(jscObject);
+    else
+        frame->setScopeChain(scope->push(JSC::asObject(jscObject)));
 }
 
 /*!
@@ -675,8 +683,16 @@ void QScriptContext::pushScope(const QScriptValue &object)
 QScriptValue QScriptContext::popScope()
 {
     JSC::CallFrame *frame = QScriptEnginePrivate::frameForContext(this);
-    QScriptValue result = QScript::scriptEngineFromExec(frame)->scriptValueFromJSCValue(frame->scopeChain()->object);
-    frame->setScopeChain(frame->scopeChain()->pop());
+    JSC::ScopeChainNode *scope = frame->scopeChain();
+    Q_ASSERT(scope != 0);
+    QScriptEnginePrivate *engine = QScript::scriptEngineFromExec(frame);
+    QScriptValue result = engine->scriptValueFromJSCValue(scope->object);
+    if (!scope->next) {
+        // We cannot have a null scope chain, so just zap the object pointer.
+        scope->object = 0;
+    } else {
+        frame->setScopeChain(scope->pop());
+    }
     return result;
 }
 
diff --git a/tests/auto/qscriptcontext/tst_qscriptcontext.cpp b/tests/auto/qscriptcontext/tst_qscriptcontext.cpp
index be0b2b7..c8b21a5 100644
--- a/tests/auto/qscriptcontext/tst_qscriptcontext.cpp
+++ b/tests/auto/qscriptcontext/tst_qscriptcontext.cpp
@@ -674,19 +674,19 @@ void tst_QScriptContext::pushAndPopScope()
     ctx->pushScope(QScriptValue());
     QCOMPARE(ctx->scopeChain().size(), 1);
 
-    qWarning("Popping the top-level scope causes crash");
-#if 0
     QVERIFY(ctx->popScope().strictlyEquals(eng.globalObject()));
     QVERIFY(ctx->scopeChain().isEmpty());
-#endif
 
     ctx->pushScope(obj);
-    QCOMPARE(ctx->scopeChain().size(), 2);
+    QCOMPARE(ctx->scopeChain().size(), 1);
     QVERIFY(ctx->scopeChain().at(0).strictlyEquals(obj));
     QVERIFY(!obj.property("foo").isValid());
+#if 0 // ### CRASHES, JSC expects last scope object to always be the Global Object
     eng.evaluate("function foo() {}");
     // function declarations should always end up in the activation object (ECMA-262, chapter 13)
     QVERIFY(!obj.property("foo").isValid());
+#endif
+    QEXPECT_FAIL("", "JSC requires last object in scope chain to be the Global Object", Continue);
     QVERIFY(ctx->activationObject().property("foo").isFunction());
 
     QScriptEngine eng2;
@@ -695,10 +695,8 @@ void tst_QScriptContext::pushAndPopScope()
     ctx->pushScope(obj2);
 
     QVERIFY(ctx->popScope().strictlyEquals(obj));
-    QEXPECT_FAIL("", "This should work once the above crash issue has been fixed", Continue);
     QVERIFY(ctx->scopeChain().isEmpty());
 
-    QSKIP("Crashes", SkipAll);
     QVERIFY(!ctx->popScope().isValid());
 }