[gjs/wip/ptomato/730101: 3/3] repo: Ignore ImportError for overrides

[Philip Chimento <pchimento src gnome org>]

commit 61441788f100457eba71a6db1f559e5b2f206a2c
Author: Philip Chimento <philip chimento gmail com>
Date:   Tue Dec 27 23:39:28 2016 -0700

    repo: Ignore ImportError for overrides
    
    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

 gi/repo.cpp                                        |   76 ++++++++++++++++----
 installed-tests/js/jsunit.gresources.xml           |    4 +
 .../js/modules/overrides/GIMarshallingTests.js     |    3 +
 installed-tests/js/modules/overrides/Gio.js        |    3 +
 installed-tests/js/modules/overrides/Regress.js    |    5 ++
 installed-tests/js/modules/overrides/WarnLib.js    |    3 +
 installed-tests/js/testImporter.js                 |   30 ++++++++
 7 files changed, 109 insertions(+), 15 deletions(-)
---
diff --git a/gi/repo.cpp b/gi/repo.cpp
index 00c6e64..53fd2a9 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 da983d4..3fe08f2 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 0000000..6264d3e
--- /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 0000000..ba7dc95
--- /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 0000000..9374d64
--- /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 0000000..e7626ae
--- /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 882b159..26be7ed 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 () {