Handle frame openers in the same FrameTree when navigating subframes.

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 c2ef320311620f6587e9483f41ed7abcb146846c..c94ce33e96601675bbb196dafbd43b20eb65def7 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -414,7 +414,8 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
     dest_render_frame_host->SetUpMojoIfNeeded();
 
     // Recreate the opener chain.
-    CreateOpenerProxiesIfNeeded(dest_render_frame_host->GetSiteInstance());
+    CreateOpenerProxies(dest_render_frame_host->GetSiteInstance(),

[Charlie Reis, 2015/08/31 20:49:43]

Looks like we can call this inside subframe RFHMs, right?  That would fail the
DCHECK in CreateOpenerProxiesForFrameTree.

[alexmos, 2015/09/01 22:12:28]

Please see my response on the DCHECK.

+                        frame_tree_node_);
     if (!InitRenderView(dest_render_frame_host->render_view_host(),
                         MSG_ROUTING_NONE,
                         frame_tree_node_->IsMainFrame()))
@@ -1039,7 +1040,7 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
   // created or has crashed), initialize it.
   if (!navigation_rfh->IsRenderFrameLive()) {
     // Recreate the opener chain.
-    CreateOpenerProxiesIfNeeded(navigation_rfh->GetSiteInstance());
+    CreateOpenerProxies(navigation_rfh->GetSiteInstance(), frame_tree_node_);

[Charlie Reis, 2015/08/31 20:49:43]

As above, I think this can happen in a subframe and fail the DCHECK below.

[alexmos, 2015/09/01 22:12:28]

Please see my response on the DCHECK.

     if (!InitRenderView(navigation_rfh->render_view_host(), MSG_ROUTING_NONE,
                         frame_tree_node_->IsMainFrame())) {
       return nullptr;
@@ -1577,15 +1578,21 @@ void RenderFrameHostManager::CreateProxiesForNewRenderFrameHost(
     int* create_render_frame_flags) {
   // Only create opener proxies if they are in the same BrowsingInstance.
   if (new_instance->IsRelatedSiteInstance(old_instance))
-    CreateOpenerProxiesIfNeeded(new_instance);
-  if (SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
-    // Ensure that the frame tree has RenderFrameProxyHosts for the new
-    // SiteInstance in all nodes except the current one.  We do this for all
-    // frames in the tree, whether they are in the same BrowsingInstance or not.
-    // We will still check whether two frames are in the same BrowsingInstance
-    // before we allow them to interact (e.g., postMessage).
+    CreateOpenerProxies(new_instance, frame_tree_node_);
+  else if (SiteIsolationPolicy::AreCrossProcessFramesPossible()) {

[Charlie Reis, 2015/08/31 20:49:43]

Style nit: The earlier if branch needs braces if the else needs braces (now that
they're combined into the same conditional).

[alexmos, 2015/09/01 22:12:28]

Done.

+    // Ensure that the frame tree has RenderFrameProxyHosts for the
+    // new SiteInstance in all nodes except the current one.  We do this for
+    // all frames in the tree, whether they are in the same BrowsingInstance or
+    // not.  If |new_instance| is in the same BrowsingInstance as
+    // |old_instance|, this will be done as part of CreateOpenerProxies above;
+    // otherwise, we do this here.  We will still check whether two frames are
+    // in the same BrowsingInstance before we allow them to interact (e.g.,
+    // postMessage).
     frame_tree_node_->frame_tree()->CreateProxiesForSiteInstance(
         frame_tree_node_, new_instance);
+  }
+
+  if (SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
     // RenderFrames in different processes from their parent RenderFrames
     // in the frame tree require RenderWidgets for rendering and processing
     // input events.
@@ -1876,9 +1883,6 @@ void RenderFrameHostManager::EnsureRenderViewInitialized(
   if (render_view_host->IsRenderViewLive())
     return;
 
-  // Recreate the opener chain.
-  CreateOpenerProxiesIfNeeded(instance);

[alexmos, 2015/08/25 22:38:08]

I don't think this is actually needed.  EnsureRenderViewInitialized is called in
only two places: CreateOpenerProxiesForFrameTree and
CreateProxiesForSiteInstance.  In the first case, calling this is redundant as
we're already in the middle of creating opener proxies, and in the second case,
it is also redundant because the only way to get to CreateProxiesForSiteInstance
is either while creating opener proxies or right after we've done that in
CreateProxiesForNewRenderFrameHost.

I saw that lazyboy@ introduced this in
https://codereview.chromium.org/782093002, but I didn't see a clear reason in
the comments, and none of the tests are affected by removing this.

[Charlie Reis, 2015/08/31 20:49:43]

Acknowledged.

-
   // If the proxy in |instance| doesn't exist, this RenderView is not swapped
   // out and shouldn't be reinitialized here.
   RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance);
@@ -2453,19 +2457,6 @@ RenderFrameHostManager::GetAllProxyHostsForTesting() {
   return result;
 }
 
-void RenderFrameHostManager::CreateOpenerProxiesIfNeeded(
-    SiteInstance* instance) {
-  FrameTreeNode* opener = frame_tree_node_->opener();
-  if (!opener)
-    return;
-
-  // TODO(alexmos): This should process all openers in the current frame tree,
-  // not just current node's opener.
-
-  // 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) {
@@ -2481,7 +2472,9 @@ void RenderFrameHostManager::CollectOpenerFrameTrees(
   }
 }
 
-void RenderFrameHostManager::CreateOpenerProxies(SiteInstance* instance) {
+void RenderFrameHostManager::CreateOpenerProxies(
+    SiteInstance* instance,
+    FrameTreeNode* skip_this_node) {
   std::vector<FrameTree*> opener_frame_trees;
   base::hash_set<FrameTreeNode*> nodes_with_back_links;
 
@@ -2495,7 +2488,7 @@ void RenderFrameHostManager::CreateOpenerProxies(SiteInstance* instance) {
     opener_frame_trees[i]
         ->root()
         ->render_manager()
-        ->CreateOpenerProxiesForFrameTree(instance);
+        ->CreateOpenerProxiesForFrameTree(instance, skip_this_node);
   }
 
   // Set openers for nodes in |nodes_with_back_links| in a second pass.
@@ -2522,39 +2515,46 @@ void RenderFrameHostManager::CreateOpenerProxies(SiteInstance* instance) {
 }
 
 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
-  // site-per-process mode).  An exception is if we are in
-  // IsSwappedOutStateForbidden mode and find a pending RenderViewHost: in this
-  // case, we should still create a proxy, which will allow communicating with
-  // the opener until the pending RenderView commits, or if the pending
-  // navigation is canceled.
-  FrameTree* frame_tree = frame_tree_node_->frame_tree();
-  RenderViewHostImpl* rvh = frame_tree->GetRenderViewHost(instance);
-  bool need_proxy_for_pending_rvh =
-      SiteIsolationPolicy::IsSwappedOutStateForbidden() &&
-      (rvh == pending_render_view_host());
-  if (rvh && rvh->IsRenderViewLive() && !need_proxy_for_pending_rvh)

[alexmos, 2015/08/25 22:38:08]

I've moved this logic to only be used when not in --site-per-process.  I found
that sometimes this isn't actually a sufficient condition to exit early and
assume all proxies have been created, with an example setup in
RenderFrameHostManagerTest.DetachPendingChild.  Basically, suppose a subframe in
this tree navigates from A to B.  This creates a swapped-out RVH for its tree in
B.  Now, while that subframe is still pending, suppose another frame navigated
to B and we get here trying to create a proxy in B for the first subframe.  We
should create that proxy so that if the second frame commits earlier, it has a
way to talk to the first subframe until that also commits (if ever), but
currently, this early-exits without creating a proxy, since the swapped-out RVH
in B already exists.

Calling CreateProxiesForSiteInstance when the proxies already exist already has
the right early-exits, and I think the RVH checks actually makes more sense this
way, as they're tied to swapped-out RVH creation.

[Charlie Reis, 2015/08/31 20:49:43]

Acknowledged.

+    SiteInstance* instance,
+    FrameTreeNode* skip_this_node) {
+  DCHECK(frame_tree_node_->IsMainFrame());

[Charlie Reis, 2015/08/31 20:49:43]

This doesn't look like it's true.  There's a few call sites above that look like
they could be made from a subframe RFHM.  Is there a reason you need it to start
from the main frame?

[alexmos, 2015/09/01 22:12:27]

Currently, CreateOpenerProxies always calls CreateOpenerProxiesForFrameTree on
the root node for each discovered FrameTree on the opener chain:

   opener_frame_trees[i]
        ->root()   // <-- here
        ->render_manager()
        ->CreateOpenerProxiesForFrameTree(instance);

This is because multiple nodes may have been pointing at the same FrameTree, so
there may not be a single node on which to call it.  So even if
CreateOpenerProxies is called on a subframe FTN, it would use the root node when
calling CreateOpenerProxiesForFrameTree.

CreateOpenerProxiesForFrameTree doesn't actually need this assumption (most of
its logic is tied to FrameTrees, not specific nodes, and it will work correctly
when called on subframe RFHMs), so the DCHECK is just to convey that today
nobody is calling this on subframes.  Given all this, what should I do with it?

I guess if we keep it, I should also mention the main frame expectation in the
function's comment and remove the unneeded root() below when creating
swapped-out RV.

[Charlie Reis, 2015/09/01 23:37:26]

Oh, I see.  Yes, that's a little subtle.  I think it's fine to keep as long as
there's a comment about it (either on the check or in the declaration).  If you
do remove the unneeded root() call below, you might want to strengthen it from a
DCHECK to a CHECK (since the method won't do the right thing anymore if we end
up with rare cases where it's called on the subframe).

[alexmos, 2015/09/02 00:02:11]

Thanks, I kept the check and added a comment above it.  I kept the root() as
defense-in-depth, since having it there doesn't really cost us much and may
prevent future bugs.

+  if (frame_tree_node_ == skip_this_node)
     return;
 
-  if (SiteIsolationPolicy::IsSwappedOutStateForbidden()) {
-    // Ensure that all the nodes in the opener's frame tree have
-    // RenderFrameProxyHosts for the new SiteInstance.
-    frame_tree->CreateProxiesForSiteInstance(nullptr, instance);
-  } else if (rvh && !rvh->IsRenderViewLive()) {
-    EnsureRenderViewInitialized(rvh, instance);
+  FrameTree* frame_tree = frame_tree_node_->frame_tree();
+  if (SiteIsolationPolicy::AreCrossProcessFramesPossible()) {

[alexmos, 2015/08/25 22:38:08]

I think we've been meaning to change this to AreCrossProcessFramesPossible in
Nick's policy CL, but forgot.  Since CreateProxiesForSiteInstance creates
proxies for subframes, I think this is the right gating function for this, and
this also matches the condition used by the other call site of
CreateProxiesForSiteInstance.

[Charlie Reis, 2015/08/31 20:49:43]

Acknowledged.

+    // Ensure that all the nodes in the opener's FrameTree have
+    // RenderFrameProxyHosts for the new SiteInstance.  Only pass the node to
+    // be skipped if it's in the same FrameTree.
+    if (skip_this_node && skip_this_node->frame_tree() != frame_tree)
+      skip_this_node = nullptr;
+    frame_tree->CreateProxiesForSiteInstance(skip_this_node, instance);

[alexmos, 2015/08/25 22:38:08]

We were always passing nullptr here, assuming none of the openers' FrameTrees
will ever be the same as the FrameTree of the node that is being navigated.  

This isn't true anymore, as I'm calling CreateOpenerProxies on the node itself,
rather than its opener, meaning we start right from the node's FrameTree (and we
need to skip the node itself).

I think this was already a problem even without this CL though: with opener
updates, the opener chain could lead us back to the original navigating node's
FrameTree and pass null here, which would crash trying to create a proxy in the
node's own current SiteInstance.

[Charlie Reis, 2015/08/31 20:49:43]

Acknowledged.

   } else {
-    // Create a swapped out RenderView in the given SiteInstance if none exists,
-    // setting its opener to the given route_id.  Since the opener can point to
-    // a subframe, do this on the root frame of the opener's frame tree.
-    // Return the new view's route_id.
-    frame_tree->root()->render_manager()->
-        CreateRenderFrame(instance, nullptr,
-                          CREATE_RF_FOR_MAIN_FRAME_NAVIGATION |
-                          CREATE_RF_SWAPPED_OUT | CREATE_RF_HIDDEN,
-                          nullptr);
+    // If any of the RenderViewHosts (current, pending, or swapped out) for this
+    // FrameTree has the same SiteInstance, then we can return early and reuse
+    // them.  An exception is if we are in IsSwappedOutStateForbidden mode and
+    // find a pending RenderViewHost: in this case, we should still create a
+    // proxy, which will allow communicating with the opener until the pending
+    // RenderView commits, or if the pending navigation is canceled.
+    RenderViewHostImpl* rvh = frame_tree->GetRenderViewHost(instance);
+    bool need_proxy_for_pending_rvh =
+        SiteIsolationPolicy::IsSwappedOutStateForbidden() &&
+        (rvh == pending_render_view_host());
+    if (rvh && rvh->IsRenderViewLive() && !need_proxy_for_pending_rvh)
+      return;
+
+    if (rvh && !rvh->IsRenderViewLive()) {
+      EnsureRenderViewInitialized(rvh, instance);
+    } else {
+      // Create a swapped out RenderView in the given SiteInstance if none
+      // exists. Since an opener can point to a subframe, do this on the root
+      // frame of the current opener's frame tree.
+      frame_tree->root()->render_manager()->
+          CreateRenderFrame(instance, nullptr,
+                            CREATE_RF_FOR_MAIN_FRAME_NAVIGATION |
+                            CREATE_RF_SWAPPED_OUT | CREATE_RF_HIDDEN,
+                            nullptr);
+    }
   }
 }