Bug 1877792 - Part 2: Change the module map key to include both URL and module type r=yulia,jonco

Jon4t4n · Jon4t4n · commit 2d109af176e6 · 2024-08-22T14:21:48.000Z
The module maps (`mFetchingModules` and `mFetchedModules`) in `ModuleLoaderBase`, and `VisitedURLSet` were previously only keyed by the URL and used the `nsURIHashKey` hashtable key class. This is no longer sufficient, and the key should also contain the module type. This patch introduces a new hashtable key class called `ModuleMapKey` and changes `mFetchingModules`, `mFetchedModules`, and `VisitedURLSet` to use the new key type. To make this a bit easier to review, this first patch only introduces the new key type and hard-codes the type to Javascript, where the key is constructed. The hard-corded module types will be fixed in later patches. Differential Revision: https://phabricator.services.mozilla.com/D155160
diff --git a/js/loader/ModuleLoadRequest.cpp b/js/loader/ModuleLoadRequest.cpp
@@ -52,7 +52,7 @@ NS_IMPL_CYCLE_COLLECTION_TRACE_END
 /* static */
 VisitedURLSet* ModuleLoadRequest::NewVisitedSetForTopLevelImport(nsIURI* aURI) {
   auto set = new VisitedURLSet();
-  set->PutEntry(aURI);
+  set->PutEntry(ModuleMapKey(aURI, ModuleType::JavaScript));
   return set;
 }
 
diff --git a/js/loader/ModuleLoadRequest.h b/js/loader/ModuleLoadRequest.h
@@ -22,9 +22,9 @@ class LoadedScript;
 class ModuleScript;
 class ModuleLoaderBase;
 
-// A reference counted set of URLs we have visited in the process of loading a
-// module graph.
-class VisitedURLSet : public nsTHashtable<nsURIHashKey> {
+// A reference counted set of module keys (URL and module type) we have visited
+// in the process of loading a module graph.
+class VisitedURLSet : public nsTHashtable<ModuleMapKey> {
   NS_INLINE_DECL_REFCOUNTING(VisitedURLSet)
 
  private:
diff --git a/js/loader/ModuleLoaderBase.cpp b/js/loader/ModuleLoaderBase.cpp
@@ -488,11 +488,11 @@ bool ModuleLoaderBase::ModuleMapContainsURL(nsIURI* aURL) const {
 }
 
 bool ModuleLoaderBase::IsModuleFetching(nsIURI* aURL) const {
-  return mFetchingModules.Contains(aURL);
+  return mFetchingModules.Contains(ModuleMapKey(aURL, ModuleType::JavaScript));
 }
 
 bool ModuleLoaderBase::IsModuleFetched(nsIURI* aURL) const {
-  return mFetchedModules.Contains(aURL);
+  return mFetchedModules.Contains(ModuleMapKey(aURL, ModuleType::JavaScript));
 }
 
 nsresult ModuleLoaderBase::GetFetchedModuleURLs(nsTArray<nsCString>& aURLs) {
@@ -512,12 +512,14 @@ nsresult ModuleLoaderBase::GetFetchedModuleURLs(nsTArray<nsCString>& aURLs) {
 void ModuleLoaderBase::SetModuleFetchStarted(ModuleLoadRequest* aRequest) {
   // Update the module map to indicate that a module is currently being fetched.
 
+  ModuleMapKey moduleMapKey(aRequest->mURI, ModuleType::JavaScript);
+
   MOZ_ASSERT(aRequest->IsFetching() || aRequest->IsPendingFetchingError());
   MOZ_ASSERT(!ModuleMapContainsURL(aRequest->mURI));
 
   RefPtr<LoadingRequest> loadingRequest = new LoadingRequest();
   loadingRequest->mRequest = aRequest;
-  mFetchingModules.InsertOrUpdate(aRequest->mURI, loadingRequest);
+  mFetchingModules.InsertOrUpdate(moduleMapKey, loadingRequest);
 }
 
 void ModuleLoaderBase::SetModuleFetchFinishedAndResumeWaitingRequests(
@@ -534,7 +536,8 @@ void ModuleLoaderBase::SetModuleFetchFinishedAndResumeWaitingRequests(
        "%u)",
        aRequest, aRequest->mModuleScript.get(), unsigned(aResult)));
 
-  auto entry = mFetchingModules.Lookup(aRequest->mURI);
+  ModuleMapKey moduleMapKey(aRequest->mURI, ModuleType::JavaScript);
+  auto entry = mFetchingModules.Lookup(moduleMapKey);
   if (!entry) {
     LOG(
         ("ScriptLoadRequest (%p): Key not found in mFetchingModules, "
@@ -557,12 +560,12 @@ void ModuleLoaderBase::SetModuleFetchFinishedAndResumeWaitingRequests(
     return;
   }
 
-  MOZ_ALWAYS_TRUE(mFetchingModules.Remove(aRequest->mURI));
+  MOZ_ALWAYS_TRUE(mFetchingModules.Remove(moduleMapKey));
 
   RefPtr<ModuleScript> moduleScript(aRequest->mModuleScript);
   MOZ_ASSERT(NS_FAILED(aResult) == !moduleScript);
 
-  mFetchedModules.InsertOrUpdate(aRequest->mURI, RefPtr{moduleScript});
+  mFetchedModules.InsertOrUpdate(moduleMapKey, RefPtr{moduleScript});
 
   LOG(("ScriptLoadRequest (%p): Resuming waiting requests", aRequest));
   MOZ_ASSERT(loadingRequest->mRequest == aRequest);
@@ -588,15 +591,16 @@ void ModuleLoaderBase::ResumeWaitingRequest(ModuleLoadRequest* aRequest,
 void ModuleLoaderBase::WaitForModuleFetch(ModuleLoadRequest* aRequest) {
   nsIURI* uri = aRequest->mURI;
   MOZ_ASSERT(ModuleMapContainsURL(uri));
+  ModuleMapKey moduleMapKey(uri, ModuleType::JavaScript);
 
-  if (auto entry = mFetchingModules.Lookup(uri)) {
+  if (auto entry = mFetchingModules.Lookup(moduleMapKey)) {
     RefPtr<LoadingRequest> loadingRequest = entry.Data();
     loadingRequest->mWaiting.AppendElement(aRequest);
     return;
   }
 
   RefPtr<ModuleScript> ms;
-  MOZ_ALWAYS_TRUE(mFetchedModules.Get(uri, getter_AddRefs(ms)));
+  MOZ_ALWAYS_TRUE(mFetchedModules.Get(moduleMapKey, getter_AddRefs(ms)));
 
   ResumeWaitingRequest(aRequest, bool(ms));
 }
@@ -608,8 +612,9 @@ ModuleScript* ModuleLoaderBase::GetFetchedModule(nsIURI* aURL) const {
     LOG(("GetFetchedModule %s", url.get()));
   }
 
+  ModuleMapKey moduleMapKey(aURL, ModuleType::JavaScript);
   bool found;
-  ModuleScript* ms = mFetchedModules.GetWeak(aURL, &found);
+  ModuleScript* ms = mFetchedModules.GetWeak(moduleMapKey, &found);
   MOZ_ASSERT(found);
   return ms;
 }
@@ -863,7 +868,8 @@ void ModuleLoaderBase::StartFetchingModuleDependencies(
   MOZ_ASSERT(aRequest->IsFetching() || aRequest->IsCompiling());
 
   auto visitedSet = aRequest->mVisitedSet;
-  MOZ_ASSERT(visitedSet->Contains(aRequest->mURI));
+  MOZ_ASSERT(visitedSet->Contains(
+      ModuleMapKey(aRequest->mURI, ModuleType::JavaScript)));
 
   aRequest->mState = ModuleLoadRequest::State::LoadingImports;
 
@@ -880,10 +886,12 @@ void ModuleLoaderBase::StartFetchingModuleDependencies(
   int32_t i = 0;
   while (i < urls.Count()) {
     nsIURI* url = urls[i];
-    if (visitedSet->Contains(url)) {
+    ModuleMapKey moduleMapKey(url, ModuleType::JavaScript);
+
+    if (visitedSet->Contains(moduleMapKey)) {
       urls.RemoveObjectAt(i);
     } else {
-      visitedSet->PutEntry(url);
+      visitedSet->PutEntry(moduleMapKey);
       i++;
     }
   }
@@ -1475,7 +1483,7 @@ void ModuleLoaderBase::CopyModulesTo(ModuleLoaderBase* aDest) {
     if (!moduleScript) {
       continue;
     }
-    aDest->mFetchedModules.InsertOrUpdate(entry.GetKey(), moduleScript);
+    aDest->mFetchedModules.InsertOrUpdate(entry, moduleScript);
   }
 }
 
@@ -1490,20 +1498,21 @@ void ModuleLoaderBase::MoveModulesTo(ModuleLoaderBase* aDest) {
     }
 
 #ifdef DEBUG
-    if (auto existingEntry = aDest->mFetchedModules.Lookup(entry.GetKey())) {
+    if (auto existingEntry = aDest->mFetchedModules.Lookup(entry)) {
       MOZ_ASSERT(moduleScript == existingEntry.Data());
     }
 #endif
 
-    aDest->mFetchedModules.InsertOrUpdate(entry.GetKey(), moduleScript);
+    aDest->mFetchedModules.InsertOrUpdate(entry, moduleScript);
   }
 
   mFetchedModules.Clear();
 }
 
 bool ModuleLoaderBase::IsFetchingAndHasWaitingRequest(
     ModuleLoadRequest* aRequest) {
-  auto entry = mFetchingModules.Lookup(aRequest->mURI);
+  auto entry = mFetchingModules.Lookup(
+      ModuleMapKey(aRequest->mURI, ModuleType::JavaScript));
   if (!entry) {
     return false;
   }
diff --git a/js/loader/ModuleLoaderBase.h b/js/loader/ModuleLoaderBase.h
@@ -109,6 +109,58 @@ class ScriptLoaderInterface : public nsISupports {
   virtual void MaybeTriggerBytecodeEncoding() {}
 };
 
+class ModuleMapKey : public PLDHashEntryHdr {
+ public:
+  using KeyType = const ModuleMapKey&;
+  using KeyTypePointer = const ModuleMapKey*;
+
+  ModuleMapKey(const nsIURI* aUri, const ModuleType aModuleType)
+      : mUri(const_cast<nsIURI*>(aUri)), mModuleType(aModuleType) {
+    MOZ_COUNT_CTOR(ModuleMapKey);
+    MOZ_ASSERT(aUri);
+  }
+  explicit ModuleMapKey(KeyTypePointer aOther)
+      : mUri(std::move(aOther->mUri)), mModuleType(aOther->mModuleType) {
+    MOZ_COUNT_CTOR(ModuleMapKey);
+    MOZ_ASSERT(mUri);
+  }
+  ModuleMapKey(ModuleMapKey&& aOther)
+      : mUri(std::move(aOther.mUri)), mModuleType(aOther.mModuleType) {
+    MOZ_COUNT_CTOR(ModuleMapKey);
+    MOZ_ASSERT(mUri);
+  }
+  MOZ_COUNTED_DTOR(ModuleMapKey)
+
+  bool KeyEquals(KeyTypePointer aKey) const {
+    if (mModuleType != aKey->mModuleType) {
+      return false;
+    }
+
+    bool eq;
+    if (NS_SUCCEEDED(mUri->Equals(aKey->mUri, &eq))) {
+      return eq;
+    }
+
+    return false;
+  }
+
+  static KeyTypePointer KeyToPointer(KeyType key) { return &key; }
+
+  static PLDHashNumber HashKey(KeyTypePointer aKey) {
+    MOZ_ASSERT(aKey->mUri);
+    nsAutoCString spec;
+    // This is based on `nsURIHashKey`, and it ignores GetSpec() failures, so
+    // do the same here.
+    (void)aKey->mUri->GetSpec(spec);
+    return mozilla::HashGeneric(mozilla::HashString(spec), aKey->mModuleType);
+  }
+
+  enum { ALLOW_MEMMOVE = true };
+
+  const nsCOMPtr<nsIURI> mUri;
+  const ModuleType mModuleType;
+};
+
 /*
  * [DOMDOC] Module Loading
  *
@@ -188,8 +240,8 @@ class ModuleLoaderBase : public nsISupports {
   };
 
   // Module map
-  nsRefPtrHashtable<nsURIHashKey, LoadingRequest> mFetchingModules;
-  nsRefPtrHashtable<nsURIHashKey, ModuleScript> mFetchedModules;
+  nsRefPtrHashtable<ModuleMapKey, LoadingRequest> mFetchingModules;
+  nsRefPtrHashtable<ModuleMapKey, ModuleScript> mFetchedModules;
 
   // List of dynamic imports that are currently being loaded.
   ScriptLoadRequestList mDynamicImportRequests;

Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ NS_IMPL_CYCLE_COLLECTION_TRACE_END`
`52`	`52`	`/* static */`
`53`	`53`	`VisitedURLSet* ModuleLoadRequest::NewVisitedSetForTopLevelImport(nsIURI* aURI) {`
`54`	`54`	`auto set = new VisitedURLSet();`
`55`		`- set->PutEntry(aURI);`
	`55`	`+ set->PutEntry(ModuleMapKey(aURI, ModuleType::JavaScript));`
`56`	`56`	`return set;`
`57`	`57`	`}`
`58`	`58`