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 <ABBAPOH@gmail.com>

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 <mutex>
 #include <vector>
 
-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<QString> 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<QString, JSValue> m_internalExtensions;
     QHash<QString, JSValue> m_stringCache;
     QHash<JSValue, int> m_evalResults;
+    std::vector<JSValue *> m_externallyCachedValues;
     QHash<QPair<Artifact *, QString>, JSValue> m_artifactsScriptValues;
     QVariantMap m_properties;
     std::recursive_mutex m_artifactsMutex;