From 94a654a67296b37a882a829d3c65d4abd12b4a1c Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Mon, 28 Feb 2011 15:03:23 +0100 Subject: Make missing line number info an expected failure The fact that the line number is incorrect was hidden away in a comment in the test data. This commit makes it an explicit failure so that we're reminded of the issue. Reviewed-by: Olivier Goffart --- tests/auto/qscriptcontext/tst_qscriptcontext.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/auto/qscriptcontext/tst_qscriptcontext.cpp b/tests/auto/qscriptcontext/tst_qscriptcontext.cpp index 9d7e896..f4833bf 100644 --- a/tests/auto/qscriptcontext/tst_qscriptcontext.cpp +++ b/tests/auto/qscriptcontext/tst_qscriptcontext.cpp @@ -725,10 +725,7 @@ void tst_QScriptContext::backtrace_data() expected << "('hey') at -1" << "() at 3" - << QString::fromLatin1("foo(arg1 = 'hello', arg2 = 456) at testfile:%0") - // interpreter unfortunately doesn't provide line number for eval() - .arg(qt_script_isJITEnabled() ? 2 : -1); - expected + << "foo(arg1 = 'hello', arg2 = 456) at testfile:2" << "() at testfile:4"; QTest::newRow("eval") << source << expected; @@ -787,10 +784,7 @@ void tst_QScriptContext::backtrace_data() expected << "('hey') at -1" << "() at 3" - << QString::fromLatin1("plop('hello', 456) at testfile:%0") - // interpreter unfortunately doesn't provide line number for eval() - .arg(qt_script_isJITEnabled() ? 3 : -1); - expected + << "plop('hello', 456) at testfile:3" << "() at testfile:5"; QTest::newRow("eval in member") << source << expected; @@ -987,6 +981,10 @@ void tst_QScriptContext::backtrace() QVERIFY(!eng.hasUncaughtException()); QVERIFY(ret.isArray()); QStringList slist = qscriptvalue_cast(ret); + if (!qt_script_isJITEnabled()) { + QEXPECT_FAIL("eval", "QTBUG-17842: Missing line number in backtrace when function calls eval()", Continue); + QEXPECT_FAIL("eval in member", "QTBUG-17842: Missing line number in backtrace when function calls eval()", Continue); + } QCOMPARE(slist, expectedbacktrace); } -- cgit v0.12 From 5c7b7f5fca8c557b14959ca338cb2fa62aea6aa0 Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Mon, 28 Feb 2011 15:53:10 +0100 Subject: Avoid asserting when computing line number for backtrace With JSC asserts enabled (QtScript built without NDEBUG defined), JSC::CodeBlock::getBytecodeIndex() would assert because we sometimes called it with an address that was not inside the range of the block's JIT code. We never caught this bug because it just so happens that even though the assert fails, the function returns a result that causes our autotests to pass. Check that the returnPC is in range and report lineNumber -1 if not; this unifies the behavior of the interpreter and JIT, even though it's not the result we want. Task-number: QTBUG-17741 Reviewed-by: Olivier Goffart --- src/script/api/qscriptcontextinfo.cpp | 10 +++++++++- tests/auto/qscriptcontext/tst_qscriptcontext.cpp | 6 ++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/script/api/qscriptcontextinfo.cpp b/src/script/api/qscriptcontextinfo.cpp index 0f9de1d..182bc4a 100644 --- a/src/script/api/qscriptcontextinfo.cpp +++ b/src/script/api/qscriptcontextinfo.cpp @@ -159,12 +159,20 @@ QScriptContextInfoPrivate::QScriptContextInfoPrivate(const QScriptContext *conte JSC::CodeBlock *codeBlock = frame->codeBlock(); if (returnPC && codeBlock && QScriptEnginePrivate::hasValidCodeBlockRegister(frame)) { #if ENABLE(JIT) - unsigned bytecodeOffset = codeBlock->getBytecodeIndex(frame, JSC::ReturnAddressPtr(returnPC)); + JSC::JITCode code = codeBlock->getJITCode(); + unsigned jitOffset = code.offsetOf(JSC::ReturnAddressPtr(returnPC).value()); + // We can only use the JIT code offset if it's smaller than the JIT size; + // otherwise calling getBytecodeIndex() is meaningless. + if (jitOffset < code.size()) { + unsigned bytecodeOffset = codeBlock->getBytecodeIndex(frame, JSC::ReturnAddressPtr(returnPC)); #else unsigned bytecodeOffset = returnPC - codeBlock->instructions().begin(); #endif bytecodeOffset--; //because returnPC is on the next instruction. We want the current one lineNumber = codeBlock->lineNumberForBytecodeOffset(const_cast(frame), bytecodeOffset); +#if ENABLE(JIT) + } +#endif } } } diff --git a/tests/auto/qscriptcontext/tst_qscriptcontext.cpp b/tests/auto/qscriptcontext/tst_qscriptcontext.cpp index f4833bf..457188c 100644 --- a/tests/auto/qscriptcontext/tst_qscriptcontext.cpp +++ b/tests/auto/qscriptcontext/tst_qscriptcontext.cpp @@ -981,10 +981,8 @@ void tst_QScriptContext::backtrace() QVERIFY(!eng.hasUncaughtException()); QVERIFY(ret.isArray()); QStringList slist = qscriptvalue_cast(ret); - if (!qt_script_isJITEnabled()) { - QEXPECT_FAIL("eval", "QTBUG-17842: Missing line number in backtrace when function calls eval()", Continue); - QEXPECT_FAIL("eval in member", "QTBUG-17842: Missing line number in backtrace when function calls eval()", Continue); - } + QEXPECT_FAIL("eval", "QTBUG-17842: Missing line number in backtrace when function calls eval()", Continue); + QEXPECT_FAIL("eval in member", "QTBUG-17842: Missing line number in backtrace when function calls eval()", Continue); QCOMPARE(slist, expectedbacktrace); } -- cgit v0.12 From 2699e8dae2b160f8d001459a7993b61fe4b1fac3 Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Tue, 1 Mar 2011 09:43:48 +0100 Subject: Don't assert in abortEvaluation() autotest When QtScript is built without NDEBUG defined, the tst_QScriptEngine::abortEvaluation() test would assert. This was due to commit 716e0284c8f569d71e42354fd6fc3b965233e019, which fixed the tst_QScriptEngine::throwErrorFromProcessEvents() autotest for a script containing an infinite while-loop with an empty body. The CHECK_FOR_EXCEPTION_AT_END() that we added should only be done if the timeout checker did not report a timeout; otherwise the JSC state becomes corrupted due to returnToThrowTrampoline() being called twice. This caused an assert later when calculating the line number of the exception. Also add test cases for scripts with try-catch statements. For abortEvaluation(), scripts should not be able to observe (i.e. catch) the interrupted exception, but if an error is thrown using QScriptContext::throwError(), the script should be able to catch it. Task-number: QTBUG-17854 Reviewed-by: Olivier Goffart --- .../javascriptcore/JavaScriptCore/jit/JITStubs.cpp | 13 +++++- tests/auto/qscriptengine/tst_qscriptengine.cpp | 51 ++++++++++++++++++++-- 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/src/3rdparty/javascriptcore/JavaScriptCore/jit/JITStubs.cpp b/src/3rdparty/javascriptcore/JavaScriptCore/jit/JITStubs.cpp index b3c229e..d8027ff 100644 --- a/src/3rdparty/javascriptcore/JavaScriptCore/jit/JITStubs.cpp +++ b/src/3rdparty/javascriptcore/JavaScriptCore/jit/JITStubs.cpp @@ -1168,8 +1168,17 @@ DEFINE_STUB_FUNCTION(int, timeout_check) globalData->exception = createInterruptedExecutionException(globalData); VM_THROW_EXCEPTION_AT_END(); } - - CHECK_FOR_EXCEPTION_AT_END(); +#ifdef QT_BUILD_SCRIPT_LIB + else { + // It's possible that the call to QtScript's implementation of + // TimeoutChecker::didTimeOut() caused an error to be thrown. + // In that case, didTimeOut() should still return false, since + // we don't want the interrupted-exception to override the + // user-thrown error. But we need to check for exception here, + // otherwise JSC would continue normal execution. + CHECK_FOR_EXCEPTION_AT_END(); + } +#endif return timeoutChecker->ticksUntilNextCheck(); } diff --git a/tests/auto/qscriptengine/tst_qscriptengine.cpp b/tests/auto/qscriptengine/tst_qscriptengine.cpp index 2d7feee..aac9601 100644 --- a/tests/auto/qscriptengine/tst_qscriptengine.cpp +++ b/tests/auto/qscriptengine/tst_qscriptengine.cpp @@ -157,6 +157,7 @@ private slots: void reportAdditionalMemoryCost(); void gcWithNestedDataStructure(); void processEventsWhileRunning(); + void throwErrorFromProcessEvents_data(); void throwErrorFromProcessEvents(); void disableProcessEventsInterval(); void stacktrace(); @@ -164,6 +165,7 @@ private slots: void numberParsing(); void automaticSemicolonInsertion(); void abortEvaluation_notEvaluating(); + void abortEvaluation_data(); void abortEvaluation(); void abortEvaluation_tryCatch(); void abortEvaluation_fromNative(); @@ -2961,17 +2963,42 @@ public: QScriptEngine *engine; }; +void tst_QScriptEngine::throwErrorFromProcessEvents_data() +{ + QTest::addColumn("script"); + QTest::addColumn("error"); + + QTest::newRow("while (1)") + << QString::fromLatin1("while (1) { }") + << QString::fromLatin1("Error: Killed"); + QTest::newRow("while (1) i++") + << QString::fromLatin1("i = 0; while (1) { i++; }") + << QString::fromLatin1("Error: Killed"); + // Unlike abortEvaluation(), scripts should be able to catch the + // exception. + QTest::newRow("try catch") + << QString::fromLatin1("try {" + " while (1) { }" + "} catch(e) {" + " throw new Error('Caught');" + "}") + << QString::fromLatin1("Error: Caught"); +} + void tst_QScriptEngine::throwErrorFromProcessEvents() { + QFETCH(QString, script); + QFETCH(QString, error); + QScriptEngine eng; EventReceiver2 receiver(&eng); QCoreApplication::postEvent(&receiver, new QEvent(QEvent::Type(QEvent::User+1))); eng.setProcessEventsInterval(100); - QScriptValue ret = eng.evaluate(QString::fromLatin1("while (1) { }")); + QScriptValue ret = eng.evaluate(script); QVERIFY(ret.isError()); - QCOMPARE(ret.toString(), QString::fromLatin1("Error: Killed")); + QCOMPARE(ret.toString(), error); } void tst_QScriptEngine::disableProcessEventsInterval() @@ -3386,8 +3413,26 @@ void tst_QScriptEngine::abortEvaluation_notEvaluating() } } +void tst_QScriptEngine::abortEvaluation_data() +{ + QTest::addColumn("script"); + + QTest::newRow("while (1)") + << QString::fromLatin1("while (1) { }"); + QTest::newRow("while (1) i++") + << QString::fromLatin1("i = 0; while (1) { i++; }"); + QTest::newRow("try catch") + << QString::fromLatin1("try {" + " while (1) { }" + "} catch(e) {" + " throw new Error('Caught');" + "}"); +} + void tst_QScriptEngine::abortEvaluation() { + QFETCH(QString, script); + QScriptEngine eng; EventReceiver3 receiver(&eng); @@ -3395,7 +3440,7 @@ void tst_QScriptEngine::abortEvaluation() for (int x = 0; x < 4; ++x) { QCoreApplication::postEvent(&receiver, new QEvent(QEvent::Type(QEvent::User+1))); receiver.resultType = EventReceiver3::AbortionResult(x); - QScriptValue ret = eng.evaluate(QString::fromLatin1("while (1) { }")); + QScriptValue ret = eng.evaluate(script); switch (receiver.resultType) { case EventReceiver3::None: QVERIFY(!eng.hasUncaughtException()); -- cgit v0.12 From d276c62812cf7404c45d447b211109f985da74a5 Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Mon, 28 Feb 2011 15:58:07 +0100 Subject: Enable QtScript/JavaScriptCore ASSERTs in debug mode The fact that QtWebKit disables asserts in debug mode doesn't mean QtScript should do the same. The asserts can make us aware of problems in our calls to JSC, and violations of internal assumptions in JSC (e.g. memory alignment). Task-number: QTBUG-17741 Reviewed-by: Olivier Goffart --- src/script/script.pro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/script/script.pro b/src/script/script.pro index 162eb9c..54f7c4a 100644 --- a/src/script/script.pro +++ b/src/script/script.pro @@ -73,7 +73,7 @@ INCLUDEPATH += $$WEBKITDIR/JavaScriptCore/generated # This line copied from WebCore.pro DEFINES += WTF_USE_JAVASCRIPTCORE_BINDINGS=1 WTF_CHANGES=1 -DEFINES += NDEBUG +CONFIG(release, debug|release):DEFINES += NDEBUG solaris-g++:isEqual(QT_ARCH,sparc) { CONFIG -= separate_debug_info -- cgit v0.12 From b991134fe4d90fcd46bab50ba164f2c28b8942db Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Wed, 2 Mar 2011 08:43:54 +0100 Subject: Don't allow non-Object values to be set as prototype It should only be possible to set an object or null as prototype. This is consistent with both JSC and V8. Additionally, it keeps JSC from asserting in debug mode. Task-number: QTBUG-15154 Reviewed-by: Jedrzej Nowacki --- src/script/api/qscriptvalue.cpp | 8 ++++++-- tests/auto/qscriptvalue/tst_qscriptvalue.cpp | 5 +---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/script/api/qscriptvalue.cpp b/src/script/api/qscriptvalue.cpp index e0dc385..4772fa1 100644 --- a/src/script/api/qscriptvalue.cpp +++ b/src/script/api/qscriptvalue.cpp @@ -531,7 +531,12 @@ void QScriptValue::setPrototype(const QScriptValue &prototype) Q_D(QScriptValue); if (!d || !d->isObject()) return; - if (prototype.isValid() && QScriptValuePrivate::getEngine(prototype) + + JSC::JSValue other = d->engine->scriptValueToJSCValue(prototype); + if (!other || !(other.isObject() || other.isNull())) + return; + + if (QScriptValuePrivate::getEngine(prototype) && (QScriptValuePrivate::getEngine(prototype) != d->engine)) { qWarning("QScriptValue::setPrototype() failed: " "cannot set a prototype created in " @@ -539,7 +544,6 @@ void QScriptValue::setPrototype(const QScriptValue &prototype) return; } JSC::JSObject *thisObject = JSC::asObject(d->jscValue); - JSC::JSValue other = d->engine->scriptValueToJSCValue(prototype); // check for cycle JSC::JSValue nextPrototypeValue = other; diff --git a/tests/auto/qscriptvalue/tst_qscriptvalue.cpp b/tests/auto/qscriptvalue/tst_qscriptvalue.cpp index 9014241..6686e2d 100644 --- a/tests/auto/qscriptvalue/tst_qscriptvalue.cpp +++ b/tests/auto/qscriptvalue/tst_qscriptvalue.cpp @@ -2328,8 +2328,7 @@ void tst_QScriptValue::getSetPrototype_invalidPrototype() inv.setPrototype(object); QCOMPARE(inv.prototype().isValid(), false); object.setPrototype(inv); - // FIXME should it be invalid or proto? - QVERIFY(object.prototype().strictlyEquals(inv)); + QVERIFY(object.prototype().strictlyEquals(proto)); } void tst_QScriptValue::getSetPrototype_twoEngines() @@ -2367,8 +2366,6 @@ void tst_QScriptValue::getSetPrototype_notObjectOrNull() QScriptValue object = eng.newObject(); QScriptValue originalProto = object.prototype(); - QEXPECT_FAIL("", "QTBUG-15154: QScriptValue::setPrototype() allows a non-Object value to be set as prototype", Abort); - // bool object.setPrototype(true); QVERIFY(object.prototype().equals(originalProto)); -- cgit v0.12