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 <string>
@@ skipping 681 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();
 }
 
 bool RenderFrameHostManager::DeleteFromPendingList(
     RenderFrameHostImpl* render_frame_host) {
+  // If this is a main frame RFH that's about to be deleted, update its RVH's
+  // swapped-out state here. https://crbug.com/505887
+  if (frame_tree_node_->IsMainFrame()) {
+    RenderViewHostImpl* rvh = render_frame_host->render_view_host();
+
+    rvh->set_is_active(false);

[lfg, 2016/11/29 17:41:22]

I had to change this back from the DCHECK, since the DCHECK isn't always true.
The RenderViewHost can be active when the OnSwappedOut is called from the
swapout_event_monitor_timeout_.

[Charlie Reis, 2016/11/29 19:35:55]

Ok.  Please mention that in a comment.

[alexmos, 2016/11/29 19:47:37]

I'm a bit confused by this actually.  We start the swapout timer in
CommitPending -> SwapOutOldFrame -> SwapOut, and that call in CommitPending
happens after we've done old_rvh->set_is_active(false).  How can we still get
here from OnSwappedOut with is_active being true?
Is this because some tests call OnSwappedOut directly, without going through the
timer and CommitPending?

+    rvh->set_is_swapped_out(true);
+  }
+
   for (RFHPendingDeleteList::iterator iter = pending_delete_hosts_.begin();
        iter != pending_delete_hosts_.end();
        iter++) {
     if (iter->get() == render_frame_host) {
       pending_delete_hosts_.erase(iter);
       return true;
     }
   }
   return false;
 }
@@ skipping 1026 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())
-      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_active())
+      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);
+    rvh->set_is_swapped_out(false);
+
+    // Tell the old RenderViewHost it is no longer active.
+    RenderViewHostImpl* old_rvh = old_render_frame_host->render_view_host();
+    old_rvh->set_main_frame_routing_id(MSG_ROUTING_NONE);
+    old_rvh->set_is_active(false);
+
+    // Destroy the old RenderWidgetHostView.
+    if (old_rvh->GetWidget()->GetView()) {
+      old_rvh->GetWidget()->GetView()->Destroy();
+      old_rvh->GetWidget()->SetView(nullptr);
+    }
+  }
+
   // 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 489 matching lines @@
                                              resolved_url)) {
     DCHECK(!dest_instance ||
            dest_instance == render_frame_host_->GetSiteInstance());
     return false;
   }
 
   return true;
 }
 
 }  // namespace content