From 73994b550f85576844df229c6c7bea44604091e9 Mon Sep 17 00:00:00 2001 From: Christian Kandeler Date: Fri, 3 Mar 2023 10:01:29 +0100 Subject: Prevent dangling JS values As opposed to QtScript, such values cannot be re-used in QuickJS. This was overlooked in 087c22e17721f37490dd2048a567b6a58065d939. I assume this is the culprit for seemingly random crashes and "not a function" messages we have been seeing sometimes when building with Qt Creator. Change-Id: Ia4e7aed0cda97439ac75db5ecfbf08ff096a02b2 Reviewed-by: Ivan Komissarov --- src/lib/corelib/buildgraph/depscanner.cpp | 9 ++------- src/lib/corelib/buildgraph/transformer.cpp | 12 +++--------- src/lib/corelib/language/language.cpp | 16 ++++++++++++++++ src/lib/corelib/language/language.h | 8 +++----- src/lib/corelib/language/scriptengine.cpp | 4 ++++ src/lib/corelib/language/scriptengine.h | 3 +++ 6 files changed, 31 insertions(+), 21 deletions(-) diff --git a/src/lib/corelib/buildgraph/depscanner.cpp b/src/lib/corelib/buildgraph/depscanner.cpp index e4e10f477..01e781a54 100644 --- a/src/lib/corelib/buildgraph/depscanner.cpp +++ b/src/lib/corelib/buildgraph/depscanner.cpp @@ -238,16 +238,11 @@ QStringList UserDependencyScanner::evaluate(Artifact *artifact, const ScopedJsValueList argsMgr(m_engine->context(), args); const TemporaryGlobalObjectSetter gos(m_engine, m_global); - JSValue &function = script.scriptFunction; - if (!JS_IsFunction(m_engine->context(), function)) { - function = m_engine->evaluate(JsValueOwner::ScriptEngine, script.sourceCode()); - if (Q_UNLIKELY(!JS_IsFunction(m_engine->context(), function))) - throw ErrorInfo(Tr::tr("Invalid scan script."), script.location()); - } + const JSValue function = script.getFunction(m_engine, Tr::tr("Invalid scan script.")); const ScopedJsValue result( m_engine->context(), JS_Call(m_engine->context(), function, m_engine->globalObject(), - int(args.size()), args.data())); + int(args.size()), args.data())); m_engine->clearRequestedProperties(); if (JsException ex = m_engine->checkAndClearException(script.location())) { ErrorInfo err = ex.toErrorInfo(); diff --git a/src/lib/corelib/buildgraph/transformer.cpp b/src/lib/corelib/buildgraph/transformer.cpp index ae044cd2c..2346ad5c9 100644 --- a/src/lib/corelib/buildgraph/transformer.cpp +++ b/src/lib/corelib/buildgraph/transformer.cpp @@ -259,18 +259,12 @@ AbstractCommandPtr Transformer::createCommandFromScriptValue( void Transformer::createCommands(ScriptEngine *engine, const PrivateScriptFunction &script, const JSValueList &args) { - if (JS_IsUndefined(script.scriptFunction)) { - script.scriptFunction = engine->evaluate(JsValueOwner::ScriptEngine, script.sourceCode(), - script.location().filePath(), - script.location().line()); - if (Q_UNLIKELY(!JS_IsFunction(engine->context(), script.scriptFunction))) - throw ErrorInfo(Tr::tr("Invalid prepare script."), script.location()); - } JSValueList argv(args.cbegin(), args.cend()); + const JSValue function = script.getFunction(engine, Tr::tr("Invalid prepare script.")); const ScopedJsValue scriptValue( engine->context(), - JS_Call(engine->context(), script.scriptFunction, engine->globalObject(), - int(argv.size()), argv.data())); + JS_Call(engine->context(), function, engine->globalObject(), + int(argv.size()), argv.data())); propertiesRequestedInPrepareScript = engine->propertiesRequestedInScript(); propertiesRequestedFromArtifactInPrepareScript = engine->propertiesRequestedFromArtifact(); importedFilesUsedInPrepareScript = engine->importedFilesUsedInScript(); diff --git a/src/lib/corelib/language/language.cpp b/src/lib/corelib/language/language.cpp index 3c6fcf454..ecfe09d71 100644 --- a/src/lib/corelib/language/language.cpp +++ b/src/lib/corelib/language/language.cpp @@ -976,5 +976,21 @@ bool operator==(const ExportedModule &m1, const ExportedModule &m2) && depMapsEqual(m1.dependencyParameters, m2.dependencyParameters); } +JSValue PrivateScriptFunction::getFunction(ScriptEngine *engine, const QString &errorMessage) const +{ + if (JS_IsUndefined(scriptFunction)) { + ScopedJsValue val(engine->context(), + engine->evaluate(JsValueOwner::Caller, sourceCode(), + location().filePath(), location().line())); + if (Q_UNLIKELY(!JS_IsFunction(engine->context(), val))) + throw ErrorInfo(errorMessage, location()); + scriptFunction = val.release(); + engine->addExternallyCachedValue(&scriptFunction); + } else { + QBS_CHECK(JS_IsFunction(engine->context(), scriptFunction)); + } + return scriptFunction; +} + } // namespace Internal } // namespace qbs diff --git a/src/lib/corelib/language/language.h b/src/lib/corelib/language/language.h index d055aa132..a1115519d 100644 --- a/src/lib/corelib/language/language.h +++ b/src/lib/corelib/language/language.h @@ -70,15 +70,12 @@ #include #include -QT_BEGIN_NAMESPACE -class QScriptEngine; -QT_END_NAMESPACE - namespace qbs { namespace Internal { class BuildGraphLocker; class BuildGraphLoader; class BuildGraphVisitor; +class ScriptEngine; class FileTagger { @@ -335,8 +332,8 @@ class PrivateScriptFunction friend bool operator==(const PrivateScriptFunction &a, const PrivateScriptFunction &b); public: void initialize(const ScriptFunctionPtr &sharedData) { m_sharedData = sharedData; } - mutable JSValue scriptFunction = JS_UNDEFINED; // not stored + JSValue getFunction(ScriptEngine *engine, const QString &errorMessage) const; QString &sourceCode() const { return m_sharedData->sourceCode; } CodeLocation &location() const { return m_sharedData->location; } ResolvedFileContextConstPtr &fileContext() const { return m_sharedData->fileContext; } @@ -349,6 +346,7 @@ public: private: ScriptFunctionPtr m_sharedData; + mutable JSValue scriptFunction = JS_UNDEFINED; // not stored }; bool operator==(const PrivateScriptFunction &a, const PrivateScriptFunction &b); diff --git a/src/lib/corelib/language/scriptengine.cpp b/src/lib/corelib/language/scriptengine.cpp index 6be9de4d4..bd7394b16 100644 --- a/src/lib/corelib/language/scriptengine.cpp +++ b/src/lib/corelib/language/scriptengine.cpp @@ -185,6 +185,10 @@ ScriptEngine::~ScriptEngine() JS_FreeValue(m_context, ext); for (const JSValue &s : qAsConst(m_stringCache)) JS_FreeValue(m_context, s); + for (JSValue * const externalRef : std::as_const(m_externallyCachedValues)) { + JS_FreeValue(m_context, *externalRef); + *externalRef = JS_UNDEFINED; + } setPropertyOnGlobalObject(QLatin1String("console"), JS_UNDEFINED); JS_FreeContext(m_context); JS_FreeRuntime(m_jsRuntime); diff --git a/src/lib/corelib/language/scriptengine.h b/src/lib/corelib/language/scriptengine.h index c0a0948b7..d7798decf 100644 --- a/src/lib/corelib/language/scriptengine.h +++ b/src/lib/corelib/language/scriptengine.h @@ -164,6 +164,8 @@ public: void addImportRequestedInScript(quintptr importValueId); std::vector importedFilesUsedInScript() const; + void addExternallyCachedValue(JSValue *v) { m_externallyCachedValues.push_back(v); } + void setUsesIo() { m_usesIo = true; } void clearUsesIo() { m_usesIo = false; } bool usesIo() const { return m_usesIo; } @@ -381,6 +383,7 @@ private: QHash m_internalExtensions; QHash m_stringCache; QHash m_evalResults; + std::vector m_externallyCachedValues; QHash, JSValue> m_artifactsScriptValues; QVariantMap m_properties; std::recursive_mutex m_artifactsMutex; -- cgit v1.2.3