[ES6 modules] HRIM may return errored module until V8 forgets instant…

…iation errors Today, we are in somewhat stale state where new module tree fetching algorithm [1][2] is partially applied. This CL (temporarily) disables an assert which exists in the final algorithm, but doesn't hold today. Specifically, the assert in HostResolveImportedModule (HRIM [3]) Step 7 currently doesn't hold, as the instantiation may have failed for a module script node, and current V8 implementation records the instantiation error as an error. See the attached test case for an example. [1] whatwg/html#2991 [2] tc39/ecma262#1006 [3] https://html.spec.whatwg.org/multipage/webappapis.html#hostresolveimportedmodule(referencingscriptormodule,-specifier) Test: external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html Bug: 772750, 763597 Change-Id: Ida600598b658c74cdbda5937219f1e62a41f2a16 Reviewed-on: https://chromium-review.googlesource.com/708078 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#507888}
mirror · Oct 11, 2017 · 1009aff · 1009aff
1 parent fb8d9d4
commit 1009aff
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 2 deletions.
diff --git a/...ernal/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html b/...ernal/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<title>Handling of instantiation errors, 8</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<!-- The below module tree should fail to instantiate, since it references undefined identifier. -->
+<script type="module" src="instantiation-error-1.js"></script>
+<script>
+setup({allow_uncaught_exception: true});
+
+promise_test(t => {
+  return new Promise(resolve => {
+    window.addEventListener("error", e => {
+      assert_equals(e.message, "Uncaught SyntaxError: The requested module does not provide an export named 'default'");
+      resolve();
+    }, { once: true });
+  }).then(() => new Promise(resolve => {
+    window.addEventListener("error", e => {
+      assert_equals(e.message, "Uncaught SyntaxError: The requested module does not provide an export named 'default'");
+      resolve();
+    }, { once: true });
+    // Load another module tree w/ previously instantiate-failed tree as its sub-tree.
+    document.head.appendChild(Object.assign(
+        document.createElement('script'),
+        { type: 'module', innerText: 'import "./instantiation-error-1.js"'}));
+  }));
+}, "Instantiate attempt on a tree w/ previously instantiate-failed tree as a sub-tree shouldn't crash.");
+</script>
diff --git a/third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp b/third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp
@@ -73,9 +73,11 @@ ScriptModule ScriptModuleResolverImpl::Resolve(
 
   // Step 6. Assert: resolved module script is a module script (i.e., is not
   // null or "fetching").
-  // Step 7. Assert: resolved module script is not errored.
   DCHECK(module_script);
-  CHECK(!module_script->IsErrored());
+  // Step 7. Assert: resolved module script is not errored.
+  // The below CHECK doesn't hold until V8 forgets about instantiation errors.
+  // TODO(hiroshige,kouhei): Re-introduce the below CHECK once V8 is updated.
+  // CHECK(!module_script->IsErrored());
 
   // Step 8. Return resolved module script's module record.
   return module_script->Record();