Destroy the old RenderWidgetHostView when swapping out a main frame.

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 e174e54d379ca834c1016b919b53d5325fb0ce0d..561a1aa8fc7cb71462b73d066e61c3c37854b21e 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -694,6 +694,11 @@ void RenderFrameHostManager::DiscardUnusedFrame(
     rvh->set_main_frame_routing_id(MSG_ROUTING_NONE);
     rvh->set_is_active(false);
     rvh->set_is_swapped_out(true);
+
+    if (rvh->GetWidget()->GetView()) {
+      rvh->GetWidget()->GetView()->Destroy();
+      rvh->GetWidget()->SetView(nullptr);
+    }
   }
 
   render_frame_host.reset();
@@ -1746,11 +1751,6 @@ std::unique_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
 
   if (frame_tree_node_->IsMainFrame()) {
     success = InitRenderView(render_view_host, proxy);
-
-    // If we are reusing the RenderViewHost and it doesn't already have a
-    // RenderWidgetHostView, we need to create one if this is the main frame.
-    if (!render_view_host->GetWidget()->GetView())

[lfg, 2016/11/15 04:37:34]

I moved this to CommitPending so it's easier to reason -- we create a
RenderWidgetHostView when a RenderViewHost transitions from swapped-out to
non-swapped-out, and the reverse is also true: we destroy the
RenderWidgetHostView when the RenderViewHost transitions from active to
swapped-out.

[Charlie Reis, 2016/11/15 19:59:05]

Sounds good-- I like the symmetry.  We don't actually need the RWHV until commit
time, right?

[lfg, 2016/11/23 00:27:04]

Right.

-      delegate_->CreateRenderWidgetHostViewForRenderManager(render_view_host);
   } else {
     DCHECK(render_view_host->IsRenderViewLive());
   }
@@ -2151,6 +2151,35 @@ void RenderFrameHostManager::CommitPending() {
   // The process will no longer try to exit, so we can decrement the count.
   render_frame_host_->GetProcess()->RemovePendingView();
 
+  // The RenderViewHost keeps track of the main RenderFrameHost routing id.
+  // If this is committing a main frame navigation, update it and set the
+  // routing id in the RenderViewHost associated with the old RenderFrameHost
+  // to MSG_ROUTING_NONE.
+  if (is_main_frame) {
+    RenderViewHostImpl* rvh = render_frame_host_->render_view_host();
+    rvh->set_main_frame_routing_id(render_frame_host_->routing_id());
+
+    // If we are reusing the RenderViewHost, we need to create the
+    // RenderWidgetHostView if this is the main frame.
+    if (rvh->IsRenderViewLive() && rvh->is_swapped_out())

[Charlie Reis, 2016/11/15 19:59:05]

The is_swapped_out clause here is new.  If we keep it, can you update the
comment for why it's needed?

[alexmos, 2016/11/17 17:55:57]

Is the logic that if the RVH is reused and transitioning from swapped-out to
active, then we need to recreate the RWHV, but if we are committing a brand new
RFH/RVH, then we've already done it earlier (specifically, in
CreateRenderViewForRenderManager where the is_swapped_out check is reversed)?

If so, the is_swapped_out() looks correct as it matches the point at which the
reused RVH's RWHV is destroyed (which is what you added in OnSwappedOut, when
is_swapped_out() becomes true).  And we don't want to use is_active() instead,
since if we get here when reusing a pending delete RFH's RVH, its RWHV still
exists, and we don't want to recreate it - right?

I agree it's unfortunate to have to expose is_swapped_out() though.  Perhaps
it's better to just move the old RVH's RWHV destruction from OnSwappedOut to
here, in which case this could use is_active?

[alexmos, 2016/11/17 17:55:57]

Is the IsRenderViewLive check necessary?  How would we be committing a RFH where
its RV isn't live?

[lfg, 2016/11/23 00:27:04]

I've switched so that the lifetime of RWHV now tracks is_active, which should be
more correct -- we only want a RWHV when the RenderViewHost is being used for
rendering.

[lfg, 2016/11/23 00:27:04]

I wasn't able to figure out when IsRenderViewLive is false, however, the code
bellow in line 2189 assumes this is possible, and has a comment about the
RenderViewHost dying while it was hidden. If this indeed isn't possible, then we
can do a bigger cleanup here.

[alexmos, 2016/11/23 17:54:45]

Ack.  I'm not sure myself, but good point about line 2189.  Perhaps this has
something to do with how the sad tab is handled.  In any case, that's something
we can investigate later on.

+      delegate_->CreateRenderWidgetHostViewForRenderManager(rvh);
+
+    // If the RenderViewHost is transitioning from swapped out to active state,
+    // it was reused, so dispatch a RenderViewReady event.  For example, this
+    // is necessary to hide the sad tab if one is currently displayed.  See
+    // https://crbug.com/591984.
+    //
+    // TODO(alexmos):  Remove this and move RenderViewReady consumers to use
+    // the main frame's RenderFrameCreated instead.
+    if (!rvh->is_active())
+      rvh->PostRenderViewReady();
+
+    rvh->set_is_active(true);

[Charlie Reis, 2016/11/15 19:59:05]

I see this was just moved from below, but I'm noticing that it looks redundant
with RFHI::OnSwappedOut.  Are both needed, or is that why you though we might
want to move that code to here?

[alexmos, 2016/11/17 17:55:57]

They are not redundant -- these are updating the new RVH's state, and
OnSwappedOut is updating the old (swapped-out) RVH's state.  (We also do
set_is_active(false) on the old RVH at commit time, in RFHI::SwapOut -- that
call should probably move here for clarity.)

[lfg, 2016/11/23 00:27:04]

I've moved the code that sets the old RVH's active state here, as well as
deleting the old RWHV.

+    rvh->set_is_swapped_out(false);
+    old_render_frame_host->render_view_host()->set_main_frame_routing_id(
+        MSG_ROUTING_NONE);
+  }
+
   // Show the new view (or a sad tab) if necessary.
   bool new_rfh_has_view = !!render_frame_host_->GetView();
   if (!delegate_->IsHidden() && new_rfh_has_view) {
@@ -2168,14 +2197,6 @@ void RenderFrameHostManager::CommitPending() {
         render_frame_host_->render_view_host());
   }
 
-  // For top-level frames, also hide the old RenderViewHost's view.
-  // TODO(creis): As long as show/hide are on RVH, we don't want to hide on
-  // subframe navigations or we will interfere with the top-level frame.
-  if (is_main_frame &&
-      old_render_frame_host->render_view_host()->GetWidget()->GetView()) {
-    old_render_frame_host->render_view_host()->GetWidget()->GetView()->Hide();
-  }
-
   // Make sure the size is up to date.  (Fix for bug 1079768.)
   delegate_->UpdateRenderViewSizeForRenderManager();
 
@@ -2197,30 +2218,6 @@ void RenderFrameHostManager::CommitPending() {
   delegate_->NotifySwappedFromRenderManager(
       old_render_frame_host.get(), render_frame_host_.get(), is_main_frame);
 
-  // The RenderViewHost keeps track of the main RenderFrameHost routing id.
-  // If this is committing a main frame navigation, update it and set the
-  // routing id in the RenderViewHost associated with the old RenderFrameHost
-  // to MSG_ROUTING_NONE.
-  if (is_main_frame) {
-    RenderViewHostImpl* rvh = render_frame_host_->render_view_host();
-    rvh->set_main_frame_routing_id(render_frame_host_->routing_id());
-
-    // If the RenderViewHost is transitioning from swapped out to active state,
-    // it was reused, so dispatch a RenderViewReady event.  For example, this
-    // is necessary to hide the sad tab if one is currently displayed.  See
-    // https://crbug.com/591984.
-    //
-    // TODO(alexmos):  Remove this and move RenderViewReady consumers to use
-    // the main frame's RenderFrameCreated instead.
-    if (!rvh->is_active())
-      rvh->PostRenderViewReady();
-
-    rvh->set_is_active(true);
-    rvh->set_is_swapped_out(false);
-    old_render_frame_host->render_view_host()->set_main_frame_routing_id(
-        MSG_ROUTING_NONE);
-  }
-
   // Swap out the old frame now that the new one is visible.
   // This will swap it out and schedule it for deletion when the swap out ack
   // arrives (or immediately if the process isn't live).