[Import Maps] Fix rejection of multiple import maps

Previously, second successful import map was rejected
during registration. It didn't trigger error events, and
the second import map wasn't rejected if the first import map
had errors.

This CL follows https://github.com/WICG/import-maps/pull/242
and changes these behavior (the second import map is rejected
with an error event no matter whether the first import map has
errors).

This CL also introduces Modulator::AcquiringImportMapsState,
just for better error messaging.

Bug: 848607
Change-Id: Iadfc59dc0ae9b64262908dbf27d44a4c24a74c05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2618838Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843277}

third_party/blink/renderer/core/loader/modulescript/module_tree_linker.cc

@@ -178,7 +178,9 @@ void ModuleTreeLinker::FetchRoot(const KURL& original_url,
 #endif
 
   // https://wicg.github.io/import-maps/#wait-for-import-maps
-  modulator_->ClearIsAcquiringImportMaps();
+  // 1.2. Set document’s acquiring import maps to false. [spec text]
+  modulator_->SetAcquiringImportMapsState(
+      Modulator::AcquiringImportMapsState::kAfterModuleScriptLoad);
 
   AdvanceState(State::kFetchingSelf);
 
@@ -249,9 +251,11 @@ void ModuleTreeLinker::FetchRootInline(ModuleScript* module_script) {
 #endif
 
   // https://wicg.github.io/import-maps/#wait-for-import-maps
+  // 1.2. Set document’s acquiring import maps to false. [spec text]
   //
   // TODO(hiroshige): This should be done before |module_script| is created.
-  modulator_->ClearIsAcquiringImportMaps();
+  modulator_->SetAcquiringImportMapsState(
+      Modulator::AcquiringImportMapsState::kAfterModuleScriptLoad);
 
   AdvanceState(State::kFetchingSelf);
 

third_party/blink/renderer/core/loader/modulescript/module_tree_linker_test.cc

@@ -128,7 +128,7 @@ class ModuleTreeLinkerTestModulator final : public DummyModulator {
                               String* failure_reason) final {
     return KURL(base_url, module_request);
   }
-  void ClearIsAcquiringImportMaps() final {}
+  void SetAcquiringImportMapsState(AcquiringImportMapsState) final {}
 
   void FetchSingle(const ModuleScriptFetchRequest& request,
                    ResourceFetcher*,

third_party/blink/renderer/core/script/dynamic_module_resolver.cc

@@ -267,14 +267,6 @@ void DynamicModuleResolver::ResolveDynamically(
       << "ResolveDynamically should be called from V8 callback, within a valid "
          "context.";
 
-  // https://github.com/WICG/import-maps/blob/master/spec.md#when-import-maps-can-be-encountered
-  // Strictly, the flag should be cleared at
-  // #internal-module-script-graph-fetching-procedure, i.e. in ModuleTreeLinker,
-  // but due to https://crbug.com/928435 https://crbug.com/928564 we also clears
-  // the flag here, as import maps can be accessed earlier than specced below
-  // (in ResolveModuleSpecifier()) and we need to clear the flag before that.
-  modulator_->ClearIsAcquiringImportMaps();
-
   // <spec step="4.1">Let referencing script be
   // referencingScriptOrModule.[[HostDefined]].</spec>
 
@@ -302,6 +294,11 @@ void DynamicModuleResolver::ResolveDynamically(
   // <specdef label="fetch-an-import()-module-script-graph"
   // href="https://html.spec.whatwg.org/C/#fetch-an-import()-module-script-graph">
 
+  // https://wicg.github.io/import-maps/#wait-for-import-maps
+  // 1.2. Set document’s acquiring import maps to false. [spec text]
+  modulator_->SetAcquiringImportMapsState(
+      Modulator::AcquiringImportMapsState::kAfterModuleScriptLoad);
+
   // <spec label="fetch-an-import()-module-script-graph" step="1">Let url be the
   // result of resolving a module specifier given base URL and specifier.</spec>
   KURL url = modulator_->ResolveModuleSpecifier(specifier, base_url);

third_party/blink/renderer/core/script/dynamic_module_resolver_test.cc

@@ -75,7 +75,7 @@ class DynamicModuleResolverTestModulator final : public DummyModulator {
     return KURL(base_url, module_request);
   }
 
-  void ClearIsAcquiringImportMaps() final {}
+  void SetAcquiringImportMapsState(AcquiringImportMapsState) final {}
 
   void FetchTree(const KURL& url,
                  ResourceFetcher*,

third_party/blink/renderer/core/script/modulator.h

@@ -183,12 +183,26 @@ class CORE_EXPORT Modulator : public GarbageCollected<Modulator>,
   virtual ScriptValue CreateSyntaxError(const String& message) const = 0;
 
   // Import maps. https://github.com/WICG/import-maps
+
+  // https://wicg.github.io/import-maps/#register-an-import-map
   virtual void RegisterImportMap(const ImportMap*,
                                  ScriptValue error_to_rethrow) = 0;
-  virtual bool IsAcquiringImportMaps() const = 0;
-  virtual void ClearIsAcquiringImportMaps() = 0;
   virtual const ImportMap* GetImportMapForTest() const = 0;
 
+  // https://wicg.github.io/import-maps/#document-acquiring-import-maps
+  enum class AcquiringImportMapsState {
+    // The flag is true.
+    kAcquiring,
+
+    // The flag is false, due to multiple import maps.
+    kMultipleImportMaps,
+
+    // The flag is false, because module script loading is already started.
+    kAfterModuleScriptLoad
+  };
+  virtual AcquiringImportMapsState GetAcquiringImportMapsState() const = 0;
+  virtual void SetAcquiringImportMapsState(AcquiringImportMapsState) = 0;
+
   // https://html.spec.whatwg.org/C/#hostgetimportmetaproperties
   virtual ModuleImportMeta HostGetImportMetaProperties(
       v8::Local<v8::Module>) const = 0;

third_party/blink/renderer/core/script/modulator_impl_base.cc

@@ -213,13 +213,10 @@ void ModulatorImplBase::RegisterImportMap(const ImportMap* import_map,
   //
   // TODO(crbug.com/927119): Implement merging. Currently only one import map is
   // allowed.
-  if (import_map_) {
-    GetExecutionContext()->AddConsoleMessage(
-        mojom::ConsoleMessageSource::kOther, mojom::ConsoleMessageLevel::kError,
-        "Multiple import maps are not yet supported. https://crbug.com/927119");
-    return;
-  }
 
+  // Because the second and subsequent import maps are already rejected in
+  // ScriptLoader::PrepareScript(), this is called only once.
+  DCHECK(!import_map_);
   import_map_ = import_map;
 }
 

third_party/blink/renderer/core/script/modulator_impl_base.h

@@ -85,8 +85,12 @@ class ModulatorImplBase : public Modulator {
   ScriptValue CreateTypeError(const String& message) const override;
   ScriptValue CreateSyntaxError(const String& message) const override;
   void RegisterImportMap(const ImportMap*, ScriptValue error_to_rethrow) final;
-  bool IsAcquiringImportMaps() const final { return acquiring_import_maps_; }
-  void ClearIsAcquiringImportMaps() final { acquiring_import_maps_ = false; }
+  AcquiringImportMapsState GetAcquiringImportMapsState() const final {
+    return acquiring_import_maps_;
+  }
+  void SetAcquiringImportMapsState(AcquiringImportMapsState value) final {
+    acquiring_import_maps_ = value;
+  }
   ModuleImportMeta HostGetImportMetaProperties(
       v8::Local<v8::Module>) const override;
   ScriptValue InstantiateModule(v8::Local<v8::Module>, const KURL&) override;
@@ -115,10 +119,11 @@ class ModulatorImplBase : public Modulator {
 
   Member<const ImportMap> import_map_;
 
-  // https://github.com/WICG/import-maps/blob/master/spec.md#when-import-maps-can-be-encountered
-  // Each realm (environment settings object) has a boolean, acquiring import
-  // maps. It is initially true. [spec text]
-  bool acquiring_import_maps_ = true;
+  // https://wicg.github.io/import-maps/#document-acquiring-import-maps
+  // Each Document has an acquiring import maps boolean. It is initially true.
+  // [spec text]
+  AcquiringImportMapsState acquiring_import_maps_ =
+      AcquiringImportMapsState::kAcquiring;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/script/script_loader.cc

@@ -505,22 +505,44 @@ bool ScriptLoader::PrepareScript(const TextPosition& script_start_position,
   auto* fetch_client_settings_object_fetcher = context_window->Fetcher();
 
   // https://wicg.github.io/import-maps/#integration-prepare-a-script
-  // If the script’s type is "importmap" and the element’s node document’s
-  // acquiring import maps is false, then queue a task to fire an event named
-  // error at the element, and return. [spec text]
+  // If the script’s type is "importmap": [spec text]
   if (GetScriptType() == ScriptTypeAtPrepare::kImportMap) {
     Modulator* modulator =
         Modulator::From(ToScriptStateForMainWorld(context_window->GetFrame()));
-    if (!modulator->IsAcquiringImportMaps()) {
-      element_document.AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
-          mojom::ConsoleMessageSource::kJavaScript,
-          mojom::ConsoleMessageLevel::kError,
-          "An import map is added after module script load was triggered."));
-      element_document.GetTaskRunner(TaskType::kDOMManipulation)
-          ->PostTask(FROM_HERE,
-                     WTF::Bind(&ScriptElementBase::DispatchErrorEvent,
-                               WrapPersistent(element_.Get())));
-      return false;
+    auto aquiring_state = modulator->GetAcquiringImportMapsState();
+    switch (aquiring_state) {
+      case Modulator::AcquiringImportMapsState::kAfterModuleScriptLoad:
+      case Modulator::AcquiringImportMapsState::kMultipleImportMaps:
+        // 1. If the element’s node document's acquiring import maps is false,
+        // then queue a task to fire an event named error at the element, and
+        // return. [spec text]
+        element_document.AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
+            mojom::blink::ConsoleMessageSource::kJavaScript,
+            mojom::blink::ConsoleMessageLevel::kError,
+            aquiring_state ==
+                    Modulator::AcquiringImportMapsState::kAfterModuleScriptLoad
+                ? "An import map is added after module script load was "
+                  "triggered."
+                : "Multiple import maps are not yet supported. "
+                  "https://crbug.com/927119"));
+        element_document.GetTaskRunner(TaskType::kDOMManipulation)
+            ->PostTask(FROM_HERE,
+                       WTF::Bind(&ScriptElementBase::DispatchErrorEvent,
+                                 WrapPersistent(element_.Get())));
+        return false;
+
+      case Modulator::AcquiringImportMapsState::kAcquiring:
+        // 2. Set the element’s node document's acquiring import maps to false.
+        // [spec text]
+        modulator->SetAcquiringImportMapsState(
+            Modulator::AcquiringImportMapsState::kMultipleImportMaps);
+
+        // 3. Assert: the element’s node document's pending import map script is
+        // null. [spec text]
+        //
+        // TODO(crbug.com/922212): Currently there are no implementation for
+        // "pending import map script" as we don't support external import maps.
+        break;
     }
   }
 

third_party/blink/renderer/core/testing/dummy_modulator.cc

@@ -138,12 +138,13 @@ void DummyModulator::RegisterImportMap(const ImportMap*,
   NOTREACHED();
 }
 
-bool DummyModulator::IsAcquiringImportMaps() const {
+Modulator::AcquiringImportMapsState
+DummyModulator::GetAcquiringImportMapsState() const {
   NOTREACHED();
-  return true;
+  return AcquiringImportMapsState::kAcquiring;
 }
 
-void DummyModulator::ClearIsAcquiringImportMaps() {
+void DummyModulator::SetAcquiringImportMapsState(AcquiringImportMapsState) {
   NOTREACHED();
 }
 

third_party/blink/renderer/core/testing/dummy_modulator.h

@@ -68,8 +68,8 @@ class DummyModulator : public Modulator {
   ScriptValue CreateSyntaxError(const String& message) const override;
   void RegisterImportMap(const ImportMap*,
                          ScriptValue error_to_rethrow) override;
-  bool IsAcquiringImportMaps() const override;
-  void ClearIsAcquiringImportMaps() override;
+  AcquiringImportMapsState GetAcquiringImportMapsState() const override;
+  void SetAcquiringImportMapsState(AcquiringImportMapsState) override;
   ModuleImportMeta HostGetImportMetaProperties(
       v8::Local<v8::Module>) const override;
   const ImportMap* GetImportMapForTest() const override;

third_party/blink/web_tests/external/wpt/import-maps/multiple-import-maps/basic-expected.txt

-This is a testharness.js-based test.
-FAIL Second import map should be rejected assert_array_equals: lengths differ, expected array ["onerror 2", "log:B1", "log:B2", "log:A3"] length 4, got ["log:B1", "log:B2", "log:A3"] length 3
-Harness: the test ran to completion.
-

third_party/blink/web_tests/external/wpt/import-maps/multiple-import-maps/with-errors-expected.txt

-This is a testharness.js-based test.
-FAIL Second import map should be rejected after an import map with errors assert_array_equals: lengths differ, expected array ["onerror 2", "log:A"] length 2, got ["log:C"] length 1
-Harness: the test ran to completion.
-