Refactor CreateOpenerProxies to support updates to frame openers.

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 50c8a599a34f0e5aab5f8bab5697b8daba1f02e1..4e5cf7be7cceaad710f46060a3e95f572405bb75 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -169,6 +169,43 @@ bool RenderFrameHostManager::ClearRFHsPendingShutdown(FrameTreeNode* node) {
   return true;
 }
 
+namespace {

[Charlie Reis, 2015/08/05 23:59:09]

Nit: This should go above the RFHM:: stuff above.

[alexmos, 2015/08/06 17:05:39]

Done.

+
+// Helper function to add the FrameTree of the current node's opener

[Charlie Reis, 2015/08/05 23:59:09]

s/current/given/

[alexmos, 2015/08/06 17:05:39]

Done.

+// to the list of |opener_trees|, if it doesn't exist there already.
+// |visited_index| indicates which FrameTrees in |opener_trees| have already
+// been visited (those at indexes less than |visited_index|).

[Charlie Reis, 2015/08/05 23:59:09]

nit: (i.e., those at indices...

[alexmos, 2015/08/06 17:05:39]

Done.

+// |nodes_with_back_links|, if not null, collects FrameTreeNodes with openers

[Charlie Reis, 2015/08/05 23:59:09]

Why would it be null?  I don't see a case where that happens.

[alexmos, 2015/08/06 17:05:39]

Removed from description and the if statement below.  Yes, there's no case right
now when that happens.  I was thinking that down the road something might want
to call this and not care about nodes with cycles, but I suppose we can always
add that case if/when it actually becomes needed.

+// that point to FrameTrees that have already been visited (such as those with

[Charlie Reis, 2015/08/05 23:59:09]

nit: with openers in FrameTrees that...

[alexmos, 2015/08/06 17:05:39]

Done.

+// cycles).
+bool OpenerForFrameTreeNode(

[Charlie Reis, 2015/08/05 23:59:09]

Does this need to return a bool?  Looks like it always returns true.  (If that's
just for the ForEach contract, let's mention that in the comment.)

[alexmos, 2015/08/06 17:05:39]

Done.  Yes, it's returning true just so it visits all nodes as part of ForEach,
so added a sentence about it in the comment.

+    size_t visited_index,
+    std::vector<FrameTree*>* opener_trees,
+    base::hash_set<FrameTreeNode*>* nodes_with_back_links,
+    FrameTreeNode* node) {

[Charlie Reis, 2015/08/05 23:59:09]

nit: Let's list |node| first, since it's the main subject.

[alexmos, 2015/08/06 17:05:39]

Hmm, I can't do that given how FrameTree::ForEach always puts the |node| last in
the arg list, can I?

[Charlie Reis, 2015/08/06 17:35:06]

Acknowledged.

+  if (!node->opener())
+    return true;
+
+  FrameTree* opener_tree = node->opener()->frame_tree();
+
+  const auto& existing_tree_it =
+      std::find(opener_trees->begin(), opener_trees->end(), opener_tree);
+  if (existing_tree_it == opener_trees->end()) {
+    // This is a new opener tree that we will need to process.
+    opener_trees->push_back(opener_tree);
+  } else if (nodes_with_back_links) {
+    // If this tree is already on our processing list *and* we have visited it,
+    // then this node's opener is a back link.  This means the node will need
+    // special treatment to process its opener.
+    size_t position = std::distance(opener_trees->begin(), existing_tree_it);
+    if (position < visited_index)
+      nodes_with_back_links->insert(node);
+  }
+  return true;
+}
+
+}  // namespace
+
 RenderFrameHostManager::RenderFrameHostManager(
     FrameTreeNode* frame_tree_node,
     RenderFrameHostDelegate* render_frame_delegate,
@@ -2398,22 +2435,55 @@ void RenderFrameHostManager::CreateOpenerProxiesIfNeeded(
   if (!opener)
     return;
 
+  // TODO(alexmos): This should process all openers in the current frame tree,
+  // not just current node's opener.

[alexmos, 2015/08/05 17:45:05]

I noticed that this actually has a bug today: if A opens a popup B, and B has a
subframe C, then calling CreateOpenerProxiesIfNeeded on C won't do the right
thing: it should be looking at the top node's opener and not C's opener.  As a
result, window.parent.opener won't work in C.  And once subframe openers are
possible, this will need to look at all FTNs in the tree.

I wanted to add a test for this and fix it in a followup CL to keep this one
less complex, but let me know if I should do that here or before this CL.

[Charlie Reis, 2015/08/05 23:59:09]

Nice catch.  Yes, a separate CL is fine, either before or after.

[alexmos, 2015/08/06 17:05:39]

OK, will do in a later CL.

+
   // Create proxies for the opener chain.
   opener->render_manager()->CreateOpenerProxies(instance);
 }
 
+void RenderFrameHostManager::CollectOpenerFrameTrees(
+    std::vector<FrameTree*>* opener_frame_trees,
+    base::hash_set<FrameTreeNode*>* nodes_with_back_links) {
+  CHECK(opener_frame_trees);
+  opener_frame_trees->push_back(frame_tree_node_->frame_tree());
+
+  size_t visited_index = 0;
+  while (visited_index < opener_frame_trees->size()) {
+    FrameTree* frame_tree = (*opener_frame_trees)[visited_index];
+    visited_index++;
+    frame_tree->ForEach(base::Bind(&OpenerForFrameTreeNode, visited_index,
+                                   opener_frame_trees, nodes_with_back_links));
+  }
+}
+
 void RenderFrameHostManager::CreateOpenerProxies(SiteInstance* instance) {
-  // If this tab has an opener, recursively create proxies for the nodes on its
-  // frame tree.
-  // TODO(alexmos): Once we allow frame openers to be updated (which can happen
-  // via window.open(url, "target_frame")), this will need to be resilient to
-  // cycles. It will also need to handle tabs that have multiple openers (e.g.,
-  // main frame and subframe could have different openers, each of which must
-  // be traversed).
-  FrameTreeNode* opener = frame_tree_node_->opener();
-  if (opener)
-    opener->render_manager()->CreateOpenerProxies(instance);
+  std::vector<FrameTree*> opener_frame_trees;
+  base::hash_set<FrameTreeNode*> nodes_with_back_links;
+
+  CollectOpenerFrameTrees(&opener_frame_trees, &nodes_with_back_links);
 
+  // Create opener proxies for frame trees, processing furthest openers from
+  // this node first and this node last.  In the common case without cycles,
+  // this will ensure that each tree's openers are created before the tree's
+  // nodes need to reference them.
+  for (int i = opener_frame_trees.size() - 1; i >= 0; i--) {
+    opener_frame_trees[i]
+        ->root()
+        ->render_manager()
+        ->CreateOpenerProxiesForFrameTree(instance);
+  }
+
+  // TODO(alexmos): Set openers for nodes in |nodes_with_back_links| in a
+  // second pass. The proxies created at these FrameTreeNodes in
+  // CreateOpenerProxiesForFrameTree won't have their opener routing ID
+  // available when created due to cycles or back links in the opener chain.
+  // They must have their openers updated as a separate step after proxy
+  // creation.
+}
+
+void RenderFrameHostManager::CreateOpenerProxiesForFrameTree(
+    SiteInstance* instance) {
   // If any of the RenderViewHosts (current, pending, or swapped out) for this
   // FrameTree has the same SiteInstance, then we can return early, since
   // proxies for other nodes in the tree should also exist (when in