[ES6 modules] Update module script error state predicate and transition.

This CL updates the following spec concepts:
"#concept-module-script-is-errored"
- Null record is now also treated as an error
"#concept-module-script-pre-instantiation-error"
- Renamed from module script's error.
"#concept-module-script-module-record"
- Implement "set its [[HostDefined]] field to undefined" step.
"#concept-module-script-set-pre-instantiation-error"
- Don't set script's state to errored.

This change is to be introduced in whatwg html spec PR:
whatwg/html#2674

The actual implementation behavior change will be introduced in separate CLs.

Bug: 594639, 727299, whatwg/html#2674
Change-Id: Ie9734344030f99bcdaa3d595f8dbec87d44a7c92
Reviewed-on: https://chromium-review.googlesource.com/541097
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482213}

Author: nyaxt

third_party/WebKit/Source/bindings/core/v8/ScriptModuleTest.cpp

@@ -35,6 +35,7 @@ class TestScriptModuleResolver final : public ScriptModuleResolver {
   // Implements ScriptModuleResolver:
 
   void RegisterModuleScript(ModuleScript*) override { NOTREACHED(); }
+  void UnregisterModuleScript(ModuleScript*) override { NOTREACHED(); }
 
   ScriptModule Resolve(const String& specifier,
                        const ScriptModule&,

third_party/WebKit/Source/core/dom/ModulatorImpl.cpp

@@ -185,9 +185,7 @@ void ModulatorImpl::ExecuteModule(const ModuleScript* module_script) {
 
   // Step 3. "If s is errored, then report the exception given by s's error for
   // s and abort these steps." [spec text]
-  // TODO(kouhei): Update "is errored".
-  ModuleInstantiationState instantiationState = module_script->State();
-  if (instantiationState == ModuleInstantiationState::kErrored) {
+  if (module_script->IsErrored()) {
     v8::Isolate* isolate = script_state_->GetIsolate();
     ScriptModule::ReportException(
         script_state_.Get(), module_script->CreateErrorInternal(isolate),

third_party/WebKit/Source/core/dom/ModuleMapTest.cpp

@@ -56,6 +56,10 @@ class TestScriptModuleResolver final : public ScriptModuleResolver {
     register_module_script_call_count_++;
   }
 
+  void UnregisterModuleScript(ModuleScript*) override {
+    FAIL() << "UnregisterModuleScript shouldn't be called in ModuleMapTest";
+  }
+
   ScriptModule Resolve(const String& specifier,
                        const ScriptModule& referrer,
                        ExceptionState&) override {

third_party/WebKit/Source/core/dom/ModulePendingScript.cpp

@@ -24,8 +24,8 @@ void ModulePendingScriptTreeClient::SetPendingScript(
 
 void ModulePendingScriptTreeClient::NotifyModuleTreeLoadFinished(
     ModuleScript* module_script) {
-  DCHECK(!(module_script && module_script->State() ==
-                                ModuleInstantiationState::kUninstantiated));
+  DCHECK(!module_script || module_script->IsErrored() ||
+         module_script->State() == ModuleInstantiationState::kInstantiated);
   DCHECK(!finished_);
   finished_ = true;
   module_script_ = module_script;

third_party/WebKit/Source/core/dom/ModuleScript.cpp

@@ -151,7 +151,7 @@ ModuleScript::ModuleScript(Modulator* settings_object,
     : settings_object_(settings_object),
       record_(this),
       base_url_(base_url),
-      error_(this),
+      preinstantiation_error_(this),
       nonce_(nonce),
       parser_state_(parser_state),
       credentials_mode_(credentials_mode),
@@ -185,31 +185,23 @@ bool ModuleScript::HasEmptyRecord() const {
 void ModuleScript::SetErrorAndClearRecord(ScriptValue error) {
   DVLOG(1) << "ModuleScript[" << this << "]::SetErrorAndClearRecord()";
 
-  // https://html.spec.whatwg.org/multipage/webappapis.html#error-a-module-script
-  // Step 1. Assert: script's state is not "errored".
-  DCHECK_NE(state_, ModuleInstantiationState::kErrored);
-
-  // Step 2. If script's module record is set, then:
+  // https://html.spec.whatwg.org/multipage/webappapis.html#concept-module-script-set-pre-instantiation-error
+  // Step 1. "If script's module record is not null, ..." [spec text]
   if (!record_.IsEmpty()) {
-    // Step 2.1. Set script module record's [[HostDefined]] field to undefined.
-    // TODO(kouhei): Implement this step.
-    // if (ScriptModuleResolver* resolver =
-    // modulator_->GetScriptModuleResolver())
-    //   resolver->UnregisterModuleScript(this);
-    NOTIMPLEMENTED();
-
-    // Step 2.2. Set script's module record to null.
-    record_.Clear();
+    // "set its [[HostDefined]] field to undefined." [spec text]
+    if (ScriptModuleResolver* resolver =
+            settings_object_->GetScriptModuleResolver())
+      resolver->UnregisterModuleScript(this);
   }
 
-  // Step 3. Set script's state to "errored".
-  state_ = ModuleInstantiationState::kErrored;
+  // Step 2. "Set script's module record to null." [spec text]
+  record_.Clear();
 
-  // Step 4. Set script's error to error.
+  // Step 3. "Set script's pre-instantiation error to error." [spec text]
   DCHECK(!error.IsEmpty());
   {
     ScriptState::Scope scope(error.GetScriptState());
-    error_.Set(error.GetIsolate(), error.V8Value());
+    preinstantiation_error_.Set(error.GetIsolate(), error.V8Value());
   }
 }
 
@@ -230,7 +222,7 @@ DEFINE_TRACE_WRAPPERS(ModuleScript) {
   // TODO(mlippautz): Support TraceWrappers(const
   // TraceWrapperV8Reference<v8::Module>&) to remove the cast.
   visitor->TraceWrappers(record_.Cast<v8::Value>());
-  visitor->TraceWrappers(error_);
+  visitor->TraceWrappers(preinstantiation_error_);
 }
 
 bool ModuleScript::IsEmpty() const {

third_party/WebKit/Source/core/dom/ModuleScript.h

@@ -61,15 +61,18 @@ class CORE_EXPORT ModuleScript final : public Script, public TraceWrapperBase {
 
   ModuleInstantiationState State() const { return state_; }
 
-  // https://html.spec.whatwg.org/multipage/webappapis.html#error-a-module-script
+  // https://html.spec.whatwg.org/multipage/webappapis.html#concept-module-script-is-errored
+  bool IsErrored() const { return record_.IsEmpty(); }
+
+  // https://html.spec.whatwg.org/multipage/webappapis.html#concept-module-script-set-pre-instantiation-error
   void SetErrorAndClearRecord(ScriptValue error);
 
   // Implements Step 7.2 of:
   // https://html.spec.whatwg.org/multipage/webappapis.html#internal-module-script-graph-fetching-procedure
   void SetInstantiationSuccess();
 
   v8::Local<v8::Value> CreateError(v8::Isolate* isolate) const {
-    return error_.NewLocal(isolate);
+    return preinstantiation_error_.NewLocal(isolate);
   }
 
   ParserDisposition ParserState() const { return parser_state_; }
@@ -113,8 +116,10 @@ class CORE_EXPORT ModuleScript final : public Script, public TraceWrapperBase {
   friend class ModuleTreeLinkerTestModulator;
   // Access this func only via ModulatorImpl::GetError(),
   // or via Modulator mocks for unit tests.
+  // TODO(kouhei): Needs update after V8 change. The error may also be stored
+  // inside record_.
   v8::Local<v8::Value> CreateErrorInternal(v8::Isolate* isolate) const {
-    return error_.NewLocal(isolate);
+    return preinstantiation_error_.NewLocal(isolate);
   }
 
   // https://html.spec.whatwg.org/multipage/webappapis.html#settings-object
@@ -129,9 +134,10 @@ class CORE_EXPORT ModuleScript final : public Script, public TraceWrapperBase {
   // https://html.spec.whatwg.org/multipage/webappapis.html#concept-module-script-instantiation-state
   ModuleInstantiationState state_ = ModuleInstantiationState::kUninstantiated;
 
-  // https://html.spec.whatwg.org/multipage/webappapis.html#concept-module-script-error
+  // https://html.spec.whatwg.org/multipage/webappapis.html#concept-module-script-pre-instantiation-error
   //
-  // |record_| and |error_| are TraceWrappers()ed and kept alive via the path of
+  // |record_| and |preinstantiation_error_| are TraceWrappers()ed and kept
+  // alive via the path of
   // DOMWindow -> Modulator/ModulatorImpl -> ModuleMap -> ModuleMap::Entry
   // -> ModuleScript, or
   // Modulator/ModulatorImpl -> ModuleTreeLinkerRegistry -> ModuleTreeLinker
@@ -140,8 +146,7 @@ class CORE_EXPORT ModuleScript final : public Script, public TraceWrapperBase {
   // ModulePendingScriptTreeClient -> ModuleScript.
   // All the classes/references on the path above should be
   // TraceWrapperBase/TraceWrapperMember<>/etc.,
-  // but other references to those classes can be normal Member<>.
-  TraceWrapperV8Reference<v8::Value> error_;
+  TraceWrapperV8Reference<v8::Value> preinstantiation_error_;
 
   // https://html.spec.whatwg.org/multipage/webappapis.html#concept-module-script-nonce
   const String nonce_;

third_party/WebKit/Source/core/dom/ScriptModuleResolver.h

@@ -28,11 +28,14 @@ class CORE_EXPORT ScriptModuleResolver
   virtual ~ScriptModuleResolver() {}
   DEFINE_INLINE_VIRTUAL_TRACE() {}
 
-  // Notify the ScriptModuleResolver that a ModuleScript exists.
+  // Notifies the ScriptModuleResolver that a ModuleScript exists.
   // This hook gives a chance for the resolver impl to populate module record
   // identifier -> ModuleScript mapping entry.
   virtual void RegisterModuleScript(ModuleScript*) = 0;
 
+  // Notifies the ScriptModuleResolver to clear its ModuleScript mapping.
+  virtual void UnregisterModuleScript(ModuleScript*) = 0;
+
   // Implements "Runtime Semantics: HostResolveImportedModule"
   // https://tc39.github.io/ecma262/#sec-hostresolveimportedmodule
   // This returns a null ScriptModule when an exception is thrown.

third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp

@@ -16,16 +16,30 @@ void ScriptModuleResolverImpl::RegisterModuleScript(
   if (module_script->Record().IsNull())
     return;
 
-  DVLOG(1) << "ScriptModuleResolverImpl::registerModuleScript(url=\""
+  DVLOG(1) << "ScriptModuleResolverImpl::RegisterModuleScript(url="
            << module_script->BaseURL().GetString()
-           << "\", hash=" << ScriptModuleHash::GetHash(module_script->Record())
+           << ", hash=" << ScriptModuleHash::GetHash(module_script->Record())
            << ")";
 
   auto result =
       record_to_module_script_map_.Set(module_script->Record(), module_script);
   DCHECK(result.is_new_entry);
 }
 
+void ScriptModuleResolverImpl::UnregisterModuleScript(
+    ModuleScript* module_script) {
+  DCHECK(module_script);
+  if (module_script->Record().IsNull())
+    return;
+
+  DVLOG(1) << "ScriptModuleResolverImpl::UnregisterModuleScript(url="
+           << module_script->BaseURL().GetString()
+           << ", hash=" << ScriptModuleHash::GetHash(module_script->Record())
+           << ")";
+
+  record_to_module_script_map_.erase(module_script->Record());
+}
+
 ScriptModule ScriptModuleResolverImpl::Resolve(
     const String& specifier,
     const ScriptModule& referrer,
@@ -68,7 +82,8 @@ ScriptModule ScriptModuleResolverImpl::Resolve(
 
   // Step 5. If resolved module script's instantiation state is "errored", then
   // throw resolved module script's instantiation error.
-  if (module_script->State() == ModuleInstantiationState::kErrored) {
+  // TODO(kouhei): Update spec references.
+  if (module_script->IsErrored()) {
     ScriptValue error = modulator_->GetError(module_script);
     exception_state.RethrowV8Exception(error.V8Value());
     return ScriptModule();

third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h

@@ -42,6 +42,8 @@ class CORE_EXPORT ScriptModuleResolverImpl final
   // Implements ScriptModuleResolver:
 
   void RegisterModuleScript(ModuleScript*) final;
+  void UnregisterModuleScript(ModuleScript*) final;
+
   // Implements "Runtime Semantics: HostResolveImportedModule" per HTML spec.
   // https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-specifier)
   ScriptModule Resolve(const String& specifier,

third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp

@@ -160,8 +160,7 @@ TEST_F(ModuleScriptLoaderTest, InvalidSpecifier) {
   EXPECT_TRUE(client->WasNotifyFinished())
       << "ModuleScriptLoader should finish synchronously.";
   ASSERT_TRUE(client->GetModuleScript());
-  EXPECT_EQ(client->GetModuleScript()->State(),
-            ModuleInstantiationState::kErrored);
+  EXPECT_TRUE(client->GetModuleScript()->IsErrored());
 }
 
 TEST_F(ModuleScriptLoaderTest, fetchInvalidURL) {