Make RenderFrame(Host) own a RenderWidget(Host).

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 6fcb94122deb087aa949dec2e8299f08e17b58d1..a410d2a8108cd0c0bf0998870d9fbd07b3a36b04 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -1026,6 +1026,7 @@ void RenderFrameHostManager::CreateRenderFrameHostForNewSiteInstance(
     SiteInstance* old_instance,
     SiteInstance* new_instance,
     bool is_main_frame) {
+  int create_render_frame_flags = is_main_frame ? FOR_MAIN_FRAME_NAVIGATION : 0;
   // Ensure that we have created RFHs for the new RFH's opener chain if
   // we are staying in the same BrowsingInstance. This allows the new RFH
   // to send cross-process script calls to its opener(s).
@@ -1039,13 +1040,21 @@ void RenderFrameHostManager::CreateRenderFrameHostForNewSiteInstance(
       // SiteInstance in all nodes except the current one.
       frame_tree_node_->frame_tree()->CreateProxiesForSiteInstance(
           frame_tree_node_, new_instance);
+      // RenderFrames in different processes from their parent RenderFrames

[nasko, 2014/10/01 16:37:14]

nit: s/different processes/different SiteInstance/

[Charlie Reis, 2014/10/01 17:01:08]

nit: s/RenderFrames/RenderFrame(Host)s/

[kenrb, 2014/10/01 19:27:51]

Changed RenderWidget(Host)s to just RenderWidgets. Use of RenderFrames is
correct since we are talking about living in different processes.

[kenrb, 2014/10/01 19:27:51]

Might processes be better? We use SiteInstance right now to mean that they are
in different processes, but eventually not all SiteInstances will be isolated,
and in that case we need a different way to test whether it is the same process
or not.

[Charlie Reis, 2014/10/02 00:06:57]

I think we do want to say SiteInstance here.

If two frames are in the same SiteInstance (even if they're cross-site), they're
definitely in the same process.  And if two frames are in different
SiteInstances, they might be in the same process (e.g., process sharing for
non-protected sites), but we should probably give them separate widgets anyway.

[kenrb, 2014/10/02 20:20:47]

I'm not sure I agree. If a non-protected SiteInstance creates an iframe of
another non-protected SiteInstance, the frame will not get a widget. It would
create confusion on the rendering side if it did.

[Charlie Reis, 2014/10/08 21:21:13]

I'm skeptical about that.  It doesn't make sense to me to use the same
RenderWidgetHost for two different SiteInstances, even if they share a process.

That said, I'm also having trouble constructing a scenario where that could
happen, since we shouldn't be putting an iframe in a different process than its
parent unless the two need to be isolated from each other.  I would be ok
asserting that it can't happen, and see if we need to revisit it.

+      // in the frame tree require RenderWidget(Host)s for rendering
+      // and processing input events.
+      if (frame_tree_node_->parent() &&
+          frame_tree_node_->parent()->render_manager()->
+              current_frame_host()->GetSiteInstance() == new_instance)

[nasko, 2014/10/01 16:37:14]

Shouldn't this be "!=" instead of "=="?

[kenrb, 2014/10/01 19:27:51]

Yes! That explains a lot about some of the problems I am seeing right now.
Thanks Nasko! :)

+        create_render_frame_flags |= CREATE_RENDER_WIDGET_HOST;
     }
   }
 
+  create_render_frame_flags |= delegate_->IsHidden() ? HIDDEN : 0;
+
   // Create a non-swapped-out RFH with the given opener.
   int route_id = CreateRenderFrame(
-      new_instance, opener_route_id, false, is_main_frame,
-      delegate_->IsHidden());
+      new_instance, opener_route_id, create_render_frame_flags);
   if (route_id == MSG_ROUTING_NONE) {
     pending_render_frame_host_.reset();
     return;
@@ -1086,11 +1095,11 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
 
 int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
                                               int opener_route_id,
-                                              bool swapped_out,
-                                              bool for_main_frame_navigation,
-                                              bool hidden) {
+                                              int flags) {
+  bool swapped_out = flags & SWAPPED_OUT;
   CHECK(instance);
-  DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden.
+  // Swapped out views should always be hidden.
+  DCHECK(!swapped_out || (flags & HIDDEN));
 
   // TODO(nasko): Remove the following CHECK once cross-site navigation no
   // longer relies on swapped out RFH for the top-level frame.
@@ -1138,8 +1147,11 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
     }
   } else {
     // Create a new RenderFrameHost if we don't find an existing one.
-    new_render_frame_host = CreateRenderFrameHost(
-        instance, MSG_ROUTING_NONE, MSG_ROUTING_NONE, swapped_out, hidden);
+    new_render_frame_host = CreateRenderFrameHost(instance,
+                                                  MSG_ROUTING_NONE,
+                                                  MSG_ROUTING_NONE,
+                                                  swapped_out,
+                                                  flags & HIDDEN);
     RenderViewHostImpl* render_view_host =
         new_render_frame_host->render_view_host();
     int proxy_routing_id = MSG_ROUTING_NONE;
@@ -1160,7 +1172,7 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
     bool success = InitRenderView(render_view_host,
                                   opener_route_id,
                                   proxy_routing_id,
-                                  for_main_frame_navigation);
+                                  flags & FOR_MAIN_FRAME_NAVIGATION);
     if (success) {
       if (frame_tree_node_->IsMainFrame()) {
         // Don't show the main frame's view until we get a DidNavigate from it.
@@ -1180,6 +1192,10 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
     frame_to_announce = new_render_frame_host.get();
   }
 
+  if (flags & CREATE_RENDER_WIDGET_HOST)
+    new_render_frame_host->CreateRenderWidgetHost(render_widget_delegate_,

[nasko, 2014/10/01 16:37:14]

Is there ever a case where we need to create a RenderWidgetHost for a
RenderFrameHost at point later than the creation?
If not, why not create the widget along with the creation of the
RenderFrameHost? This will also allow us to send only one IPC down to the
renderer instead of having this extra one. Also, we avoid possible races due to
having two separate IPCs and events happening in-between.

[Charlie Reis, 2014/10/01 17:01:08]

+1.  We do something similar for creating the RenderView and main RenderFrame at
the same time, if I understand correctly.

[kenrb, 2014/10/01 19:27:51]

I had thought of this, but the comment in line 1125 talks about reusing a
RenderFrameHost that had previously been attached to a RenderFrameProxyHost.

[Charlie Reis, 2014/10/02 00:06:57]

That's only for main frames, which store their RFH inside the
RenderFrameProxyHost.  It'll go away when we remove swappedout://.  (I don't
we're planning to create a separate widget for main frames before then, are we?)

[kenrb, 2014/10/02 20:20:48]

Probably not. This is good to know, I have made the change.

+                                                  flags & HIDDEN);
+
   // Use this as our new pending RFH if it isn't swapped out.
   if (!swapped_out)
     pending_render_frame_host_ = new_render_frame_host.Pass();