PM: Keep track of frame->worker connection counts.

This allows the WorkerWatcher to cope with the case where there are
multiple Frame<->Worker relationships in existence at the same time
for a single Frame, Worker pair. Multiple relationships are
represented as a single edge in the graph, which is added/removed
on 0<->1 count transitions.

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

components/performance_manager/worker_watcher.cc

@@ -80,6 +80,18 @@ void DisconnectClientsOnGraph(base::flat_set<WorkerNodeImpl*> worker_nodes,
                      std::move(worker_nodes), client_frame_node));
 }
 
+void DisconnectClientsOnGraph(
+    base::flat_map<WorkerNodeImpl*, size_t> worker_node_connections,
+    FrameNodeImpl* client_frame_node) {
+  base::flat_set<WorkerNodeImpl*>::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),
+      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,
@@ -128,7 +140,7 @@ WorkerWatcher::WorkerWatcher(
 
 WorkerWatcher::~WorkerWatcher() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(frame_node_child_workers_.empty());
+  DCHECK(frame_node_child_worker_connections_.empty());
   DCHECK(dedicated_worker_nodes_.empty());
   DCHECK(!dedicated_worker_service_observation_.IsObserving());
   DCHECK(shared_worker_nodes_.empty());
@@ -141,10 +153,10 @@ void WorkerWatcher::TearDown() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // First clear client-child connections between frames and workers.
-  for (auto& kv : frame_node_child_workers_) {
+  for (auto& kv : frame_node_child_worker_connections_) {
     const content::GlobalFrameRoutingId& render_frame_host_id = kv.first;
-    base::flat_set<WorkerNodeImpl*>& child_workers = kv.second;
-    DCHECK(!child_workers.empty());
+    WorkerNodeConnections& child_worker_connections = kv.second;
+    DCHECK(!child_worker_connections.empty());
 
     frame_node_source_->UnsubscribeFromFrameNode(render_frame_host_id);
 
@@ -152,9 +164,9 @@ void WorkerWatcher::TearDown() {
     FrameNodeImpl* frame_node =
         frame_node_source_->GetFrameNode(render_frame_host_id);
     DCHECK(frame_node);
-    DisconnectClientsOnGraph(std::move(child_workers), frame_node);
+    DisconnectClientsOnGraph(std::move(child_worker_connections), frame_node);
   }
-  frame_node_child_workers_.clear();
+  frame_node_child_worker_connections_.clear();
 
   // Then clear client-child connections for dedicated workers.
   for (auto& kv : dedicated_worker_child_workers_) {
@@ -224,7 +236,7 @@ void WorkerWatcher::OnWorkerCreated(
       dedicated_worker_token, std::move(worker_node));
   DCHECK(insertion_result.second);
 
-  ConnectFrameClient(insertion_result.first->second.get(),
+  AddFrameClientConnection(insertion_result.first->second.get(),
                            ancestor_render_frame_host_id);
 }
 
@@ -239,7 +251,7 @@ void WorkerWatcher::OnBeforeWorkerDestroyed(
   auto worker_node = std::move(it->second);
 
   // First disconnect the ancestor's frame node from this worker node.
-  DisconnectFrameClient(worker_node.get(), ancestor_render_frame_host_id);
+  RemoveFrameClientConnection(worker_node.get(), ancestor_render_frame_host_id);
 
 #if DCHECK_IS_ON()
   DCHECK(!base::Contains(detached_frame_count_per_worker_, worker_node.get()));
@@ -311,7 +323,7 @@ void WorkerWatcher::OnClientAdded(
     content::GlobalFrameRoutingId render_frame_host_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  ConnectFrameClient(GetSharedWorkerNode(shared_worker_token),
+  AddFrameClientConnection(GetSharedWorkerNode(shared_worker_token),
                            render_frame_host_id);
 }
 
@@ -320,7 +332,7 @@ void WorkerWatcher::OnClientRemoved(
     content::GlobalFrameRoutingId render_frame_host_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  DisconnectFrameClient(GetSharedWorkerNode(shared_worker_token),
+  RemoveFrameClientConnection(GetSharedWorkerNode(shared_worker_token),
                               render_frame_host_id);
 }
 
@@ -460,7 +472,7 @@ void WorkerWatcher::OnControlleeRemoved(int64_t version_id,
 
   switch (client.type()) {
     case blink::mojom::ServiceWorkerClientType::kWindow:
-      DisconnectFrameClient(worker_node, client.GetRenderFrameHostId());
+      RemoveFrameClientConnection(worker_node, client.GetRenderFrameHostId());
       break;
     case blink::mojom::ServiceWorkerClientType::kDedicatedWorker:
       DisconnectDedicatedWorkerClient(worker_node,
@@ -494,10 +506,10 @@ void WorkerWatcher::OnControlleeNavigationCommitted(
 
   WorkerNodeImpl* service_worker_node = GetServiceWorkerNode(version_id);
   if (service_worker_node)
-    ConnectFrameClient(service_worker_node, render_frame_host_id);
+    AddFrameClientConnection(service_worker_node, render_frame_host_id);
 }
 
-void WorkerWatcher::ConnectFrameClient(
+void WorkerWatcher::AddFrameClientConnection(
     WorkerNodeImpl* worker_node,
     content::GlobalFrameRoutingId client_render_frame_host_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -511,8 +523,8 @@ void WorkerWatcher::ConnectFrameClient(
   if (!frame_node) {
     RecordWorkerClientFound(false);
 #if DCHECK_IS_ON()
-    // A call to DisconnectFrameClient() is still expected to be received for
-    // this worker and frame pair.
+    // A call to RemoveFrameClientConnection() is still expected to be received
+    // for this worker and frame pair.
     detached_frame_count_per_worker_[worker_node]++;
 #endif  // DCHECK_IS_ON()
     return;
@@ -520,18 +532,28 @@ void WorkerWatcher::ConnectFrameClient(
 
   RecordWorkerClientFound(true);
 
-  ConnectClientFrameOnGraph(worker_node, frame_node);
-
   // Keep track of the workers that this frame is a client to.
-  if (AddChildWorker(client_render_frame_host_id, worker_node)) {
+  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);
+
+  if (is_first_child_worker) {
     frame_node_source_->SubscribeToFrameNode(
         client_render_frame_host_id,
         base::BindOnce(&WorkerWatcher::OnBeforeFrameNodeRemoved,
                        base::Unretained(this), client_render_frame_host_id));
   }
+
+  if (is_first_child_worker_connection) {
+    // Connect the nodes on the graph only on the 0->1 transition.
+    ConnectClientFrameOnGraph(worker_node, frame_node);
+  }
 }
 
-void WorkerWatcher::DisconnectFrameClient(
+void WorkerWatcher::RemoveFrameClientConnection(
     WorkerNodeImpl* worker_node,
     content::GlobalFrameRoutingId client_render_frame_host_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -550,8 +572,8 @@ void WorkerWatcher::DisconnectFrameClient(
   if (!frame_node) {
 #if DCHECK_IS_ON()
     // These debug only checks are used to ensure that this
-    // DisconnectFrameClient() call was still expected even though the client
-    // frame node no longer exist.
+    // RemoveFrameClientConnection() call was still expected even though the
+    // client frame node no longer exist.
     auto it = detached_frame_count_per_worker_.find(worker_node);
     DCHECK(it != detached_frame_count_per_worker_.end());
 
@@ -564,13 +586,21 @@ void WorkerWatcher::DisconnectFrameClient(
 #endif  // DCHECK_IS_ON()
     return;
   }
-
-  DisconnectClientFrameOnGraph(worker_node, frame_node);
-
   // Remove |worker_node| from the set of workers that this frame is a client
   // of.
-  if (RemoveChildWorker(client_render_frame_host_id, worker_node))
+  bool was_last_child_worker = false;
+  bool was_last_child_worker_connection = false;
+  RemoveChildWorkerConnection(client_render_frame_host_id, worker_node,
+                              &was_last_child_worker,
+                              &was_last_child_worker_connection);
+
+  if (was_last_child_worker)
     frame_node_source_->UnsubscribeFromFrameNode(client_render_frame_host_id);
+
+  if (was_last_child_worker_connection) {
+    // Disconnect the nodes on the graph only on the 1->0 transition.
+    DisconnectClientFrameOnGraph(worker_node, frame_node);
+  }
 }
 
 void WorkerWatcher::ConnectDedicatedWorkerClient(
@@ -668,7 +698,8 @@ void WorkerWatcher::ConnectAllServiceWorkerClients(
 
     switch (client.type()) {
       case blink::mojom::ServiceWorkerClientType::kWindow:
-        ConnectFrameClient(service_worker_node, client.GetRenderFrameHostId());
+        AddFrameClientConnection(service_worker_node,
+                                 client.GetRenderFrameHostId());
         break;
       case blink::mojom::ServiceWorkerClientType::kDedicatedWorker:
         ConnectDedicatedWorkerClient(service_worker_node,
@@ -698,7 +729,7 @@ void WorkerWatcher::DisconnectAllServiceWorkerClients(
 
     switch (client.type()) {
       case blink::mojom::ServiceWorkerClientType::kWindow:
-        DisconnectFrameClient(service_worker_node,
+        RemoveFrameClientConnection(service_worker_node,
                                     client.GetRenderFrameHostId());
         break;
       case blink::mojom::ServiceWorkerClientType::kDedicatedWorker:
@@ -721,60 +752,69 @@ void WorkerWatcher::OnBeforeFrameNodeRemoved(
     FrameNodeImpl* frame_node) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  auto it = frame_node_child_workers_.find(render_frame_host_id);
-  DCHECK(it != frame_node_child_workers_.end());
+  auto it = frame_node_child_worker_connections_.find(render_frame_host_id);
+  DCHECK(it != frame_node_child_worker_connections_.end());
 
   // Clean up all child workers of this frame node.
-  base::flat_set<WorkerNodeImpl*> child_workers = std::move(it->second);
-  frame_node_child_workers_.erase(it);
+  WorkerNodeConnections child_worker_connections = std::move(it->second);
+  frame_node_child_worker_connections_.erase(it);
 
   // Disconnect all child workers from |frame_node|.
-  DCHECK(!child_workers.empty());
-  DisconnectClientsOnGraph(child_workers, frame_node);
+  DCHECK(!child_worker_connections.empty());
+  DisconnectClientsOnGraph(child_worker_connections, frame_node);
 
 #if DCHECK_IS_ON()
-  for (WorkerNodeImpl* worker_node : child_workers) {
-    // A call to DisconnectFrameClient() is still expected to be received for
-    // this frame to all workers in |child_workers|.
+  for (auto kv : child_worker_connections) {
+    // A call to RemoveFrameClientConnection() is still expected to be received
+    // for this frame for each connection workers in |child_worker_connections|.
     // Note: the [] operator is intentionally used to default initialize the
     // count to zero if needed.
-    detached_frame_count_per_worker_[worker_node]++;
+    detached_frame_count_per_worker_[kv.first] += kv.second;
   }
 #endif  // DCHECK_IS_ON()
 }
 
-bool WorkerWatcher::AddChildWorker(
+void WorkerWatcher::AddChildWorkerConnection(
     content::GlobalFrameRoutingId render_frame_host_id,
-    WorkerNodeImpl* child_worker_node) {
+    WorkerNodeImpl* child_worker_node,
+    bool* is_first_child_worker,
+    bool* is_first_child_worker_connection) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   auto insertion_result =
-      frame_node_child_workers_.insert({render_frame_host_id, {}});
-
-  auto& child_workers = insertion_result.first->second;
-  bool inserted = child_workers.insert(child_worker_node).second;
-  DCHECK(inserted);
+      frame_node_child_worker_connections_.insert({render_frame_host_id, {}});
+  *is_first_child_worker = insertion_result.second;
 
-  return insertion_result.second;
+  auto& child_worker_connections = insertion_result.first->second;
+  const size_t count = ++child_worker_connections[child_worker_node];
+  DCHECK_LE(0u, count);
+  *is_first_child_worker_connection = count == 1;
 }
 
-bool WorkerWatcher::RemoveChildWorker(
+void WorkerWatcher::RemoveChildWorkerConnection(
     content::GlobalFrameRoutingId render_frame_host_id,
-    WorkerNodeImpl* child_worker_node) {
+    WorkerNodeImpl* child_worker_node,
+    bool* was_last_child_worker,
+    bool* was_last_child_worker_connection) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  auto it = frame_node_child_workers_.find(render_frame_host_id);
-  DCHECK(it != frame_node_child_workers_.end());
-  auto& child_workers = it->second;
+  auto it = frame_node_child_worker_connections_.find(render_frame_host_id);
+  DCHECK(it != frame_node_child_worker_connections_.end());
+  auto& child_worker_connections = it->second;
+
+  DCHECK_LE(1u, child_worker_connections[child_worker_node]);
+  const size_t count = --child_worker_connections[child_worker_node];
+  *was_last_child_worker_connection = count == 0;
 
-  size_t removed = child_workers.erase(child_worker_node);
+  if (count == 0) {
+    const size_t removed = child_worker_connections.erase(child_worker_node);
     DCHECK_EQ(removed, 1u);
+  }
 
-  if (child_workers.empty()) {
-    frame_node_child_workers_.erase(it);
-    return true;
+  *was_last_child_worker = child_worker_connections.empty();
+  if (child_worker_connections.empty()) {
+    frame_node_child_worker_connections_.erase(it);
   }
-  return false;
 }
 
 WorkerNodeImpl* WorkerWatcher::GetDedicatedWorkerNode(

components/performance_manager/worker_watcher.h

@@ -118,12 +118,16 @@ class WorkerWatcher : public content::DedicatedWorkerService::Observer,
  private:
   friend class WorkerWatcherTest;
 
-  // Posts a task to the PM graph to connect/disconnect |worker_node| with the
-  // frame node associated to |client_render_frame_host_id|.
-  void ConnectFrameClient(
+  // Adds a connection between |worker_node| and the frame node represented by
+  // |client_render_frame_host_id|. Connects them in the graph when the first
+  // connection is added.
+  void AddFrameClientConnection(
       WorkerNodeImpl* worker_node,
       content::GlobalFrameRoutingId client_render_frame_host_id);
-  void DisconnectFrameClient(
+  // Removes a connection between |worker_node| and the frame node represented
+  // by |client_render_frame_host_id|. Disconnects them in the graph when the
+  // last connection is removed.
+  void RemoveFrameClientConnection(
       WorkerNodeImpl* worker_node,
       content::GlobalFrameRoutingId client_render_frame_host_id);
 
@@ -157,12 +161,24 @@ class WorkerWatcher : public content::DedicatedWorkerService::Observer,
       content::GlobalFrameRoutingId render_frame_host_id,
       FrameNodeImpl* frame_node);
 
-  // Inserts/removes |child_worker_node| into the set of child workers of a
-  // frame. Returns true if this is the first child added to that frame.
-  bool AddChildWorker(content::GlobalFrameRoutingId render_frame_host_id,
-                      WorkerNodeImpl* child_worker_node);
-  bool RemoveChildWorker(content::GlobalFrameRoutingId render_frame_host_id,
-                         WorkerNodeImpl* child_worker_node);
+  // Adds/removes a connection to |child_worker_node| in the set of child
+  // workers of a frame.
+  // On exit |is_first_child_worker| is true if this is the first child worker
+  // added to the frame and |is_first_child_worker_connection| is true if
+  // this was the first connection from the frame and |child_worker_node|.
+  // Conversely |was_last_child_worker| is true if this was the last client
+  // worker removed, and |was_last_child_worker_connection| is true if this
+  // removed the last connection between the frame and |child_worker_node|.
+  void AddChildWorkerConnection(
+      content::GlobalFrameRoutingId render_frame_host_id,
+      WorkerNodeImpl* child_worker_node,
+      bool* is_first_child_worker,
+      bool* is_first_child_worker_connection);
+  void RemoveChildWorkerConnection(
+      content::GlobalFrameRoutingId render_frame_host_id,
+      WorkerNodeImpl* child_worker_node,
+      bool* was_last_child_worker,
+      bool* was_last_child_worker_connection);
 
   // Helper functions to retrieve an existing worker node.
   WorkerNodeImpl* GetDedicatedWorkerNode(
@@ -221,12 +237,14 @@ class WorkerWatcher : public content::DedicatedWorkerService::Observer,
       base::flat_map<std::string /*client_uuid*/, ServiceWorkerClient>>
       service_worker_clients_;
 
-  // Maps each frame to the workers that this frame is a client of in the graph.
-  // This is used when a frame is torn down before the
-  // OnBeforeWorkerTerminated() is received, to ensure the deletion of the
-  // worker nodes in the right order (workers before frames).
-  base::flat_map<content::GlobalFrameRoutingId, base::flat_set<WorkerNodeImpl*>>
-      frame_node_child_workers_;
+  // Maps each frame to the number of connections to each worker that this frame
+  // is a client of in the graph. Note that normally there's a single connection
+  // from a frame to a worker, except in rare circumstances where it appears
+  // that a single frame can have multiple "controllee" relationships to the
+  // same service worker. This is represented as a single edge in the PM graph.
+  using WorkerNodeConnections = base::flat_map<WorkerNodeImpl*, size_t>;
+  base::flat_map<content::GlobalFrameRoutingId, WorkerNodeConnections>
+      frame_node_child_worker_connections_;
 
   // Maps each dedicated worker to all its child workers.
   base::flat_map<blink::DedicatedWorkerToken, base::flat_set<WorkerNodeImpl*>>

components/performance_manager/worker_watcher_unittest.cc

@@ -979,6 +979,104 @@ TEST_F(WorkerWatcherTest, ServiceWorkerFrameClientOfTwoWorkers) {
       second_service_worker_version_id);
 }
 
+// Ensures that the WorkerWatcher handles the case where a frame with a service
+// worker has a double client relationship with a service worker.
+// This appears to be happening out in the real world, if quite rarely.
+// See https://crbug.com/1143281#c33.
+TEST_F(WorkerWatcherTest, ServiceWorkerTwoFrameClientRelationships) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      features::kServiceWorkerRelationshipsInGraph);
+
+  int render_process_id = process_node_source()->CreateProcessNode();
+
+  // Create and start a service worker.
+  int64_t service_worker_version_id =
+      service_worker_context()->CreateServiceWorker();
+  service_worker_context()->StartServiceWorker(service_worker_version_id,
+                                               render_process_id);
+
+  // Add a frame tree node as a client of a service worker.
+  int frame_tree_node_id = GenerateNextId();
+  std::string first_client_uuid = service_worker_context()->AddClient(
+      service_worker_version_id,
+      content::ServiceWorkerClientInfo(frame_tree_node_id));
+
+  // Check expectations on the graph.
+  CallOnGraphAndWait(base::BindLambdaForTesting(
+      [process_node = process_node_source()->GetProcessNode(render_process_id),
+       worker_node =
+           GetServiceWorkerNode(service_worker_version_id)](GraphImpl* graph) {
+        EXPECT_TRUE(graph->NodeInGraph(worker_node));
+        EXPECT_EQ(worker_node->worker_type(), WorkerNode::WorkerType::kService);
+        // The frame was not yet added as a client.
+        EXPECT_TRUE(worker_node->client_frames().empty());
+      }));
+
+  // Add a second client relationship between the same two entities.
+  std::string second_client_uuid = service_worker_context()->AddClient(
+      service_worker_version_id,
+      content::ServiceWorkerClientInfo(frame_tree_node_id));
+
+  // Now simulate the navigation commit.
+  content::GlobalFrameRoutingId render_frame_host_id =
+      frame_node_source()->CreateFrameNode(
+          render_process_id,
+          process_node_source()->GetProcessNode(render_process_id));
+  service_worker_context()->OnControlleeNavigationCommitted(
+      service_worker_version_id, first_client_uuid, render_frame_host_id);
+
+  CallOnGraphAndWait(base::BindLambdaForTesting(
+      [process_node = process_node_source()->GetProcessNode(render_process_id),
+       service_worker_node = GetServiceWorkerNode(service_worker_version_id),
+       client_frame_node = frame_node_source()->GetFrameNode(
+           render_frame_host_id)](GraphImpl* graph) {
+        EXPECT_TRUE(graph->NodeInGraph(service_worker_node));
+        EXPECT_EQ(service_worker_node->worker_type(),
+                  WorkerNode::WorkerType::kService);
+        EXPECT_EQ(1u, service_worker_node->client_frames().size());
+        EXPECT_TRUE(IsWorkerClient(service_worker_node, client_frame_node));
+      }));
+
+  // Commit the second controllee navigation.
+  service_worker_context()->OnControlleeNavigationCommitted(
+      service_worker_version_id, second_client_uuid, render_frame_host_id);
+  // Verify that the graph is still the same.
+  CallOnGraphAndWait(base::BindLambdaForTesting(
+      [process_node = process_node_source()->GetProcessNode(render_process_id),
+       service_worker_node = GetServiceWorkerNode(service_worker_version_id),
+       client_frame_node = frame_node_source()->GetFrameNode(
+           render_frame_host_id)](GraphImpl* graph) {
+        EXPECT_TRUE(graph->NodeInGraph(service_worker_node));
+        EXPECT_EQ(service_worker_node->worker_type(),
+                  WorkerNode::WorkerType::kService);
+        EXPECT_EQ(1u, service_worker_node->client_frames().size());
+        EXPECT_TRUE(IsWorkerClient(service_worker_node, client_frame_node));
+      }));
+
+  // Remove the first client relationship.
+  service_worker_context()->RemoveClient(service_worker_version_id,
+                                         first_client_uuid);
+  // Verify that the graph is still the same.
+  CallOnGraphAndWait(base::BindLambdaForTesting(
+      [process_node = process_node_source()->GetProcessNode(render_process_id),
+       service_worker_node = GetServiceWorkerNode(service_worker_version_id),
+       client_frame_node = frame_node_source()->GetFrameNode(
+           render_frame_host_id)](GraphImpl* graph) {
+        EXPECT_TRUE(graph->NodeInGraph(service_worker_node));
+        EXPECT_EQ(service_worker_node->worker_type(),
+                  WorkerNode::WorkerType::kService);
+        EXPECT_EQ(1u, service_worker_node->client_frames().size());
+        EXPECT_TRUE(IsWorkerClient(service_worker_node, client_frame_node));
+      }));
+
+  // Teardown.
+  service_worker_context()->RemoveClient(service_worker_version_id,
+                                         second_client_uuid);
+  service_worker_context()->StopServiceWorker(service_worker_version_id);
+  service_worker_context()->DestroyServiceWorker(service_worker_version_id);
+}
+
 // Ensures that the WorkerWatcher handles the case where a frame with a service
 // worker is created but it's navigation is never committed before the
 // FrameTreeNode is destroyed.