[PM] Remove uses of frame_tree_node_id from FrameNode

With Prerender2 the semantics of the frame tree node ID have changed:
it's no longer constant for the life of a frame. Since the semantics are
now confusing, stop tracking the property in PerformanceManager.

This removes a DCHECK of the invariant in FrameNodeImpl::SetIsCurrent.
As a followup we should find some other way to validate that invariant.

PerformanceManagerTabHelper still uses main_frame_tree_node_id as a key
to the `pages_` set, since it would be too invasive to remove right now.
There's no reliable way to look up a PageNode in the set given the
current frame_tree_node_id of its main frame, since it may have changed
since the PageNode was added to the set. This is harmless in the short
term because there's currently no code that tries to do this, but the set
should be changed to use a different key as a followup.

R=altimin

Bug: 1177859
Change-Id: I54e57c9debdbe195d945fdbd0f705a3bb2e3ecca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3087728
Commit-Queue: Joe Mason <joenotcharles@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#911493}

chrome/browser/performance_manager/decorators/frozen_frame_aggregator_unittest.cc

@@ -84,10 +84,9 @@ class FrozenFrameAggregatorTest : public GraphTestHarness {
     EXPECT_EQ(LifecycleState::kFrozen, page_node_.get()->lifecycle_state());
   }
 
-  TestNodeWrapper<FrameNodeImpl> CreateFrame(FrameNodeImpl* parent_frame_node,
-                                             int frame_tree_node_id) {
+  TestNodeWrapper<FrameNodeImpl> CreateFrame(FrameNodeImpl* parent_frame_node) {
     return CreateFrameNodeAutoId(process_node_.get(), page_node_.get(),
-                                 parent_frame_node, frame_tree_node_id);
+                                 parent_frame_node);
   }
 
   FrozenFrameAggregator* ffa_;
@@ -106,7 +105,7 @@ TEST_F(FrozenFrameAggregatorTest, ProcessAggregation) {
   ExpectNoProcessData();
 
   // Add a main frame.
-  auto f0 = CreateFrame(nullptr, 0);
+  auto f0 = CreateFrame(nullptr);
   ExpectNoProcessData();
 
   // Make the frame current.
@@ -125,7 +124,7 @@ TEST_F(FrozenFrameAggregatorTest, ProcessAggregation) {
   ExpectProcessData(1, 1);
 
   // Create a child frame for the first page hosted in the second process.
-  auto f1 = CreateFrameNodeAutoId(proc2.get(), page_node_.get(), f0.get(), 1);
+  auto f1 = CreateFrameNodeAutoId(proc2.get(), page_node_.get(), f0.get());
   ExpectProcessData(1, 1);
 
   // Immediately make it current.
@@ -145,7 +144,7 @@ TEST_F(FrozenFrameAggregatorTest, ProcessAggregation) {
   ExpectProcessData(1, 0);
 
   // Create a main frame in the second page, but that's in the first process.
-  auto f2 = CreateFrameNodeAutoId(process_node_.get(), page2.get(), nullptr, 2);
+  auto f2 = CreateFrameNodeAutoId(process_node_.get(), page2.get(), nullptr);
   ExpectProcessData(1, 0);
 
   // Freeze the main frame in the second page.
@@ -189,7 +188,7 @@ TEST_F(FrozenFrameAggregatorTest, PageAggregation) {
   ExpectRunning();
 
   // Add a non-current frame.
-  auto f0 = CreateFrame(nullptr, 0);
+  auto f0 = CreateFrame(nullptr);
   ExpectPageData(0, 0);
   ExpectRunning();
 
@@ -209,7 +208,7 @@ TEST_F(FrozenFrameAggregatorTest, PageAggregation) {
   ExpectRunning();
 
   // Add a child frame.
-  auto f1 = CreateFrame(f0.get(), 1);
+  auto f1 = CreateFrame(f0.get());
   ExpectPageData(1, 0);
   ExpectRunning();
 
@@ -235,7 +234,7 @@ TEST_F(FrozenFrameAggregatorTest, PageAggregation) {
   ExpectRunning();
 
   // Create a third frame.
-  auto f1a = CreateFrame(f0.get(), 1);
+  auto f1a = CreateFrame(f0.get());
   ExpectPageData(2, 0);
   ExpectRunning();
 

chrome/browser/performance_manager/observers/isolation_context_metrics_unittest.cc

@@ -83,8 +83,8 @@ class IsolationContextMetricsTest : public GraphTestHarness {
       int32_t site_instance_id,
       FrameNodeImpl* parent_frame_node = nullptr) {
     return CreateNode<FrameNodeImpl>(
-        process_node, page_node, parent_frame_node, 0 /* frame_tree_node_id */,
-        ++next_render_frame_id_, blink::LocalFrameToken(),
+        process_node, page_node, parent_frame_node, ++next_render_frame_id_,
+        blink::LocalFrameToken(),
         content::BrowsingInstanceId(browsing_instance_id),
         content::SiteInstanceId(site_instance_id));
   }

components/performance_manager/graph/frame_node_impl.cc

@@ -26,15 +26,13 @@ using PriorityAndReason = execution_context_priority::PriorityAndReason;
 FrameNodeImpl::FrameNodeImpl(ProcessNodeImpl* process_node,
                              PageNodeImpl* page_node,
                              FrameNodeImpl* parent_frame_node,
-                             int frame_tree_node_id,
                              int render_frame_id,
                              const blink::LocalFrameToken& frame_token,
                              content::BrowsingInstanceId browsing_instance_id,
                              content::SiteInstanceId site_instance_id)
     : parent_frame_node_(parent_frame_node),
       page_node_(page_node),
       process_node_(process_node),
-      frame_tree_node_id_(frame_tree_node_id),
       render_frame_id_(render_frame_id),
       frame_token_(frame_token),
       browsing_instance_id_(browsing_instance_id),
@@ -133,11 +131,6 @@ ProcessNodeImpl* FrameNodeImpl::process_node() const {
   return process_node_;
 }
 
-int FrameNodeImpl::frame_tree_node_id() const {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return frame_tree_node_id_;
-}
-
 int FrameNodeImpl::render_frame_id() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return render_frame_id_;
@@ -256,27 +249,17 @@ void FrameNodeImpl::SetIsCurrent(bool is_current) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   is_current_.SetAndMaybeNotify(this, is_current);
 
-#if DCHECK_IS_ON()
-  // We maintain an invariant that of all sibling nodes with the same
-  // |frame_tree_node_id|, at most one may be current.
-  if (is_current) {
-    const base::flat_set<FrameNodeImpl*>* siblings = nullptr;
-    if (parent_frame_node_) {
-      siblings = &parent_frame_node_->child_frame_nodes();
-    } else {
-      siblings = &page_node_->main_frame_nodes();
-    }
-
-    size_t current_siblings = 0;
-    for (auto* frame : *siblings) {
-      if (frame->frame_tree_node_id() == frame_tree_node_id_ &&
-          frame->is_current()) {
-        ++current_siblings;
-      }
-    }
-    DCHECK_EQ(1u, current_siblings);
-  }
-#endif
+  // TODO(crbug.com/1211368): We maintain an invariant that of all sibling
+  // frame nodes in the same FrameTreeNode, at most one may be current. We used
+  // to save the RenderFrameHost's `frame_tree_node_id` at FrameNode creation
+  // time to check this invariant, but prerendering RenderFrameHost's can be
+  // moved to a new FrameTreeNode when they're activated so the
+  // `frame_tree_node_id` can go out of date. Because of this,
+  // RenderFrameHost::GetFrameTreeNodeId() is being deprecated. (See the
+  // discussion at crbug.com/1179502 and in the comment thread at
+  // https://chromium-review.googlesource.com/c/chromium/src/+/2966195/comments/58550eac_5795f790
+  // for more details.) We need to find another way to check this invariant
+  // here.
 }
 
 void FrameNodeImpl::SetIsHoldingWebLock(bool is_holding_weblock) {
@@ -433,11 +416,6 @@ const ProcessNode* FrameNodeImpl::GetProcessNode() const {
   return process_node();
 }
 
-int FrameNodeImpl::GetFrameTreeNodeId() const {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return frame_tree_node_id();
-}
-
 const blink::LocalFrameToken& FrameNodeImpl::GetFrameToken() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return frame_token();

components/performance_manager/graph/frame_node_impl.h

@@ -32,28 +32,8 @@ namespace execution_context {
 class ExecutionContextAccess;
 }  // namespace execution_context
 
-// Frame nodes form a tree structure, each FrameNode at most has one parent that
-// is a FrameNode. Conceptually, a frame corresponds to a
-// content::RenderFrameHost in the browser, and a content::RenderFrameImpl /
-// blink::LocalFrame in a renderer.
-//
-// Note that a frame in a frame tree can be replaced with another, with the
-// continuity of that position represented via the |frame_tree_node_id|. It is
-// possible to have multiple "sibling" nodes that share the same
-// |frame_tree_node_id|. Only one of these may contribute to the content being
-// rendered, and this node is designated the "current" node in content
-// terminology. A swap is effectively atomic but will take place in two steps
-// in the graph: the outgoing frame will first be marked as not current, and the
-// incoming frame will be marked as current. As such, the graph invariant is
-// that there will be 0 or 1 |is_current| frames with a given
-// |frame_tree_node_id|.
-//
-// This occurs when a frame is navigated and the existing frame can't be reused.
-// In that case a "provisional" frame is created to start the navigation. Once
-// the navigation completes (which may actually involve a redirect to another
-// origin meaning the frame has to be destroyed and another one created in
-// another process!) and commits, the frame will be swapped with the previously
-// active frame.
+// Frame nodes for a tree structure that is described in
+// components/performance_manager/public/graph/frame_node.h.
 class FrameNodeImpl
     : public PublicNodeImpl<FrameNodeImpl, FrameNode>,
       public TypedNodeBase<FrameNodeImpl, FrameNode, FrameNodeObserver>,
@@ -69,7 +49,6 @@ class FrameNodeImpl
   FrameNodeImpl(ProcessNodeImpl* process_node,
                 PageNodeImpl* page_node,
                 FrameNodeImpl* parent_frame_node,
-                int frame_tree_node_id,
                 int render_frame_id,
                 const blink::LocalFrameToken& frame_token,
                 content::BrowsingInstanceId browsing_instance_id,
@@ -100,7 +79,6 @@ class FrameNodeImpl
   FrameNodeImpl* parent_frame_node() const;
   PageNodeImpl* page_node() const;
   ProcessNodeImpl* process_node() const;
-  int frame_tree_node_id() const;
   int render_frame_id() const;
   const blink::LocalFrameToken& frame_token() const;
   content::BrowsingInstanceId browsing_instance_id() const;
@@ -182,7 +160,6 @@ class FrameNodeImpl
   const FrameNode* GetParentFrameNode() const override;
   const PageNode* GetPageNode() const override;
   const ProcessNode* GetProcessNode() const override;
-  int GetFrameTreeNodeId() const override;
   const blink::LocalFrameToken& GetFrameToken() const override;
   content::BrowsingInstanceId GetBrowsingInstanceId() const override;
   content::SiteInstanceId GetSiteInstanceId() const override;
@@ -272,9 +249,6 @@ class FrameNodeImpl
   FrameNodeImpl* const parent_frame_node_;
   PageNodeImpl* const page_node_;
   ProcessNodeImpl* const process_node_;
-  // Can be used to tie together "sibling" frames, where a navigation is ongoing
-  // in a new frame that will soon replace the existing one.
-  const int frame_tree_node_id_;
   // The routing id of the frame.
   const int render_frame_id_;
 

components/performance_manager/graph/frame_node_impl_describer.cc

@@ -71,7 +71,6 @@ base::Value FrameNodeImplDescriber::DescribeFrameNodeData(
   ret.SetKey("document", std::move(doc));
 
   // Frame node properties.
-  ret.SetIntKey("frame_tree_node_id", impl->frame_tree_node_id_);
   ret.SetIntKey("render_frame_id", impl->render_frame_id_);
   ret.SetStringKey("frame_token", impl->frame_token_.value().ToString());
   ret.SetIntKey("browsing_instance_id", impl->browsing_instance_id_.value());

components/performance_manager/graph/frame_node_impl_unittest.cc

@@ -57,9 +57,9 @@ TEST_F(FrameNodeImplTest, AddFrameHierarchyBasic) {
   auto page = CreateNode<PageNodeImpl>();
   auto parent_node = CreateFrameNodeAutoId(process.get(), page.get());
   auto child2_node =
-      CreateFrameNodeAutoId(process.get(), page.get(), parent_node.get(), 1);
+      CreateFrameNodeAutoId(process.get(), page.get(), parent_node.get());
   auto child3_node =
-      CreateFrameNodeAutoId(process.get(), page.get(), parent_node.get(), 2);
+      CreateFrameNodeAutoId(process.get(), page.get(), parent_node.get());
 
   EXPECT_EQ(nullptr, parent_node->parent_frame_node());
   EXPECT_EQ(2u, parent_node->child_frame_nodes().size());
@@ -114,8 +114,8 @@ TEST_F(FrameNodeImplTest, RemoveChildFrame) {
   auto process = CreateNode<ProcessNodeImpl>();
   auto page = CreateNode<PageNodeImpl>();
   auto parent_frame_node = CreateFrameNodeAutoId(process.get(), page.get());
-  auto child_frame_node = CreateFrameNodeAutoId(process.get(), page.get(),
-                                                parent_frame_node.get(), 1);
+  auto child_frame_node =
+      CreateFrameNodeAutoId(process.get(), page.get(), parent_frame_node.get());
 
   // Ensure correct Parent-child relationships have been established.
   EXPECT_EQ(1u, parent_frame_node->child_frame_nodes().size());
@@ -494,8 +494,6 @@ TEST_F(FrameNodeImplTest, PublicInterface) {
             public_frame_node->GetPageNode());
   EXPECT_EQ(static_cast<const ProcessNode*>(frame_node->process_node()),
             public_frame_node->GetProcessNode());
-  EXPECT_EQ(frame_node->frame_tree_node_id(),
-            public_frame_node->GetFrameTreeNodeId());
   EXPECT_EQ(frame_node->frame_token(), public_frame_node->GetFrameToken());
   EXPECT_EQ(frame_node->browsing_instance_id(),
             public_frame_node->GetBrowsingInstanceId());

components/performance_manager/graph/graph_impl_operations_unittest.cc

@@ -26,18 +26,16 @@ class GraphImplOperationsTest : public GraphTestHarness {
     process2_ = CreateNode<ProcessNodeImpl>();
     page1_ = CreateNode<PageNodeImpl>();
     page2_ = CreateNode<PageNodeImpl>();
-    mainframe1_ =
-        CreateFrameNodeAutoId(process1_.get(), page1_.get(), nullptr, 0);
-    mainframe2_ =
-        CreateFrameNodeAutoId(process2_.get(), page2_.get(), nullptr, 1);
-    childframe1a_ = CreateFrameNodeAutoId(process2_.get(), page1_.get(),
-                                          mainframe1_.get(), 2);
-    childframe1b_ = CreateFrameNodeAutoId(process2_.get(), page1_.get(),
-                                          mainframe1_.get(), 3);
-    childframe2a_ = CreateFrameNodeAutoId(process1_.get(), page2_.get(),
-                                          mainframe2_.get(), 4);
-    childframe2b_ = CreateFrameNodeAutoId(process1_.get(), page2_.get(),
-                                          mainframe2_.get(), 5);
+    mainframe1_ = CreateFrameNodeAutoId(process1_.get(), page1_.get(), nullptr);
+    mainframe2_ = CreateFrameNodeAutoId(process2_.get(), page2_.get(), nullptr);
+    childframe1a_ =
+        CreateFrameNodeAutoId(process2_.get(), page1_.get(), mainframe1_.get());
+    childframe1b_ =
+        CreateFrameNodeAutoId(process2_.get(), page1_.get(), mainframe1_.get());
+    childframe2a_ =
+        CreateFrameNodeAutoId(process1_.get(), page2_.get(), mainframe2_.get());
+    childframe2b_ =
+        CreateFrameNodeAutoId(process1_.get(), page2_.get(), mainframe2_.get());
   }
 
   TestNodeWrapper<ProcessNodeImpl> process1_;
@@ -78,8 +76,8 @@ TEST_F(GraphImplOperationsTest, GetAssociatedProcessNodes) {
 
 TEST_F(GraphImplOperationsTest, GetFrameNodes) {
   // Add a grandchild frame.
-  auto grandchild = CreateFrameNodeAutoId(process1_.get(), page1_.get(),
-                                          childframe1a_.get(), 6);
+  auto grandchild =
+      CreateFrameNodeAutoId(process1_.get(), page1_.get(), childframe1a_.get());
 
   auto frame_nodes = GraphImplOperations::GetFrameNodes(page1_.get());
   EXPECT_THAT(frame_nodes, testing::UnorderedElementsAre(

components/performance_manager/graph/graph_operations_unittest.cc

@@ -26,18 +26,16 @@ class GraphOperationsTest : public GraphTestHarness {
     process2_ = CreateNode<ProcessNodeImpl>();
     page1_ = CreateNode<PageNodeImpl>();
     page2_ = CreateNode<PageNodeImpl>();
-    mainframe1_ =
-        CreateFrameNodeAutoId(process1_.get(), page1_.get(), nullptr, 0);
-    mainframe2_ =
-        CreateFrameNodeAutoId(process2_.get(), page2_.get(), nullptr, 1);
-    childframe1a_ = CreateFrameNodeAutoId(process2_.get(), page1_.get(),
-                                          mainframe1_.get(), 2);
-    childframe1b_ = CreateFrameNodeAutoId(process2_.get(), page1_.get(),
-                                          mainframe1_.get(), 3);
-    childframe2a_ = CreateFrameNodeAutoId(process1_.get(), page2_.get(),
-                                          mainframe2_.get(), 4);
-    childframe2b_ = CreateFrameNodeAutoId(process1_.get(), page2_.get(),
-                                          mainframe2_.get(), 5);
+    mainframe1_ = CreateFrameNodeAutoId(process1_.get(), page1_.get(), nullptr);
+    mainframe2_ = CreateFrameNodeAutoId(process2_.get(), page2_.get(), nullptr);
+    childframe1a_ =
+        CreateFrameNodeAutoId(process2_.get(), page1_.get(), mainframe1_.get());
+    childframe1b_ =
+        CreateFrameNodeAutoId(process2_.get(), page1_.get(), mainframe1_.get());
+    childframe2a_ =
+        CreateFrameNodeAutoId(process1_.get(), page2_.get(), mainframe2_.get());
+    childframe2b_ =
+        CreateFrameNodeAutoId(process1_.get(), page2_.get(), mainframe2_.get());
   }
 
   TestNodeWrapper<ProcessNodeImpl> process1_;
@@ -84,8 +82,8 @@ TEST_F(GraphOperationsTest, GetAssociatedProcessNodes) {
 
 TEST_F(GraphOperationsTest, GetFrameNodes) {
   // Add a grandchild frame.
-  auto grandchild = CreateFrameNodeAutoId(process1_.get(), page1_.get(),
-                                          childframe1a_.get(), 6);
+  auto grandchild =
+      CreateFrameNodeAutoId(process1_.get(), page1_.get(), childframe1a_.get());
 
   auto frame_nodes = GraphOperations::GetFrameNodes(page1_.get());
   EXPECT_THAT(frame_nodes,

components/performance_manager/graph/page_node_impl_unittest.cc

@@ -61,9 +61,9 @@ TEST_F(PageNodeImplTest, AddFrameBasic) {
   auto parent_frame =
       CreateFrameNodeAutoId(process_node.get(), page_node.get());
   auto child1_frame = CreateFrameNodeAutoId(process_node.get(), page_node.get(),
-                                            parent_frame.get(), 1);
+                                            parent_frame.get());
   auto child2_frame = CreateFrameNodeAutoId(process_node.get(), page_node.get(),
-                                            parent_frame.get(), 2);
+                                            parent_frame.get());
 
   // Validate that all frames are tallied to the page.
   EXPECT_EQ(3u, GraphImplOperations::GetFrameNodes(page_node.get()).size());
@@ -73,7 +73,7 @@ TEST_F(PageNodeImplTest, RemoveFrame) {
   auto process_node = CreateNode<ProcessNodeImpl>();
   auto page_node = CreateNode<PageNodeImpl>();
   auto frame_node =
-      CreateFrameNodeAutoId(process_node.get(), page_node.get(), nullptr, 0);
+      CreateFrameNodeAutoId(process_node.get(), page_node.get(), nullptr);
 
   // Ensure correct page-frame relationship has been established.
   auto frame_nodes = GraphImplOperations::GetFrameNodes(page_node.get());

components/performance_manager/performance_manager_impl.cc

@@ -130,16 +130,14 @@ std::unique_ptr<FrameNodeImpl> PerformanceManagerImpl::CreateFrameNode(
     ProcessNodeImpl* process_node,
     PageNodeImpl* page_node,
     FrameNodeImpl* parent_frame_node,
-    int frame_tree_node_id,
     int render_frame_id,
     const blink::LocalFrameToken& frame_token,
     content::BrowsingInstanceId browsing_instance_id,
     content::SiteInstanceId site_instance_id,
     FrameNodeCreationCallback creation_callback) {
   return CreateNodeImpl<FrameNodeImpl>(
       std::move(creation_callback), process_node, page_node, parent_frame_node,
-      frame_tree_node_id, render_frame_id, frame_token, browsing_instance_id,
-      site_instance_id);
+      render_frame_id, frame_token, browsing_instance_id, site_instance_id);
 }
 
 // static

components/performance_manager/performance_manager_impl.h

@@ -94,7 +94,6 @@ class PerformanceManagerImpl : public PerformanceManager {
       ProcessNodeImpl* process_node,
       PageNodeImpl* page_node,
       FrameNodeImpl* parent_frame_node,
-      int frame_tree_node_id,
       int render_frame_id,
       const blink::LocalFrameToken& frame_token,
       content::BrowsingInstanceId browsing_instance_id,