Destroy the old RenderWidgetHostView when swapping out a main frame.

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/render_frame_host_manager.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
@@ skipping 676 matching lines @@
   // Doing this is important in the case where the replacement proxy is created
   // above, as the RenderViewHost will continue to exist and should be
   // considered swapped out if it is ever reused.  When there's no replacement
   // proxy, this doesn't really matter, as the RenderViewHost will be destroyed
   // shortly, since |render_frame_host| is its last active frame and will be
   // deleted below.  See https://crbug.com/627400.
   if (frame_tree_node_->IsMainFrame()) {
     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();
 
   // If a new RenderFrameProxyHost was created above, or if the old proxy isn't
   // live, create the RenderFrameProxy in the renderer, so that other frames
   // can still communicate with this frame.  See https://crbug.com/653746.
   if (proxy && !proxy->is_render_frame_proxy_live())
     proxy->InitRenderFrameProxy();
 }
@@ skipping 1032 matching lines @@
       instance, MSG_ROUTING_NONE, MSG_ROUTING_NONE, widget_routing_id, hidden,
       false);
   RenderViewHostImpl* render_view_host =
       new_render_frame_host->render_view_host();
 
   // Prevent the process from exiting while we're trying to navigate in it.
   new_render_frame_host->GetProcess()->AddPendingView();
 
   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());
   }
 
   if (success) {
     if (frame_tree_node_->IsMainFrame()) {
       // Don't show the main frame's view until we get a DidNavigate from it.
       // Only the RenderViewHost for the top-level RenderFrameHost has a
       // RenderWidgetHostView; RenderWidgetHosts for out-of-process iframes
       // will be created later and hidden.
@@ skipping 380 matching lines @@
   } else {
     // PlzNavigate
     DCHECK(speculative_render_frame_host_);
     old_render_frame_host =
         SetRenderFrameHost(std::move(speculative_render_frame_host_));
   }
 
   // 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) {
     // In most cases, we need to show the new view.
     render_frame_host_->GetView()->Show();
   }
   if (!new_rfh_has_view) {
     // If the view is gone, then this RenderViewHost died while it was hidden.
     // We ignored the RenderProcessGone call at the time, so we should send it
     // now to make sure the sad tab shows up, etc.
     DCHECK(!render_frame_host_->IsRenderFrameLive());
     DCHECK(!render_frame_host_->render_view_host()->IsRenderViewLive());
     render_frame_host_->ResetLoadingState();
     delegate_->RenderProcessGoneFromRenderManager(
         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();
 
   if (will_focus_location_bar) {
     delegate_->SetFocusToLocationBar(false);
   } else if (focus_render_view && render_frame_host_->GetView()) {
     if (is_main_frame) {
       render_frame_host_->GetView()->Focus();
     } else {
       // The main frame's view is already focused, but we need to set
       // page-level focus in the subframe's renderer.
       frame_tree_node_->frame_tree()->SetPageFocus(
           render_frame_host_->GetSiteInstance(), true);
     }
   }
 
   // Notify that we've swapped RenderFrameHosts. We do this before shutting down
   // the RFH so that we can clean up RendererResources related to the RFH first.
   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).
   SwapOutOldFrame(std::move(old_render_frame_host));
 
   // Since the new RenderFrameHost is now committed, there must be no proxies
   // for its SiteInstance. Delete any existing ones.
   DeleteRenderFrameProxyHost(render_frame_host_->GetSiteInstance());
 
   // If this is a subframe, it should have a CrossProcessFrameConnector
@@ skipping 488 matching lines @@
                                              resolved_url)) {
     DCHECK(!dest_instance ||
            dest_instance == render_frame_host_->GetSiteInstance());
     return false;
   }
 
   return true;
 }
 
 }  // namespace content