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

[Philip Chimento <pchimento src gnome org>]

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

    SQUASH: fixes

 gjs/module.cpp | 126 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 67 insertions(+), 59 deletions(-)
---
diff --git a/gjs/module.cpp b/gjs/module.cpp
index 03deec25..9f2d5b3d 100644
--- a/gjs/module.cpp
+++ b/gjs/module.cpp
@@ -523,61 +523,59 @@ 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;
-
-    g_assert(v_specifier.isString());
-    g_assert(v_internal_promise.isObject());
+    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");
 
-    specifier.set(v_specifier.toString());
-    internal_promise.set(&v_internal_promise.toObject());
+    g_assert(v_specifier.isString() && "Wrong type for specifier");
+    g_assert(v_internal_promise.isObject() && "Wrong type for promise");
 
-    return true;
-}
-
-bool gjs_dynamic_module_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());
-
-    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;
+    JS::RootedString specifier(cx, v_specifier.toString());
+    JS::RootedObject internal_promise(cx, &v_internal_promise.toObject());
 
+    args.rval().setUndefined();
     return JS::FinishDynamicModuleImport(cx, importing_module_priv, specifier,
                                          internal_promise);
 }
 
-bool gjs_dynamic_module_resolved(JSContext* cx, unsigned argc, JS::Value* vp) {
+// 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;
+}
+
+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 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());
+    // FIXME: What actually happens here? Do we need to throw the value that the
+    // promise is rejected with, so that FinishDynamicModuleImport will reject
+    // the internal_promise with it?
+    return finish_import(cx, args);
+}
 
-    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_JSAPI_RETURN_CONVENTION
+static bool import_resolved(JSContext* cx, unsigned argc, JS::Value* vp) {
+    JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
 
     JS::RootedObject global(cx, gjs_get_import_global(cx));
     JSAutoRealm ar(cx, global);
@@ -586,11 +584,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 +605,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,
+              "Dynamic 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 +632,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, 2, 0, "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, 2, 0, "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);
 }