Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1794)

Unified Diff: content/renderer/render_view_browsertest.cc

Issue 2628133002: When a proxy is detached, immediately delete its associated provisional frame. (Closed)
Patch Set: Charlie's nit Created 3 years, 11 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « content/renderer/render_frame_proxy.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: content/renderer/render_view_browsertest.cc
diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc
index 3970bb9eae387ea0cd902b844d78ecbbb3635be9..25bd75ddf6df4df19ea4587e635252edc40aa72a 100644
--- a/content/renderer/render_view_browsertest.cc
+++ b/content/renderer/render_view_browsertest.cc
@@ -846,12 +846,11 @@ TEST_F(RenderViewImplTest, OriginReplicationForSwapOut) {
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;
@@ -877,9 +876,11 @@ TEST_F(RenderViewImplTest, NavigateProxyAndDetachBeforeOnNavigate) {
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(
@@ -888,25 +889,14 @@ TEST_F(RenderViewImplTest, NavigateProxyAndDetachBeforeOnNavigate) {
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(),
- 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
« no previous file with comments | « content/renderer/render_frame_proxy.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698