Fix remote-to-local navigations in crashed subframes.

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 be46fd8b8128c5eb43ff8e6d8c75d1861c037bce..42f81dd12b022135b2ae8d78b71e361b617c219b 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1495,6 +1495,7 @@ bool RenderFrameImpl::OnMessageReceived(const IPC::Message& msg) {
     IPC_MESSAGE_HANDLER(FrameMsg_Navigate, OnNavigate)
     IPC_MESSAGE_HANDLER(FrameMsg_BeforeUnload, OnBeforeUnload)
     IPC_MESSAGE_HANDLER(FrameMsg_SwapOut, OnSwapOut)
+    IPC_MESSAGE_HANDLER(FrameMsg_SwapIn, OnSwapIn)
     IPC_MESSAGE_HANDLER(FrameMsg_Delete, OnDeleteFrame)
     IPC_MESSAGE_HANDLER(FrameMsg_Stop, OnStop)
     IPC_MESSAGE_HANDLER(FrameMsg_ContextMenuClosed, OnContextMenuClosed)
@@ -1768,6 +1769,10 @@ void RenderFrameImpl::OnSwapOut(
   RenderThread::Get()->Send(new FrameHostMsg_SwapOut_ACK(routing_id));
 }
 
+void RenderFrameImpl::OnSwapIn() {
+  SwapIn();
+}
+
 void RenderFrameImpl::OnDeleteFrame() {
   // TODO(nasko): If this message is received right after a commit has
   // swapped a RenderFrameProxy with this RenderFrame, the proxy needs to be
@@ -3590,46 +3595,11 @@ void RenderFrameImpl::didCommitProvisionalLoad(
   }
 
   if (proxy_routing_id_ != MSG_ROUTING_NONE) {
-    RenderFrameProxy* proxy =
-        RenderFrameProxy::FromRoutingID(proxy_routing_id_);
-
-    // The proxy might have been detached while the provisional LocalFrame was
-    // being navigated.  In that case, don't swap the frame back in the tree
-    // and return early (to avoid sending confusing IPCs to the browser
-    // process).  See https://crbug.com/526304 and https://crbug.com/568676.
-    // TODO(nasko, alexmos): Eventually, the browser process will send an IPC
-    // to clean this frame up after https://crbug.com/548275 is fixed.
-    if (!proxy)
+    // If this is a provisional frame associated with a proxy (i.e., a frame
+    // created for a remote-to-local navigation), swap it into the frame tree
+    // now.
+    if (!SwapIn())
       return;
-
-    int proxy_routing_id = proxy_routing_id_;
-    if (!proxy->web_frame()->swap(frame_))
-      return;
-    proxy_routing_id_ = MSG_ROUTING_NONE;
-    in_frame_tree_ = true;
-
-    // If this is the main frame going from a remote frame to a local frame,
-    // it needs to set RenderViewImpl's pointer for the main frame to itself
-    // and ensure RenderWidget is no longer in swapped out mode.
-    if (is_main_frame_) {
-      // Debug cases of https://crbug.com/575245.
-      base::debug::SetCrashKeyValue("commit_frame_id",
-                                    base::IntToString(GetRoutingID()));
-      base::debug::SetCrashKeyValue("commit_proxy_id",
-                                    base::IntToString(proxy_routing_id));
-      base::debug::SetCrashKeyValue(
-          "commit_view_id", base::IntToString(render_view_->GetRoutingID()));
-      if (render_view_->main_render_frame_) {
-        base::debug::SetCrashKeyValue(
-            "commit_main_render_frame_id",
-            base::IntToString(
-                render_view_->main_render_frame_->GetRoutingID()));
-      }
-      CHECK(!render_view_->main_render_frame_);
-      render_view_->main_render_frame_ = this;
-      if (render_view_->is_swapped_out())
-        render_view_->SetSwappedOut(false);
-    }
   }
 
   // For new page navigations, the browser process needs to be notified of the
@@ -5021,6 +4991,52 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(
   navigation_state->set_transition_type(ui::PAGE_TRANSITION_LINK);
 }
 
+bool RenderFrameImpl::SwapIn() {
+  CHECK_NE(proxy_routing_id_, MSG_ROUTING_NONE);
+  CHECK(!in_frame_tree_);
+  RenderFrameProxy* proxy = RenderFrameProxy::FromRoutingID(proxy_routing_id_);
+
+  // The proxy might have been detached while the provisional LocalFrame was
+  // being navigated.  In that case, don't swap the frame back in the tree
+  // and return early (to avoid sending confusing IPCs to the browser
+  // process).  See https://crbug.com/526304 and https://crbug.com/568676.
+  // TODO(nasko, alexmos): Eventually, the browser process will send an IPC
+  // to clean this frame up after https://crbug.com/548275 is fixed.

[Charlie Reis, 2017/01/12 00:29:05]

Note that this bug is marked fixed.  Does that mean we already send an IPC to
clean it up?  (I do think we should try out the idea of proactively deleting the
provisional frame when a proxy is detached, in a separate CL.)

[alexmos, 2017/01/12 01:38:12]

I think we do send the IPC for subframes to clean up dead provisional frames
(this is FrameMsg_Delete sent in ~RenderFrameHostImpl, added by Nasko in
https://codereview.chromium.org/1409693009).  But we could still end up in a
race where we detach the proxy and then get to didCommit right before receiving
FrameMsg_Delete, so we still need to abort the navigation in that case (and the
FrameMsg_Delete will then destroy it).  Destroying the provisional frame right
away when its proxy goes away might allow us to remove this early return, as you
say, but for now I've removed this TODO.

+  if (!proxy)
+    return false;
+
+  int proxy_routing_id = proxy_routing_id_;
+  if (!proxy->web_frame()->swap(frame_))
+    return false;
+
+  proxy_routing_id_ = MSG_ROUTING_NONE;
+  in_frame_tree_ = true;
+
+  // If this is the main frame going from a remote frame to a local frame,
+  // it needs to set RenderViewImpl's pointer for the main frame to itself
+  // and ensure RenderWidget is no longer in swapped out mode.
+  if (is_main_frame_) {
+    // Debug cases of https://crbug.com/575245.
+    base::debug::SetCrashKeyValue("commit_frame_id",
+                                  base::IntToString(GetRoutingID()));
+    base::debug::SetCrashKeyValue("commit_proxy_id",
+                                  base::IntToString(proxy_routing_id));
+    base::debug::SetCrashKeyValue(
+        "commit_view_id", base::IntToString(render_view_->GetRoutingID()));
+    if (render_view_->main_render_frame_) {
+      base::debug::SetCrashKeyValue(
+          "commit_main_render_frame_id",
+          base::IntToString(render_view_->main_render_frame_->GetRoutingID()));
+    }
+    CHECK(!render_view_->main_render_frame_);
+    render_view_->main_render_frame_ = this;
+    if (render_view_->is_swapped_out())
+      render_view_->SetSwappedOut(false);
+  }
+
+  return true;
+}
+
 void RenderFrameImpl::didStartLoading(bool to_different_document) {
   TRACE_EVENT1("navigation,rail", "RenderFrameImpl::didStartLoading",
                "id", routing_id_);