Support frame tree propagation between renderers in the same browsing instance.

content/browser/web_contents/render_view_host_manager.cc

Index: content/browser/web_contents/render_view_host_manager.cc
diff --git a/content/browser/web_contents/render_view_host_manager.cc b/content/browser/web_contents/render_view_host_manager.cc
index 62fec79d74de5e16ec285817f4bc86b6f9eb9689..92c7f3e8c565bfa474406ebb811ccd59ac8b4efb 100644
--- a/content/browser/web_contents/render_view_host_manager.cc
+++ b/content/browser/web_contents/render_view_host_manager.cc
@@ -239,6 +239,26 @@ void RenderViewHostManager::DidNavigateMainFrame(
   }
 }
 
+void RenderViewHostManager::DidUpdateFrameTree(
+    RenderViewHost* render_view_host) {
+  DCHECK_EQ(render_view_host, current_host());

[Charlie Reis, 2012/08/22 22:08:34]

Are there any races that could cause this to be called when render_view_host is
no longer active (e.g., right after the browser process has swapped in a new
one), or does the filtering of swapped out messages prevent that?

Also, should we use a CHECK here, or do we need to return early if the DCHECK
fails?

[nasko, 2012/08/23 21:55:53]

Based on the cross-site transition timeline, it shouldn't be possible to receive
a frame tree update because of the filtering. The renderer starts filtering the
moment it runs the unload handler. It is possible for a frame update to be sent
prior to that, but the swap of pending render to active renderer doesn't happen
until we do RVHM::CommitPending. At that point, we either have processed any
outstanding frame update messages or are filtering it in the renderer. 
As such, I believe a CHECK is useful here.

+
+  RenderViewHostImpl* render_view_host_impl = static_cast<RenderViewHostImpl*>(
+      render_view_host);
+
+  for (RenderViewHostMap::iterator iter = swapped_out_hosts_.begin();
+       iter != swapped_out_hosts_.end();
+       ++iter) {
+    DCHECK_NE(iter->second->GetSiteInstance(),
+        current_host()->GetSiteInstance());
+
+    iter->second->UpdateFrameTree(
+        render_view_host_impl->GetProcess()->GetID(),

[Charlie Reis, 2012/08/22 22:08:34]

Interesting...  I wonder what happens if one of these processes has crashed. 
Does its RenderViewHosts automatically get removed from the swapped out list, or
might we be creating a new process here in by calling GetProcess()?

[nasko, 2012/08/23 21:55:53]

GetProcess() returns the RenderProcessHost, which is the wrapper around the
actual underlying process. It's ID doesn't change, even though the underlying OS
process can crash and be re-spawned. So this line should not be creating a new
OS process.

[Charlie Reis, 2012/08/24 23:26:07]

Ah, right you are!  I was thinking of SiteInstance::GetProcess.

+        render_view_host_impl->GetRoutingID(),
+        render_view_host_impl->frame_tree());
+  }
+}
+
 void RenderViewHostManager::SetWebUIPostCommit(WebUIImpl* web_ui) {
   DCHECK(!web_ui_.get());
   web_ui_.reset(web_ui);
@@ -495,6 +515,15 @@ SiteInstance* RenderViewHostManager::GetSiteInstanceForEntry(
     if (entry.IsViewSourceMode())
       return SiteInstance::CreateForURL(browser_context, dest_url);
 
+    // If we are navigating from a blank SiteInstance to a WebUI, make sure we
+    // create a new SiteInstance.

[Charlie Reis, 2012/08/22 22:08:34]

Wait, shouldn't HasWrongProcessForURL have caught this case?  The comment on
line 504 suggests that it's supposed to catch WebUI.

[nasko, 2012/08/23 21:55:53]

The first thing HasWrongProcessForURL checks is whether there is a process
attached to the site instance or not. If not, it returns false. This is the
reason why it doesn't catch it, since the site instance is empty and no process
has been created to render "about:blank".

[Charlie Reis, 2012/08/24 23:26:07]

:(

Good catch.  This is fine, then, and I'll clean it up as part of 123007.

+    const WebUIControllerFactory* web_ui_factory =
+        content::GetContentClient()->browser()->GetWebUIControllerFactory();
+    if (web_ui_factory &&
+        web_ui_factory->UseWebUIForURL(browser_context, dest_url)) {
+        return SiteInstance::CreateForURL(browser_context, dest_url);
+    }
+
     // Normally the "site" on the SiteInstance is set lazily when the load
     // actually commits. This is to support better process sharing in case
     // the site redirects to some other site: we want to use the destination
@@ -577,6 +606,16 @@ int RenderViewHostManager::CreateRenderView(
     bool swapped_out) {
   CHECK(instance);
 
+  // If the current host is already in the site instance we are looking for

[Charlie Reis, 2012/08/22 22:08:34]

nit: site instance -> SiteInstance

[nasko, 2012/08/23 21:55:53]

Done.

+  // and we are creating a swapped out renderer, just return the routing id.
+  if (current_host()->GetSiteInstance() == instance) {
+    // TODO(nasko): Convert this if statement to a DCHECK once all causes
+    // of creating a new non-swapped out RVH for the same SiteInstance as the
+    // active one are tracked down and resolved.

[Charlie Reis, 2012/08/22 22:08:34]

Nice.  Please mention http://crbug.com/123007 here.

[nasko, 2012/08/23 21:55:53]

Done.

+    if (swapped_out)

[Charlie Reis, 2012/08/22 22:08:34]

This is actually a little confusing.  The caller is requesting a swapped_out
RenderView, but we're returning the routing ID of an active one.  I'm now
wondering if it's clearer to have the caller check whether there's an active one
in the given SiteInstance before requesting a swapped out one.  Thoughts?

[nasko, 2012/08/23 21:55:53]

Yes, changing the caller makes a bit more sense. I've done it and removed this
piece of code. I will file a separate bug about creating duplicate renderers for
the same SiteInstance and we can track those cases separately.

[Charlie Reis, 2012/08/24 23:26:07]

Great.  Please CC me on the bug.

+      return current_host()->GetRoutingID();
+  }
+
   // Check if we've already created an RVH for this SiteInstance.  If so, try
   // to re-use the existing one, which has already been initialized.  We'll
   // remove it from the list of swapped out hosts if it commits.
@@ -607,6 +646,15 @@ int RenderViewHostManager::CreateRenderView(
     if (success) {
       // Don't show the view until we get a DidNavigate from it.
       new_render_view_host->GetView()->Hide();
+
+      // If we are creating a swapped out RVH, send a message to update the

[Charlie Reis, 2012/08/22 22:08:34]

nit: the -> its

[nasko, 2012/08/23 21:55:53]

Done.

+      // frame tree based on the active RVH for this RenderViewHostManager.
+      if (swapped_out) {
+        new_render_view_host->UpdateFrameTree(
+            current_host()->GetProcess()->GetID(),
+            current_host()->GetRoutingID(),
+            current_host()->frame_tree());
+      }
     } else if (!swapped_out) {
       CancelPending();
     }
@@ -920,7 +968,7 @@ bool RenderViewHostManager::IsSwappedOut(RenderViewHost* rvh) {
       swapped_out_hosts_.end();
 }
 
-RenderViewHost* RenderViewHostManager::GetSwappedOutRenderViewHost(
+RenderViewHostImpl* RenderViewHostManager::GetSwappedOutRenderViewHost(
     SiteInstance* instance) {
   RenderViewHostMap::iterator iter = swapped_out_hosts_.find(instance->GetId());
   if (iter != swapped_out_hosts_.end())