Create RenderFrameProxyHost at time of swap-out instead of commit.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index d6d922d96be90d58192fdb5d3a85fb791701f115..4732204616a6a694015abae8ed096c0c637a9788 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -165,6 +165,10 @@ namespace {
 
 const size_t kExtraCharsBeforeAndAfterSelection = 100;
 
+typedef std::map<int, RenderFrameImpl*> RoutingIDFrameMap;
+static base::LazyInstance<RoutingIDFrameMap> g_routing_id_frame_map =
+    LAZY_INSTANCE_INITIALIZER;
+
 typedef std::map<blink::WebFrame*, RenderFrameImpl*> FrameMap;
 base::LazyInstance<FrameMap> g_frame_map = LAZY_INSTANCE_INITIALIZER;
 
@@ -358,6 +362,13 @@ RenderFrameImpl* RenderFrameImpl::Create(RenderViewImpl* render_view,
 }
 
 // static
+RenderFrameImpl* RenderFrameImpl::FromRoutingID(int32 routing_id) {

[Charlie Reis, 2014/05/15 00:32:50]

This code looks fine, but any reason that it differs from FromWebFrame below?

[nasko, 2014/05/15 18:47:13]

Yes, it is identical to the code in RenderViewImpl ;). I've changed it to
conform to the one in this file.

+  RoutingIDFrameMap* frames = g_routing_id_frame_map.Pointer();
+  RoutingIDFrameMap::iterator it = frames->find(routing_id);
+  return it == frames->end() ? NULL : it->second;
+}
+
+// static
 RenderFrame* RenderFrame::FromWebFrame(blink::WebFrame* web_frame) {
   return RenderFrameImpl::FromWebFrame(web_frame);
 }
@@ -380,6 +391,7 @@ void RenderFrameImpl::InstallCreateHook(
 RenderFrameImpl::RenderFrameImpl(RenderViewImpl* render_view, int routing_id)
     : frame_(NULL),
       render_view_(render_view->AsWeakPtr()),
+      render_frame_proxy_(NULL),
       routing_id_(routing_id),
       is_swapped_out_(false),
       is_detaching_(false),
@@ -393,6 +405,10 @@ RenderFrameImpl::RenderFrameImpl(RenderViewImpl* render_view, int routing_id)
       weak_factory_(this) {
   RenderThread::Get()->AddRoute(routing_id_, this);
 
+  std::pair<RoutingIDFrameMap::iterator, bool> result =
+      g_routing_id_frame_map.Get().insert(std::make_pair(routing_id_, this));
+  CHECK(result.second) << "Inserting a duplicate item.";
+
 #if defined(OS_ANDROID)
   new JavaBridgeDispatcher(this);
 #endif
@@ -405,6 +421,7 @@ RenderFrameImpl::RenderFrameImpl(RenderViewImpl* render_view, int routing_id)
 RenderFrameImpl::~RenderFrameImpl() {
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, RenderFrameGone());
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, OnDestruct());
+  g_routing_id_frame_map.Get().erase(routing_id_);
   RenderThread::Get()->RemoveRoute(routing_id_);
 }
 
@@ -624,12 +641,27 @@ void RenderFrameImpl::SetMediaStreamClientForTesting(
 }
 
 bool RenderFrameImpl::Send(IPC::Message* message) {
-  if (is_detaching_ ||
-      ((is_swapped_out_ || render_view_->is_swapped_out()) &&
-       !SwappedOutMessages::CanSendWhileSwappedOut(message))) {
+  if (is_detaching_) {
     delete message;
     return false;
   }
+  if (is_swapped_out_ || render_view_->is_swapped_out()) {
+    if (render_frame_proxy_) {
+      // Some call sites use the associated RenderViewImpl routing id to send

[Charlie Reis, 2014/05/15 00:32:50]

Whew, this is tricky!  Maybe start with a sentence like "In most cases, send
IPCs through the proxy when swapped out."

[nasko, 2014/05/15 18:47:13]

Done.

+      // messages, so it shouldn't be overwritten in that case. Additionally,
+      // The SwapOut_ACK message must be delivered to the RenderFrameHost

[Charlie Reis, 2014/05/15 00:32:50]

nit: the

[nasko, 2014/05/15 18:47:13]

Done.

+      // instead of the proxy object and needs to be special-cased here.
+      if (message->routing_id() == routing_id_ &&
+          message->type() != FrameHostMsg_SwapOut_ACK::ID) {
+        return render_frame_proxy_->Send(message);
+      }
+    }
+
+    if (!SwappedOutMessages::CanSendWhileSwappedOut(message)) {

[Charlie Reis, 2014/05/15 00:32:50]

I don't think this is right.  When CanSendWhileSwappedOut returns false, we'll
have already sent the message through the proxy on line 656.  We should probably
leave it in the initial if statement as it was before.

[Edit: I see that RenderFrameProxy::Send calls CanSendWhileSwappedOut as well,
and deletes the message if not.  Wouldn't that be a double free in most cases,
apart from SwapOut_ACK, etc?]

[nasko, 2014/05/15 18:47:13]

If we ever get to call Send on the RenderFrameProxy, we will return and never
hit this statement or am I missing something?
The double free could happen if the call to RenderFrameProxy::Send didn't do an
early return out of this method.

+      delete message;
+      return false;
+    }
+  }
 
   return RenderThread::Get()->Send(message);
 }
@@ -891,13 +923,15 @@ void RenderFrameImpl::OnBeforeUnload() {
                                          before_unload_end_time));
 }
 
-void RenderFrameImpl::OnSwapOut() {
+void RenderFrameImpl::OnSwapOut(int proxy_routing_id) {
   // Only run unload if we're not swapped out yet, but send the ack either way.
   if (!is_swapped_out_ || !render_view_->is_swapped_out_) {
     // Swap this RenderFrame out so the frame can navigate to a page rendered by
     // a different process.  This involves running the unload handler and
     // clearing the page.  Once WasSwappedOut is called, we also allow this
     // process to exit if there are no other active RenderFrames in it.
+    RenderFrameProxy* proxy = RenderFrameProxy::CreateFrameProxy(

[Charlie Reis, 2014/05/15 00:32:50]

I'm unclear on why this is declared up here, rather than as part of the
set_render_frame_proxy call below.  Does it need to happen before we get swapped
out, similar to SyncNavigationState?  If so, let's put it in that block (of
things to do before swap out actually occurs).

[nasko, 2014/05/15 18:47:13]

I think it should be created prior to running the unload handler, which can
potentially run for a long time. The reason is that if the browser starts
sending IPCs, the routing id is registered and messages aren't dropped.

+        proxy_routing_id, routing_id_);
 
     // Send an UpdateState message before we get swapped out.
     render_view_->SyncNavigationState();
@@ -912,6 +946,7 @@ void RenderFrameImpl::OnSwapOut() {
     if (!frame_->parent())
       render_view_->SetSwappedOut(true);
     is_swapped_out_ = true;
+    set_render_frame_proxy(proxy);
 
     // Now that we're swapped out and filtering IPC messages, stop loading to
     // ensure that no other in-progress navigation continues.  We do this here