When a proxy is detached, immediately delete its associated provisional frame.

content/renderer/render_view_browsertest.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 <stddef.h>
 #include <stdint.h>
 #include <tuple>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 824 matching lines @@
   // replicated correctly.
   replication_state.origin = url::Origin();
   TestRenderFrame* child_frame2 = static_cast<TestRenderFrame*>(
       RenderFrame::FromWebFrame(web_frame->firstChild()->nextSibling()));
   child_frame2->SwapOut(kProxyRoutingId + 1, true, replication_state);
   EXPECT_TRUE(web_frame->firstChild()->nextSibling()->isWebRemoteFrame());
   EXPECT_TRUE(
       web_frame->firstChild()->nextSibling()->getSecurityOrigin().isUnique());
 }
 
-// Test for https://crbug.com/568676, where a parent detaches a remote child
-// while the browser navigates it to the parent's site in parallel, with the
-// detach happening after the provisional RenderFrame is created but before
-// FrameMsg_Navigate is received.  This is a variant of
-// https://crbug.com/526304.
-TEST_F(RenderViewImplTest, NavigateProxyAndDetachBeforeOnNavigate) {
+// Test that when a parent detaches a remote child after the provisional
+// RenderFrame is created but before it is navigated, the RenderFrame is
+// destroyed along with the proxy.  This protects against races in
+// https://crbug.com/526304 and https://crbug.com/568676.
+TEST_F(RenderViewImplTest, DetachingProxyAlsoDestroysProvisionalFrame) {
   // This test should only run with --site-per-process.
   if (!AreAllSitesIsolatedForTesting())
     return;
 
   LoadHTML("Hello <iframe src='data:text/html,frame 1'></iframe>");
   WebFrame* web_frame = frame()->GetWebFrame();
   TestRenderFrame* child_frame = static_cast<TestRenderFrame*>(
       RenderFrame::FromWebFrame(web_frame->firstChild()));
 
   // Swap the child frame out.
   FrameReplicationState replication_state =
       ReconstructReplicationStateForTesting(child_frame);
   child_frame->SwapOut(kProxyRoutingId, true, replication_state);
   EXPECT_TRUE(web_frame->firstChild()->isWebRemoteFrame());
 
   // Do the first step of a remote-to-local transition for the child proxy,
   // which is to create a provisional local frame.
   int routing_id = kProxyRoutingId + 1;
   mojom::CreateFrameWidgetParams widget_params;
   widget_params.routing_id = MSG_ROUTING_NONE;
   widget_params.hidden = false;
   RenderFrameImpl::CreateFrame(routing_id, kProxyRoutingId, MSG_ROUTING_NONE,
                                frame()->GetRoutingID(), MSG_ROUTING_NONE,
                                replication_state, nullptr, widget_params,
                                FrameOwnerProperties());
-  TestRenderFrame* provisional_frame =
-      static_cast<TestRenderFrame*>(RenderFrameImpl::FromRoutingID(routing_id));
-  EXPECT_TRUE(provisional_frame);
+  {
+    TestRenderFrame* provisional_frame = static_cast<TestRenderFrame*>(
+        RenderFrameImpl::FromRoutingID(routing_id));
+    EXPECT_TRUE(provisional_frame);
+  }
 
   // Detach the child frame (currently remote) in the main frame.
   ExecuteJavaScriptForTests(
       "document.body.removeChild(document.querySelector('iframe'));");
   RenderFrameProxy* child_proxy =
       RenderFrameProxy::FromRoutingID(kProxyRoutingId);
   EXPECT_FALSE(child_proxy);
 
-  // Attempt to start a navigation on the RenderFrame that was created to
-  // replace the now-detached RenderFrameProxy.   This shouldn't crash and
-  // should abort the navigation, since the frame no longer exists.
-  CommonNavigationParams common_params;
-  common_params.navigation_type = FrameMsg_Navigate_Type::NORMAL;
-  common_params.url = GURL(url::kAboutBlankURL);
-  provisional_frame->Navigate(common_params, StartNavigationParams(),

[alexmos, 2017/01/13 02:26:54]

I didn't find a way to test this Navigate() call anymore, given that the frame
is gone, so I just repurposed the test to check that the frame was deleted. 
(The only thing we could still test would be to see if OnNavigate would be
dropped for a non-existent frame, but I don't know if that's possible to do in a
RenderViewTest)

[Charlie Reis, 2017/01/18 00:18:42]

Acknowledged.

-                              RequestNavigationParams());
-  ProcessPendingMessages();
-
-  // Check that there was no DidCommitProvisionalLoad.
-  const IPC::Message* frame_navigate_msg =
-      render_thread_->sink().GetUniqueMessageMatching(
-          FrameHostMsg_DidCommitProvisionalLoad::ID);
-  EXPECT_FALSE(frame_navigate_msg);
-
-  // Detach the provisional frame to clean it up.  Normally, the browser
-  // process would trigger this via FrameMsg_Delete.
-  provisional_frame->GetWebFrame()->detach();
+  // The provisional frame should have been deleted along with the proxy, and
+  // thus any subsequent messages (such as OnNavigate) already in flight for it
+  // should be dropped.
+  {
+    TestRenderFrame* provisional_frame = static_cast<TestRenderFrame*>(
+        RenderFrameImpl::FromRoutingID(routing_id));
+    EXPECT_FALSE(provisional_frame);
+  }
 }
 
 // Verify that the renderer process doesn't crash when device scale factor
 // changes after a cross-process navigation has commited.
 // See https://crbug.com/571603.
 TEST_F(RenderViewImplTest, SetZoomLevelAfterCrossProcessNavigation) {
   // This test should only run with out-of-process iframes enabled.
   if (!SiteIsolationPolicy::AreCrossProcessFramesPossible())
     return;
 
@@ skipping 1640 matching lines @@
   ExpectPauseAndResume(3);
   blink::WebScriptSource source2(
       WebString::fromUTF8("function func2() { func1(); }; func2();"));
   frame()->GetWebFrame()->executeScriptInIsolatedWorld(17, &source2, 1, 1);
 
   EXPECT_FALSE(IsPaused());
   Detach();
 }
 
 }  // namespace content