[modules] Allow resolve-callback to signal failure.

When asked for a module that previously failed to compile or
instantiate, the embedder necessarily has to signal failure.  In this
case, we expect an exception to be scheduled, which we will rethrow.

BUG=v8:1569

Review-Url: https://codereview.chromium.org/2827733002
Cr-Commit-Position: refs/heads/master@{#44729}

include/v8.h

@@ -1095,7 +1095,8 @@ class V8_EXPORT Module {
   /**
    * ModuleDeclarationInstantiation
    *
-   * Returns false if an exception occurred during instantiation.
+   * Returns false if an exception occurred during instantiation. (In the case
+   * where the callback throws an exception, that exception is propagated.)
    */
   V8_WARN_UNUSED_RESULT bool Instantiate(Local<Context> context,
                                          ResolveCallback callback);

src/objects.cc

@@ -20334,15 +20334,10 @@ bool Module::PrepareInstantiate(Handle<Module> module,
   for (int i = 0, length = module_requests->length(); i < length; ++i) {
     Handle<String> specifier(String::cast(module_requests->get(i)), isolate);
     v8::Local<v8::Module> api_requested_module;
-    // TODO(adamk): Revisit these failure cases once d8 knows how to
-    // persist a module_map across multiple top-level module loads, as
-    // the current module is left in a "half-instantiated" state.
     if (!callback(context, v8::Utils::ToLocal(specifier),
                   v8::Utils::ToLocal(module))
              .ToLocal(&api_requested_module)) {
-      // TODO(adamk): Give this a better error message. But this is a
-      // misuse of the API anyway.
-      isolate->ThrowIllegalOperation();
+      isolate->PromoteScheduledException();
       return false;
     }
     Handle<Module> requested_module = Utils::OpenHandle(*api_requested_module);

src/objects.h

@@ -6724,7 +6724,8 @@ class Module : public Struct {
 
   // Implementation of spec operation ModuleDeclarationInstantiation.
   // Returns false if an exception occurred during instantiation, true
-  // otherwise.
+  // otherwise. (In the case where the callback throws an exception, that
+  // exception is propagated.)
   static MUST_USE_RESULT bool Instantiate(Handle<Module> module,
                                           v8::Local<v8::Context> context,
                                           v8::Module::ResolveCallback callback);

test/cctest/test-modules.cc

@@ -27,9 +27,11 @@ ScriptOrigin ModuleOrigin(Local<v8::Value> resource_name, Isolate* isolate) {
   return origin;
 }
 
-MaybeLocal<Module> AlwaysEmptyResolveCallback(Local<Context> context,
-                                              Local<String> specifier,
-                                              Local<Module> referrer) {
+MaybeLocal<Module> FailAlwaysResolveCallback(Local<Context> context,
+                                             Local<String> specifier,
+                                             Local<Module> referrer) {
+  Isolate* isolate = context->GetIsolate();
+  isolate->ThrowException(v8_str("boom"));
   return MaybeLocal<Module>();
 }
 
@@ -37,18 +39,22 @@ static int g_count = 0;
 MaybeLocal<Module> FailOnSecondCallResolveCallback(Local<Context> context,
                                                    Local<String> specifier,
                                                    Local<Module> referrer) {
-  if (g_count++ > 0) return MaybeLocal<Module>();
+  Isolate* isolate = CcTest::isolate();
+  if (g_count++ > 0) {
+    isolate->ThrowException(v8_str("booom"));
+    return MaybeLocal<Module>();
+  }
   Local<String> source_text = v8_str("");
-  ScriptOrigin origin = ModuleOrigin(v8_str("module.js"), CcTest::isolate());
+  ScriptOrigin origin = ModuleOrigin(v8_str("module.js"), isolate);
   ScriptCompiler::Source source(source_text, origin);
-  return ScriptCompiler::CompileModule(CcTest::isolate(), &source)
-      .ToLocalChecked();
+  return ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
 }
 
 TEST(ModuleInstantiationFailures) {
   Isolate* isolate = CcTest::isolate();
   HandleScope scope(isolate);
   LocalContext env;
+  v8::TryCatch try_catch(isolate);
 
   Local<String> source_text = v8_str(
       "import './foo.js';"
@@ -62,14 +68,26 @@ TEST(ModuleInstantiationFailures) {
   CHECK(v8_str("./bar.js")->StrictEquals(module->GetModuleRequest(1)));
 
   // Instantiation should fail.
-  CHECK(!module->Instantiate(env.local(), AlwaysEmptyResolveCallback));
+  {
+    v8::TryCatch inner_try_catch(isolate);
+    CHECK(!module->Instantiate(env.local(), FailAlwaysResolveCallback));
+    CHECK(inner_try_catch.HasCaught());
+    CHECK(inner_try_catch.Exception()->StrictEquals(v8_str("boom")));
+  }
 
   // Start over again...
   module = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
 
   // Instantiation should fail if a sub-module fails to resolve.
   g_count = 0;
-  CHECK(!module->Instantiate(env.local(), FailOnSecondCallResolveCallback));
+  {
+    v8::TryCatch inner_try_catch(isolate);
+    CHECK(!module->Instantiate(env.local(), FailOnSecondCallResolveCallback));
+    CHECK(inner_try_catch.HasCaught());
+    CHECK(inner_try_catch.Exception()->StrictEquals(v8_str("booom")));
+  }
+
+  CHECK(!try_catch.HasCaught());
 }
 
 static MaybeLocal<Module> CompileSpecifierAsModuleResolveCallback(
@@ -84,6 +102,7 @@ TEST(ModuleEvaluation) {
   Isolate* isolate = CcTest::isolate();
   HandleScope scope(isolate);
   LocalContext env;
+  v8::TryCatch try_catch(isolate);
 
   Local<String> source_text = v8_str(
       "import 'Object.expando = 5';"
@@ -96,12 +115,15 @@ TEST(ModuleEvaluation) {
                             CompileSpecifierAsModuleResolveCallback));
   CHECK(!module->Evaluate(env.local()).IsEmpty());
   ExpectInt32("Object.expando", 10);
+
+  CHECK(!try_catch.HasCaught());
 }
 
 TEST(ModuleEvaluationCompletion1) {
   Isolate* isolate = CcTest::isolate();
   HandleScope scope(isolate);
   LocalContext env;
+  v8::TryCatch try_catch(isolate);
 
   const char* sources[] = {
       "",
@@ -133,12 +155,15 @@ TEST(ModuleEvaluationCompletion1) {
                               CompileSpecifierAsModuleResolveCallback));
     CHECK(module->Evaluate(env.local()).ToLocalChecked()->IsUndefined());
   }
+
+  CHECK(!try_catch.HasCaught());
 }
 
 TEST(ModuleEvaluationCompletion2) {
   Isolate* isolate = CcTest::isolate();
   HandleScope scope(isolate);
   LocalContext env;
+  v8::TryCatch try_catch(isolate);
 
   const char* sources[] = {
       "'gaga'; ",
@@ -171,6 +196,8 @@ TEST(ModuleEvaluationCompletion2) {
               .ToLocalChecked()
               ->StrictEquals(v8_str("gaga")));
   }
+
+  CHECK(!try_catch.HasCaught());
 }
 
 }  // anonymous namespace