[PM] Wire up openers in graph.

This adds dashed links w/ arrows to the graph, indicating opener/openee
embedder/embeddee relationships. It also makes pinned nodes visually
distinct by changing their stroke color.

BUG=1085129

Change-Id: I0bd75a90c83a735b7d56ff2905d7d3d7b7be6e84
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2227369
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774935}

chrome/browser/resources/discards/graph_doc.js

@@ -316,11 +316,22 @@ class GraphNode {
     return -200;
   }
 
-  /** @return {!Array<number>} */
+  /** @return {!Array<number>} an array of node ids. */
   get linkTargets() {
     return [];
   }
 
+  /**
+   * Dashed links express ownership relationships. An object can own multiple
+   * things, but be owned by exactly one (per relationship type). As such, the
+   * relationship is expressed on the *owned* object. These links are drawn with
+   * an arrow at the beginning of the link, pointing to the owned object.
+   * @return {!Array<number>} an array of node ids.
+   */
+  get dashedLinkTargets() {
+    return [];
+  }
+
   /**
    * Selects a color string from an id.
    * @param {number} id The id the returned color is selected from.
@@ -370,6 +381,14 @@ class PageNode extends GraphNode {
   get manyBodyStrength() {
     return -600;
   }
+
+  /** override */
+  get dashedLinkTargets() {
+    if (this.page.openerFrameId) {
+      return [this.page.openerFrameId];
+    }
+    return [];
+  }
 }
 
 class FrameNode extends GraphNode {
@@ -590,6 +609,12 @@ class Graph {
      */
     this.linkGroup_ = null;
 
+    /**
+     * A selection for the top-level <g> node that contains all dashed edges.
+     * @private {d3.selection}
+     */
+    this.dashedLinkGroup_ = null;
+
     /** @private {!Map<number, !GraphNode>} */
     this.nodes_ = new Map();
 
@@ -599,6 +624,12 @@ class Graph {
      */
     this.links_ = [];
 
+    /**
+     * The dashed links.
+     * @private {!Array<!d3.ForceLink>}
+     */
+    this.dashedLinks_ = [];
+
     /**
      * The host window.
      * @private {Window}
@@ -655,8 +686,9 @@ class Graph {
     // Create the <g> elements that host nodes and links.
     // The link groups are created first so that all links end up behind nodes.
     const svg = d3.select(this.svg_);
-    this.toolTipLinkGroup_ = svg.append('g').attr('class', 'toolTipLinks');
+    this.toolTipLinkGroup_ = svg.append('g').attr('class', 'tool-tip-links');
     this.linkGroup_ = svg.append('g').attr('class', 'links');
+    this.dashedLinkGroup_ = svg.append('g').attr('class', 'dashed-links');
     this.nodeGroup_ = svg.append('g').attr('class', 'nodes');
     this.separatorGroup_ = svg.append('g').attr('class', 'separators');
 
@@ -697,7 +729,11 @@ class Graph {
   /** @override */
   pageChanged(page) {
     const pageNode = /** @type {!PageNode} */ (this.nodes_.get(page.id));
+
+    // Page node dashed links may change dynamically, so account for that here.
+    this.removeDashedNodeLinks_(pageNode);
     pageNode.page = page;
+    this.addDashedNodeLinks_(pageNode);
   }
 
   /** @override */
@@ -732,6 +768,7 @@ class Graph {
 
     // Remove any links, and then the node itself.
     this.removeNodeLinks_(node);
+    this.removeDashedNodeLinks_(node);
     this.nodes_.delete(nodeId);
   }
 
@@ -776,11 +813,21 @@ class Graph {
    * @private
    */
   removeNodeLinks_(node) {
-    // Filter away any links to or from the deleted node.
+    // Filter away any links to or from the provided node.
     this.links_ = this.links_.filter(
         link => link.source !== node && link.target !== node);
   }
 
+  /**
+   * @param {!GraphNode} node
+   * @private
+   */
+  removeDashedNodeLinks_(node) {
+    // Filter away any dashed links to or from the provided node.
+    this.dashedLinks_ = this.dashedLinks_.filter(
+        link => link.source !== node && link.target !== node);
+  }
+
   /**
    * @param {!Object<string>} nodeDescriptions
    * @private
@@ -915,10 +962,18 @@ class Graph {
     // Select the links.
     const link = this.linkGroup_.selectAll('line').data(this.links_);
     // Add new links.
-    link.enter().append('line').attr('stroke-width', 1);
+    link.enter().append('line');
     // Remove dead links.
     link.exit().remove();
 
+    // Select the dashed links.
+    const dashedLink =
+        this.dashedLinkGroup_.selectAll('line').data(this.dashedLinks_);
+    // Add new dashed links.
+    dashedLink.enter().append('line');
+    // Remove dead dashed links.
+    dashedLink.exit().remove();
+
     // Select the nodes, except for any dead ones that are still transitioning.
     const nodes = Array.from(this.nodes_.values());
     const node =
@@ -931,6 +986,7 @@ class Graph {
                            .call(this.drag_)
                            .on('click', this.onGraphNodeClick_.bind(this));
       const circles = newNodes.append('circle')
+                          .attr('id', d => `circle-${d.id}`)
                           .attr('r', kNodeRadius * 1.5)
                           .attr('fill', 'green');  // New nodes appear green.
 
@@ -981,9 +1037,11 @@ class Graph {
 
     // Update and restart the simulation if the graph changed.
     if (!node.enter().empty() || !node.exit().empty() ||
-        !link.enter().empty() || !link.exit().empty()) {
+        !link.enter().empty() || !link.exit().empty() ||
+        !dashedLink.enter().empty() || !dashedLink.exit().empty()) {
       this.simulation_.nodes(nodes);
-      this.simulation_.force('link').links(this.links_);
+      const links = this.links_.concat(this.dashedLinks_);
+      this.simulation_.force('link').links(links);
 
       this.restartSimulation_();
     }
@@ -1000,6 +1058,12 @@ class Graph {
         .attr('x2', d => d.target.x)
         .attr('y2', d => d.target.y);
 
+    const dashedLines = this.dashedLinkGroup_.selectAll('line');
+    dashedLines.attr('x1', d => d.source.x)
+        .attr('y1', d => d.source.y)
+        .attr('x2', d => d.target.x)
+        .attr('y2', d => d.target.y);
+
     this.updateToolTipLinks();
   }
 
@@ -1013,6 +1077,7 @@ class Graph {
   addNode_(node) {
     this.nodes_.set(node.id, node);
     this.addNodeLinks_(node);
+    this.addDashedNodeLinks_(node);
     node.setInitialPosition(this.width_, this.height_);
   }
 
@@ -1023,8 +1088,7 @@ class Graph {
    * @private
    */
   addNodeLinks_(node) {
-    const linkTargets = node.linkTargets;
-    for (const linkTarget of linkTargets) {
+    for (const linkTarget of node.linkTargets) {
       const target = this.nodes_.get(linkTarget);
       if (target) {
         this.links_.push({source: node, target: target});
@@ -1032,6 +1096,21 @@ class Graph {
     }
   }
 
+  /**
+   * Adds all the dashed links for a node to the graph.
+   *
+   * @param {!GraphNode} node
+   * @private
+   */
+  addDashedNodeLinks_(node) {
+    for (const dashedLinkTarget of node.dashedLinkTargets) {
+      const target = this.nodes_.get(dashedLinkTarget);
+      if (target) {
+        this.dashedLinks_.push({source: node, target: target});
+      }
+    }
+  }
+
   /**
    * @param {!GraphNode} d The dragged node.
    * @private
@@ -1068,6 +1147,9 @@ class Graph {
       d.fx = null;
       d.fy = null;
     }
+
+    // Toggle the pinned class as appropriate for the circle backing this node.
+    d3.select(`#circle-${d.id}`).classed('pinned', d.fx != null);
   }
 
   /**

chrome/browser/resources/discards/graph_doc_template.html

@@ -21,6 +21,20 @@ URL. As result, this document needs to be self-contained, hence inline scripts.
       .links line {
         stroke: #999;
         stroke-opacity: 0.6;
+        stroke-width: 1;
+      }
+
+      .dashed-links line {
+        marker-start: url(#arrowToSource);
+        stroke: #999;
+        stroke-dasharray: 3;
+        stroke-opacity: 0.6;
+        stroke-width: 1;
+      }
+
+      #arrowToSource {
+        fill: #999;
+        stroke: #999;
       }
 
       .nodes circle {
@@ -28,6 +42,10 @@ URL. As result, this document needs to be self-contained, hence inline scripts.
         stroke-width: 1.5px;
       }
 
+      .nodes circle.pinned {
+        stroke: red;
+      }
+
       .dead image {
         display: none;
       }
@@ -70,6 +88,13 @@ ${javascript_file}
   </head>
   <body>
     <div id="toolTips" width="100%" height="100%"></div>
-    <svg id="graphBody" width="100%" height="100%"></svg>
+    <svg id="graphBody" width="100%" height="100%">
+      <defs>
+        <marker id="arrowToSource" viewBox="0 -5 10 10" refX="-12" refY="0"
+                markerWidth="9" markerHeight="6" orient="auto">
+          <path d="M15,-7 L0,0 L15,7" />
+        </marker>
+      </defs>
+    </svg>
   </body>
 </html>

chrome/browser/ui/webui/discards/discards.mojom

@@ -9,14 +9,16 @@ import "mojo/public/mojom/base/process_id.mojom";
 import "mojo/public/mojom/base/time.mojom";
 import "url/mojom/url.mojom";
 
-// Identical to content::Visibility.
+// Identical to content::Visibility. Sent from browser to the chrome://discards
+// WebUI.
 enum LifecycleUnitVisibility {
   HIDDEN = 0,
   OCCLUDED = 1,
   VISIBLE = 2,
 };
 
-// Discard related information about a single tab in a browser.
+// Discard related information about a single tab in a browser. Sent from
+// browser to the chrome://discards WebUI.
 struct TabDiscardsInfo {
   // The URL associated with the tab. This corresponds to GetLastCommittedURL,
   // and is also what is visible in the Omnibox for a given tab.
@@ -98,12 +100,16 @@ interface DetailsProvider {
   Discard() => ();
 };
 
-// Represents the momentary state of a Page node.
+// Represents the momentary state of a Page node. Sent from browser to the
+// chrome://discards WebUI via the GraphChangeStream (defined below).
 struct PageInfo {
   int64 id;
 
   url.mojom.Url main_frame_url;
 
+  // The id of the frame that "opened" this page, if any.
+  int64 opener_frame_id;
+
   // This field is a dictionary of values, where each value is generated by
   // a performance_manager::NodeDataDescriber implementation and keyed by the
   // name it registered with. The intent is for each describer to describe
@@ -112,46 +118,74 @@ struct PageInfo {
   string description_json;
 };
 
-// Represents the momentary state of a Frame node.
+// Represents the momentary state of a Frame node. Sent from browser to the
+// chrome://discards WebUI via the GraphChangeStream (defined below).
 struct FrameInfo {
   int64 id;
 
+  // The last committed URL of this frame.
   url.mojom.Url url;
+
+  // The ID of the page node this frame is associated with.
   int64 page_id;
+
+  // The ID of the parent frame, if there is one. If not, this is a main frame.
   int64 parent_frame_id;
+
+  // The ID of the process in which this frame is hosted.
   int64 process_id;
 
   // See PageInfo::description_json.
   string description_json;
 };
 
-// Represents the momentary state of a Process node.
+// Represents the momentary state of a Process node. Sent from browser to the
+// chrome://discards WebUI via the GraphChangeStream (defined below).
 struct ProcessInfo {
   int64 id;
 
+  // The PID of the process associated with this node.
   mojo_base.mojom.ProcessId pid;
+
+  // The private memory usage of this process in KB.
   uint64 private_footprint_kb;
 
   // See PageInfo::description_json.
   string description_json;
 };
 
-// Represents the momentary state of a Worker node.
+// Represents the momentary state of a Worker node. Sent from browser to the
+// chrome://discards WebUI via the GraphChangeStream (defined below).
 struct WorkerInfo {
   int64 id;
 
+  // The URL of the worker.
   url.mojom.Url url;
+
+  // The ID of the process is which this worker is hosted.
   int64 process_id;
 
+  // An array of frames (by ID) that are clients of this worker (the worker is
+  // doing work on behalf of this frame). See
+  // WorkerNode::GetClientFrames() for details.
   array<int64> client_frame_ids;
+
+  // An array of other workers (by ID) that are clients of this worker (the
+  // worker is doing work on behalf of these other workers). See
+  // WorkerNode::GetClientWorkers() for details.
   array<int64> client_worker_ids;
+
+  // An array of workers (by ID) that are children of this worker. This can
+  // occur with shared and service workers owning their own dedicated workers.
+  // See WorkerNode::GetChildWorkers() for details.
   array<int64> child_worker_ids;
 
   // See PageInfo::description_json.
   string description_json;
 };
 
-// Used to transport favicon data.
+// Used to transport favicon data. Sent from browser to the chrome://discards
+// WebUI via the GraphChangeStream (defined below).
 struct FavIconInfo {
   int64 node_id;
 
@@ -161,7 +195,11 @@ struct FavIconInfo {
 };
 
 // Implement to receive a stream of notifications when performance manager
-// graph nodes are created, changed or deleted.
+// graph nodes are created, changed or deleted. Implemented in Javascript code
+// running in the chrome://discards WebUI, with data routed to it from an
+// observer of the performance_manager::Graph in the browser. The implementation
+// is injected into the browser via the browser-exposed GraphDump interface,
+// defined below.
 interface GraphChangeStream {
   // The |frame| was created.
   FrameCreated(FrameInfo frame);
@@ -190,7 +228,8 @@ interface GraphChangeStream {
 };
 
 // This interface allows subscribing to a stream of events that track the state
-// of the performance manager graph.
+// of the performance manager graph. Implemented in browser code, and used from
+// Javascript code running in the chrome://discards WebUI.
 interface GraphDump {
   // Subscribes |change_subscriber| to a graph change stream.
   SubscribeToChanges(pending_remote<GraphChangeStream> change_subscriber);

chrome/browser/ui/webui/discards/graph_dump_impl.cc

@@ -192,31 +192,18 @@ void DiscardsGraphDumpImpl::SubscribeToChanges(
   }
 
   // Send creation notifications for all existing nodes.
-  for (const performance_manager::ProcessNode* process_node :
-       graph_->GetAllProcessNodes())
-    SendProcessNotification(process_node, true);
-
-  for (const performance_manager::PageNode* page_node :
-       graph_->GetAllPageNodes()) {
-    SendPageNotification(page_node, true);
-    StartPageFaviconRequest(page_node);
-
-    // Dispatch preorder frame notifications.
-    for (const performance_manager::FrameNode* main_frame_node :
-         page_node->GetMainFrameNodes()) {
-      ForFrameAndOffspring(
-          main_frame_node,
-          [this](const performance_manager::FrameNode* frame_node) {
-            this->SendFrameNotification(frame_node, true);
-            this->StartFrameFaviconRequest(frame_node);
-          });
-    }
-  }
-
-  for (const performance_manager::WorkerNode* worker_node :
-       graph_->GetAllWorkerNodes()) {
-    SendWorkerNotification(worker_node, true);
-  }
+  SendNotificationToAllNodes(/* created = */ true);
+
+  // It is entirely possible for there to be circular link references between
+  // nodes that already existed at the point this object was created (the loop
+  // was closed after the two nodes themselves were created). We don't have the
+  // exact order of historical events that led to the current graph state, so we
+  // simply fire off a node changed notification for all nodes after the node
+  // creation. This ensures that all targets exist the second time through, and
+  // any loops are closed. Afterwards any newly created loops will be properly
+  // maintained as node creation/destruction/link events will be fed to the
+  // graph in the proper order.
+  SendNotificationToAllNodes(/* created = */ false);
 
   // Subscribe to subsequent notifications.
   graph_->AddFrameNodeObserver(this);
@@ -304,6 +291,18 @@ void DiscardsGraphDumpImpl::OnBeforePageNodeRemoved(
   RemoveNode(page_node);
 }
 
+void DiscardsGraphDumpImpl::OnOpenerFrameNodeChanged(
+    const performance_manager::PageNode* page_node,
+    const performance_manager::FrameNode*,
+    OpenedType) {
+  // This notification can arrive for a page node that has already been
+  // removed, because it fires as part of the |page_node| destructor. If that's
+  // the case then ignore this silently.
+  if (!HasNode(page_node))
+    return;
+  SendPageNotification(page_node, false);
+}
+
 void DiscardsGraphDumpImpl::OnFaviconUpdated(
     const performance_manager::PageNode* page_node) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -390,13 +389,19 @@ void DiscardsGraphDumpImpl::RemoveNode(const performance_manager::Node* node) {
   DCHECK_EQ(1u, erased);
 }
 
+bool DiscardsGraphDumpImpl::HasNode(
+    const performance_manager::Node* node) const {
+  return node_ids_.find(node) != node_ids_.end();
+}
+
 int64_t DiscardsGraphDumpImpl::GetNodeId(
-    const performance_manager::Node* node) {
+    const performance_manager::Node* node) const {
   if (node == nullptr)
     return 0;
 
-  DCHECK(node_ids_.find(node) != node_ids_.end());
-  return node_ids_[node].GetUnsafeValue();
+  auto it = node_ids_.find(node);
+  DCHECK(it != node_ids_.end());
+  return it->second.GetUnsafeValue();
 }
 
 DiscardsGraphDumpImpl::FaviconRequestHelper*
@@ -436,6 +441,36 @@ void DiscardsGraphDumpImpl::StartFrameFaviconRequest(
                                 GetNodeId(frame_node)));
 }
 
+void DiscardsGraphDumpImpl::SendNotificationToAllNodes(bool created) {
+  for (const performance_manager::ProcessNode* process_node :
+       graph_->GetAllProcessNodes())
+    SendProcessNotification(process_node, created);
+
+  for (const performance_manager::PageNode* page_node :
+       graph_->GetAllPageNodes()) {
+    SendPageNotification(page_node, created);
+    if (created)
+      StartPageFaviconRequest(page_node);
+
+    // Dispatch preorder frame notifications.
+    for (const performance_manager::FrameNode* main_frame_node :
+         page_node->GetMainFrameNodes()) {
+      ForFrameAndOffspring(
+          main_frame_node,
+          [this, created](const performance_manager::FrameNode* frame_node) {
+            this->SendFrameNotification(frame_node, created);
+            if (created)
+              this->StartFrameFaviconRequest(frame_node);
+          });
+    }
+  }
+
+  for (const performance_manager::WorkerNode* worker_node :
+       graph_->GetAllWorkerNodes()) {
+    SendWorkerNotification(worker_node, created);
+  }
+}
+
 void DiscardsGraphDumpImpl::SendFrameNotification(
     const performance_manager::FrameNode* frame,
     bool created) {
@@ -458,10 +493,11 @@ void DiscardsGraphDumpImpl::SendFrameNotification(
   frame_info->description_json =
       ToJSON(graph_->GetNodeDataDescriberRegistry()->DescribeNodeData(frame));
 
-  if (created)
+  if (created) {
     change_subscriber_->FrameCreated(std::move(frame_info));
-  else
+  } else {
     change_subscriber_->FrameChanged(std::move(frame_info));
+  }
 }
 
 void DiscardsGraphDumpImpl::SendPageNotification(
@@ -473,6 +509,7 @@ void DiscardsGraphDumpImpl::SendPageNotification(
 
   page_info->id = GetNodeId(page_node);
   page_info->main_frame_url = page_node->GetMainFrameUrl();
+  page_info->opener_frame_id = GetNodeId(page_node->GetOpenerFrameNode());
   page_info->description_json = ToJSON(
       graph_->GetNodeDataDescriberRegistry()->DescribeNodeData(page_node));
 

chrome/browser/ui/webui/discards/graph_dump_impl.h

@@ -110,12 +110,10 @@ class DiscardsGraphDumpImpl : public discards::mojom::GraphDump,
   void OnPageNodeAdded(const performance_manager::PageNode* page_node) override;
   void OnBeforePageNodeRemoved(
       const performance_manager::PageNode* page_node) override;
-  // Ignored for now.
-  // TODO(chrisha): Wire this relationship up with a dotted line!
   void OnOpenerFrameNodeChanged(
       const performance_manager::PageNode* page_node,
       const performance_manager::FrameNode* previous_opener,
-      OpenedType previous_opened_type) override {}
+      OpenedType previous_opened_type) override;
   void OnIsVisibleChanged(
       const performance_manager::PageNode* page_node) override {}  // Ignored.
   void OnIsAudibleChanged(
@@ -195,7 +193,8 @@ class DiscardsGraphDumpImpl : public discards::mojom::GraphDump,
 
   void AddNode(const performance_manager::Node* node);
   void RemoveNode(const performance_manager::Node* node);
-  int64_t GetNodeId(const performance_manager::Node* node);
+  bool HasNode(const performance_manager::Node* node) const;
+  int64_t GetNodeId(const performance_manager::Node* node) const;
 
   FaviconRequestHelper* EnsureFaviconRequestHelper();
 
@@ -203,6 +202,7 @@ class DiscardsGraphDumpImpl : public discards::mojom::GraphDump,
   void StartFrameFaviconRequest(
       const performance_manager::FrameNode* frame_node);
 
+  void SendNotificationToAllNodes(bool created);
   void SendFrameNotification(const performance_manager::FrameNode* frame,
                              bool created);
   void SendPageNotification(const performance_manager::PageNode* page,

chrome/browser/ui/webui/discards/graph_dump_impl_unittest.cc

@@ -212,8 +212,9 @@ TEST_F(DiscardsGraphDumpImplTest, ChangeStream) {
 
   task_environment.RunUntilIdle();
 
-  // Validate that the initial graph state dump is complete.
-  EXPECT_EQ(0u, change_stream.num_changes());
+  // Validate that the initial graph state dump is complete. Note that there is
+  // an update for each node as part of the initial state dump.
+  EXPECT_EQ(8u, change_stream.num_changes());
   EXPECT_EQ(8u, change_stream.id_set().size());
 
   EXPECT_EQ(2u, change_stream.process_map().size());
@@ -277,7 +278,7 @@ TEST_F(DiscardsGraphDumpImplTest, ChangeStream) {
   task_environment.RunUntilIdle();
 
   // Main frame navigation results in a notification for the url.
-  EXPECT_EQ(1u, change_stream.num_changes());
+  EXPECT_EQ(9u, change_stream.num_changes());
   EXPECT_FALSE(base::Contains(change_stream.id_set(), child_frame_id));
 
   const auto main_page_it = change_stream.page_map().find(

components/performance_manager/public/graph/page_node.h

@@ -175,7 +175,8 @@ class PageNodeObserver {
   // Invoked when this page has been assigned an opener, had the opener change,
   // or had the opener removed. This can happen if a page is opened via
   // window.open, webviews, portals, etc, or when that relationship is
-  // subsequently severed or reparented.
+  // subsequently severed or reparented. Note that this can be invoked *after*
+  // OnBeforePageNodeRemoved() if a page disappears while still attached.
   virtual void OnOpenerFrameNodeChanged(const PageNode* page_node,
                                         const FrameNode* previous_opener,
                                         OpenedType previous_opened_type) = 0;