repo: Ignore ImportError for overrideswip/ptomato/730101

Previously, lookup_override_function() would unconditionally clear any
exceptions, meaning that if an overrides file threw an exception or had a
syntax error, it would be silently swallowed and the overrides file would
be ignored.

Now that we can distinguish between module imports that are not found and
module imports with errors, we can clear the exception only in the case
where no overrides file was found.

The trick used for testing this probably bears some explanation; we want
to provide some mock overrides files which cause various errors. We can't
set the search path as normal, because the overrides importer (exposed to
JS as "imports.overrides") already exists by the time we start our tests.
So we set our search path to the mock overrides files directly on that
object.

https://bugzilla.gnome.org/show_bug.cgi?id=730101

7 files changed, 109 insertions, 15 deletions

diff --git a/gi/repo.cpp b/gi/repo.cpp
index 00c6e645..53fd2a9f 100644
--- a/gi/repo.cpp
+++ b/gi/repo.cpp
@@ -54,7 +54,8 @@ extern struct JSClass gjs_repo_class;
 
 GJS_DEFINE_PRIV_FROM_JS(Repo, gjs_repo_class)
 
-static JSObject *lookup_override_function(JSContext *, JS::HandleId);
+static bool lookup_override_function(JSContext *, JS::HandleId,
+                                     JS::MutableHandleValue);
 
 static bool
 get_version_for_ns (JSContext       *context,
@@ -124,10 +125,12 @@ resolve_namespace_object(JSContext       *context,
                            GJS_MODULE_PROP_FLAGS))
         g_error("no memory to define ns property");
 
-    JS::RootedValue override(context,
-        JS::ObjectOrNullValue(lookup_override_function(context, ns_id)));
+    JS::RootedValue override(context);
+    if (!lookup_override_function(context, ns_id, &override))
+        return false;
+
     JS::RootedValue result(context);
-    if (!override.isNull() &&
+    if (!override.isUndefined() &&
         !JS_CallFunctionValue (context, gi_namespace, /* thisp */
                                override, /* callee */
                                JS::HandleValueArray::empty(), &result))
@@ -562,16 +565,48 @@ gjs_lookup_namespace_object(JSContext  *context,
     return gjs_lookup_namespace_object_by_name(context, ns_name);
 }
 
-static JSObject*
-lookup_override_function(JSContext   *cx,
-                         JS::HandleId ns_name)
+/* Check if an exception's 'name' property is equal to compare_name. Ignores
+ * all errors that might arise. Requires request. */
+static bool
+error_has_name(JSContext       *cx,
+               JS::HandleValue  thrown_value,
+               JSString        *compare_name)
+{
+    if (!thrown_value.isObject())
+        return false;
+
+    JS::AutoSaveExceptionState saved_exc(cx);
+    JS::RootedId name_id(cx, gjs_context_get_const_string(cx, GJS_STRING_NAME));
+    JS::RootedObject exc(cx, &thrown_value.toObject());
+    JS::RootedValue exc_name(cx);
+    bool retval = false;
+
+    if (!JS_GetPropertyById(cx, exc, name_id, &exc_name))
+        goto out;
+
+    int32_t cmp_result;
+    if (!JS_CompareStrings(cx, exc_name.toString(), compare_name, &cmp_result))
+        goto out;
+
+    if (cmp_result == 0)
+        retval = true;
+
+out:
+    saved_exc.restore();
+    return retval;
+}
+
+static bool
+lookup_override_function(JSContext             *cx,
+                         JS::HandleId           ns_name,
+                         JS::MutableHandleValue function)
 {
     JSAutoRequest ar(cx);
+    JS::AutoSaveExceptionState saved_exc(cx);
 
     JS::RootedValue importer(cx, gjs_get_global_slot(cx, GJS_GLOBAL_SLOT_IMPORTS));
     g_assert(importer.isObject());
 
-    JS::RootedValue function(cx);
     JS::RootedId overrides_name(cx,
         gjs_context_get_const_string(cx, GJS_STRING_GI_OVERRIDES));
     JS::RootedId object_init_name(cx,
@@ -584,19 +619,30 @@ lookup_override_function(JSContext   *cx,
         goto fail;
 
     if (!gjs_object_require_property_value(cx, overridespkg, "GI repository object",
-                                           ns_name, &module))
+                                           ns_name, &module)) {
+        JS::RootedValue exc(cx);
+        JS_GetPendingException(cx, &exc);
+
+        /* If the exception was an ImportError (i.e., module not found) then
+         * we simply didn't have an override, don't throw an exception */
+        if (error_has_name(cx, exc, JS_InternString(cx, "ImportError"))) {
+            saved_exc.restore();
+            return true;
+        }
+
         goto fail;
+    }
 
     if (!gjs_object_require_property(cx, module, "override module",
-                                     object_init_name, &function) ||
-        !function.isObjectOrNull())
+                                     object_init_name, function) ||
+        !function.isObjectOrNull()) {
+        gjs_throw(cx, "Unexpected value for _init in overrides module");
         goto fail;
-
-    return function.toObjectOrNull();
+    }
 
  fail:
-    JS_ClearPendingException(cx);
-    return NULL;
+    saved_exc.drop();
+    return false;
 }
 
 JSObject*
diff --git a/installed-tests/js/jsunit.gresources.xml b/installed-tests/js/jsunit.gresources.xml
index da983d43..3fe08f2f 100644
--- a/installed-tests/js/jsunit.gresources.xml
+++ b/installed-tests/js/jsunit.gresources.xml
@@ -10,6 +10,10 @@
     <file>modules/mutualImport/a.js</file>
     <file>modules/mutualImport/b.js</file>
     <file>modules/subA/subB/__init__.js</file>
+    <file>modules/overrides/GIMarshallingTests.js</file>
+    <file>modules/overrides/Gio.js</file>
+    <file>modules/overrides/Regress.js</file>
+    <file>modules/overrides/WarnLib.js</file>
     <file>modules/subA/subB/baz.js</file>
     <file>modules/subA/subB/foobar.js</file>
   </gresource>
diff --git a/installed-tests/js/modules/overrides/GIMarshallingTests.js b/installed-tests/js/modules/overrides/GIMarshallingTests.js
new file mode 100644
index 00000000..6264d3e2
--- /dev/null
+++ b/installed-tests/js/modules/overrides/GIMarshallingTests.js
@@ -0,0 +1,3 @@
+// Sabotage the import of imports.gi.GIMarshallingTests!
+
+throw '💩';
diff --git a/installed-tests/js/modules/overrides/Gio.js b/installed-tests/js/modules/overrides/Gio.js
new file mode 100644
index 00000000..ba7dc95c
--- /dev/null
+++ b/installed-tests/js/modules/overrides/Gio.js
@@ -0,0 +1,3 @@
+// Sabotage the import of imports.gi.Gio!
+
+var _init = '💩';
diff --git a/installed-tests/js/modules/overrides/Regress.js b/installed-tests/js/modules/overrides/Regress.js
new file mode 100644
index 00000000..9374d64e
--- /dev/null
+++ b/installed-tests/js/modules/overrides/Regress.js
@@ -0,0 +1,5 @@
+// Sabotage the import of imports.gi.Regress!
+
+function _init() {
+    throw '💩';
+}
diff --git a/installed-tests/js/modules/overrides/WarnLib.js b/installed-tests/js/modules/overrides/WarnLib.js
new file mode 100644
index 00000000..e7626aef
--- /dev/null
+++ b/installed-tests/js/modules/overrides/WarnLib.js
@@ -0,0 +1,3 @@
+// Sabotage the import of imports.gi.WarnLib!
+
+k$^s^%$#^*($%jdghdsfjkgd
diff --git a/installed-tests/js/testImporter.js b/installed-tests/js/testImporter.js
index 882b1591..26be7ed6 100644
--- a/installed-tests/js/testImporter.js
+++ b/installed-tests/js/testImporter.js
@@ -3,6 +3,36 @@ describe('GI importer', function () {
         var GLib = imports.gi.GLib;
         expect(GLib.MAJOR_VERSION).toEqual(2);
     });
+
+    describe('on failure', function () {
+        // For these tests, we provide special overrides files to sabotage the
+        // import, at the path resource:///org/gjs/jsunit/modules/overrides.
+        let oldSearchPath;
+        beforeAll(function () {
+            oldSearchPath = imports.overrides.searchPath.slice();
+            imports.overrides.searchPath = ['resource:///org/gjs/jsunit/modules/overrides'];
+        });
+
+        afterAll(function () {
+            imports.overrides.searchPath = oldSearchPath;
+        });
+
+        it("throws an exception when the overrides file can't be imported", function () {
+            expect(() => imports.gi.WarnLib).toThrowError(SyntaxError);
+        });
+
+        it('throws an exception when the overrides import throws one', function () {
+            expect(() => imports.gi.GIMarshallingTests).toThrow('💩');
+        });
+
+        it('throws an exception when the overrides _init throws one', function () {
+            expect(() => imports.gi.Regress).toThrow('💩');
+        });
+
+        it("throws an exception when the overrides _init isn't a function", function () {
+            expect(() => imports.gi.Gio).toThrowError(/_init/);
+        });
+    });
 });
 
 describe('Importer', function () {