Destroy the old RenderWidgetHostView when swapping out a main frame.

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 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/web_contents/web_contents_impl.h"
 
 #include <stddef.h>
 
 #include <cmath>
 #include <utility>
@@ skipping 4840 matching lines @@
 }
 
 bool WebContentsImpl::CreateRenderViewForRenderManager(
     RenderViewHost* render_view_host,
     int opener_frame_routing_id,
     int proxy_routing_id,
     const FrameReplicationState& replicated_frame_state) {
   TRACE_EVENT0("browser,navigation",
                "WebContentsImpl::CreateRenderViewForRenderManager");
 
-  if (proxy_routing_id == MSG_ROUTING_NONE)
+  RenderViewHostImpl* rvhi = static_cast<RenderViewHostImpl*>(render_view_host);
+
+  if (!rvhi->is_swapped_out())

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

This is also a no-op, but it makes it easier to understand and associate the
creation of the RenderWidgetHostView with a non-swapped-out RenderViewHost.

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

When you say no-op, you mean is_swapped_out() and proxy_routing_id ==
MSG_ROUTING_NONE are equivalent?  Can you add a DCHECK for that if so?  I wonder
if it would also help to say something like:
// Only create a RenderWidgetHostView if the RVH's main frame is active.

And looking closer, it seems unfortunate that we're adding a call to
RVHI::is_swapped_out().  I was hoping to remove that member entirely, as the
TODO in render_view_host_impl.h indicates.  Basically, it's confusing to try to
track swapped out state on RVH.  Do you think there's a way around this, or do
we need to stick with this concept for now?

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

See my earlier comment about moving the GetView() destruction to CommitPending,
which might allow using rvhi->is_active() here instead.

Also, let's add a comment here explaining why this is only done for active RVHs
and mention that for reused swapped-out RVHs we do this in CommitPending when
they become active.

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

Added the DCHECK and switched to is_active.

     CreateRenderWidgetHostViewForRenderManager(render_view_host);
 
-  if (!static_cast<RenderViewHostImpl*>(render_view_host)
-           ->CreateRenderView(opener_frame_routing_id,
-                              proxy_routing_id,
-                              replicated_frame_state,
-                              created_with_opener_)) {
+  if (!rvhi->CreateRenderView(opener_frame_routing_id, proxy_routing_id,
+                              replicated_frame_state, created_with_opener_)) {
     return false;
   }
 
   SetHistoryOffsetAndLengthForView(render_view_host,
                                    controller_.GetLastCommittedEntryIndex(),
                                    controller_.GetEntryCount());
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
   // Force a ViewMsg_Resize to be sent, needed to make plugins show up on
   // linux. See crbug.com/83941.
@@ skipping 335 matching lines @@
   dialog_manager_ = dialog_manager;
 }
 
 void WebContentsImpl::RemoveBindingSet(const std::string& interface_name) {
   auto it = binding_sets_.find(interface_name);
   if (it != binding_sets_.end())
     binding_sets_.erase(it);
 }
 
 }  // namespace content