Assign a unique ID to each SharedWorkerInstance

This allows to tell apart 2 different shared workers that have
the same url, name and constructor origin. This is possible when
the script url and/or the constructor origin have file: scheme.

Also removed the SharedWorkerInstance::Match() overload that takes
in a SharedWorkerInstance to make it clear we're not comparing 2
instances.

Bug: 1013168
Change-Id: I7a0efbfd71dae283ecfc97f3ae87df69a0fb5695
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1850832Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707131}

components/performance_manager/shared_worker_watcher_unittest.cc

@@ -86,6 +86,9 @@ class TestSharedWorkerService : public content::SharedWorkerService {
  private:
   base::ObserverList<Observer> observer_list_;
 
+  // The ID that the next SharedWorkerInstance will be assigned.
+  int64_t next_shared_worker_instance_id_ = 0;
+
   // Contains the set of clients for each running workers.
   base::flat_map<content::SharedWorkerInstance,
                  base::flat_set<std::pair<int, int>>>
@@ -118,7 +121,8 @@ content::SharedWorkerInstance TestSharedWorkerService::StartSharedWorker(
   // Create a new SharedWorkerInstance and add it to the map.
   GURL worker_url = GenerateWorkerUrl();
   content::SharedWorkerInstance instance(
-      worker_url, "SharedWorker", url::Origin::Create(worker_url), "",
+      next_shared_worker_instance_id_++, worker_url, "SharedWorker",
+      url::Origin::Create(worker_url), "",
       blink::mojom::ContentSecurityPolicyType::kReport,
       network::mojom::IPAddressSpace::kPublic,
       blink::mojom::SharedWorkerCreationContextType::kSecure);

content/browser/devtools/shared_worker_devtools_agent_host.cc

@@ -88,7 +88,9 @@ void SharedWorkerDevToolsAgentHost::DetachSession(DevToolsSession* session) {
 }
 
 bool SharedWorkerDevToolsAgentHost::Matches(SharedWorkerHost* worker_host) {
-  return instance_.Matches(worker_host->instance());
+  return instance_.Matches(worker_host->instance().url(),
+                           worker_host->instance().name(),
+                           worker_host->instance().constructor_origin());
 }
 
 void SharedWorkerDevToolsAgentHost::WorkerReadyForInspection(

content/browser/worker_host/shared_worker_host_unittest.cc

@@ -67,10 +67,10 @@ class SharedWorkerHostTest : public testing::Test {
     blink::mojom::SharedWorkerCreationContextType creation_context_type =
         blink::mojom::SharedWorkerCreationContextType::kSecure;
 
-    SharedWorkerInstance instance(url, name, origin, content_security_policy,
-                                  content_security_policy_type,
-                                  creation_address_space,
-                                  creation_context_type);
+    SharedWorkerInstance instance(
+        service_.next_shared_worker_instance_id_++, url, name, origin,
+        content_security_policy, content_security_policy_type,
+        creation_address_space, creation_context_type);
     auto host = std::make_unique<SharedWorkerHost>(
         &service_, instance, mock_render_process_host_.GetID());
     auto weak_host = host->AsWeakPtr();

content/browser/worker_host/shared_worker_instance_unittest.cc

@@ -21,7 +21,8 @@ class SharedWorkerInstanceTest : public testing::Test {
                                       const std::string& name,
                                       const url::Origin& constructor_origin) {
     return SharedWorkerInstance(
-        script_url, name, constructor_origin, std::string(),
+        next_service_worker_instance_id_++, script_url, name,
+        constructor_origin, std::string(),
         blink::mojom::ContentSecurityPolicyType::kReport,
         network::mojom::IPAddressSpace::kPublic,
         blink::mojom::SharedWorkerCreationContextType::kNonsecure);
@@ -39,6 +40,8 @@ class SharedWorkerInstanceTest : public testing::Test {
   }
 
  private:
+  int64_t next_service_worker_instance_id_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(SharedWorkerInstanceTest);
 };
 
@@ -268,7 +271,7 @@ TEST_F(SharedWorkerInstanceTest, AddressSpace) {
       network::mojom::IPAddressSpace::kPublic};
   for (auto address_space : kAddressSpaces) {
     SharedWorkerInstance instance(
-        GURL("http://example.com/w.js"), "name",
+        0, GURL("http://example.com/w.js"), "name",
         url::Origin::Create(GURL("http://example.com/")), std::string(),
         blink::mojom::ContentSecurityPolicyType::kReport, address_space,
         blink::mojom::SharedWorkerCreationContextType::kNonsecure);
@@ -276,4 +279,30 @@ TEST_F(SharedWorkerInstanceTest, AddressSpace) {
   }
 }
 
+// This test ensures that 2 distinct SharedWorkerInstance using the same file:
+// script URL have different identities and can be ordered.
+TEST_F(SharedWorkerInstanceTest, StrictWeakOrderingFileURLs) {
+  GURL script_url("file://path/to/script.js");
+  std::string name = "name";
+  url::Origin constructor_origin = url::Origin::Create(script_url);
+
+  SharedWorkerInstance instance1 =
+      CreateInstance(script_url, name, constructor_origin);
+  SharedWorkerInstance instance2 =
+      CreateInstance(script_url, name, constructor_origin);
+
+  // file: URLs are treated as opaque. Both instances should not match.
+  EXPECT_FALSE(instance1.Matches(instance2.url(), instance2.name(),
+                                 instance2.constructor_origin()));
+  EXPECT_FALSE(instance2.Matches(instance1.url(), instance1.name(),
+                                 instance1.constructor_origin()));
+
+  // The instances are not equivalent
+  EXPECT_TRUE(instance1 < instance2 || instance2 < instance1);
+
+  // An instance is equivalent to itself.
+  EXPECT_FALSE(instance1 < instance1);
+  EXPECT_FALSE(instance2 < instance2);
+}
+
 }  // namespace content

content/browser/worker_host/shared_worker_service_impl.cc

@@ -142,13 +142,8 @@ void SharedWorkerServiceImpl::ConnectToWorker(
     return;
   }
 
-  SharedWorkerInstance instance(
-      info->url, info->name, constructor_origin, info->content_security_policy,
-      info->content_security_policy_type, info->creation_address_space,
-      creation_context_type);
-
-  SharedWorkerHost* host = FindMatchingSharedWorkerHost(
-      instance.url(), instance.name(), instance.constructor_origin());
+  SharedWorkerHost* host =
+      FindMatchingSharedWorkerHost(info->url, info->name, constructor_origin);
   if (host) {
     // Non-secure contexts cannot connect to secure workers, and secure contexts
     // cannot connect to non-secure workers:
@@ -184,6 +179,11 @@ void SharedWorkerServiceImpl::ConnectToWorker(
       storage_partition_->browser_context(), site_instance->GetSiteURL(),
       /*can_be_default=*/true, &storage_domain, &partition_name, &in_memory);
 
+  SharedWorkerInstance instance(
+      next_shared_worker_instance_id_++, info->url, info->name,
+      constructor_origin, info->content_security_policy,
+      info->content_security_policy_type, info->creation_address_space,
+      creation_context_type);
   CreateWorker(instance, std::move(outside_fetch_client_settings_object),
                std::move(client), client_process_id, frame_id, storage_domain,
                message_port, std::move(blob_url_loader_factory));

content/browser/worker_host/shared_worker_service_impl.h

@@ -142,6 +142,9 @@ class CONTENT_EXPORT SharedWorkerServiceImpl : public SharedWorkerService {
   void ScriptLoadFailed(
       mojo::PendingRemote<blink::mojom::SharedWorkerClient> client);
 
+  // The ID that the next SharedWorkerInstance will be assigned.
+  int64_t next_shared_worker_instance_id_ = 0;
+
   std::set<std::unique_ptr<SharedWorkerHost>, base::UniquePtrComparator>
       worker_hosts_;
 

content/public/browser/shared_worker_instance.cc

@@ -13,6 +13,7 @@
 namespace content {
 
 SharedWorkerInstance::SharedWorkerInstance(
+    int64_t id,
     const GURL& url,
     const std::string& name,
     const url::Origin& constructor_origin,
@@ -20,7 +21,8 @@ SharedWorkerInstance::SharedWorkerInstance(
     blink::mojom::ContentSecurityPolicyType security_policy_type,
     network::mojom::IPAddressSpace creation_address_space,
     blink::mojom::SharedWorkerCreationContextType creation_context_type)
-    : url_(url),
+    : id_(id),
+      url_(url),
       name_(name),
       constructor_origin_(constructor_origin),
       content_security_policy_(content_security_policy),
@@ -58,8 +60,9 @@ bool SharedWorkerInstance::Matches(
   // options's name member, then set worker global scope to that
   // SharedWorkerGlobalScope object."
   if (!constructor_origin_.IsSameOriginWith(constructor_origin) ||
-      url_ != url || name_ != name)
+      url_ != url || name_ != name) {
     return false;
+  }
 
   // TODO(https://crbug.com/794098): file:// URLs should be treated as opaque
   // origins, but not in url::Origin. Therefore, we manually check it here.
@@ -69,17 +72,9 @@ bool SharedWorkerInstance::Matches(
   return true;
 }
 
-bool SharedWorkerInstance::Matches(const SharedWorkerInstance& other) const {
-  return Matches(other.url(), other.name(), other.constructor_origin());
-}
-
 bool operator<(const SharedWorkerInstance& lhs,
                const SharedWorkerInstance& rhs) {
-  if (lhs.Matches(rhs))
-    return false;
-
-  return std::tie(lhs.url(), lhs.name(), lhs.constructor_origin()) <
-         std::tie(rhs.url(), rhs.name(), rhs.constructor_origin());
+  return lhs.id_ < rhs.id_;
 }
 
 }  // namespace content

content/public/browser/shared_worker_instance.h

@@ -16,11 +16,12 @@
 
 namespace content {
 
-// SharedWorkerInstance is a value-type class that is the browser-side
-// representation of one instance of a shared worker.
+// SharedWorkerInstance is the browser-side representation of one instance of a
+// shared worker.
 class CONTENT_EXPORT SharedWorkerInstance {
  public:
   SharedWorkerInstance(
+      int64_t id,
       const GURL& url,
       const std::string& name,
       const url::Origin& constructor_origin,
@@ -41,13 +42,12 @@ class CONTENT_EXPORT SharedWorkerInstance {
   bool Matches(const GURL& url,
                const std::string& name,
                const url::Origin& constructor_origin) const;
-  bool Matches(const SharedWorkerInstance& other) const;
 
   // Accessors.
   const GURL& url() const { return url_; }
-  const std::string name() const { return name_; }
+  const std::string& name() const { return name_; }
   const url::Origin& constructor_origin() const { return constructor_origin_; }
-  const std::string content_security_policy() const {
+  const std::string& content_security_policy() const {
     return content_security_policy_;
   }
   blink::mojom::ContentSecurityPolicyType content_security_policy_type() const {
@@ -61,6 +61,17 @@ class CONTENT_EXPORT SharedWorkerInstance {
   }
 
  private:
+  // Compares SharedWorkerInstances using the |id_|.
+  CONTENT_EXPORT friend bool operator<(const SharedWorkerInstance& lhs,
+                                       const SharedWorkerInstance& rhs);
+
+  // An internal ID that is unique within a storage partition. It is needed to
+  // differentiate 2 SharedWorkerInstance that have the same url, name and
+  // constructor origin but actually represent different workers. This is
+  // possible with a file: |url| or |constructor_origin| since they are treated
+  // as opaque in this class.
+  int64_t id_;
+
   GURL url_;
   std::string name_;
 
@@ -75,9 +86,6 @@ class CONTENT_EXPORT SharedWorkerInstance {
   blink::mojom::SharedWorkerCreationContextType creation_context_type_;
 };
 
-CONTENT_EXPORT bool operator<(const SharedWorkerInstance& lhs,
-                              const SharedWorkerInstance& rhs);
-
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_SHARED_WORKER_INSTANCE_H_