Always swap with a replacement proxy in OnSwapOut.

content/browser/frame_host/render_frame_host_impl.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_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 1196 matching lines @@
   if (!is_active()) {
     NOTREACHED() << "RFH should be in default state when calling SwapOut.";
     return;
   }
 
   if (swapout_event_monitor_timeout_) {
     swapout_event_monitor_timeout_->Start(base::TimeDelta::FromMilliseconds(
         RenderViewHostImpl::kUnloadTimeoutMS));
   }
 
-  // There may be no proxy if there are no active views in the process.
-  int proxy_routing_id = MSG_ROUTING_NONE;
-  FrameReplicationState replication_state;
-  if (proxy) {
-    set_render_frame_proxy_host(proxy);
-    proxy_routing_id = proxy->GetRoutingID();
-    replication_state = proxy->frame_tree_node()->current_replication_state();
-  }
+  // There should always be a proxy to replace the old RenderFrameHost.  If
+  // there are no remaining active views in the process, the proxy will be
+  // short-lived and will be deleted when the SwapOut ACK is received.
+  DCHECK(proxy);

[Charlie Reis, 2016/04/22 19:11:33]

Given the trouble we've had here, let's make this a CHECK.  :)

[alexmos, 2016/04/25 18:17:08]

Done.

+
+  set_render_frame_proxy_host(proxy);
+  int proxy_routing_id = proxy->GetRoutingID();

[Charlie Reis, 2016/04/22 19:11:33]

nit: Move proxy_routing_id and replication_state into the IsRenderFrameLive
block, since that's the only place they're used.

[alexmos, 2016/04/25 18:17:08]

Done.

+  FrameReplicationState replication_state =
+      proxy->frame_tree_node()->current_replication_state();
 
   if (IsRenderFrameLive()) {
     Send(new FrameMsg_SwapOut(routing_id_, proxy_routing_id, is_loading,
                               replication_state));
   }
 
   // TODO(nasko): If the frame is not live, the RFH should just be deleted by
   // simulating the receipt of swap out ack.
   is_waiting_for_swapout_ack_ = true;
   if (frame_tree_node_->IsMainFrame())
     render_view_host_->set_is_active(false);
 
-  // If this is the last active frame in the SiteInstance, the
-  // DecrementActiveFrameCount call will trigger the deletion of the
-  // SiteInstance's proxies.
-  GetSiteInstance()->DecrementActiveFrameCount();

[Charlie Reis, 2016/04/22 19:11:33]

This seems like a real change-- pending delete RFHs will now be considered
active frames.  I see how it's necessary for this CL, but I'm wondering what the
implications are.  (Maybe it's an improvement over how things were before?  It
looks like we might have been able to kill the process before its unload handler
finished, for example.)

Some questions:

1) What happens if this RFH is destroyed (e.g., by closing the tab or detaching
the frame) while it's in the pending delete state?  ~RFHI won't call
DecrementActiveFrameCount because is_active() returns false.  It almost looks
like we could unconditionally call DecrementActiveFrameCount in the RFHI
destructor and not bother calling in OnSwappedOut, since OnSwappedOut should
lead to deletion.

2) What were the implications of it being decremented before swap out before?  I
see that active_frame_count() was used when deciding whether to create proxies
on SwapOut, so that's moot now (i.e., we always create the proxy).  We also call
active_frame_count() in DiscardUnusedFrame to decide whether to create a proxy
after canceling a pending RFH.  I'm less clear here-- we will now treat pending
delete RFHs as a reason to create a proxy when canceling a pending RFH.

To be honest, though, I don't see that as a big problem.  Once the pending
delete RFH goes away and the count reaches 0, I assume that extra proxy can be
cleaned up?  (I don't see where that happens at first glance.)

If so, this change seems safe to me.

[alexmos, 2016/04/25 18:17:08]

You're right, I've verified that we would miss calling DecrementActiveFrameCount
in that case.  I've moved the decrement to ~RFHI, which indeed makes more sense.
That's a great point.  Yes, canceling a pending RFH may lead to another extra
proxy here if there's also a pending shutdown RFH around.  That extra proxy
should also get cleaned up in that case: deleting the pending RFH here will
decrement active_frame_count to 1, then when the pending delete RFH goes away,
it will decrement active_frame_count to 0, SiteInstanceImpl will realize that it
has reached 0 and call ActiveFrameCountIsZero on all RFHMs with proxies, and
that should delete all of the proxies, including the one created here.  I've
manually verified that this is indeed what happens.

[Charlie Reis, 2016/04/26 05:06:36]

Acknowledged.

-
   if (!GetParent())
     delegate_->SwappedOut(this);
 }
 
 void RenderFrameHostImpl::OnBeforeUnloadACK(
     bool proceed,
     const base::TimeTicks& renderer_before_unload_start_time,
     const base::TimeTicks& renderer_before_unload_end_time) {
   TRACE_EVENT_ASYNC_END1("navigation", "RenderFrameHostImpl BeforeUnload", this,
                          "FrameTreeNode id",
@@ skipping 131 matching lines @@
   // Ignore spurious swap out ack.
   if (!is_waiting_for_swapout_ack_)
     return;
 
   TRACE_EVENT_ASYNC_END0("navigation", "RenderFrameHostImpl::SwapOut", this);
   if (swapout_event_monitor_timeout_)
     swapout_event_monitor_timeout_->Stop();
 
   ClearAllWebUI();
 
+  // If this was the last active frame in the SiteInstance, the
+  // DecrementActiveFrameCount call will trigger the deletion of the
+  // SiteInstance's proxies.
+  GetSiteInstance()->DecrementActiveFrameCount();

[Charlie Reis, 2016/04/22 19:11:33]

As noted above, we may just want to do this unconditionally in the destructor.

[alexmos, 2016/04/25 18:17:08]

Done.

+
   // 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()) {
     render_view_host_->set_is_active(false);
     render_view_host_->set_is_swapped_out(true);
   }
 
   bool deleted =
       frame_tree_node_->render_manager()->DeleteFromPendingList(this);
   CHECK(deleted);
@@ skipping 1369 matching lines @@
   // handler after it's destroyed so it can't run after the RFHI is destroyed.
   web_bluetooth_service_->SetClientConnectionErrorHandler(base::Bind(
       &RenderFrameHostImpl::DeleteWebBluetoothService, base::Unretained(this)));
 }
 
 void RenderFrameHostImpl::DeleteWebBluetoothService() {
   web_bluetooth_service_.reset();
 }
 
 }  // namespace content