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..c47467fb46ee7f1fe128a2b78a235dea03ead7a8 100644
--- a/content/browser/web_contents/render_view_host_manager.cc
+++ b/content/browser/web_contents/render_view_host_manager.cc
@@ -239,6 +239,25 @@ void RenderViewHostManager::DidNavigateMainFrame(
   }
 }
 
+void RenderViewHostManager::DidUpdateFrameTree(
+    RenderViewHost* render_view_host) {
+  DCHECK(render_view_host == current_host());

[awong, 2012/08/21 19:27:28]

DCHECK_EQ?

[nasko, 2012/08/21 22:24:13]

Done.

+
+  RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(render_view_host);

[awong, 2012/08/21 19:27:28]

Shortenings like rvh aren't commonly used in this file.

Maybe render_view_host_impl? (yeah, it's longer :( )

[nasko, 2012/08/21 22:24:13]

Done.

+  if (!rvh)

[awong, 2012/08/21 19:27:28]

For parameter validation, do this as close to the top as possible (before the
cast even, but probably after the DCHECK which is enforcing a stronger
invariant).

However, I would have expected this function to never be called if rvh is NULL. 
When could rvh be NULL in normal operation?  If that's not supposed to happen,
then we should crash rather than try to silently recover and mask bad behavior.

[nasko, 2012/08/21 22:24:13]

Done.

+    return;
+
+  for (RenderViewHostMap::iterator iter = swapped_out_hosts_.begin();
+       iter != swapped_out_hosts_.end();
+       ++iter) {
+    DCHECK(iter->second->GetSiteInstance() !=

[awong, 2012/08/21 19:27:28]

DCHECK_NE?

[nasko, 2012/08/21 22:24:13]

Done.

+        current_host()->GetSiteInstance());
+
+    iter->second->UpdateFrameTree(
+        rvh->GetProcess()->GetID(), rvh->GetRoutingID(), rvh->frame_tree());
+  }
+}
+
 void RenderViewHostManager::SetWebUIPostCommit(WebUIImpl* web_ui) {
   DCHECK(!web_ui_.get());
   web_ui_.reset(web_ui);
@@ -495,6 +514,14 @@ SiteInstance* RenderViewHostManager::GetSiteInstanceForEntry(
     if (entry.IsViewSourceMode())
       return SiteInstance::CreateForURL(browser_context, dest_url);
 
+    // If we are navigating from blank SiteInstance to WebUI, make sure we

[awong, 2012/08/21 19:27:28]

from blank -> from a blank
to WebUI -> to a WebUI.=

[nasko, 2012/08/21 22:24:13]

Done.

+    // create a new one.

[awong, 2012/08/21 19:27:28]

"new one" is ambiguous. "new SiteInstance" is clearer.

[nasko, 2012/08/21 22:24:13]

Done.

+    const WebUIControllerFactory* web_ui_factory =
+        content::GetContentClient()->browser()->GetWebUIControllerFactory();
+    if (web_ui_factory &&
+        web_ui_factory->UseWebUIForURL(browser_context, dest_url))

[awong, 2012/08/21 19:27:28]

Use braces around the body of a conditional that has multiple lines in either
the condition or the statement.

[nasko, 2012/08/21 22:24:13]

Done.

+        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 +604,17 @@ int RenderViewHostManager::CreateRenderView(
     bool swapped_out) {
   CHECK(instance);
 
+  // If the current host is already in the site instance we are looking for
+  // 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.

[awong, 2012/08/21 19:27:28]

File a bug for this?

Also, I can't tell how severe this behavior is. In this comment and/or bug, can
we describe what is the effect of having this workaround?

[nasko, 2012/08/21 22:24:13]

I'm not quite sure myself yet. I've hit almost 400 cases in tests where we are
creating a new RVH with the same site instance as the current one. This doesn't
seem right, but I also don't want to spend time investigating all of those cases
prior to committing this CL.

+    if (swapped_out) {

[awong, 2012/08/21 19:27:28]

remove brace here :)

[nasko, 2012/08/21 22:24:13]

Done.

+      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 +645,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
+      // 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 +967,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())