ModuleLoading: Clarify and restrict the usage of ModuleScriptFetcher

This CL adds util::PassKey<ModuleScriptLoader> to the arguments of
ModuleScriptFetcher constructor.
ModuleScriptFetcher is allowed to be called only from ModuleScriptLoader
but currently it can be called from elsewhere as an implementation. To
prohibit accessing from other classes, we use PassKey.
PassKey value must be private, but we allow accessing this value from
outside only for testing via ModuleScriptLoader::CreatePassKeyForTest().

Also, ModuleScriptFetcher::Fetch() must be called only once and this
change clarifies this restriction as the comments.

Change-Id: If284f4d5ff4ab21fe7f672ac98017d3835497889
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2071336Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Eriko Kurimoto <elkurin@google.com>
Cr-Commit-Position: refs/heads/master@{#744968}

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

@@ -12,6 +12,10 @@
 
 namespace blink {
 
+DocumentModuleScriptFetcher::DocumentModuleScriptFetcher(
+    util::PassKey<ModuleScriptLoader> pass_key)
+    : ModuleScriptFetcher(pass_key) {}
+
 void DocumentModuleScriptFetcher::Fetch(
     FetchParameters& fetch_params,
     ResourceFetcher* fetch_client_settings_object_fetcher,
@@ -51,6 +55,7 @@ void DocumentModuleScriptFetcher::NotifyFinished(Resource* resource) {
 }
 
 void DocumentModuleScriptFetcher::Trace(Visitor* visitor) {
+  ModuleScriptFetcher::Trace(visitor);
   visitor->Trace(client_);
   ResourceClient::Trace(visitor);
 }

third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.h

@@ -21,8 +21,7 @@ class CORE_EXPORT DocumentModuleScriptFetcher final
   USING_GARBAGE_COLLECTED_MIXIN(DocumentModuleScriptFetcher);
 
  public:
-  DocumentModuleScriptFetcher() = default;
-  ~DocumentModuleScriptFetcher() override = default;
+  explicit DocumentModuleScriptFetcher(util::PassKey<ModuleScriptLoader>);
 
   // Implements ModuleScriptFetcher.
   void Fetch(FetchParameters&,

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

@@ -18,8 +18,10 @@
 namespace blink {
 
 InstalledServiceWorkerModuleScriptFetcher::
-    InstalledServiceWorkerModuleScriptFetcher(WorkerGlobalScope* global_scope)
-    : global_scope_(global_scope) {
+    InstalledServiceWorkerModuleScriptFetcher(
+        WorkerGlobalScope* global_scope,
+        util::PassKey<ModuleScriptLoader> pass_key)
+    : ModuleScriptFetcher(pass_key), global_scope_(global_scope) {
   DCHECK(global_scope_->IsServiceWorkerGlobalScope());
 }
 

third_party/blink/renderer/core/loader/modulescript/installed_service_worker_module_script_fetcher.h

@@ -20,7 +20,8 @@ class CORE_EXPORT InstalledServiceWorkerModuleScriptFetcher final
   USING_GARBAGE_COLLECTED_MIXIN(InstalledServiceWorkerModuleScriptFetcher);
 
  public:
-  explicit InstalledServiceWorkerModuleScriptFetcher(WorkerGlobalScope*);
+  InstalledServiceWorkerModuleScriptFetcher(WorkerGlobalScope*,
+                                            util::PassKey<ModuleScriptLoader>);
 
   // Implements ModuleScriptFetcher.
   void Fetch(FetchParameters&,

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

@@ -16,6 +16,9 @@
 
 namespace blink {
 
+ModuleScriptFetcher::ModuleScriptFetcher(
+    util::PassKey<ModuleScriptLoader> pass_key) {}
+
 void ModuleScriptFetcher::Client::OnFetched(
     const base::Optional<ModuleScriptCreationParams>& params) {
   NotifyFetchFinished(params, HeapVector<Member<ConsoleMessage>>());
@@ -25,6 +28,10 @@ void ModuleScriptFetcher::Client::OnFailed() {
   NotifyFetchFinished(base::nullopt, HeapVector<Member<ConsoleMessage>>());
 }
 
+void ModuleScriptFetcher::Trace(Visitor* visitor) {
+  ResourceClient::Trace(visitor);
+}
+
 // <specdef href="https://html.spec.whatwg.org/C/#fetch-a-single-module-script">
 bool ModuleScriptFetcher::WasModuleLoadSuccessful(
     Resource* resource,

third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h

@@ -12,17 +12,21 @@
 #include "third_party/blink/renderer/core/script/modulator.h"
 #include "third_party/blink/renderer/platform/heap/heap_allocator.h"
 #include "third_party/blink/renderer/platform/loader/fetch/fetch_parameters.h"
+#include "third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h"
 
 namespace blink {
 
 class ConsoleMessage;
-class ResourceFetcher;
+class ModuleScriptLoader;
 
 // ModuleScriptFetcher is an abstract class to fetch module scripts. Derived
 // classes are expected to fetch a module script for the given FetchParameters
 // and return its client a fetched resource as ModuleScriptCreationParams.
 class CORE_EXPORT ModuleScriptFetcher : public ResourceClient {
  public:
+  // ModuleScriptFetcher should only be called from ModuleScriptLoader.
+  explicit ModuleScriptFetcher(util::PassKey<ModuleScriptLoader>);
+
   class CORE_EXPORT Client : public GarbageCollectedMixin {
    public:
     virtual void NotifyFetchFinished(
@@ -35,6 +39,9 @@ class CORE_EXPORT ModuleScriptFetcher : public ResourceClient {
     void OnFailed();
   };
 
+  // Fetch() must be called right after ModuleScriptFetcher is constructed.
+  // Fetch() must not be called more than once.
+  //
   // Takes a non-const reference to FetchParameters because
   // ScriptResource::Fetch() requires it.
   //
@@ -49,6 +56,8 @@ class CORE_EXPORT ModuleScriptFetcher : public ResourceClient {
                      ModuleGraphLevel,
                      Client*) = 0;
 
+  void Trace(Visitor*) override;
+
  protected:
   static bool WasModuleLoadSuccessful(
       Resource* resource,

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

@@ -213,7 +213,8 @@ void ModuleScriptLoader::FetchInternal(
   // steps. Otherwise, fetch request. Return from this algorithm, and run the
   // remaining steps as part of the fetch's process response for the response
   // response.</spec>
-  module_fetcher_ = modulator_->CreateModuleScriptFetcher(custom_fetch_type);
+  module_fetcher_ =
+      modulator_->CreateModuleScriptFetcher(custom_fetch_type, PassKey());
   module_fetcher_->Fetch(fetch_params, fetch_client_settings_object_fetcher,
                          modulator_, level, this);
 }

third_party/blink/renderer/core/loader/modulescript/module_script_loader.h

@@ -69,6 +69,8 @@ class CORE_EXPORT ModuleScriptLoader final
 
   void Trace(Visitor*) override;
 
+  friend class WorkletModuleResponsesMapTest;
+
  private:
   void FetchInternal(const ModuleScriptFetchRequest&,
                      ResourceFetcher* fetch_client_settings_object_fetcher,
@@ -76,6 +78,14 @@ class CORE_EXPORT ModuleScriptLoader final
                      ModuleScriptCustomFetchType);
 
   void AdvanceState(State new_state);
+
+  using PassKey = util::PassKey<ModuleScriptLoader>;
+  // PassKey should be private and cannot be accessed from outside, but allow
+  // accessing only from friend classes for testing.
+  static util::PassKey<ModuleScriptLoader> CreatePassKeyForTests() {
+    return PassKey();
+  }
+
 #if DCHECK_IS_ON()
   static const char* StateToString(State);
 #endif

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

@@ -95,16 +95,17 @@ class ModuleScriptLoaderTestModulator final : public DummyModulator {
   }
 
   ModuleScriptFetcher* CreateModuleScriptFetcher(
-      ModuleScriptCustomFetchType custom_fetch_type) override {
+      ModuleScriptCustomFetchType custom_fetch_type,
+      util::PassKey<ModuleScriptLoader> pass_key) override {
     auto* execution_context = ExecutionContext::From(script_state_);
     if (auto* scope = DynamicTo<WorkletGlobalScope>(execution_context)) {
       EXPECT_EQ(ModuleScriptCustomFetchType::kWorkletAddModule,
                 custom_fetch_type);
       return MakeGarbageCollected<WorkletModuleScriptFetcher>(
-          scope->GetModuleResponsesMap());
+          scope->GetModuleResponsesMap(), pass_key);
     }
     EXPECT_EQ(ModuleScriptCustomFetchType::kNone, custom_fetch_type);
-    return MakeGarbageCollected<DocumentModuleScriptFetcher>();
+    return MakeGarbageCollected<DocumentModuleScriptFetcher>(pass_key);
   }
 
   void Trace(Visitor*) override;

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

@@ -19,8 +19,9 @@
 namespace blink {
 
 WorkerModuleScriptFetcher::WorkerModuleScriptFetcher(
-    WorkerGlobalScope* global_scope)
-    : global_scope_(global_scope) {}
+    WorkerGlobalScope* global_scope,
+    util::PassKey<ModuleScriptLoader> pass_key)
+    : ModuleScriptFetcher(pass_key), global_scope_(global_scope) {}
 
 // <specdef href="https://html.spec.whatwg.org/C/#run-a-worker">
 void WorkerModuleScriptFetcher::Fetch(

third_party/blink/renderer/core/loader/modulescript/worker_module_script_fetcher.h

@@ -22,7 +22,8 @@ class CORE_EXPORT WorkerModuleScriptFetcher final
   USING_GARBAGE_COLLECTED_MIXIN(WorkerModuleScriptFetcher);
 
  public:
-  explicit WorkerModuleScriptFetcher(WorkerGlobalScope*);
+  WorkerModuleScriptFetcher(WorkerGlobalScope*,
+                            util::PassKey<ModuleScriptLoader>);
 
   // Implements ModuleScriptFetcher.
   void Fetch(FetchParameters&,

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

@@ -9,8 +9,10 @@
 namespace blink {
 
 WorkletModuleScriptFetcher::WorkletModuleScriptFetcher(
-    WorkletModuleResponsesMap* module_responses_map)
-    : module_responses_map_(module_responses_map) {}
+    WorkletModuleResponsesMap* module_responses_map,
+    util::PassKey<ModuleScriptLoader> pass_key)
+    : ModuleScriptFetcher(pass_key),
+      module_responses_map_(module_responses_map) {}
 
 void WorkletModuleScriptFetcher::Fetch(
     FetchParameters& fetch_params,

third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.h

@@ -28,7 +28,8 @@ class CORE_EXPORT WorkletModuleScriptFetcher final
   USING_GARBAGE_COLLECTED_MIXIN(WorkletModuleScriptFetcher);
 
  public:
-  explicit WorkletModuleScriptFetcher(WorkletModuleResponsesMap*);
+  WorkletModuleScriptFetcher(WorkletModuleResponsesMap*,
+                             util::PassKey<ModuleScriptLoader>);
 
   // Implements ModuleScriptFetcher.
   void Fetch(FetchParameters&,

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

@@ -16,9 +16,10 @@ DocumentModulatorImpl::DocumentModulatorImpl(ScriptState* script_state)
     : ModulatorImplBase(script_state) {}
 
 ModuleScriptFetcher* DocumentModulatorImpl::CreateModuleScriptFetcher(
-    ModuleScriptCustomFetchType custom_fetch_type) {
+    ModuleScriptCustomFetchType custom_fetch_type,
+    util::PassKey<ModuleScriptLoader> pass_key) {
   DCHECK_EQ(ModuleScriptCustomFetchType::kNone, custom_fetch_type);
-  return MakeGarbageCollected<DocumentModuleScriptFetcher>();
+  return MakeGarbageCollected<DocumentModuleScriptFetcher>(pass_key);
 }
 
 bool DocumentModulatorImpl::IsDynamicImportForbidden(String* reason) {

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

@@ -24,7 +24,8 @@ class DocumentModulatorImpl final : public ModulatorImplBase {
 
   // Implements Modulator.
   ModuleScriptFetcher* CreateModuleScriptFetcher(
-      ModuleScriptCustomFetchType) override;
+      ModuleScriptCustomFetchType,
+      util::PassKey<ModuleScriptLoader>) override;
 
  private:
   // Implements ModulatorImplBase.

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

@@ -28,6 +28,7 @@ namespace blink {
 class ModuleScript;
 class ModuleScriptFetchRequest;
 class ModuleScriptFetcher;
+class ModuleScriptLoader;
 class ImportMap;
 class ReferrerScriptInfo;
 class ResourceFetcher;
@@ -217,7 +218,8 @@ class CORE_EXPORT Modulator : public GarbageCollected<Modulator>,
   virtual ScriptValue ExecuteModule(ModuleScript*, CaptureEvalErrorFlag) = 0;
 
   virtual ModuleScriptFetcher* CreateModuleScriptFetcher(
-      ModuleScriptCustomFetchType) = 0;
+      ModuleScriptCustomFetchType,
+      util::PassKey<ModuleScriptLoader> pass_key) = 0;
 };
 
 }  // namespace blink

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

@@ -108,8 +108,9 @@ class ModuleMapTestModulator final : public DummyModulator {
     USING_GARBAGE_COLLECTED_MIXIN(TestModuleScriptFetcher);
 
    public:
-    explicit TestModuleScriptFetcher(ModuleMapTestModulator* modulator)
-        : modulator_(modulator) {}
+    TestModuleScriptFetcher(ModuleMapTestModulator* modulator,
+                            util::PassKey<ModuleScriptLoader> pass_key)
+        : ModuleScriptFetcher(pass_key), modulator_(modulator) {}
     void Fetch(FetchParameters& request,
                ResourceFetcher*,
                const Modulator* modulator_for_built_in_modules,
@@ -135,8 +136,9 @@ class ModuleMapTestModulator final : public DummyModulator {
   };
 
   ModuleScriptFetcher* CreateModuleScriptFetcher(
-      ModuleScriptCustomFetchType) override {
-    return MakeGarbageCollected<TestModuleScriptFetcher>(this);
+      ModuleScriptCustomFetchType,
+      util::PassKey<ModuleScriptLoader> pass_key) override {
+    return MakeGarbageCollected<TestModuleScriptFetcher>(this, pass_key);
   }
 
   Vector<ModuleRequest> ModuleRequestsFromModuleRecord(

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

@@ -18,18 +18,20 @@ WorkerModulatorImpl::WorkerModulatorImpl(ScriptState* script_state)
     : ModulatorImplBase(script_state) {}
 
 ModuleScriptFetcher* WorkerModulatorImpl::CreateModuleScriptFetcher(
-    ModuleScriptCustomFetchType custom_fetch_type) {
+    ModuleScriptCustomFetchType custom_fetch_type,
+    util::PassKey<ModuleScriptLoader> pass_key) {
   auto* global_scope = To<WorkerGlobalScope>(GetExecutionContext());
   switch (custom_fetch_type) {
     case ModuleScriptCustomFetchType::kNone:
-      return MakeGarbageCollected<DocumentModuleScriptFetcher>();
+      return MakeGarbageCollected<DocumentModuleScriptFetcher>(pass_key);
     case ModuleScriptCustomFetchType::kWorkerConstructor:
-      return MakeGarbageCollected<WorkerModuleScriptFetcher>(global_scope);
+      return MakeGarbageCollected<WorkerModuleScriptFetcher>(global_scope,
+                                                             pass_key);
     case ModuleScriptCustomFetchType::kWorkletAddModule:
       break;
     case ModuleScriptCustomFetchType::kInstalledServiceWorker:
       return MakeGarbageCollected<InstalledServiceWorkerModuleScriptFetcher>(
-          global_scope);
+          global_scope, pass_key);
   }
   NOTREACHED();
   return nullptr;

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

@@ -21,7 +21,8 @@ class WorkerModulatorImpl final : public ModulatorImplBase {
 
   // Implements ModulatorImplBase.
   ModuleScriptFetcher* CreateModuleScriptFetcher(
-      ModuleScriptCustomFetchType) override;
+      ModuleScriptCustomFetchType,
+      util::PassKey<ModuleScriptLoader> pass_key) override;
 
  private:
   // Implements ModulatorImplBase.

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

@@ -13,12 +13,13 @@ WorkletModulatorImpl::WorkletModulatorImpl(ScriptState* script_state)
     : ModulatorImplBase(script_state) {}
 
 ModuleScriptFetcher* WorkletModulatorImpl::CreateModuleScriptFetcher(
-    ModuleScriptCustomFetchType custom_fetch_type) {
+    ModuleScriptCustomFetchType custom_fetch_type,
+    util::PassKey<ModuleScriptLoader> pass_key) {
   DCHECK_EQ(ModuleScriptCustomFetchType::kWorkletAddModule, custom_fetch_type);
   WorkletGlobalScope* global_scope =
       To<WorkletGlobalScope>(GetExecutionContext());
   return MakeGarbageCollected<WorkletModuleScriptFetcher>(
-      global_scope->GetModuleResponsesMap());
+      global_scope->GetModuleResponsesMap(), pass_key);
 }
 
 bool WorkletModulatorImpl::IsDynamicImportForbidden(String* reason) {

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

@@ -23,7 +23,8 @@ class WorkletModulatorImpl final : public ModulatorImplBase {
 
   // Implements ModulatorImplBase.
   ModuleScriptFetcher* CreateModuleScriptFetcher(
-      ModuleScriptCustomFetchType) override;
+      ModuleScriptCustomFetchType,
+      util::PassKey<ModuleScriptLoader>) override;
 
  private:
   // Implements ModulatorImplBase.

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

@@ -184,7 +184,8 @@ ScriptValue DummyModulator::ExecuteModule(ModuleScript*, CaptureEvalErrorFlag) {
 }
 
 ModuleScriptFetcher* DummyModulator::CreateModuleScriptFetcher(
-    ModuleScriptCustomFetchType) {
+    ModuleScriptCustomFetchType,
+    util::PassKey<ModuleScriptLoader> pass_key) {
   NOTREACHED();
   return nullptr;
 }

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

@@ -79,7 +79,8 @@ class DummyModulator : public Modulator {
       v8::Local<v8::Module>) override;
   ScriptValue ExecuteModule(ModuleScript*, CaptureEvalErrorFlag) override;
   ModuleScriptFetcher* CreateModuleScriptFetcher(
-      ModuleScriptCustomFetchType) override;
+      ModuleScriptCustomFetchType,
+      util::PassKey<ModuleScriptLoader>) override;
 
   Member<ModuleRecordResolver> resolver_;
 };

third_party/blink/renderer/core/workers/worklet_module_responses_map_test.cc

@@ -8,8 +8,10 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/platform/web_url_loader_mock_factory.h"
 #include "third_party/blink/renderer/core/loader/modulescript/module_script_creation_params.h"
+#include "third_party/blink/renderer/core/loader/modulescript/module_script_loader.h"
 #include "third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.h"
 #include "third_party/blink/renderer/core/script/modulator.h"
+#include "third_party/blink/renderer/core/testing/dummy_modulator.h"
 #include "third_party/blink/renderer/platform/loader/testing/fetch_testing_platform_support.h"
 #include "third_party/blink/renderer/platform/loader/testing/mock_fetch_context.h"
 #include "third_party/blink/renderer/platform/loader/testing/test_loader_factory.h"
@@ -71,7 +73,8 @@ class WorkletModuleResponsesMapTest : public testing::Test {
     resource_request.SetRequestContext(mojom::RequestContextType::SCRIPT);
     FetchParameters fetch_params(std::move(resource_request));
     WorkletModuleScriptFetcher* module_fetcher =
-        MakeGarbageCollected<WorkletModuleScriptFetcher>(map_.Get());
+        MakeGarbageCollected<WorkletModuleScriptFetcher>(
+            map_.Get(), ModuleScriptLoader::CreatePassKeyForTests());
     module_fetcher->Fetch(fetch_params, fetcher_.Get(),
                           nullptr /* modulator_for_built_in_modules */,
                           ModuleGraphLevel::kTopLevelModuleFetch, client);