Move browser-to-renderer opener plumbing to frame routing IDs

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 cd85433714fe8f693e255ef4be2f90db85e957f3..958c6910df9cc98f7793232a3293995975b55451 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -277,12 +277,11 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
     dest_render_frame_host->SetUpMojoIfNeeded();
 
     // Recreate the opener chain.
-    int opener_route_id =
-        CreateOpenerProxiesIfNeeded(dest_render_frame_host->GetSiteInstance());
+    CreateOpenerProxiesIfNeeded(dest_render_frame_host->GetSiteInstance());
     if (!InitRenderView(dest_render_frame_host->render_view_host(),
-                        opener_route_id,
                         MSG_ROUTING_NONE,
-                        frame_tree_node_->IsMainFrame()))
+                        frame_tree_node_->IsMainFrame(),
+                        false))
       return NULL;
 
     // Now that we've created a new renderer, be sure to hide it if it isn't
@@ -863,10 +862,9 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
   // created or has crashed), initialize it.
   if (!navigation_rfh->IsRenderFrameLive()) {
     // Recreate the opener chain.
-    int opener_route_id =
-        CreateOpenerProxiesIfNeeded(navigation_rfh->GetSiteInstance());
-    if (!InitRenderView(navigation_rfh->render_view_host(), opener_route_id,
-                        MSG_ROUTING_NONE, frame_tree_node_->IsMainFrame())) {
+    CreateOpenerProxiesIfNeeded(navigation_rfh->GetSiteInstance());
+    if (!InitRenderView(navigation_rfh->render_view_host(), MSG_ROUTING_NONE,
+                        frame_tree_node_->IsMainFrame(), false)) {

[nasko, 2015/07/07 16:26:21]

Sprinkling a ton of extra "false" params is bugging me a bit. Can we do it some
other way, such that the majority of the call sites (using false) will be
simpler?

[alexmos, 2015/07/08 04:42:18]

Yeah, I agree it's pretty annoying.  One alternative is to switch InitRenderView
to take a flags enum like CreateRenderFrameFlags.  That would be more readable,
but I don't know if it's worth doing for just two bools (plus, it'd need to
convert things like IsMainFrame() here into flag values).  Another option could
be different versions of InitRenderView (e.g., InitRenderView and
InitRenderViewWithoutOpener calling a private third method that takes the bool) 
-- that may be a bit much for this though?  Or maybe just add a
/*suppress_opener=*/ before the "false" to improve call site readability, which
I see is sometimes used elsewhere in the code base.  Any thoughts on these?

[nasko, 2015/07/08 09:35:19]

Yeah, I think the flag version is probably not worthwhile at this time.
It isn't just InitRenderView that is a problem. CreateRenderFrameProxies also
has
the bool, which it passes over to InitRenderView. From all the callers of 
CreateRenderFrameProxies, only <webview> cares to suppress the opener.
Do we really have an opener in this case? Where does it come from? Can we neuter
the opener relationship earlier so we don't need this boolean?
An alternative (which I don't quite like) is to create it still unconditionally,
but have the <webview> code send an IPC to disown the opener. The problem with 
that is the extra IPC and async nature of creation and clearing the opener.
Nah, I think adding comment isn't really worth it.

[alexmos, 2015/07/08 15:09:27]

I suppose the scenario here is some page A opening a new window with a
<webview>.  In this case,
we want the embedder to still be able to talk to A as its opener, and we want to
allow the guest to talk to its embedder, but we want to disallow the guest from
talking to embedder's opener.  Doesn't seem like we can sever the opener
relationship earlier, as then the embedder won't be able to talk to its opener. 
Disowning the opener may work if we send the IPC it to the guest's process for
the embedder's swapped-out RV, without actually setting the embedder's opener to
null in the browser process, but that seems like it introduces more complexity
and will require exposing the disown-opener mechanism at the WebContents layer
(which I just took away in my last CL).

Doing some code archealogy, this was introduced with
https://codereview.chromium.org/10829225, and this comment by Charlie for a
related piece of code kind of covers the rationale:
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...

Looking at this, CreateSwappedOutRenderView is really intended as a version of
CreateOpenerProxies that only walks one level of the opener chain.  I was
thinking about this alternative: have CreateOpenerProxies take a suppress_opener
bool and not recurse further if it's true.  Then, the call site in
CreateOpenerProxiesIfNeeded (which redirects most other calls to this) will pass
false, and the two calls to CreateSwappedOutRenderView from <webview> can become
CreateOpenerProxies which will pass true.  So the bool, while still there, will
affect only one function, CreateOpenerProxies, with only three call sites (plus
one in a test).  Plus, CreateSwappedOutRenderView can go away.  However, that
requires us to still pass back the RenderView ID from CreateOpenerProxies (since
<webview> needs it in
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...)
-- yuck.  That won't play well if your swapped-out removal patch for
CreateOpenerProxies, though it seems like you're already breaking that, judging
by how that view ID is left as NONE in CreateSwappedOutRenderView in case of
IsSwappedOutForbidden. 

Another alternative is to maintain a flag like
suppress_opener_for_swapped_out_views in RFHM, which InitRenderView would
consult, and set the flag on the on the embedder's RFHM as first thing in
CreateSwappedOutRenderView.  Thoughts?

[alexmos, 2015/07/09 02:43:23]

Thinking more about this, I think we may actually be ok if we just always look
up the opener in InitRenderView and remove suppress_opener altogether.  The
reason is that <webview> already calls CreateSwappedOutRenderView instead of
CreateOpenerProxies for the embedder, so we won't create any new RVs for the
embedder's opener(s) in the guest SiteInstance.  Thus, when we later go and look
up the embedder's opener for the guest's SiteInstance in InitRenderView, we
won't find it, *unless* it has existed before.  The only scenario in which the
latter can happen is if the guest is in the same SiteInstance as the embedder's
opener.  Neither I nor Charlie could come up with an example of how this could
happen today, though it's plausible that it may happen in the future with
extensionview/appview -- but even then, what I think this means is that if the
embedder sends a postMessage to guest, the guest will be able to reach the
embedder's opener via event.source.opener, but since the two are in the same
SiteInstance, I think this is ok since we aren't really increasing the attack
surface.

So, I've rolled back the suppress_opener plumbing - please take a look at PS13
and let me know what you think.  All tests still pass. :)  Note that there will
be another slight change of behavior (which I discovered with some experimental
DCHECKs back in PS6).  If the guest opens a new window, <webview> code will call
CreateSwappedOutRenderView to create an RV for the new window in embedder's
process, here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br....
 With my new approach, the new window will have a valid window.opener pointer to
its opener in the embedder process, whereas before, it didn't (since
CreateSwappedOutRenderView always passed NONE for opener ID).  I think the new
behavior is actually more correct than old behavior (e.g., I think it allows the
embedder to listen for a newwindow event and then use event.window.opener, which
wouldn't have worked previously - see WebViewAPITest.TestNewWindow for an
example of how this can be set up), though I don't know if anyone cares about
this in practice.

       return nullptr;
     }
 
@@ -1399,23 +1397,21 @@ void RenderFrameHostManager::CreatePendingRenderFrameHost(
   if (!new_instance->GetProcess()->Init())
     return;
 
-  int opener_route_id = CreateProxiesForNewRenderFrameHost(
-      old_instance, new_instance, &create_render_frame_flags);
+  CreateProxiesForNewRenderFrameHost(old_instance, new_instance,
+                                     &create_render_frame_flags);
 
   // Create a non-swapped-out RFH with the given opener.
-  pending_render_frame_host_ =
-      CreateRenderFrame(new_instance, pending_web_ui(), opener_route_id,
-                        create_render_frame_flags, nullptr);
+  pending_render_frame_host_ = CreateRenderFrame(
+      new_instance, pending_web_ui(), create_render_frame_flags, nullptr);
 }
 
-int RenderFrameHostManager::CreateProxiesForNewRenderFrameHost(
+void RenderFrameHostManager::CreateProxiesForNewRenderFrameHost(
     SiteInstance* old_instance,
     SiteInstance* new_instance,
     int* create_render_frame_flags) {
-  int opener_route_id = MSG_ROUTING_NONE;
   // Only create opener proxies if they are in the same BrowsingInstance.
   if (new_instance->IsRelatedSiteInstance(old_instance))
-    opener_route_id = CreateOpenerProxiesIfNeeded(new_instance);
+    CreateOpenerProxiesIfNeeded(new_instance);
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSitePerProcess)) {
     // Ensure that the frame tree has RenderFrameProxyHosts for the new
@@ -1433,7 +1429,6 @@ int RenderFrameHostManager::CreateProxiesForNewRenderFrameHost(
             new_instance)
       *create_render_frame_flags |= CREATE_RF_NEEDS_RENDER_WIDGET_HOST;
   }
-  return opener_route_id;
 }
 
 scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
@@ -1491,8 +1486,8 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
     return false;
 
   int create_render_frame_flags = 0;
-  int opener_route_id = CreateProxiesForNewRenderFrameHost(
-      old_instance, new_instance, &create_render_frame_flags);
+  CreateProxiesForNewRenderFrameHost(old_instance, new_instance,
+                                     &create_render_frame_flags);
 
   if (frame_tree_node_->IsMainFrame())
     create_render_frame_flags |= CREATE_RF_FOR_MAIN_FRAME_NAVIGATION;
@@ -1500,7 +1495,7 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
     create_render_frame_flags |= CREATE_RF_HIDDEN;
   speculative_render_frame_host_ =
       CreateRenderFrame(new_instance, speculative_web_ui_.get(),
-                        opener_route_id, create_render_frame_flags, nullptr);
+                        create_render_frame_flags, nullptr);
 
   if (!speculative_render_frame_host_) {
     speculative_web_ui_.reset();
@@ -1512,7 +1507,6 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
 scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
     SiteInstance* instance,
     WebUIImpl* web_ui,
-    int opener_route_id,
     int flags,
     int* view_routing_id_ptr) {
   bool swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
@@ -1574,9 +1568,9 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
       proxy->TakeFrameHostOwnership(new_render_frame_host.Pass());
     }
 
-    success =
-        InitRenderView(render_view_host, opener_route_id, proxy_routing_id,
-                       !!(flags & CREATE_RF_FOR_MAIN_FRAME_NAVIGATION));
+    success = InitRenderView(render_view_host, proxy_routing_id,
+                             !!(flags & CREATE_RF_FOR_MAIN_FRAME_NAVIGATION),
+                             !!(flags & CREATE_RF_SUPPRESS_OPENER));
     if (success) {
       // Remember that InitRenderView also created the RenderFrameProxy.
       if (swapped_out)
@@ -1629,7 +1623,8 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
   return nullptr;
 }
 
-int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance) {
+int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance,
+                                                   bool suppress_opener) {
   // A RenderFrameProxyHost should never be created in the same SiteInstance as
   // the current RFH.
   CHECK(instance);
@@ -1661,8 +1656,8 @@ int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance) {
 
   if (RenderFrameHostManager::IsSwappedOutStateForbidden() &&
       frame_tree_node_->IsMainFrame()) {
-    InitRenderView(
-        render_view_host, MSG_ROUTING_NONE, proxy->GetRoutingID(), true);
+    InitRenderView(render_view_host, proxy->GetRoutingID(), true,
+                   suppress_opener);
     proxy->set_render_frame_proxy_created(true);
   } else {
     proxy->InitRenderFrameProxy();
@@ -1674,7 +1669,7 @@ int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance) {
 void RenderFrameHostManager::CreateProxiesForChildFrame(FrameTreeNode* child) {
   for (const auto& pair : proxy_hosts_) {
     child->render_manager()->CreateRenderFrameProxy(
-        pair.second->GetSiteInstance());
+        pair.second->GetSiteInstance(), false);
   }
 }
 
@@ -1687,7 +1682,7 @@ void RenderFrameHostManager::EnsureRenderViewInitialized(
     return;
 
   // Recreate the opener chain.
-  int opener_route_id = CreateOpenerProxiesIfNeeded(instance);
+  CreateOpenerProxiesIfNeeded(instance);
 
   // If the proxy in |instance| doesn't exist, this RenderView is not swapped
   // out and shouldn't be reinitialized here.
@@ -1695,8 +1690,7 @@ void RenderFrameHostManager::EnsureRenderViewInitialized(
   if (!proxy)
     return;
 
-  InitRenderView(render_view_host, opener_route_id, proxy->GetRoutingID(),
-                 false);
+  InitRenderView(render_view_host, proxy->GetRoutingID(), false, false);
   proxy->set_render_frame_proxy_created(true);
 }
 
@@ -1731,9 +1725,9 @@ void RenderFrameHostManager::SetRWHViewForInnerContents(
 
 bool RenderFrameHostManager::InitRenderView(
     RenderViewHostImpl* render_view_host,
-    int opener_route_id,
     int proxy_routing_id,
-    bool for_main_frame_navigation) {
+    bool for_main_frame_navigation,
+    bool suppress_opener) {
   // Ensure the renderer process is initialized before creating the
   // RenderView.
   if (!render_view_host->GetProcess()->Init())
@@ -1765,12 +1759,15 @@ bool RenderFrameHostManager::InitRenderView(
     }
   }
 
+  int opener_frame_routing_id = MSG_ROUTING_NONE;
+  if (!suppress_opener) {
+    opener_frame_routing_id =
+        GetOpenerRoutingID(render_view_host->GetSiteInstance());
+  }
+
   return delegate_->CreateRenderViewForRenderManager(
-      render_view_host,
-      opener_route_id,
-      proxy_routing_id,
-      frame_tree_node_->current_replication_state(),
-      for_main_frame_navigation);
+      render_view_host, opener_frame_routing_id, proxy_routing_id,
+      frame_tree_node_->current_replication_state(), for_main_frame_navigation);
 }
 
 bool RenderFrameHostManager::InitRenderFrame(
@@ -1826,10 +1823,13 @@ int RenderFrameHostManager::GetRoutingIdForSiteInstance(
   if (render_frame_host_->GetSiteInstance() == site_instance)
     return render_frame_host_->GetRoutingID();
 
-  RenderFrameProxyHostMap::iterator iter =
-      proxy_hosts_.find(site_instance->GetId());
-  if (iter != proxy_hosts_.end())
-    return iter->second->GetRoutingID();
+  if (pending_render_frame_host_ &&
+      pending_render_frame_host_->GetSiteInstance() == site_instance)
+    return pending_render_frame_host_->GetRoutingID();

[alexmos, 2015/07/06 23:56:42]

See WebContentsImplTest.FindOpenerRVHWhenPending for context of why this is
needed.  Previously, CreateOpenerProxies took care of this by looking up and
returning the pending RVH's routing ID.

[nasko, 2015/07/07 16:26:21]

This has me slightly concerned. If we have already a proxy for SiteInstance A
and the current RFH is for SiteInstance B, then navigating cross-process to a
URL in SiteInstance A will create a pending RFH for it, which now will be
returned. Previously, we would've returned the proxy.

This method is called in a bunch of contexts for currently running documents, so
I think it is not quite correct to return a pending RFH. Maybe we can gate this
on "IsSwappedOutForbidden", in which case we will never store RFHs and expect to
find them again. I assume that if we reorder the pending RFH and the proxy, we
will return the proxy and that will still be breaking the test case, right?

[alexmos, 2015/07/08 04:42:18]

Interestingly, if I reorder the pending RFH and the proxy checks, the
FindOpenerRVHWhenPending test still passes: it still returns the pending RFH,
both with --site-per-process and without.  That's because for that scenario
there's no proxy today even if IsSwappedOutForbidden is true. Charlie and I
discussed this and thought that this is probably wrong - there should be a proxy
if IsSwappedOutForbidden is true.

So just to walk through what's happening with this test (which is a bit
different from your example).  There's a window currently at A that navigates to
B and creates a pending RFH for B.  With that pending, A opens a popup and also
navigates it to B.  The question is what should happen when the popup's pending
RFH creation runs CreateOpenerProxies(B) on the popup's opener.  At that point
the opener doesn't have a proxy for B, and the question is - should it create
one, or should it reuse the pending RFH's routing ID?  Currently, it doesn't
create the proxy, not even in --site-per-process (CreateOpenerProxies returns
early if there's a pending RVH).  This seems wrong, since if the opener's
navigation never commits, we still want to deliver window.opener.postMessages to
A.  It seems like think we should fix this to create a proxy for the opener in B
(if IsSwappedOutForbidden is true), and the proxy should go away if/when the
opener's navigation commits.  Does that make sense?

I haven't tried to fix this here yet.  Actually, I'm wondering whether it's
better to split that off into a follow-up CL, since we're already broken here
and this CL is about something a bit different.  In the meantime, I've followed
your suggestion to only return a pending RFH if swapped out exists -- I agree we
need that regardless of this proxy creation fix.  Same for the corresponding
check in the FindOpenerRVHWhenPending test.

Re: "this method is called in a bunch of contexts for currently running
documents" - I saw only two call sites, both in InitRenderFrame, for looking up
the parent and previous sibling's routing IDs.  Were you thinking of some other
uses?  Both of these ones assume the result is always a RenderFrameProxy on the
renderer side, so yeah, it'd be problematic if they ever receive a pending RFH's
routing ID instead (though I'm not sure if either call site is reachable at a
time when there's a pending RFH).

[nasko, 2015/07/08 09:35:19]

Thanks for the thorough explanation. Yes, indeed this makes sense. I don't think

we should reuse the pending RFH for B in the opener as the object/routing id. We
should be creating a proxy, which will then be deleted if/when the opener
commits.
This way we don't have to have special casing code dealing with the case of 
opener cancelling the navigation, destroying the pending RFH and having to
create
a proxy. It seems way more convoluted than using our existing primitives -
create
a proxy, swap it with a frame on commit.
I agree it should be a separate CL. Big/complex CLs are hard to review, are hard
to reason about, and take a ton more time. Also, distinct CLs for distinct
problems
are better approach from code history perspective too.
 
Hmmm, I thought it was more than that. My bad.

+
+  RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(site_instance);
+  if (proxy)
+    return proxy->GetRoutingID();
 
   return MSG_ROUTING_NONE;
 }
@@ -2272,19 +2272,17 @@ void RenderFrameHostManager::DeleteRenderFrameProxyHost(
   }
 }
 
-int RenderFrameHostManager::CreateOpenerProxiesIfNeeded(
+void RenderFrameHostManager::CreateOpenerProxiesIfNeeded(
     SiteInstance* instance) {
   FrameTreeNode* opener = frame_tree_node_->opener();
   if (!opener)
-    return MSG_ROUTING_NONE;
+    return;
 
   // Create proxies for the opener chain.
-  return opener->render_manager()->CreateOpenerProxies(instance);
+  opener->render_manager()->CreateOpenerProxies(instance);
 }
 
-int RenderFrameHostManager::CreateOpenerProxies(SiteInstance* instance) {
-  int opener_route_id = MSG_ROUTING_NONE;
-
+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
@@ -2294,39 +2292,42 @@ int RenderFrameHostManager::CreateOpenerProxies(SiteInstance* instance) {
   // be traversed).
   FrameTreeNode* opener = frame_tree_node_->opener();
   if (opener)
-    opener_route_id = opener->render_manager()->CreateOpenerProxies(instance);
+    opener->render_manager()->CreateOpenerProxies(instance);
 
   // If any of the RenderViews (current, pending, or swapped out) for this
-  // FrameTree has the same SiteInstance, use it.  If such a RenderView exists,
-  // that implies that proxies for all nodes in the tree should also exist
-  // (when in site-per-process mode), so it is safe to exit early.
+  // 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).
   FrameTree* frame_tree = frame_tree_node_->frame_tree();
   RenderViewHostImpl* rvh = frame_tree->GetRenderViewHost(instance);
   if (rvh && rvh->IsRenderViewLive())
-    return rvh->GetRoutingID();
+    return;
 
-  int render_view_routing_id = MSG_ROUTING_NONE;
   if (RenderFrameHostManager::IsSwappedOutStateForbidden()) {
     // Ensure that all the nodes in the opener's frame tree have
     // RenderFrameProxyHosts for the new SiteInstance.
     frame_tree->CreateProxiesForSiteInstance(nullptr, instance);
-    rvh = frame_tree->GetRenderViewHost(instance);
-    render_view_routing_id = rvh->GetRoutingID();
   } else if (rvh && !rvh->IsRenderViewLive()) {
     EnsureRenderViewInitialized(rvh, instance);
-    render_view_routing_id = rvh->GetRoutingID();
   } 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, opener_route_id,
+        CreateRenderFrame(instance, nullptr,
                           CREATE_RF_FOR_MAIN_FRAME_NAVIGATION |
                           CREATE_RF_SWAPPED_OUT | CREATE_RF_HIDDEN,
-                          &render_view_routing_id);
+                          nullptr);
   }
-  return render_view_routing_id;
+}
+
+int RenderFrameHostManager::GetOpenerRoutingID(SiteInstance* instance) {
+  if (!frame_tree_node_->opener())
+    return MSG_ROUTING_NONE;
+  return frame_tree_node_->opener()

[nasko, 2015/07/07 16:26:21]

nit: Empty line between the two returns makes it slightly more readable.

[alexmos, 2015/07/08 04:42:18]

Done.

+      ->render_manager()
+      ->GetRoutingIdForSiteInstance(instance);
 }
 
 }  // namespace content