ScriptEngine: Make import functionality exception safe

Fixes: QBS-1730
Change-Id: I83324b7d859412580213dc4eb9f1f60e0f9063f2
Reviewed-by: Ivan Komissarov <ABBAPOH@gmail.com>

8 files changed, 69 insertions, 29 deletions

diff --git a/src/lib/corelib/language/evaluator.cpp b/src/lib/corelib/language/evaluator.cpp
index 5252f06be..f7c3824f2 100644
--- a/src/lib/corelib/language/evaluator.cpp
+++ b/src/lib/corelib/language/evaluator.cpp
@@ -269,9 +269,9 @@ Evaluator::FileContextScopes Evaluator::fileContextScopes(const FileContextConst
     }
     if (!JS_IsObject(result.importScope)) {
         try {
-            result.importScope = m_scriptEngine->newObject();
-            setupScriptEngineForFile(m_scriptEngine, file, result.importScope,
-                                     ObserveMode::Enabled);
+            ScopedJsValue importScope(m_scriptEngine->context(), m_scriptEngine->newObject());
+            setupScriptEngineForFile(m_scriptEngine, file, importScope, ObserveMode::Enabled);
+            result.importScope = importScope.release();
         } catch (const ErrorInfo &e) {
             result.importScope = throwError(m_scriptEngine->context(), e.toString());
         }
diff --git a/src/lib/corelib/language/scriptengine.cpp b/src/lib/corelib/language/scriptengine.cpp
index bd7394b16..a8e6cb657 100644
--- a/src/lib/corelib/language/scriptengine.cpp
+++ b/src/lib/corelib/language/scriptengine.cpp
@@ -233,22 +233,7 @@ void ScriptEngine::reset()
 void ScriptEngine::import(const FileContextBaseConstPtr &fileCtx, JSValue &targetObject,
                           ObserveMode observeMode)
 {
-    installImportFunctions(targetObject);
-    m_currentDirPathStack.push(FileInfo::path(fileCtx->filePath()));
-    m_extensionSearchPathsStack.push(fileCtx->searchPaths());
-    m_observeMode = observeMode;
-
-    for (const JsImport &jsImport : fileCtx->jsImports())
-        import(jsImport, targetObject);
-    if (m_observeMode == ObserveMode::Enabled) {
-        for (JSValue &sv : m_requireResults)
-            observeImport(sv);
-        m_requireResults.clear();
-    }
-
-    m_currentDirPathStack.pop();
-    m_extensionSearchPathsStack.pop();
-    uninstallImportFunctions();
+    Importer(*this, fileCtx, targetObject, observeMode).run();
 }
 
 void ScriptEngine::import(const JsImport &jsImport, JSValue &targetObject)
@@ -266,9 +251,10 @@ void ScriptEngine::import(const JsImport &jsImport, JSValue &targetObject)
         if (debugJSImports)
             qDebug() << "[ENGINE] " << jsImport.filePaths << " (cache miss)";
 
-        jsImportValue = JS_NewObject(m_context);
+        ScopedJsValue scopedImportValue(m_context, JS_NewObject(m_context));
         for (const QString &filePath : jsImport.filePaths)
-            importFile(filePath, jsImportValue);
+            importFile(filePath, scopedImportValue);
+        jsImportValue = scopedImportValue.release();
         m_jsImportCache.insert(jsImport, jsImportValue);
         std::vector<QString> &filePathsForScriptValue
                 = m_filePathsPerImport[jsObjectId(jsImportValue)];
@@ -444,7 +430,7 @@ void ScriptEngine::setEnvironment(const QProcessEnvironment &env)
     m_environment = env;
 }
 
-void ScriptEngine::importFile(const QString &filePath, JSValue &targetObject)
+void ScriptEngine::importFile(const QString &filePath, JSValue targetObject)
 {
     AccumulatingTimer importTimer(m_elapsedTimeImporting != -1 ? &m_elapsedTimeImporting : nullptr);
     JSValue &evaluationResult = m_jsFileCache[filePath];
@@ -598,9 +584,9 @@ JSValue ScriptEngine::js_require(JSContext *ctx, JSValueConst this_val,
                         engine->m_logger.qbsDebug()
                                 << "[require] importing file " << filePath;
                     }
-                    JSValue obj = engine->newObject();
+                    ScopedJsValue obj(engine->context(), engine->newObject());
                     engine->importFile(filePath, obj);
-                    values << obj;
+                    values << obj.release();
                     filePaths.push_back(filePath);
                 }
             } catch (const ErrorInfo &e) {
@@ -631,8 +617,9 @@ JSValue ScriptEngine::js_require(JSContext *ctx, JSValueConst this_val,
         result = getJsProperty(ctx, func_data[0], scopeName);
         if (JS_IsObject(result))
             return result; // Same JS file imported from same qbs file via different JS files (e.g. codesign.js from DarwinGCC.qbs via gcc.js and darwin.js).
-        result = engine->newObject();
-        engine->importFile(filePath, result);
+        ScopedJsValue scopedResult(engine->context(), engine->newObject());
+        engine->importFile(filePath, scopedResult);
+        result = scopedResult.release();
         setJsProperty(ctx, result, StringConstants::importScopeNamePropertyInternal(), scopeName);
         setJsProperty(ctx, func_data[0], scopeName, result);
         engine->m_requireResults.push_back(result);
@@ -1027,5 +1014,35 @@ void ScriptEngine::takeOwnership(JSValue v)
     ++m_evalResults[v];
 }
 
+ScriptEngine::Importer::Importer(
+            ScriptEngine &engine, const FileContextBaseConstPtr &fileCtx, JSValue &targetObject,
+            ObserveMode observeMode)
+    : m_engine(engine), m_fileCtx(fileCtx), m_targetObject(targetObject)
+{
+    m_engine.installImportFunctions(targetObject);
+    m_engine.m_currentDirPathStack.push(FileInfo::path(fileCtx->filePath()));
+    m_engine.m_extensionSearchPathsStack.push(fileCtx->searchPaths());
+    m_engine.m_observeMode = observeMode;
+}
+
+ScriptEngine::Importer::~Importer()
+{
+    if (m_engine.m_observeMode == ObserveMode::Enabled)
+        m_engine.m_requireResults.clear();
+    m_engine.m_currentDirPathStack.pop();
+    m_engine.m_extensionSearchPathsStack.pop();
+    m_engine.uninstallImportFunctions();
+}
+
+void ScriptEngine::Importer::run()
+{
+    for (const JsImport &jsImport : m_fileCtx->jsImports())
+        m_engine.import(jsImport, m_targetObject);
+    if (m_engine.m_observeMode == ObserveMode::Enabled) {
+        for (JSValue &sv : m_engine.m_requireResults)
+            m_engine.observeImport(sv);
+    }
+}
+
 } // namespace Internal
 } // namespace qbs
diff --git a/src/lib/corelib/language/scriptengine.h b/src/lib/corelib/language/scriptengine.h
index d7798decf..8c97e3079 100644
--- a/src/lib/corelib/language/scriptengine.h
+++ b/src/lib/corelib/language/scriptengine.h
@@ -292,6 +292,19 @@ public:
     void setProperty(const char *k, const QVariant &v) { m_properties.insert(QLatin1String(k), v); }
 
 private:
+    class Importer {
+    public:
+        Importer(ScriptEngine &engine, const FileContextBaseConstPtr &fileCtx,
+                 JSValue &targetObject, ObserveMode observeMode);
+        ~Importer();
+        void run();
+
+    private:
+        ScriptEngine &m_engine;
+        const FileContextBaseConstPtr &m_fileCtx;
+        JSValue &m_targetObject;
+    };
+
     static int interruptor(JSRuntime *rt, void *opaqueEngine);
 
     bool gatherFileResults() const;
@@ -305,7 +318,7 @@ private:
     void uninstallImportFunctions();
     void import(const JsImport &jsImport, JSValue &targetObject);
     void observeImport(JSValue &jsImport);
-    void importFile(const QString &filePath, JSValue &targetObject);
+    void importFile(const QString &filePath, JSValue targetObject);
     static JSValue js_require(JSContext *ctx, JSValueConst this_val,
                               int argc, JSValueConst *argv, int magic, JSValue *func_data);
     JSValue mergeExtensionObjects(const JSValueList &lst);
diff --git a/src/lib/corelib/language/scriptimporter.cpp b/src/lib/corelib/language/scriptimporter.cpp
index 1b012f3c3..fdb0689ad 100644
--- a/src/lib/corelib/language/scriptimporter.cpp
+++ b/src/lib/corelib/language/scriptimporter.cpp
@@ -142,10 +142,11 @@ JSValue ScriptImporter::importSourceCode(const QString &sourceCode, const QStrin
         code = QLatin1String("(function(){\n") + sourceCode + extractor.suffix();
     }
 
-    JSValue result = m_engine->evaluate(JsValueOwner::Caller, code, filePath, 0);
+    ScopedJsValue result(m_engine->context(),
+                         m_engine->evaluate(JsValueOwner::Caller, code, filePath, 0));
     throwOnEvaluationError(m_engine, [&filePath] () { return CodeLocation(filePath, 0); });
     copyProperties(m_engine->context(), result, targetObject);
-    return result;
+    return result.release();
 }
 
 void ScriptImporter::copyProperties(JSContext *ctx, const JSValue &src, JSValue &dst)
diff --git a/tests/auto/language/testdata/erroneous/missing-js-file.qbs b/tests/auto/language/testdata/erroneous/missing-js-file.qbs
new file mode 100644
index 000000000..a3274a35b
--- /dev/null
+++ b/tests/auto/language/testdata/erroneous/missing-js-file.qbs
@@ -0,0 +1,3 @@
+Product {
+    Depends { name: "missing-js-file-module" }
+}
diff --git a/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file-module.qbs b/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file-module.qbs
new file mode 100644
index 000000000..31302b5c2
--- /dev/null
+++ b/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file-module.qbs
@@ -0,0 +1,3 @@
+import "missing-js-file.js" as MissingJsFile
+
+Module { }
diff --git a/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file.js b/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file.js
new file mode 100644
index 000000000..b66048a8f
--- /dev/null
+++ b/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file.js
@@ -0,0 +1 @@
+var userfile = require("javascriptfile.js")
diff --git a/tests/auto/language/tst_language.cpp b/tests/auto/language/tst_language.cpp
index 689b3bd7f..50269a7d7 100644
--- a/tests/auto/language/tst_language.cpp
+++ b/tests/auto/language/tst_language.cpp
@@ -947,6 +947,8 @@ void TestLanguage::erroneousFiles_data()
                "Product.multiplexByQbsProperties.";
     QTest::newRow("invalid-references")
             << "invalid-references.qbs:2:17.*Cannot open '.*nosuchproject.qbs'";
+    QTest::newRow("missing-js-file")
+            << "missing-js-file-module.qbs.*Cannot open '.*javascriptfile.js'";
 }
 
 void TestLanguage::erroneousFiles()