From 61441788f100457eba71a6db1f559e5b2f206a2c Mon Sep 17 00:00:00 2001 From: Philip Chimento Date: Tue, 27 Dec 2016 23:39:28 -0700 Subject: 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(-) create mode 100644 installed-tests/js/modules/overrides/GIMarshallingTests.js create mode 100644 installed-tests/js/modules/overrides/Gio.js create mode 100644 installed-tests/js/modules/overrides/Regress.js create mode 100644 installed-tests/js/modules/overrides/WarnLib.js 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 @@ modules/mutualImport/a.js modules/mutualImport/b.js modules/subA/subB/__init__.js + modules/overrides/GIMarshallingTests.js + modules/overrides/Gio.js + modules/overrides/Regress.js + modules/overrides/WarnLib.js modules/subA/subB/baz.js modules/subA/subB/foobar.js 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 () { -- cgit v1.2.1