PM: Cope with MIA service worker (worker) clients.

It appears that dead shared and/or dedicated worker clients of
a shared worker may have outstanding client registrations as the
service worker starts.

Bug: 1168113
Change-Id: Ie19f3fb90c9252502a29eadfa1bec52acd6e5f64
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2637256
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Patrick Monette <pmonette@chromium.org>
Auto-Submit: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845396}

components/performance_manager/service_worker_client.cc

@@ -40,3 +40,20 @@ blink::SharedWorkerToken ServiceWorkerClient::GetSharedWorkerToken() const {
   DCHECK_EQ(type_, blink::mojom::ServiceWorkerClientType::kSharedWorker);
   return worker_token_.GetAs<blink::SharedWorkerToken>();
 }
+
+bool ServiceWorkerClient::operator<(const ServiceWorkerClient& o) const {
+  if (type() == o.type()) {
+    switch (type()) {
+      case blink::mojom::ServiceWorkerClientType::kWindow:
+        return GetRenderFrameHostId() < o.GetRenderFrameHostId();
+      case blink::mojom::ServiceWorkerClientType::kDedicatedWorker:
+        return GetDedicatedWorkerToken() < o.GetDedicatedWorkerToken();
+      case blink::mojom::ServiceWorkerClientType::kSharedWorker:
+        return GetSharedWorkerToken() < o.GetSharedWorkerToken();
+      case blink::mojom::ServiceWorkerClientType::kAll:
+        NOTREACHED();
+        return false;
+    }
+  }
+  return type() < o.type();
+}

components/performance_manager/service_worker_client.h

@@ -33,6 +33,8 @@ class ServiceWorkerClient {
   blink::DedicatedWorkerToken GetDedicatedWorkerToken() const;
   blink::SharedWorkerToken GetSharedWorkerToken() const;
 
+  bool operator<(const ServiceWorkerClient& other) const;
+
  private:
   // The client type.
   blink::mojom::ServiceWorkerClientType type_;

components/performance_manager/worker_watcher.cc

@@ -18,6 +18,8 @@
 
 namespace performance_manager {
 
+using WorkerNodeSet = base::flat_set<WorkerNodeImpl*>;
+
 namespace {
 
 // Emits a boolean value that indicates if the client frame's node was found
@@ -68,12 +70,12 @@ void DisconnectClientWorkerOnGraph(WorkerNodeImpl* worker_node,
 
 // Helper function to remove |client_frame_node| as a client of all worker nodes
 // in |worker_nodes| on the PM sequence.
-void DisconnectClientsOnGraph(base::flat_set<WorkerNodeImpl*> worker_nodes,
+void DisconnectClientsOnGraph(WorkerNodeSet worker_nodes,
                               FrameNodeImpl* client_frame_node) {
   PerformanceManagerImpl::CallOnGraphImpl(
-      FROM_HERE, base::BindOnce(
-                     [](base::flat_set<WorkerNodeImpl*> worker_nodes,
-                        FrameNodeImpl* client_frame_node) {
+      FROM_HERE,
+      base::BindOnce(
+          [](WorkerNodeSet worker_nodes, FrameNodeImpl* client_frame_node) {
             for (auto* worker_node : worker_nodes)
               worker_node->RemoveClientFrame(client_frame_node);
           },
@@ -83,23 +85,22 @@ void DisconnectClientsOnGraph(base::flat_set<WorkerNodeImpl*> worker_nodes,
 void DisconnectClientsOnGraph(
     base::flat_map<WorkerNodeImpl*, size_t> worker_node_connections,
     FrameNodeImpl* client_frame_node) {
-  base::flat_set<WorkerNodeImpl*>::container_type client_workers;
+  WorkerNodeSet::container_type client_workers;
   for (auto& kv : worker_node_connections)
     client_workers.push_back(kv.first);
 
-  DisconnectClientsOnGraph(
-      base::flat_set<WorkerNodeImpl*>(base::sorted_unique, client_workers),
+  DisconnectClientsOnGraph(WorkerNodeSet(base::sorted_unique, client_workers),
                            client_frame_node);
 }
 
 // Helper function to remove |client_worker_node| as a client of all worker
 // nodes in |worker_nodes| on the PM sequence.
-void DisconnectClientsOnGraph(base::flat_set<WorkerNodeImpl*> worker_nodes,
+void DisconnectClientsOnGraph(WorkerNodeSet worker_nodes,
                               WorkerNodeImpl* client_worker_node) {
   PerformanceManagerImpl::CallOnGraphImpl(
-      FROM_HERE, base::BindOnce(
-                     [](base::flat_set<WorkerNodeImpl*> worker_nodes,
-                        WorkerNodeImpl* client_worker_node) {
+      FROM_HERE,
+      base::BindOnce(
+          [](WorkerNodeSet worker_nodes, WorkerNodeImpl* client_worker_node) {
             for (auto* worker_node : worker_nodes)
               worker_node->RemoveClientWorker(client_worker_node);
           },
@@ -171,7 +172,7 @@ void WorkerWatcher::TearDown() {
   // Then clear client-child connections for dedicated workers.
   for (auto& kv : dedicated_worker_child_workers_) {
     const blink::DedicatedWorkerToken& dedicated_worker_token = kv.first;
-    base::flat_set<WorkerNodeImpl*>& child_workers = kv.second;
+    WorkerNodeSet& child_workers = kv.second;
     DCHECK(!child_workers.empty());
 
     // Disconnect all child workers from |dedicated_worker_token|.
@@ -184,7 +185,7 @@ void WorkerWatcher::TearDown() {
   // Finally, clear client-child connections for shared workers.
   for (auto& kv : shared_worker_child_workers_) {
     const blink::SharedWorkerToken& shared_worker_token = kv.first;
-    base::flat_set<WorkerNodeImpl*>& child_workers = kv.second;
+    WorkerNodeSet& child_workers = kv.second;
     DCHECK(!child_workers.empty());
 
     // Disconnect all child workers from |shared_worker_token|.
@@ -298,7 +299,20 @@ void WorkerWatcher::OnBeforeWorkerDestroyed(
   // Disconnect all child workers before destroying the node.
   auto child_it = shared_worker_child_workers_.find(shared_worker_token);
   if (child_it != shared_worker_child_workers_.end()) {
-    DisconnectClientsOnGraph(child_it->second, worker_node.get());
+    const WorkerNodeSet& child_workers = child_it->second;
+    DisconnectClientsOnGraph(child_workers, worker_node.get());
+
+#if DCHECK_IS_ON()
+    for (WorkerNodeImpl* worker : child_workers) {
+      // If this is a service worker client, mark it as a missing client.
+      if (IsServiceWorkerNode(worker)) {
+        DCHECK(missing_service_worker_clients_[worker]
+                   .insert(ServiceWorkerClient(shared_worker_token))
+                   .second);
+      }
+    }
+#endif
+
     shared_worker_child_workers_.erase(child_it);
   }
 
@@ -535,7 +549,6 @@ void WorkerWatcher::AddFrameClientConnection(
   // Keep track of the workers that this frame is a client to.
   bool is_first_child_worker = false;
   bool is_first_child_worker_connection = false;
-  ;
   AddChildWorkerConnection(client_render_frame_host_id, worker_node,
                            &is_first_child_worker,
                            &is_first_child_worker_connection);
@@ -608,8 +621,20 @@ void WorkerWatcher::ConnectDedicatedWorkerClient(
     blink::DedicatedWorkerToken client_dedicated_worker_token) {
   DCHECK(worker_node);
 
-  ConnectClientWorkerOnGraph(
-      worker_node, GetDedicatedWorkerNode(client_dedicated_worker_token));
+  WorkerNodeImpl* client_dedicated_worker_node =
+      GetDedicatedWorkerNode(client_dedicated_worker_token);
+  if (!client_dedicated_worker_node) {
+#if DCHECK_IS_ON()
+    bool inserted =
+        missing_service_worker_clients_[worker_node]
+            .insert(ServiceWorkerClient(client_dedicated_worker_token))
+            .second;
+    DCHECK(inserted);
+#endif
+    return;
+  }
+
+  ConnectClientWorkerOnGraph(worker_node, client_dedicated_worker_node);
 
   // Remember that |worker_node| is a child worker of this dedicated worker.
   bool inserted = dedicated_worker_child_workers_[client_dedicated_worker_token]
@@ -623,6 +648,20 @@ void WorkerWatcher::DisconnectDedicatedWorkerClient(
     blink::DedicatedWorkerToken client_dedicated_worker_token) {
   DCHECK(worker_node);
 
+  WorkerNodeImpl* client_dedicated_worker =
+      GetDedicatedWorkerNode(client_dedicated_worker_token);
+  if (!client_dedicated_worker) {
+#if DCHECK_IS_ON()
+    auto it = missing_service_worker_clients_.find(worker_node);
+    DCHECK(it != missing_service_worker_clients_.end());
+    DCHECK_EQ(1u, it->second.erase(
+                      ServiceWorkerClient(client_dedicated_worker_token)));
+    if (it->second.empty())
+      missing_service_worker_clients_.erase(it);
+#endif
+    return;
+  }
+
   // Remove |worker_node| from the set of child workers of this dedicated
   // worker.
   auto it = dedicated_worker_child_workers_.find(client_dedicated_worker_token);
@@ -635,8 +674,7 @@ void WorkerWatcher::DisconnectDedicatedWorkerClient(
   if (child_workers.empty())
     dedicated_worker_child_workers_.erase(it);
 
-  DisconnectClientWorkerOnGraph(
-      worker_node, GetDedicatedWorkerNode(client_dedicated_worker_token));
+  DisconnectClientWorkerOnGraph(worker_node, client_dedicated_worker);
 }
 
 void WorkerWatcher::ConnectSharedWorkerClient(
@@ -644,8 +682,19 @@ void WorkerWatcher::ConnectSharedWorkerClient(
     blink::SharedWorkerToken client_shared_worker_token) {
   DCHECK(worker_node);
 
-  ConnectClientWorkerOnGraph(worker_node,
-                             GetSharedWorkerNode(client_shared_worker_token));
+  WorkerNodeImpl* client_shared_worker_node =
+      GetSharedWorkerNode(client_shared_worker_token);
+  if (!client_shared_worker_node) {
+#if DCHECK_IS_ON()
+    bool inserted = missing_service_worker_clients_[worker_node]
+                        .insert(ServiceWorkerClient(client_shared_worker_token))
+                        .second;
+    DCHECK(inserted);
+#endif
+    return;
+  }
+
+  ConnectClientWorkerOnGraph(worker_node, client_shared_worker_node);
 
   // Remember that |worker_node| is a child worker of this shared worker.
   bool inserted = shared_worker_child_workers_[client_shared_worker_token]
@@ -667,6 +716,14 @@ void WorkerWatcher::DisconnectSharedWorkerClient(
     DCHECK(shared_worker_child_workers_.find(client_shared_worker_token) ==
            shared_worker_child_workers_.end());
 
+#if DCHECK_IS_ON()
+    auto it = missing_service_worker_clients_.find(worker_node);
+    DCHECK(it != missing_service_worker_clients_.end());
+    DCHECK_EQ(
+        1u, it->second.erase(ServiceWorkerClient(client_shared_worker_token)));
+    if (it->second.empty())
+      missing_service_worker_clients_.erase(it);
+#endif
     return;
   }
 
@@ -822,10 +879,9 @@ WorkerNodeImpl* WorkerWatcher::GetDedicatedWorkerNode(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   auto it = dedicated_worker_nodes_.find(dedicated_worker_token);
-  if (it == dedicated_worker_nodes_.end()) {
-    NOTREACHED();
+  if (it == dedicated_worker_nodes_.end())
     return nullptr;
-  }
+
   return it->second.get();
 }
 
@@ -834,10 +890,9 @@ WorkerNodeImpl* WorkerWatcher::GetSharedWorkerNode(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   auto it = shared_worker_nodes_.find(shared_worker_token);
-  if (it == shared_worker_nodes_.end()) {
-    NOTREACHED();
+  if (it == shared_worker_nodes_.end())
     return nullptr;
-  }
+
   return it->second.get();
 }
 
@@ -845,10 +900,21 @@ WorkerNodeImpl* WorkerWatcher::GetServiceWorkerNode(int64_t version_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   auto it = service_worker_nodes_.find(version_id);
-  if (it == service_worker_nodes_.end()) {
+  if (it == service_worker_nodes_.end())
     return nullptr;
-  }
+
   return it->second.get();
 }
 
+#if DCHECK_IS_ON()
+bool WorkerWatcher::IsServiceWorkerNode(WorkerNodeImpl* worker_node) {
+  for (const auto& kv : service_worker_nodes_) {
+    if (kv.second.get() == worker_node)
+      return true;
+  }
+
+  return false;
+}
+#endif
+
 }  // namespace performance_manager

components/performance_manager/worker_watcher.h

@@ -131,7 +131,8 @@ class WorkerWatcher : public content::DedicatedWorkerService::Observer,
       WorkerNodeImpl* worker_node,
       content::GlobalFrameRoutingId client_render_frame_host_id);
 
-  // Posts a task to the PM graph to connect/disconnect |worker_node| with the
+  // If a node with |client_dedicated_worker_token| exists, posts a task to
+  // the PM graph to connect/disconnect |worker_node| with the
   // dedicated worker node associated to |client_dedicated_worker_token|.
   void ConnectDedicatedWorkerClient(
       WorkerNodeImpl* worker_node,
@@ -140,8 +141,9 @@ class WorkerWatcher : public content::DedicatedWorkerService::Observer,
       WorkerNodeImpl* worker_node,
       blink::DedicatedWorkerToken client_dedicated_worker_token);
 
-  // Posts a task to the PM graph to connect/disconnect |worker_node| with the
-  // shared worker node associated to |client_shared_worker_id|.
+  // If a node with |client_shared_worker_token| exists, posts a task to
+  // the PM graph to connect/disconnect |worker_node| with the
+  // dedicated worker node associated to |client_shared_worker_token|.
   void ConnectSharedWorkerClient(
       WorkerNodeImpl* worker_node,
       blink::SharedWorkerToken client_shared_worker_token);
@@ -181,12 +183,17 @@ class WorkerWatcher : public content::DedicatedWorkerService::Observer,
       bool* was_last_child_worker_connection);
 
   // Helper functions to retrieve an existing worker node.
+  // Return the requested node, or nullptr if no such node registered.
   WorkerNodeImpl* GetDedicatedWorkerNode(
       const blink::DedicatedWorkerToken& dedicated_worker_token);
   WorkerNodeImpl* GetSharedWorkerNode(
       const blink::SharedWorkerToken& shared_worker_token);
   WorkerNodeImpl* GetServiceWorkerNode(int64_t version_id);
 
+#if DCHECK_IS_ON()
+  bool IsServiceWorkerNode(WorkerNodeImpl* worker_node);
+#endif
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   // The ID of the BrowserContext who owns the shared worker service.
@@ -247,11 +254,12 @@ class WorkerWatcher : public content::DedicatedWorkerService::Observer,
       frame_node_child_worker_connections_;
 
   // Maps each dedicated worker to all its child workers.
-  base::flat_map<blink::DedicatedWorkerToken, base::flat_set<WorkerNodeImpl*>>
+  using WorkerNodeSet = base::flat_set<WorkerNodeImpl*>;
+  base::flat_map<blink::DedicatedWorkerToken, WorkerNodeSet>
       dedicated_worker_child_workers_;
 
   // Maps each shared worker to all its child workers.
-  base::flat_map<blink::SharedWorkerToken, base::flat_set<WorkerNodeImpl*>>
+  base::flat_map<blink::SharedWorkerToken, WorkerNodeSet>
       shared_worker_child_workers_;
 
 #if DCHECK_IS_ON()
@@ -260,6 +268,11 @@ class WorkerWatcher : public content::DedicatedWorkerService::Observer,
   // before OnClientRemoved(), or when it wasn't possible to initially attach
   // a client frame node to a worker.
   base::flat_map<WorkerNodeImpl*, int> detached_frame_count_per_worker_;
+
+  // Keeps track of shared and dedicated workers that were not present when
+  // a service worker tried to add a client relationship for them.
+  base::flat_map<WorkerNodeImpl*, base::flat_set<ServiceWorkerClient>>
+      missing_service_worker_clients_;
 #endif  // DCHECK_IS_ON()
 
   DISALLOW_COPY_AND_ASSIGN(WorkerWatcher);

components/performance_manager/worker_watcher_unittest.cc

@@ -1359,6 +1359,84 @@ TEST_F(WorkerWatcherTest, SharedWorkerCrossProcessClient) {
   shared_worker_service()->DestroySharedWorker(shared_worker_token);
 }
 
+// Tests that the WorkerWatcher can handle the case where the service worker
+// starts after it has been assigned a worker client, but the client has
+// already died by the time the service worker starts.
+TEST_F(WorkerWatcherTest, SharedWorkerStartsWithDeadWorkerClients) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      features::kServiceWorkerRelationshipsInGraph);
+
+  int render_process_id = process_node_source()->CreateProcessNode();
+  content::GlobalFrameRoutingId render_frame_host_id =
+      frame_node_source()->CreateFrameNode(
+          render_process_id,
+          process_node_source()->GetProcessNode(render_process_id));
+
+  // Create the worker.
+  int64_t service_worker_version_id =
+      service_worker_context()->CreateServiceWorker();
+
+  // Create a worker client of each type and connect them to the service worker.
+  // Dedicated worker client.
+  blink::DedicatedWorkerToken dedicated_worker_token =
+      dedicated_worker_service()->CreateDedicatedWorker(render_process_id,
+                                                        render_frame_host_id);
+  std::string dedicated_worker_client_uuid =
+      service_worker_context()->AddClient(
+          service_worker_version_id,
+          content::ServiceWorkerClientInfo(dedicated_worker_token));
+
+  // Shared worker client.
+  blink::SharedWorkerToken shared_worker_token =
+      shared_worker_service()->CreateSharedWorker(render_process_id);
+  std::string shared_worker_client_uuid = service_worker_context()->AddClient(
+      service_worker_version_id,
+      content::ServiceWorkerClientInfo(shared_worker_token));
+
+  // Destroy the workers before the service worker starts.
+  shared_worker_service()->DestroySharedWorker(shared_worker_token);
+  dedicated_worker_service()->DestroyDedicatedWorker(dedicated_worker_token);
+
+  // Now start the service worker.
+  service_worker_context()->StartServiceWorker(service_worker_version_id,
+                                               render_process_id);
+
+  // Check expectations on the graph.
+  CallOnGraphAndWait(base::BindLambdaForTesting(
+      [process_node = process_node_source()->GetProcessNode(render_process_id),
+       service_worker_node =
+           GetServiceWorkerNode(service_worker_version_id)](GraphImpl* graph) {
+        EXPECT_TRUE(graph->NodeInGraph(service_worker_node));
+        EXPECT_EQ(service_worker_node->worker_type(),
+                  WorkerNode::WorkerType::kService);
+        EXPECT_EQ(service_worker_node->process_node(), process_node);
+        EXPECT_TRUE(service_worker_node->child_workers().empty());
+      }));
+
+  // Disconnect the non-existent clients.
+  service_worker_context()->RemoveClient(service_worker_version_id,
+                                         shared_worker_client_uuid);
+  service_worker_context()->RemoveClient(service_worker_version_id,
+                                         dedicated_worker_client_uuid);
+
+  // No changes in the graph.
+  CallOnGraphAndWait(base::BindLambdaForTesting(
+      [process_node = process_node_source()->GetProcessNode(render_process_id),
+       service_worker_node =
+           GetServiceWorkerNode(service_worker_version_id)](GraphImpl* graph) {
+        EXPECT_TRUE(graph->NodeInGraph(service_worker_node));
+        EXPECT_EQ(service_worker_node->worker_type(),
+                  WorkerNode::WorkerType::kService);
+        EXPECT_EQ(service_worker_node->process_node(), process_node);
+        EXPECT_TRUE(service_worker_node->child_workers().empty());
+      }));
+
+  // Stop and destroy the service worker.
+  service_worker_context()->StopServiceWorker(service_worker_version_id);
+  service_worker_context()->DestroyServiceWorker(service_worker_version_id);
+}
+
 TEST_F(WorkerWatcherTest, SharedWorkerDiesAsServiceWorkerClient) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(