[gjs/esm/dynamic-imports: 5/7] SQUASH: fixes

[Philip Chimento <pchimento src gnome org>]

commit f6e44655471fb0b2b56798a731a2fd42e676f978
Author: Philip Chimento <philip chimento gmail com>
Date:   Sun Feb 7 17:46:25 2021 -0800

    SQUASH: fixes

 gjs/module.cpp     | 126 +++++++++++++++++++++++++++++------------------------
 test/gjs-tests.cpp |  32 +++++++++++++-
 2 files changed, 101 insertions(+), 57 deletions(-)
---
diff --git a/gjs/module.cpp b/gjs/module.cpp
index e047ef58..61bc4e9a 100644
--- a/gjs/module.cpp
+++ b/gjs/module.cpp
@@ -523,61 +523,65 @@ JSObject* gjs_module_resolve(JSContext* cx, JS::HandleValue importingModulePriv,
     return &result.toObject();
 }
 
-bool gjs_dynamic_module_get_meta(JSContext* cx, JS::HandleObject meta,
-                                 JS::MutableHandleValue importing_module_priv,
-                                 JS::MutableHandleString specifier,
-                                 JS::MutableHandleObject internal_promise) {
+// Call JS::FinishDynamicModuleImport() with the values stashed in the function.
+// Can fail in JS::FinishDynamicModuleImport(), but will assert if anything
+// fails in fetching the stashed values, since that would be a serious GJS bug.
+GJS_JSAPI_RETURN_CONVENTION
+static bool finish_import(JSContext* cx, const JS::CallArgs& args) {
+    JS::Value callback_priv = js::GetFunctionNativeReserved(&args.callee(), 0);
+    g_assert(callback_priv.isObject() && "Wrong private value");
+    JS::RootedObject callback_data(cx, &callback_priv.toObject());
+
+    JS::RootedValue importing_module_priv(cx);
     JS::RootedValue v_specifier(cx);
     JS::RootedValue v_internal_promise(cx);
-    if (!JS_GetProperty(cx, meta, "priv", importing_module_priv) ||
-        !JS_GetProperty(cx, meta, "promise", &v_internal_promise) ||
-        !JS_GetProperty(cx, meta, "specifier", &v_specifier))
-        return false;
+    bool ok =
+        JS_GetProperty(cx, callback_data, "priv", &importing_module_priv) &&
+        JS_GetProperty(cx, callback_data, "promise", &v_internal_promise) &&
+        JS_GetProperty(cx, callback_data, "specifier", &v_specifier);
+    g_assert(ok && "Wrong properties on private value");
 
-    g_assert(v_specifier.isString());
-    g_assert(v_internal_promise.isObject());
+    g_assert(v_specifier.isString() && "Wrong type for specifier");
+    g_assert(v_internal_promise.isObject() && "Wrong type for promise");
 
-    specifier.set(v_specifier.toString());
-    internal_promise.set(&v_internal_promise.toObject());
+    JS::RootedString specifier(cx, v_specifier.toString());
+    JS::RootedObject internal_promise(cx, &v_internal_promise.toObject());
 
-    return true;
+    args.rval().setUndefined();
+    return JS::FinishDynamicModuleImport(cx, importing_module_priv, specifier,
+                                         internal_promise);
+}
+
+// Failing a JSAPI function may result either in an exception pending on the
+// context, in which case we must call JS::FinishDynamicModuleImport() to reject
+// the internal promise; or in an uncatchable exception such as OOM, in which
+// case we must not call JS::FinishDynamicModuleImport().
+GJS_JSAPI_RETURN_CONVENTION
+static bool fail_import(JSContext* cx, const JS::CallArgs& args) {
+    if (JS_IsExceptionPending(cx))
+        return finish_import(cx, args);
+    return false;
 }
 
-bool gjs_dynamic_module_rejected(JSContext* cx, unsigned argc, JS::Value* vp) {
+GJS_JSAPI_RETURN_CONVENTION
+static bool import_rejected(JSContext* cx, unsigned argc, JS::Value* vp) {
     JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
 
-    JS::Value priv_value = js::GetFunctionNativeReserved(&args.callee(), 0);
-    g_assert(!priv_value.isNull() && "Dynamic module rejection called twice");
-    JS::RootedObject meta(cx, &priv_value.toObject());
-    g_assert(meta && "Dynamic module rejection called twice");
-    js::SetFunctionNativeReserved(&args.callee(), 0, JS::NullValue());
+    gjs_debug(GJS_DEBUG_IMPORTER, "Async import promise rejected");
 
-    JS::RootedValue importing_module_priv(cx);
-    JS::RootedString specifier(cx);
-    JS::RootedObject internal_promise(cx);
-    if (!gjs_dynamic_module_get_meta(cx, meta, &importing_module_priv,
-                                     &specifier, &internal_promise))
-        return false;
+    // Throw the value that the promise is rejected with, so that
+    // FinishDynamicModuleImport will reject the internal_promise with it.
+    JS_SetPendingException(cx, args.get(0),
+                           JS::ExceptionStackBehavior::DoNotCapture);
 
-    return JS::FinishDynamicModuleImport(cx, importing_module_priv, specifier,
-                                         internal_promise);
+    return finish_import(cx, args);
 }
 
-bool gjs_dynamic_module_resolved(JSContext* cx, unsigned argc, JS::Value* vp) {
+GJS_JSAPI_RETURN_CONVENTION
+static bool import_resolved(JSContext* cx, unsigned argc, JS::Value* vp) {
     JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
 
-    JS::Value priv_value = js::GetFunctionNativeReserved(&args.callee(), 0);
-    g_assert(!priv_value.isNull() && "Dynamic module resolution called twice");
-    JS::RootedObject meta(cx, &priv_value.toObject());
-    g_assert(meta && "Dynamic module resolution called twice");
-    js::SetFunctionNativeReserved(&args.callee(), 0, JS::NullValue());
-
-    JS::RootedValue importing_module_priv(cx);
-    JS::RootedString specifier(cx);
-    JS::RootedObject internal_promise(cx);
-    if (!gjs_dynamic_module_get_meta(cx, meta, &importing_module_priv,
-                                     &specifier, &internal_promise))
-        return false;
+    gjs_debug(GJS_DEBUG_IMPORTER, "Async import promise resolved");
 
     JS::RootedObject global(cx, gjs_get_import_global(cx));
     JSAutoRealm ar(cx, global);
@@ -586,11 +590,9 @@ bool gjs_dynamic_module_resolved(JSContext* cx, unsigned argc, JS::Value* vp) {
     JS::RootedObject module(cx, &args[0].toObject());
 
     if (!JS::ModuleInstantiate(cx, module) || !JS::ModuleEvaluate(cx, module))
-        return JS::FinishDynamicModuleImport(cx, importing_module_priv,
-                                             specifier, internal_promise);
+        return fail_import(cx, args);
 
-    return JS::FinishDynamicModuleImport(cx, importing_module_priv, specifier,
-                                         internal_promise);
+    return finish_import(cx, args);
 }
 
 bool gjs_dynamic_module_resolve(JSContext* cx,
@@ -609,15 +611,22 @@ bool gjs_dynamic_module_resolve(JSContext* cx,
     g_assert(v_loader.isObject());
     JS::RootedObject loader(cx, &v_loader.toObject());
 
-    JS::RootedObject meta(cx, JS_NewPlainObject(cx));
-    if (!JS_DefineProperty(cx, meta, "specifier", specifier,
+    JS::RootedObject callback_data(cx, JS_NewPlainObject(cx));
+    if (!callback_data ||
+        !JS_DefineProperty(cx, callback_data, "specifier", specifier,
                            JSPROP_PERMANENT) ||
-        !JS_DefineProperty(cx, meta, "promise", internal_promise,
+        !JS_DefineProperty(cx, callback_data, "promise", internal_promise,
                            JSPROP_PERMANENT) ||
-        !JS_DefineProperty(cx, meta, "priv", importing_module_priv,
+        !JS_DefineProperty(cx, callback_data, "priv", importing_module_priv,
                            JSPROP_PERMANENT))
         return false;
 
+    gjs_debug(GJS_DEBUG_IMPORTER,
+              "Async module resolve hook for module '%s' (relative to %p), "
+              "global %p",
+              gjs_debug_string(specifier).c_str(),
+              &importing_module_priv.toObject(), global.get());
+
     JS::RootedValueArray<2> args(cx);
     args[0].set(importing_module_priv);
     args[1].setString(specifier);
@@ -629,17 +638,22 @@ bool gjs_dynamic_module_resolve(JSContext* cx,
 
     JS::RootedObject resolved(
         cx, JS_GetFunctionObject(js::NewFunctionWithReserved(
-                cx, gjs_dynamic_module_resolved, 2, 0, "import resolved")));
+                cx, import_resolved, 1, 0, "async import resolved")));
+    if (!resolved)
+        return false;
     JS::RootedObject rejected(
         cx, JS_GetFunctionObject(js::NewFunctionWithReserved(
-                cx, gjs_dynamic_module_rejected, 2, 0, "import rejected")));
-    js::SetFunctionNativeReserved(resolved, 0, JS::ObjectValue(*meta));
-    js::SetFunctionNativeReserved(rejected, 0, JS::ObjectValue(*meta));
+                cx, import_rejected, 1, 0, "async import rejected")));
+    if (!rejected)
+        return false;
+    js::SetFunctionNativeReserved(resolved, 0, JS::ObjectValue(*callback_data));
+    js::SetFunctionNativeReserved(rejected, 0, JS::ObjectValue(*callback_data));
 
     JS::RootedObject promise(cx, &result.toObject());
 
-    if (!JS::AddPromiseReactions(cx, promise, resolved, rejected))
-        return false;
-
-    return true;
+    // Calling JS::FinishDynamicModuleImport() at the end of the resolve and
+    // reject handlers will also call the module resolve hook. The module will
+    // already have been resolved, but that is how SpiderMonkey obtains the
+    // module object.
+    return JS::AddPromiseReactions(cx, promise, resolved, rejected);
 }
diff --git a/test/gjs-tests.cpp b/test/gjs-tests.cpp
index 71cc8f34..d8a32102 100644
--- a/test/gjs-tests.cpp
+++ b/test/gjs-tests.cpp
@@ -119,7 +119,7 @@ static void gjstest_test_func_gjs_context_eval_dynamic_import() {
 
     bool ok = gjs_context_eval(gjs, R"js(
         import('system')
-            .catch(err => logError(err))
+            .catch(logError)
             .finally(() => imports.mainloop.quit());
         imports.mainloop.run();
     )js",
@@ -129,6 +129,34 @@ static void gjstest_test_func_gjs_context_eval_dynamic_import() {
     g_assert_no_error(error);
 }
 
+static void gjstest_test_func_gjs_context_eval_dynamic_import_relative() {
+    GjsAutoUnref<GjsContext> gjs = gjs_context_new();
+    GError* error = NULL;
+    int status;
+
+    bool ok = g_file_set_contents("num.js", "export default 77;", -1, &error);
+
+    g_assert_true(ok);
+    g_assert_no_error(error);
+
+    ok = gjs_context_eval(gjs, R"js(
+        let num;
+        import('./num.js')
+            .then(module => (num = module.default))
+            .catch(logError)
+            .finally(() => imports.mainloop.quit());
+        imports.mainloop.run();
+        num;
+    )js",
+                          -1, "<main>", &status, &error);
+
+    g_assert_true(ok);
+    g_assert_no_error(error);
+    g_assert_cmpint(status, ==, 77);
+
+    g_unlink("num.js");
+}
+
 static void gjstest_test_func_gjs_context_eval_dynamic_import_bad() {
     GjsAutoUnref<GjsContext> gjs = gjs_context_new();
     GError* error = NULL;
@@ -814,6 +842,8 @@ main(int    argc,
     g_test_add_func("/gjs/context/construct/eval", gjstest_test_func_gjs_context_construct_eval);
     g_test_add_func("/gjs/context/eval/dynamic-import",
                     gjstest_test_func_gjs_context_eval_dynamic_import);
+    g_test_add_func("/gjs/context/eval/dynamic-import/relative",
+                    gjstest_test_func_gjs_context_eval_dynamic_import_relative);
     g_test_add_func("/gjs/context/eval/dynamic-import/bad",
                     gjstest_test_func_gjs_context_eval_dynamic_import_bad);
     g_test_add_func("/gjs/context/eval/non-zero-terminated",