[modules] Implement recent ES revisions.

- Instantiation errors are no longer recorded. If instantiation fails,
  the module(s) are reset to "uninstantiated". When instantiation is
  re-attempted, the thrown exception will be fresh.
- Instantiation can succeed even where there are modules in the graph
  that previously failed evaluation.

Bug: v8:1569
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: I429f616918afe5f8ab1a956024f0a22f464b8c44
Reviewed-on: https://chromium-review.googlesource.com/763369
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50301}

include/v8.h

@@ -1129,15 +1129,15 @@ class V8_EXPORT Location {
 };
 
 /**
- * This is an unfinished experimental feature, and is only exposed
- * here for internal testing purposes. DO NOT USE.
- *
  * A compiled JavaScript module.
  */
 class V8_EXPORT Module {
  public:
   /**
    * The different states a module can be in.
+   * This corresponds to the states used in ECMAScript except that "evaluated"
+   * is split into kEvaluated and kErrored, indicating success and failure,
+   * respectively.
    */
   enum Status {
     kUninstantiated,
@@ -1542,9 +1542,6 @@ class V8_EXPORT ScriptCompiler {
   static uint32_t CachedDataVersionTag();
 
   /**
-   * This is an unfinished experimental feature, and is only exposed
-   * here for internal testing purposes. DO NOT USE.
-   *
    * Compile an ES module, returning a Module that encapsulates
    * the compiled code.
    *
@@ -7142,9 +7139,6 @@ class V8_EXPORT Isolate {
       HostImportModuleDynamicallyCallback callback);
 
   /**
-   * This is an unfinished experimental feature, and is only exposed
-   * here for internal testing purposes. DO NOT USE.
-   *
    * This specifies the callback called by the upcoming importa.meta
    * language feature to retrieve host-defined meta data for a module.
    */

src/objects/module.cc

@@ -199,29 +199,69 @@ void Module::SetStatus(Status new_status) {
   set_status(new_status);
 }
 
+void Module::ResetGraph(Handle<Module> module) {
+  DCHECK_NE(module->status(), kInstantiating);
+  DCHECK_NE(module->status(), kEvaluating);
+  if (module->status() != kPreInstantiating) return;
+  Isolate* isolate = module->GetIsolate();
+  Handle<FixedArray> requested_modules(module->requested_modules(), isolate);
+  Reset(module);
+  for (int i = 0; i < requested_modules->length(); ++i) {
+    Handle<Object> descendant(requested_modules->get(i), isolate);
+    if (descendant->IsModule()) {
+      ResetGraph(Handle<Module>::cast(descendant));
+    } else {
+      DCHECK(descendant->IsUndefined(isolate));
+    }
+  }
+}
+
+void Module::Reset(Handle<Module> module) {
+  Isolate* isolate = module->GetIsolate();
+  Factory* factory = isolate->factory();
+
+  DCHECK(module->status() == kPreInstantiating ||
+         module->status() == kInstantiating);
+  DCHECK(module->exception()->IsTheHole(isolate));
+  DCHECK(module->import_meta()->IsTheHole(isolate));
+  // The namespace object cannot exist, because it would have been created
+  // by RunInitializationCode, which is called only after this module's SCC
+  // succeeds instantiation.
+  DCHECK(!module->module_namespace()->IsJSModuleNamespace());
+
+  Handle<ObjectHashTable> exports =
+      ObjectHashTable::New(isolate, module->info()->RegularExportCount());
+  Handle<FixedArray> regular_exports =
+      factory->NewFixedArray(module->regular_exports()->length());
+  Handle<FixedArray> regular_imports =
+      factory->NewFixedArray(module->regular_imports()->length());
+  Handle<FixedArray> requested_modules =
+      factory->NewFixedArray(module->requested_modules()->length());
+
+  if (module->status() == kInstantiating) {
+    module->set_code(JSFunction::cast(module->code())->shared());
+  }
+#ifdef DEBUG
+  module->PrintStatusTransition(kUninstantiated);
+#endif  // DEBUG
+  module->set_status(kUninstantiated);
+  module->set_exports(*exports);
+  module->set_regular_exports(*regular_exports);
+  module->set_regular_imports(*regular_imports);
+  module->set_requested_modules(*requested_modules);
+  module->set_dfs_index(-1);
+  module->set_dfs_ancestor_index(-1);
+}
+
 void Module::RecordError() {
   DisallowHeapAllocation no_alloc;
-
   Isolate* isolate = GetIsolate();
+
+  DCHECK(exception()->IsTheHole(isolate));
   Object* the_exception = isolate->pending_exception();
   DCHECK(!the_exception->IsTheHole(isolate));
 
-  switch (status()) {
-    case Module::kUninstantiated:
-    case Module::kPreInstantiating:
-    case Module::kInstantiating:
-    case Module::kEvaluating:
-      break;
-    case Module::kErrored:
-      DCHECK_EQ(exception(), the_exception);
-      return;
-    default:
-      UNREACHABLE();
-  }
-
   set_code(info());
-
-  DCHECK(exception()->IsTheHole(isolate));
 #ifdef DEBUG
   PrintStatusTransition(Module::kErrored);
 #endif  // DEBUG
@@ -232,9 +272,8 @@ void Module::RecordError() {
 Object* Module::GetException() {
   DisallowHeapAllocation no_alloc;
   DCHECK_EQ(status(), Module::kErrored);
-  Object* the_exception = exception();
-  DCHECK(!the_exception->IsTheHole(GetIsolate()));
-  return the_exception;
+  DCHECK(!exception()->IsTheHole(GetIsolate()));
+  return exception();
 }
 
 MaybeHandle<Cell> Module::ResolveImport(Handle<Module> module,
@@ -249,14 +288,7 @@ MaybeHandle<Cell> Module::ResolveImport(Handle<Module> module,
       isolate);
   MaybeHandle<Cell> result = Module::ResolveExport(
       requested_module, specifier, name, loc, must_resolve, resolve_set);
-  if (isolate->has_pending_exception()) {
-    DCHECK(result.is_null());
-    if (must_resolve) module->RecordError();
-    // If {must_resolve} is false and there's an exception, then either that
-    // exception was already recorded where it happened, or it's the
-    // kAmbiguousExport exception (see ResolveExportUsingStarExports) and the
-    // culprit module is still to be determined.
-  }
+  DCHECK_IMPLIES(isolate->has_pending_exception(), result.is_null());
   return result;
 }
 
@@ -265,9 +297,8 @@ MaybeHandle<Cell> Module::ResolveExport(Handle<Module> module,
                                         Handle<String> export_name,
                                         MessageLocation loc, bool must_resolve,
                                         Module::ResolveSet* resolve_set) {
-  DCHECK_NE(module->status(), kErrored);
-  DCHECK_NE(module->status(), kEvaluating);
   DCHECK_GE(module->status(), kPreInstantiating);
+  DCHECK_NE(module->status(), kEvaluating);
 
   Isolate* isolate = module->GetIsolate();
   Handle<Object> object(module->exports()->Lookup(export_name), isolate);
@@ -400,27 +431,24 @@ bool Module::Instantiate(Handle<Module> module, v8::Local<v8::Context> context,
   }
 #endif  // DEBUG
 
-  Isolate* isolate = module->GetIsolate();
-  if (module->status() == kErrored) {
-    isolate->Throw(module->GetException());
-    return false;
-  }
-
   if (!PrepareInstantiate(module, context, callback)) {
+    ResetGraph(module);
     return false;
   }
 
+  Isolate* isolate = module->GetIsolate();
   Zone zone(isolate->allocator(), ZONE_NAME);
   ZoneForwardList<Handle<Module>> stack(&zone);
   unsigned dfs_index = 0;
   if (!FinishInstantiate(module, &stack, &dfs_index, &zone)) {
     for (auto& descendant : stack) {
-      descendant->RecordError();
+      Reset(descendant);
     }
-    DCHECK_EQ(module->GetException(), isolate->pending_exception());
+    DCHECK_EQ(module->status(), kUninstantiated);
     return false;
   }
-  DCHECK(module->status() == kInstantiated || module->status() == kEvaluated);
+  DCHECK(module->status() == kInstantiated || module->status() == kEvaluated ||
+         module->status() == kErrored);
   DCHECK(stack.empty());
   return true;
 }
@@ -428,7 +456,6 @@ bool Module::Instantiate(Handle<Module> module, v8::Local<v8::Context> context,
 bool Module::PrepareInstantiate(Handle<Module> module,
                                 v8::Local<v8::Context> context,
                                 v8::Module::ResolveCallback callback) {
-  DCHECK_NE(module->status(), kErrored);
   DCHECK_NE(module->status(), kEvaluating);
   DCHECK_NE(module->status(), kInstantiating);
   if (module->status() >= kPreInstantiating) return true;
@@ -446,17 +473,9 @@ bool Module::PrepareInstantiate(Handle<Module> module,
                   v8::Utils::ToLocal(module))
              .ToLocal(&api_requested_module)) {
       isolate->PromoteScheduledException();
-      module->RecordError();
       return false;
     }
     Handle<Module> requested_module = Utils::OpenHandle(*api_requested_module);
-    if (requested_module->status() == kErrored) {
-      // TODO(neis): Move this into callback?
-      isolate->Throw(requested_module->GetException());
-      module->RecordError();
-      DCHECK_EQ(module->GetException(), requested_module->GetException());
-      return false;
-    }
     requested_modules->set(i, *requested_module);
   }
 
@@ -465,8 +484,6 @@ bool Module::PrepareInstantiate(Handle<Module> module,
     Handle<Module> requested_module(Module::cast(requested_modules->get(i)),
                                     isolate);
     if (!PrepareInstantiate(requested_module, context, callback)) {
-      module->RecordError();
-      DCHECK_EQ(module->GetException(), requested_module->GetException());
       return false;
     }
   }
@@ -538,7 +555,6 @@ void Module::MaybeTransitionComponent(Handle<Module> module,
 bool Module::FinishInstantiate(Handle<Module> module,
                                ZoneForwardList<Handle<Module>>* stack,
                                unsigned* dfs_index, Zone* zone) {
-  DCHECK_NE(module->status(), kErrored);
   DCHECK_NE(module->status(), kEvaluating);
   if (module->status() >= kInstantiating) return true;
   DCHECK_EQ(module->status(), kPreInstantiating);
@@ -567,7 +583,6 @@ bool Module::FinishInstantiate(Handle<Module> module,
       return false;
     }
 
-    DCHECK_NE(requested_module->status(), kErrored);
     DCHECK_NE(requested_module->status(), kEvaluating);
     DCHECK_GE(requested_module->status(), kInstantiating);
     SLOW_DCHECK(
@@ -729,7 +744,6 @@ namespace {
 
 void FetchStarExports(Handle<Module> module, Zone* zone,
                       UnorderedModuleSet* visited) {
-  DCHECK_NE(module->status(), Module::kErrored);
   DCHECK_GE(module->status(), Module::kInstantiating);
 
   if (module->module_namespace()->IsJSModuleNamespace()) return;  // Shortcut.

src/objects/module.h

@@ -182,6 +182,11 @@ class Module : public Struct {
                                        ZoneForwardList<Handle<Module>>* stack,
                                        Status new_status);
 
+  // Set module's status back to kUninstantiated and reset other internal state.
+  // This is used when instantiation fails.
+  static void Reset(Handle<Module> module);
+  static void ResetGraph(Handle<Module> module);
+
   // To set status to kErrored, RecordError should be used.
   void SetStatus(Status status);
   void RecordError();

test/cctest/test-modules.cc

@@ -27,87 +27,166 @@ ScriptOrigin ModuleOrigin(Local<v8::Value> resource_name, Isolate* isolate) {
   return origin;
 }
 
-MaybeLocal<Module> FailAlwaysResolveCallback(Local<Context> context,
-                                             Local<String> specifier,
-                                             Local<Module> referrer) {
-  Isolate* isolate = context->GetIsolate();
-  isolate->ThrowException(v8_str("boom"));
-  return MaybeLocal<Module>();
-}
-
-static int g_count = 0;
-MaybeLocal<Module> FailOnSecondCallResolveCallback(Local<Context> context,
-                                                   Local<String> specifier,
-                                                   Local<Module> referrer) {
+static Local<Module> dep1;
+static Local<Module> dep2;
+MaybeLocal<Module> ResolveCallback(Local<Context> context,
+                                   Local<String> specifier,
+                                   Local<Module> referrer) {
   Isolate* isolate = CcTest::isolate();
-  if (g_count++ > 0) {
-    isolate->ThrowException(v8_str("booom"));
+  if (specifier->StrictEquals(v8_str("./dep1.js"))) {
+    return dep1;
+  } else if (specifier->StrictEquals(v8_str("./dep2.js"))) {
+    return dep2;
+  } else {
+    isolate->ThrowException(v8_str("boom"));
     return MaybeLocal<Module>();
   }
-  Local<String> source_text = v8_str("");
-  ScriptOrigin origin = ModuleOrigin(v8_str("module.js"), isolate);
-  ScriptCompiler::Source source(source_text, origin);
-  return ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
 }
 
-TEST(ModuleInstantiationFailures) {
+TEST(ModuleInstantiationFailures1) {
   Isolate* isolate = CcTest::isolate();
   HandleScope scope(isolate);
   LocalContext env;
   v8::TryCatch try_catch(isolate);
 
-  Local<String> source_text = v8_str(
-      "import './foo.js';\n"
-      "export {} from './bar.js';");
-  ScriptOrigin origin = ModuleOrigin(v8_str("file.js"), CcTest::isolate());
-  ScriptCompiler::Source source(source_text, origin);
-  Local<Module> module =
-      ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
-  CHECK_EQ(Module::kUninstantiated, module->GetStatus());
-  CHECK_EQ(2, module->GetModuleRequestsLength());
-  CHECK(v8_str("./foo.js")->StrictEquals(module->GetModuleRequest(0)));
-  v8::Location loc = module->GetModuleRequestLocation(0);
-  CHECK_EQ(0, loc.GetLineNumber());
-  CHECK_EQ(7, loc.GetColumnNumber());
-
-  CHECK(v8_str("./bar.js")->StrictEquals(module->GetModuleRequest(1)));
-  loc = module->GetModuleRequestLocation(1);
-  CHECK_EQ(1, loc.GetLineNumber());
-  CHECK_EQ(15, loc.GetColumnNumber());
+  Local<Module> module;
+  {
+    Local<String> source_text = v8_str(
+        "import './foo.js';\n"
+        "export {} from './bar.js';");
+    ScriptOrigin origin = ModuleOrigin(v8_str("file.js"), CcTest::isolate());
+    ScriptCompiler::Source source(source_text, origin);
+    module = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
+    CHECK_EQ(Module::kUninstantiated, module->GetStatus());
+    CHECK_EQ(2, module->GetModuleRequestsLength());
+    CHECK(v8_str("./foo.js")->StrictEquals(module->GetModuleRequest(0)));
+    v8::Location loc = module->GetModuleRequestLocation(0);
+    CHECK_EQ(0, loc.GetLineNumber());
+    CHECK_EQ(7, loc.GetColumnNumber());
+    CHECK(v8_str("./bar.js")->StrictEquals(module->GetModuleRequest(1)));
+    loc = module->GetModuleRequestLocation(1);
+    CHECK_EQ(1, loc.GetLineNumber());
+    CHECK_EQ(15, loc.GetColumnNumber());
+  }
 
   // Instantiation should fail.
   {
     v8::TryCatch inner_try_catch(isolate);
-    CHECK(module->InstantiateModule(env.local(), FailAlwaysResolveCallback)
-              .IsNothing());
+    CHECK(module->InstantiateModule(env.local(), ResolveCallback).IsNothing());
     CHECK(inner_try_catch.HasCaught());
     CHECK(inner_try_catch.Exception()->StrictEquals(v8_str("boom")));
-    CHECK_EQ(Module::kErrored, module->GetStatus());
-    Local<Value> exception = module->GetException();
-    CHECK(exception->StrictEquals(v8_str("boom")));
-    // TODO(neis): Check object identity.
+    CHECK_EQ(Module::kUninstantiated, module->GetStatus());
   }
 
   // Start over again...
-  module = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
+  {
+    Local<String> source_text = v8_str(
+        "import './dep1.js';\n"
+        "export {} from './bar.js';");
+    ScriptOrigin origin = ModuleOrigin(v8_str("file.js"), CcTest::isolate());
+    ScriptCompiler::Source source(source_text, origin);
+    module = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
+  }
 
-  // Instantiation should fail if a sub-module fails to resolve.
-  g_count = 0;
+  // dep1.js
+  {
+    Local<String> source_text = v8_str("");
+    ScriptOrigin origin = ModuleOrigin(v8_str("dep1.js"), CcTest::isolate());
+    ScriptCompiler::Source source(source_text, origin);
+    dep1 = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
+  }
+
+  // Instantiation should fail because a sub-module fails to resolve.
   {
     v8::TryCatch inner_try_catch(isolate);
-    CHECK(
-        module->InstantiateModule(env.local(), FailOnSecondCallResolveCallback)
-            .IsNothing());
+    CHECK(module->InstantiateModule(env.local(), ResolveCallback).IsNothing());
     CHECK(inner_try_catch.HasCaught());
-    CHECK(inner_try_catch.Exception()->StrictEquals(v8_str("booom")));
-    CHECK_EQ(Module::kErrored, module->GetStatus());
-    Local<Value> exception = module->GetException();
-    CHECK(exception->StrictEquals(v8_str("booom")));
+    CHECK(inner_try_catch.Exception()->StrictEquals(v8_str("boom")));
+    CHECK_EQ(Module::kUninstantiated, module->GetStatus());
   }
 
   CHECK(!try_catch.HasCaught());
 }
 
+TEST(ModuleInstantiationFailures2) {
+  Isolate* isolate = CcTest::isolate();
+  HandleScope scope(isolate);
+  LocalContext env;
+  v8::TryCatch try_catch(isolate);
+
+  // root1.js
+  Local<Module> root;
+  {
+    Local<String> source_text =
+        v8_str("import './dep1.js'; import './dep2.js'");
+    ScriptOrigin origin = ModuleOrigin(v8_str("root1.js"), CcTest::isolate());
+    ScriptCompiler::Source source(source_text, origin);
+    root = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
+  }
+
+  // dep1.js
+  {
+    Local<String> source_text = v8_str("export let x = 42");
+    ScriptOrigin origin = ModuleOrigin(v8_str("dep1.js"), CcTest::isolate());
+    ScriptCompiler::Source source(source_text, origin);
+    dep1 = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
+  }
+
+  // dep2.js
+  {
+    Local<String> source_text = v8_str("import {foo} from './dep3.js'");
+    ScriptOrigin origin = ModuleOrigin(v8_str("dep2.js"), CcTest::isolate());
+    ScriptCompiler::Source source(source_text, origin);
+    dep2 = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
+  }
+
+  {
+    v8::TryCatch inner_try_catch(isolate);
+    CHECK(root->InstantiateModule(env.local(), ResolveCallback).IsNothing());
+    CHECK(inner_try_catch.HasCaught());
+    CHECK(inner_try_catch.Exception()->StrictEquals(v8_str("boom")));
+    CHECK_EQ(Module::kUninstantiated, root->GetStatus());
+    CHECK_EQ(Module::kUninstantiated, dep1->GetStatus());
+    CHECK_EQ(Module::kUninstantiated, dep2->GetStatus());
+  }
+
+  // Change dep2.js
+  {
+    Local<String> source_text = v8_str("import {foo} from './dep2.js'");
+    ScriptOrigin origin = ModuleOrigin(v8_str("dep2.js"), CcTest::isolate());
+    ScriptCompiler::Source source(source_text, origin);
+    dep2 = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
+  }
+
+  {
+    v8::TryCatch inner_try_catch(isolate);
+    CHECK(root->InstantiateModule(env.local(), ResolveCallback).IsNothing());
+    CHECK(inner_try_catch.HasCaught());
+    CHECK(!inner_try_catch.Exception()->StrictEquals(v8_str("boom")));
+    CHECK_EQ(Module::kUninstantiated, root->GetStatus());
+    CHECK_EQ(Module::kInstantiated, dep1->GetStatus());
+    CHECK_EQ(Module::kUninstantiated, dep2->GetStatus());
+  }
+
+  // Change dep2.js again
+  {
+    Local<String> source_text = v8_str("import {foo} from './dep3.js'");
+    ScriptOrigin origin = ModuleOrigin(v8_str("dep2.js"), CcTest::isolate());
+    ScriptCompiler::Source source(source_text, origin);
+    dep2 = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
+  }
+
+  {
+    v8::TryCatch inner_try_catch(isolate);
+    CHECK(root->InstantiateModule(env.local(), ResolveCallback).IsNothing());
+    CHECK(inner_try_catch.HasCaught());
+    CHECK(inner_try_catch.Exception()->StrictEquals(v8_str("boom")));
+    CHECK_EQ(Module::kUninstantiated, root->GetStatus());
+    CHECK_EQ(Module::kInstantiated, dep1->GetStatus());
+    CHECK_EQ(Module::kUninstantiated, dep2->GetStatus());
+  }
+}
+
 static MaybeLocal<Module> CompileSpecifierAsModuleResolveCallback(
     Local<Context> context, Local<String> specifier, Local<Module> referrer) {
   ScriptOrigin origin = ModuleOrigin(v8_str("module.js"), CcTest::isolate());