[ES6 modules] Update #creating-a-module-script to record its compilat…

…ion error This CL updates Blink's impl of #creating-a-module-script to match recent spec change: whatwg/html#2595 . Before this CL, ModuleScript::Create, the Blink's impl of #creating-a-module-script reported the compilation error to console. After this CL, ModuleScript::Create records module script compilation error to ModuleScript::error_, and stops reporting the error to console. Tests: external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-*.html Bug: 727299, 594639 Change-Id: Iad166780aa93db491abe1c5185fb2da07b61b8f0 Reviewed-on: https://chromium-review.googlesource.com/526555 Commit-Queue: Kouhei Ueno <[email protected]> Reviewed-by: Yutaka Hirano <[email protected]> Cr-Commit-Position: refs/heads/master@{#478207}
mirror · Jun 9, 2017 · 505cc74 · 505cc74
1 parent 29997a9
commit 505cc74
Show file tree

Hide file tree

Showing 14 changed files with 117 additions and 72 deletions.
diff --git a/third_party/WebKit/LayoutTests/TestExpectations b/third_party/WebKit/LayoutTests/TestExpectations
@@ -1796,8 +1796,6 @@ crbug.com/694958 virtual/stable/http/tests/navigation/same-and-different-back.ht
 
 # Failing because of module-related implementation/test issues.
 crbug.com/594639 external/wpt/html/semantics/scripting-1/the-script-element/module/errorhandling.html [ Failure ]
-crbug.com/594639 external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-1.html [ Failure ]
-crbug.com/594639 external/wpt/html/semantics/scripting-1/the-script-element/module/compilation-error-2.html [ Failure ]
 crbug.com/594639 external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-1.html [ Failure ]
 crbug.com/594639 external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-2.html [ Failure ]
 crbug.com/594639 external/wpt/html/semantics/scripting-1/the-script-element/module/evaluation-error-3.html [ Failure ]

diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp
@@ -4,6 +4,7 @@
 
 #include "bindings/core/v8/ScriptModule.h"
 
+#include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/V8BindingForCore.h"
 #include "core/dom/Modulator.h"
 #include "core/dom/ScriptModuleResolver.h"
@@ -40,22 +41,19 @@ ScriptModule ScriptModule::Compile(v8::Isolate* isolate,
                                    const String& source,
                                    const String& file_name,
                                    AccessControlStatus access_control_status,
-                                   const TextPosition& start_position) {
+                                   const TextPosition& start_position,
+                                   ExceptionState& exception_state) {
   // We ensure module-related code is not executed without the flag.
   // https://crbug.com/715376
   CHECK(RuntimeEnabledFeatures::ModuleScriptsEnabled());
 
   v8::TryCatch try_catch(isolate);
-  try_catch.SetVerbose(true);
   v8::Local<v8::Module> module;
   if (!V8ScriptRunner::CompileModule(isolate, source, file_name,
                                      access_control_status, start_position)
            .ToLocal(&module)) {
-    // Compilation error is not used in Blink implementaion logic.
-    // Note: Error message is delivered to user (e.g. console) by message
-    // listeners set on v8::Isolate. See V8Initializer::initalizeMainThread().
-    // TODO(nhiroki): Revisit this when supporting modules on worker threads.
     DCHECK(try_catch.HasCaught());
+    exception_state.RethrowV8Exception(try_catch.Exception());
     return ScriptModule();
   }
   DCHECK(!try_catch.HasCaught());

diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptModule.h b/third_party/WebKit/Source/bindings/core/v8/ScriptModule.h
@@ -17,6 +17,8 @@
 
 namespace blink {
 
+class ExceptionState;
+
 // ScriptModule wraps a handle to a v8::Module for use in core.
 //
 // Using ScriptModules needs a ScriptState and its scope to operate in. You
@@ -28,12 +30,12 @@ class CORE_EXPORT ScriptModule final {
   DISALLOW_NEW_EXCEPT_PLACEMENT_NEW();
 
  public:
-  static ScriptModule Compile(
-      v8::Isolate*,
-      const String& source,
-      const String& file_name,
-      AccessControlStatus,
-      const TextPosition& start_position = TextPosition::MinimumPosition());
+  static ScriptModule Compile(v8::Isolate*,
+                              const String& source,
+                              const String& file_name,
+                              AccessControlStatus,
+                              const TextPosition& start_position,
+                              ExceptionState&);
 
   // TODO(kouhei): Remove copy ctor
   ScriptModule();

diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptModuleTest.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptModuleTest.cpp
@@ -78,30 +78,33 @@ DEFINE_TRACE(ScriptModuleTestModulator) {
 
 TEST(ScriptModuleTest, compileSuccess) {
   V8TestingScope scope;
-  ScriptModule module =
-      ScriptModule::Compile(scope.GetIsolate(), "export const a = 42;",
-                            "foo.js", kSharableCrossOrigin);
+  ScriptModule module = ScriptModule::Compile(
+      scope.GetIsolate(), "export const a = 42;", "foo.js",
+      kSharableCrossOrigin, TextPosition::MinimumPosition(),
+      ASSERT_NO_EXCEPTION);
   ASSERT_FALSE(module.IsNull());
 }
 
 TEST(ScriptModuleTest, compileFail) {
   V8TestingScope scope;
-  ScriptModule module = ScriptModule::Compile(scope.GetIsolate(), "123 = 456",
-                                              "foo.js", kSharableCrossOrigin);
+  ScriptModule module = ScriptModule::Compile(
+      scope.GetIsolate(), "123 = 456", "foo.js", kSharableCrossOrigin,
+      TextPosition::MinimumPosition(), scope.GetExceptionState());
   ASSERT_TRUE(module.IsNull());
+  EXPECT_TRUE(scope.GetExceptionState().HadException());
 }
 
 TEST(ScriptModuleTest, equalAndHash) {
   V8TestingScope scope;
 
   ScriptModule module_null;
-  ScriptModule module_a =
-      ScriptModule::Compile(scope.GetIsolate(), "export const a = 'a';", "a.js",
-                            kSharableCrossOrigin);
+  ScriptModule module_a = ScriptModule::Compile(
+      scope.GetIsolate(), "export const a = 'a';", "a.js", kSharableCrossOrigin,
+      TextPosition::MinimumPosition(), ASSERT_NO_EXCEPTION);
   ASSERT_FALSE(module_a.IsNull());
-  ScriptModule module_b =
-      ScriptModule::Compile(scope.GetIsolate(), "export const b = 'b';", "b.js",
-                            kSharableCrossOrigin);
+  ScriptModule module_b = ScriptModule::Compile(
+      scope.GetIsolate(), "export const b = 'b';", "b.js", kSharableCrossOrigin,
+      TextPosition::MinimumPosition(), ASSERT_NO_EXCEPTION);
   ASSERT_FALSE(module_b.IsNull());
   Vector<char> module_deleted_buffer(sizeof(ScriptModule));
   ScriptModule& module_deleted =
@@ -141,7 +144,8 @@ TEST(ScriptModuleTest, moduleRequests) {
   V8TestingScope scope;
   ScriptModule module = ScriptModule::Compile(
       scope.GetIsolate(), "import 'a'; import 'b'; export const c = 'c';",
-      "foo.js", kSharableCrossOrigin);
+      "foo.js", kSharableCrossOrigin, TextPosition::MinimumPosition(),
+      ASSERT_NO_EXCEPTION);
   ASSERT_FALSE(module.IsNull());
 
   auto requests = module.ModuleRequests(scope.GetScriptState());
@@ -156,9 +160,10 @@ TEST(ScriptModuleTest, instantiateNoDeps) {
 
   Modulator::SetModulator(scope.GetScriptState(), modulator);
 
-  ScriptModule module =
-      ScriptModule::Compile(scope.GetIsolate(), "export const a = 42;",
-                            "foo.js", kSharableCrossOrigin);
+  ScriptModule module = ScriptModule::Compile(
+      scope.GetIsolate(), "export const a = 42;", "foo.js",
+      kSharableCrossOrigin, TextPosition::MinimumPosition(),
+      ASSERT_NO_EXCEPTION);
   ASSERT_FALSE(module.IsNull());
   ScriptValue exception = module.Instantiate(scope.GetScriptState());
   ASSERT_TRUE(exception.IsEmpty());
@@ -174,21 +179,24 @@ TEST(ScriptModuleTest, instantiateWithDeps) {
 
   Modulator::SetModulator(scope.GetScriptState(), modulator);
 
-  ScriptModule module_a =
-      ScriptModule::Compile(scope.GetIsolate(), "export const a = 'a';",
-                            "foo.js", kSharableCrossOrigin);
+  ScriptModule module_a = ScriptModule::Compile(
+      scope.GetIsolate(), "export const a = 'a';", "foo.js",
+      kSharableCrossOrigin, TextPosition::MinimumPosition(),
+      ASSERT_NO_EXCEPTION);
   ASSERT_FALSE(module_a.IsNull());
   resolver->PushScriptModule(module_a);
 
-  ScriptModule module_b =
-      ScriptModule::Compile(scope.GetIsolate(), "export const b = 'b';",
-                            "foo.js", kSharableCrossOrigin);
+  ScriptModule module_b = ScriptModule::Compile(
+      scope.GetIsolate(), "export const b = 'b';", "foo.js",
+      kSharableCrossOrigin, TextPosition::MinimumPosition(),
+      ASSERT_NO_EXCEPTION);
   ASSERT_FALSE(module_b.IsNull());
   resolver->PushScriptModule(module_b);
 
   ScriptModule module = ScriptModule::Compile(
       scope.GetIsolate(), "import 'a'; import 'b'; export const c = 123;",
-      "c.js", kSharableCrossOrigin);
+      "c.js", kSharableCrossOrigin, TextPosition::MinimumPosition(),
+      ASSERT_NO_EXCEPTION);
   ASSERT_FALSE(module.IsNull());
   ScriptValue exception = module.Instantiate(scope.GetScriptState());
   ASSERT_TRUE(exception.IsEmpty());
@@ -206,7 +214,8 @@ TEST(ScriptModuleTest, Evaluate) {
 
   ScriptModule module = ScriptModule::Compile(
       scope.GetIsolate(), "export const a = 42; window.foo = 'bar';", "foo.js",
-      kSharableCrossOrigin);
+      kSharableCrossOrigin, TextPosition::MinimumPosition(),
+      ASSERT_NO_EXCEPTION);
   ASSERT_FALSE(module.IsNull());
   ScriptValue exception = module.Instantiate(scope.GetScriptState());
   ASSERT_TRUE(exception.IsEmpty());

diff --git a/third_party/WebKit/Source/core/dom/Modulator.h b/third_party/WebKit/Source/core/dom/Modulator.h
@@ -18,6 +18,7 @@
 
 namespace blink {
 
+class ExceptionState;
 class ModuleScript;
 class ModuleScriptFetchRequest;
 class ModuleScriptLoaderClient;
@@ -107,7 +108,8 @@ class CORE_EXPORT Modulator : public GarbageCollectedFinalized<Modulator>,
   virtual ScriptModule CompileModule(const String& script,
                                      const String& url_str,
                                      AccessControlStatus,
-                                     const TextPosition&) = 0;
+                                     const TextPosition&,
+                                     ExceptionState&) = 0;
 
   virtual ScriptValue InstantiateModule(ScriptModule) = 0;
 

diff --git a/third_party/WebKit/Source/core/dom/ModulatorImpl.cpp b/third_party/WebKit/Source/core/dom/ModulatorImpl.cpp
@@ -121,8 +121,9 @@ ScriptModule ModulatorImpl::CompileModule(
     const String& provided_source,
     const String& url_str,
     AccessControlStatus access_control_status,
-    const TextPosition& position) {
-  // Implements Steps 3-6 of
+    const TextPosition& position,
+    ExceptionState& exception_state) {
+  // Implements Steps 3-5 of
   // https://html.spec.whatwg.org/multipage/webappapis.html#creating-a-module-script
 
   // Step 3. Let realm be the provided environment settings object's Realm.
@@ -136,12 +137,10 @@ ScriptModule ModulatorImpl::CompileModule(
     script_source = provided_source;
 
   // Step 5. Let result be ParseModule(script source, realm, script).
-  // Step 6. If result is a List of errors, report the exception given by the
-  // first element of result for script, return null, and abort these steps.
-  // Note: reporting is routed via V8Initializer::messageHandlerInMainThread.
   ScriptState::Scope scope(script_state_.Get());
   return ScriptModule::Compile(script_state_->GetIsolate(), script_source,
-                               url_str, access_control_status, position);
+                               url_str, access_control_status, position,
+                               exception_state);
 }
 
 ScriptValue ModulatorImpl::InstantiateModule(ScriptModule script_module) {

diff --git a/third_party/WebKit/Source/core/dom/ModulatorImpl.h b/third_party/WebKit/Source/core/dom/ModulatorImpl.h
@@ -63,7 +63,8 @@ class ModulatorImpl final : public Modulator {
   ScriptModule CompileModule(const String& script,
                              const String& url_str,
                              AccessControlStatus,
-                             const TextPosition&) override;
+                             const TextPosition&,
+                             ExceptionState&) override;
   ScriptValue InstantiateModule(ScriptModule) override;
   ScriptValue GetError(const ModuleScript*) override;
   Vector<String> ModuleRequestsFromScriptModule(ScriptModule) override;

diff --git a/third_party/WebKit/Source/core/dom/ModuleScript.cpp b/third_party/WebKit/Source/core/dom/ModuleScript.cpp
@@ -27,15 +27,40 @@ ModuleScript* ModuleScript::Create(
   // provided.
   // Note: "script's settings object" will be "modulator".
 
-  // Delegate to Modulator::compileModule to process Steps 3-6.
+  v8::HandleScope scope(modulator->GetScriptState()->GetIsolate());
+  ExceptionState exception_state(modulator->GetScriptState()->GetIsolate(),
+                                 ExceptionState::kExecutionContext,
+                                 "ModuleScript", "Create");
+
+  // Delegate to Modulator::CompileModule to process Steps 3-5.
   ScriptModule result = modulator->CompileModule(
-      source_text, base_url.GetString(), access_control_status, start_position);
-  // Step 6: "...return null, and abort these steps."
-  if (result.IsNull())
-    return nullptr;
+      source_text, base_url.GetString(), access_control_status, start_position,
+      exception_state);
+
+  // CreateInternal processes Steps 7-13.
+  // [nospec] We initialize the other ModuleScript members anyway by running
+  // Steps 7-13 before Step 6. In a case that compile failed, we will
+  // immediately turn the script into errored state. Thus the members will not
+  // be used for the speced algorithms, but may be used from inspector.
+  ModuleScript* script =
+      CreateInternal(source_text, modulator, result, base_url, nonce,
+                     parser_state, credentials_mode, start_position);
+
+  // Step 6. If result is a List of errors, then:
+  if (exception_state.HadException()) {
+    DCHECK(result.IsNull());
+
+    // Step 6.1. Error script with errors[0].
+    v8::Local<v8::Value> error = exception_state.GetException();
+    exception_state.ClearException();
+    script->SetErrorAndClearRecord(
+        ScriptValue(modulator->GetScriptState(), error));
+
+    // Step 6.2. Return script.
+    return script;
+  }
 
-  return CreateInternal(source_text, modulator, result, base_url, nonce,
-                        parser_state, credentials_mode, start_position);
+  return script;
 }
 
 ModuleScript* ModuleScript::CreateForTest(
@@ -61,13 +86,14 @@ ModuleScript* ModuleScript::CreateInternal(
     WebURLRequest::FetchCredentialsMode credentials_mode,
     const TextPosition& start_position) {
   // https://html.spec.whatwg.org/#creating-a-module-script
-  // Step 7. Set script's module record to result.
-  // Step 8. Set script's base URL to the script base URL provided.
-  // Step 9. Set script's cryptographic nonce to the cryptographic nonce
+  // Step 7. Set script's state to "uninstantiated".
+  // Step 8. Set script's module record to result.
+  // Step 9. Set script's base URL to the script base URL provided.
+  // Step 10. Set script's cryptographic nonce to the cryptographic nonce
   // provided.
-  // Step 10. Set script's parser state to the parser state.
-  // Step 11. Set script's credentials mode to the credentials mode provided.
-  // Step 12. Return script.
+  // Step 11. Set script's parser state to the parser state.
+  // Step 12. Set script's credentials mode to the credentials mode provided.
+  // Step 13. Return script.
   // [not specced] |source_text| is saved for CSP checks.
   ModuleScript* module_script =
       new ModuleScript(modulator, result, base_url, nonce, parser_state,

diff --git a/third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp b/third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp
@@ -12,13 +12,15 @@ namespace blink {
 
 void ScriptModuleResolverImpl::RegisterModuleScript(
     ModuleScript* module_script) {
+  DCHECK(module_script);
+  if (module_script->Record().IsNull())
+    return;
+
   DVLOG(1) << "ScriptModuleResolverImpl::registerModuleScript(url=\""
            << module_script->BaseURL().GetString()
            << "\", hash=" << ScriptModuleHash::GetHash(module_script->Record())
            << ")";
 
-  DCHECK(module_script);
-  DCHECK(!module_script->Record().IsNull());
   auto result =
       record_to_module_script_map_.Set(module_script->Record(), module_script);
   DCHECK(result.is_new_entry);

diff --git a/third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp b/third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp
@@ -74,7 +74,8 @@ ModuleScript* CreateReferrerModuleScript(Modulator* modulator,
                                          V8TestingScope& scope) {
   ScriptModule referrer_record = ScriptModule::Compile(
       scope.GetIsolate(), "import './target.js'; export const a = 42;",
-      "referrer.js", kSharableCrossOrigin);
+      "referrer.js", kSharableCrossOrigin, TextPosition::MinimumPosition(),
+      ASSERT_NO_EXCEPTION);
   KURL referrer_url(kParsedURLString, "https://example.com/referrer.js");
   ModuleScript* referrer_module_script = ModuleScript::CreateForTest(
       modulator, referrer_record, referrer_url, "", kParserInserted,
@@ -87,9 +88,10 @@ ModuleScript* CreateTargetModuleScript(
     Modulator* modulator,
     V8TestingScope& scope,
     ModuleInstantiationState state = ModuleInstantiationState::kInstantiated) {
-  ScriptModule record =
-      ScriptModule::Compile(scope.GetIsolate(), "export const pi = 3.14;",
-                            "target.js", kSharableCrossOrigin);
+  ScriptModule record = ScriptModule::Compile(
+      scope.GetIsolate(), "export const pi = 3.14;", "target.js",
+      kSharableCrossOrigin, TextPosition::MinimumPosition(),
+      ASSERT_NO_EXCEPTION);
   KURL url(kParsedURLString, "https://example.com/target.js");
   ModuleScript* module_script =
       ModuleScript::CreateForTest(modulator, record, url, "", kParserInserted,

diff --git a/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp b/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp
@@ -69,11 +69,13 @@ class ModuleScriptLoaderTestModulator final : public DummyModulator {
   ScriptModule CompileModule(const String& script,
                              const String& url_str,
                              AccessControlStatus access_control_status,
-                             const TextPosition& position) override {
+                             const TextPosition& position,
+                             ExceptionState& exception_state) override {
     ScriptState::Scope scope(script_state_.Get());
-    return ScriptModule::Compile(script_state_->GetIsolate(),
-                                 "export default 'foo';", "",
-                                 access_control_status);
+    return ScriptModule::Compile(
+        script_state_->GetIsolate(), "export default 'foo';", "",
+        access_control_status, TextPosition::MinimumPosition(),
+        exception_state);
   }
 
   DECLARE_TRACE();

diff --git a/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp b/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
@@ -78,7 +78,8 @@ class ModuleTreeLinkerTestModulator final : public DummyModulator {
 
     ScriptModule script_module = ScriptModule::Compile(
         script_state_->GetIsolate(), source_text.ToString(), url.GetString(),
-        kSharableCrossOrigin);
+        kSharableCrossOrigin, TextPosition::MinimumPosition(),
+        ASSERT_NO_EXCEPTION);
     ModuleScript* module_script = ModuleScript::CreateForTest(
         this, script_module, url, "", kParserInserted,
         WebURLRequest::kFetchCredentialsModeOmit);
@@ -131,7 +132,8 @@ class ModuleTreeLinkerTestModulator final : public DummyModulator {
 
     ScriptModule script_module = ScriptModule::Compile(
         script_state_->GetIsolate(), "export default 'pineapples';",
-        url.GetString(), kSharableCrossOrigin);
+        url.GetString(), kSharableCrossOrigin, TextPosition::MinimumPosition(),
+        ASSERT_NO_EXCEPTION);
     ModuleScript* module_script = ModuleScript::CreateForTest(
         this, script_module, url, "", kParserInserted,
         WebURLRequest::kFetchCredentialsModeOmit);

diff --git a/third_party/WebKit/Source/core/testing/DummyModulator.cpp b/third_party/WebKit/Source/core/testing/DummyModulator.cpp
@@ -103,7 +103,8 @@ bool DummyModulator::HasValidContext() {
 ScriptModule DummyModulator::CompileModule(const String& script,
                                            const String& url_str,
                                            AccessControlStatus,
-                                           const TextPosition&) {
+                                           const TextPosition&,
+                                           ExceptionState&) {
   NOTREACHED();
   return ScriptModule();
 }