From d4166fa6ce24b55b483f29e8ef447c0f63f0a30f Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Fri, 25 Feb 2011 13:23:07 +0100 Subject: Don't crash when marking arguments object of native context JSC assumes that the callee is always valid, since JSC::Arguments is used for JS frames, which must have a callee. But we use JSC::Arguments for arguments object of pushContext()-created contexts, and then there is no callee. But the callee member can't be null, so now we put a fake callee there and make sure it doesn't bleed up to the public API. Alternative solution: Add "if (d->callee)" to JSC::Arguments::markChildren(), then no other changes would be needed. But we don't want to patch JSC any more. Non-solution: Subclass JSC::Arguments and reimplement markChildren() to temporarily set a dummy callee during marking. Can't be done, as JSC::Arguments::d is private (again, we don't want to patch JSC). Task-number: QTBUG-17788 Reviewed-by: Olivier Goffart --- src/script/api/qscriptcontext.cpp | 10 ++++++++-- src/script/api/qscriptengine.cpp | 8 ++++++++ tests/auto/qscriptengine/tst_qscriptengine.cpp | 11 +++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/script/api/qscriptcontext.cpp b/src/script/api/qscriptcontext.cpp index 2468a46..5454df5 100644 --- a/src/script/api/qscriptcontext.cpp +++ b/src/script/api/qscriptcontext.cpp @@ -268,8 +268,14 @@ QScriptValue QScriptContext::argument(int index) const QScriptValue QScriptContext::callee() const { const JSC::CallFrame *frame = QScriptEnginePrivate::frameForContext(this); - QScript::APIShim shim(QScript::scriptEngineFromExec(frame)); - return QScript::scriptEngineFromExec(frame)->scriptValueFromJSCValue(frame->callee()); + QScriptEnginePrivate *eng = QScript::scriptEngineFromExec(frame); + QScript::APIShim shim(eng); + if (frame->callee() == eng->originalGlobalObject()) { + // This is a pushContext()-created context; the callee is a lie. + Q_ASSERT(QScriptEnginePrivate::contextFlags(const_cast(frame)) & QScriptEnginePrivate::NativeContext); + return QScriptValue(); + } + return eng->scriptValueFromJSCValue(frame->callee()); } /*! diff --git a/src/script/api/qscriptengine.cpp b/src/script/api/qscriptengine.cpp index d3e5f2f..9e880b6 100644 --- a/src/script/api/qscriptengine.cpp +++ b/src/script/api/qscriptengine.cpp @@ -2727,6 +2727,14 @@ JSC::CallFrame *QScriptEnginePrivate::pushContext(JSC::CallFrame *exec, JSC::JSV bool clearScopeChain) { JSC::JSValue thisObject = _thisObject; + if (!callee) { + // callee can't be zero, as this can cause JSC to crash during GC + // marking phase if the context's Arguments object has been created. + // Fake it by using the global object. Note that this is also handled + // in QScriptContext::callee(), as that function should still return + // an invalid value. + callee = originalGlobalObject(); + } if (calledAsConstructor) { //JSC doesn't create default created object for native functions. so we do it JSC::JSValue prototype = callee->get(exec, exec->propertyNames().prototype); diff --git a/tests/auto/qscriptengine/tst_qscriptengine.cpp b/tests/auto/qscriptengine/tst_qscriptengine.cpp index 8de6fbc..6c89bcb 100644 --- a/tests/auto/qscriptengine/tst_qscriptengine.cpp +++ b/tests/auto/qscriptengine/tst_qscriptengine.cpp @@ -169,6 +169,7 @@ private slots: void nativeFunctionScopes(); void evaluateProgram(); void collectGarbageAfterConnect(); + void collectGarbageAfterNativeArguments(); void promoteThisObjectToQObjectInConstructor(); void qRegExpInport_data(); @@ -5040,6 +5041,16 @@ void tst_QScriptEngine::collectGarbageAfterConnect() QVERIFY(widget == 0); } +void tst_QScriptEngine::collectGarbageAfterNativeArguments() +{ + // QTBUG-17788 + QScriptEngine eng; + QScriptContext *ctx = eng.pushContext(); + QScriptValue arguments = ctx->argumentsObject(); + // Shouldn't crash when marking the arguments object. + collectGarbage_helper(eng); +} + static QScriptValue constructQObjectFromThisObject(QScriptContext *ctx, QScriptEngine *eng) { Q_ASSERT(ctx->isCalledAsConstructor()); -- cgit v0.12 From 2af7bc6259e41415817cadb456909f33249225ed Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Fri, 25 Feb 2011 15:33:47 +0100 Subject: Add missing API shims to QScriptValue constructors This is needed to ensure that the calls into JSC are safe, e.g. with regards to reentrancy. I was not able to construct a testcase, but several of our autotests crash without this change if COLLECT_ON_EVERY_ALLOCATION is set to 1 in Collector.cpp (this forces a garbage collection every time an object is allocated). Task-number: QTBUG-17815 Reviewed-by: Olivier Goffart --- src/script/api/qscriptvalue.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/script/api/qscriptvalue.cpp b/src/script/api/qscriptvalue.cpp index ac57918..e289636 100644 --- a/src/script/api/qscriptvalue.cpp +++ b/src/script/api/qscriptvalue.cpp @@ -254,6 +254,7 @@ QScriptValue::QScriptValue(QScriptEngine *engine, int val) : d_ptr(new (QScriptEnginePrivate::get(engine))QScriptValuePrivate(QScriptEnginePrivate::get(engine))) { if (engine) { + QScript::APIShim shim(d_ptr->engine); JSC::ExecState *exec = d_ptr->engine->currentFrame; d_ptr->initFrom(JSC::jsNumber(exec, val)); } else @@ -271,6 +272,7 @@ QScriptValue::QScriptValue(QScriptEngine *engine, uint val) : d_ptr(new (QScriptEnginePrivate::get(engine))QScriptValuePrivate(QScriptEnginePrivate::get(engine))) { if (engine) { + QScript::APIShim shim(d_ptr->engine); JSC::ExecState *exec = d_ptr->engine->currentFrame; d_ptr->initFrom(JSC::jsNumber(exec, val)); } else @@ -288,6 +290,7 @@ QScriptValue::QScriptValue(QScriptEngine *engine, qsreal val) : d_ptr(new (QScriptEnginePrivate::get(engine))QScriptValuePrivate(QScriptEnginePrivate::get(engine))) { if (engine) { + QScript::APIShim shim(d_ptr->engine); JSC::ExecState *exec = d_ptr->engine->currentFrame; d_ptr->initFrom(JSC::jsNumber(exec, val)); } else @@ -305,6 +308,7 @@ QScriptValue::QScriptValue(QScriptEngine *engine, const QString &val) : d_ptr(new (QScriptEnginePrivate::get(engine))QScriptValuePrivate(QScriptEnginePrivate::get(engine))) { if (engine) { + QScript::APIShim shim(d_ptr->engine); JSC::ExecState *exec = d_ptr->engine->currentFrame; d_ptr->initFrom(JSC::jsString(exec, val)); } else { @@ -325,6 +329,7 @@ QScriptValue::QScriptValue(QScriptEngine *engine, const char *val) : d_ptr(new (QScriptEnginePrivate::get(engine))QScriptValuePrivate(QScriptEnginePrivate::get(engine))) { if (engine) { + QScript::APIShim shim(d_ptr->engine); JSC::ExecState *exec = d_ptr->engine->currentFrame; d_ptr->initFrom(JSC::jsString(exec, val)); } else { -- cgit v0.12 From aa1e47a5a1a0978979e98f503cb44c85fc88dece Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Fri, 25 Feb 2011 16:45:34 +0100 Subject: Make QtScript support COLLECT_ON_EVERY_ALLOCATION define JSC has a define in runtime/Collector.cpp that can be enabled to force garbage collection on every allocation. This can be useful when investigating GC-related issues. When the define is enabled, GC callbacks will happen before the QScriptEngine(Private) is completely initialized, so we need to initialize some things earlier (nice cleanup, actually), and add some guards in the marking callback. All tests pass with define off and on. Task-number: QTBUG-17781 Reviewed-by: Olivier Goffart --- src/script/api/qscriptengine.cpp | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/script/api/qscriptengine.cpp b/src/script/api/qscriptengine.cpp index 9e880b6..160058e 100644 --- a/src/script/api/qscriptengine.cpp +++ b/src/script/api/qscriptengine.cpp @@ -955,8 +955,11 @@ static QScriptValue __setupPackage__(QScriptContext *ctx, QScriptEngine *eng) } // namespace QScript QScriptEnginePrivate::QScriptEnginePrivate() - : registeredScriptValues(0), freeScriptValues(0), freeScriptValuesCount(0), - registeredScriptStrings(0), inEval(false) + : originalGlobalObjectProxy(0), currentFrame(0), + qobjectPrototype(0), qmetaobjectPrototype(0), variantPrototype(0), + activeAgent(0), agentLineNumber(-1), + registeredScriptValues(0), freeScriptValues(0), freeScriptValuesCount(0), + registeredScriptStrings(0), processEventsInterval(-1), inEval(false) { qMetaTypeId(); qMetaTypeId >(); @@ -1002,10 +1005,6 @@ QScriptEnginePrivate::QScriptEnginePrivate() currentFrame = exec; - originalGlobalObjectProxy = 0; - activeAgent = 0; - agentLineNumber = -1; - processEventsInterval = -1; cachedTranslationUrl = JSC::UString(); cachedTranslationContext = JSC::UString(); JSC::setCurrentIdentifierTable(oldTable); @@ -1253,10 +1252,12 @@ void QScriptEnginePrivate::mark(JSC::MarkStack& markStack) { Q_Q(QScriptEngine); - markStack.append(originalGlobalObject()); - markStack.append(globalObject()); - if (originalGlobalObjectProxy) - markStack.append(originalGlobalObjectProxy); + if (originalGlobalObject()) { + markStack.append(originalGlobalObject()); + markStack.append(globalObject()); + if (originalGlobalObjectProxy) + markStack.append(originalGlobalObjectProxy); + } if (qobjectPrototype) markStack.append(qobjectPrototype); @@ -1281,7 +1282,7 @@ void QScriptEnginePrivate::mark(JSC::MarkStack& markStack) } } - { + if (q) { QScriptContext *context = q->currentContext(); while (context) { -- cgit v0.12