Bug 1657066 - Dynamic module import doesn't handle uncatchable exceptions r=jandem

Previously this used |cx->isExceptionPending()| to determine whether the import had succeeded, which doesn't work if there was an uncatchable exception.  The patch changes this to pass an explicit status.

Differential Revision: https://phabricator.services.mozilla.com/D85856

dom/script/ScriptLoader.cpp

@@ -1061,7 +1061,13 @@ void ScriptLoader::FinishDynamicImport(JSContext* aCx,
   // Complete the dynamic import, report failures indicated by aResult or as a
   // pending exception on the context.
 
-  if (NS_FAILED(aResult)) {
+  JS::DynamicImportStatus status =
+      (NS_FAILED(aResult) || JS_IsExceptionPending(aCx))
+          ? JS::DynamicImportStatus::Failed
+          : JS::DynamicImportStatus::Ok;
+
+  if (NS_FAILED(aResult) &&
+      aResult != NS_SUCCESS_DOM_SCRIPT_EVALUATION_THREW_UNCATCHABLE) {
     MOZ_ASSERT(!JS_IsExceptionPending(aCx));
     JS_ReportErrorNumberUC(aCx, js::GetErrorMessage, nullptr,
                            JSMSG_DYNAMIC_IMPORT_FAILED);
@@ -1072,7 +1078,8 @@ void ScriptLoader::FinishDynamicImport(JSContext* aCx,
   JS::Rooted<JSString*> specifier(aCx, aRequest->mDynamicSpecifier);
   JS::Rooted<JSObject*> promise(aCx, aRequest->mDynamicPromise);
 
-  JS::FinishDynamicModuleImport(aCx, referencingScript, specifier, promise);
+  JS::FinishDynamicModuleImport(aCx, status, referencingScript, specifier,
+                                promise);
 
   // FinishDynamicModuleImport clears any pending exception.
   MOZ_ASSERT(!JS_IsExceptionPending(aCx));

js/public/Modules.h

@@ -81,8 +81,20 @@ GetModuleDynamicImportHook(JSRuntime* rt);
 extern JS_PUBLIC_API void SetModuleDynamicImportHook(
     JSRuntime* rt, ModuleDynamicImportHook func);
 
+/**
+ * Passed to FinishDynamicModuleImport to indicate the result of the dynamic
+ * import operation.
+ */
+enum class DynamicImportStatus { Failed = 0, Ok };
+
+/**
+ * This must be called after a dynamic import operation is complete.
+ *
+ * If |status| is Failed, any pending exception on the context will be used to
+ * complete the user's promise.
+ */
 extern JS_PUBLIC_API bool FinishDynamicModuleImport(
-    JSContext* cx, Handle<Value> referencingPrivate,
+    JSContext* cx, DynamicImportStatus status, Handle<Value> referencingPrivate,
     Handle<JSString*> specifier, Handle<JSObject*> promise);
 
 /**

js/src/builtin/ModuleObject.cpp

@@ -1834,15 +1834,19 @@ JSObject* js::StartDynamicModuleImport(JSContext* cx, HandleScript script,
 }
 
 bool js::FinishDynamicModuleImport(JSContext* cx,
+                                   JS::DynamicImportStatus status,
                                    HandleValue referencingPrivate,
                                    HandleString specifier,
                                    HandleObject promiseArg) {
+  MOZ_ASSERT_IF(cx->isExceptionPending(),
+                status == JS::DynamicImportStatus::Failed);
+
   Handle<PromiseObject*> promise = promiseArg.as<PromiseObject>();
 
   auto releasePrivate = mozilla::MakeScopeExit(
       [&] { cx->runtime()->releaseScriptPrivate(referencingPrivate); });
 
-  if (cx->isExceptionPending()) {
+  if (status == JS::DynamicImportStatus::Failed) {
     return RejectPromiseWithPendingError(cx, promise);
   }
 

js/src/builtin/ModuleObject.h

@@ -346,7 +346,8 @@ JSObject* CallModuleResolveHook(JSContext* cx, HandleValue referencingPrivate,
 JSObject* StartDynamicModuleImport(JSContext* cx, HandleScript script,
                                    HandleValue specifier);
 
-bool FinishDynamicModuleImport(JSContext* cx, HandleValue referencingPrivate,
+bool FinishDynamicModuleImport(JSContext* cx, JS::DynamicImportStatus status,
+                               HandleValue referencingPrivate,
                                HandleString specifier, HandleObject promise);
 
 template <XDRMode mode>

js/src/jit-test/tests/modules/bug-1657066.js

@@ -0,0 +1,3 @@
+let g = newGlobal({newCompartment: true});
+new Debugger(g).onExceptionUnwind = () => null;
+g.eval(`import("javascript: throw 1")`).catch(() => 0);

js/src/shell/ModuleLoader.cpp

@@ -211,11 +211,11 @@ bool ModuleLoader::doDynamicImport(JSContext* cx,
                                    JS::HandleObject promise) {
   // Exceptions during dynamic import are handled by calling
   // FinishDynamicModuleImport with a pending exception on the context.
-  mozilla::DebugOnly<bool> ok =
-      tryDynamicImport(cx, referencingPrivate, specifier, promise);
-  MOZ_ASSERT_IF(!ok, JS_IsExceptionPending(cx));
-  return JS::FinishDynamicModuleImport(cx, referencingPrivate, specifier,
-                                       promise);
+  bool ok = tryDynamicImport(cx, referencingPrivate, specifier, promise);
+  JS::DynamicImportStatus status =
+      ok ? JS::DynamicImportStatus::Ok : JS::DynamicImportStatus::Failed;
+  return JS::FinishDynamicModuleImport(cx, status, referencingPrivate,
+                                       specifier, promise);
 }
 
 bool ModuleLoader::tryDynamicImport(JSContext* cx,

js/src/vm/Modules.cpp

@@ -73,14 +73,15 @@ JS_PUBLIC_API void JS::SetModuleDynamicImportHook(
 }
 
 JS_PUBLIC_API bool JS::FinishDynamicModuleImport(
-    JSContext* cx, Handle<Value> referencingPrivate,
-    Handle<JSString*> specifier, Handle<JSObject*> promise) {
+    JSContext* cx, JS::DynamicImportStatus status,
+    Handle<Value> referencingPrivate, Handle<JSString*> specifier,
+    Handle<JSObject*> promise) {
   AssertHeapIsIdle();
   CHECK_THREAD(cx);
   cx->check(referencingPrivate, promise);
 
-  return js::FinishDynamicModuleImport(cx, referencingPrivate, specifier,
-                                       promise);
+  return js::FinishDynamicModuleImport(cx, status, referencingPrivate,
+                                       specifier, promise);
 }
 
 template <typename Unit>