Destroy the old RenderWidgetHostView when swapping out a main frame.

content/browser/frame_host/render_frame_host_impl.cc

Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index 4bbb482a128f7e38a442e9e70b217e17c2595a4b..cd3c836d791c0f86cabbf037f09432786e1cd409 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -1513,6 +1513,11 @@ void RenderFrameHostImpl::OnSwappedOut() {
   if (frame_tree_node_->IsMainFrame()) {

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

This part I think it should be moved to RenderFrameHostManager::CommitPending,
but I didn't want to do this to avoid changing too many things in this patch.

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

Hmm, I see the dilemma.  We're basically doing the same thing in
RFHM::DiscardUnusedFrame right here, though we're missing the
set_main_frame_routing_id(MSG_ROUTING_NONE) call here.  (I guess that was taken
care of in CommitPending, along with redundant calls to set_is_active and
set_is_swapped_out?)

It's definitely odd to be doing the same work in 3 places.  If we don't need the
RWHV after CommitPending (given that this runs later after we've heard the swap
out ack), I'm inclined to move all this to CommitPending, perhaps using a shared
helper function with DiscardUnusedFrame to do the work?

If we're concerned about regressions, we should at least put a TODO here
explaining that.  I'd kind of like to do it all at once, but if you're aiming to
land this before the branch, I can understand the argument to do it separately.

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

Do you mean all of the things in the if, or just some of them?  

My initial look is that:
- render_view_host_->set_is_active(false) looks redundant with the call in
RenderFrameHostImpl::SwapOut
- set_is_swapped_out(true) is done here intentionally and not in CommitPending,
as
we currently distinguish between a swapped-out RVH which has a RFH pending
deletion, and a swapped-out RVH where the
RFH is deleted.  For the former, is_active() -> false, is_swapped_out() ->
false.  For the latter, is_active() -> false, is_swapped_out() -> true.  The
only place where this matters is for SwappedOutMessages filtering.

The RWHV destruction, I agree, seems better in CommitPending, where it'll be
symmetrical with the CreateRenderWidgetHostViewForRenderManager call to create
the RWHV on the new active RVH.

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

I moved all the code to RFHM. I think it makes more sense to control the state
of the RenderViewHost in RFHM than in RFH.

I moved this particular code to DeleteFromPendingList, which should preserve the
today's behavior between active/swapped_out.

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

Acknowledged.

     render_view_host_->set_is_active(false);
     render_view_host_->set_is_swapped_out(true);
+
+    if (render_view_host_->GetWidget()->GetView()) {
+      render_view_host_->GetWidget()->GetView()->Destroy();
+      render_view_host_->GetWidget()->SetView(nullptr);
+    }
   }
 
   bool deleted =