PlzNavigate: have renderer-initiated navigations be same-process

content/browser/frame_host/navigator_impl_unittest.cc

Index: content/browser/frame_host/navigator_impl_unittest.cc
diff --git a/content/browser/frame_host/navigator_impl_unittest.cc b/content/browser/frame_host/navigator_impl_unittest.cc
index d7e41e5856023ca3d42de60dac4c68754f8c151d..0055e35ad279aff10cfb588e2c2dabc977af6cec 100644
--- a/content/browser/frame_host/navigator_impl_unittest.cc
+++ b/content/browser/frame_host/navigator_impl_unittest.cc
@@ -154,7 +154,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
 
 // PlzNavigate: Test a complete renderer-initiated same-site navigation.
 TEST_F(NavigatorTestWithBrowserSideNavigation,
-       SimpleRendererInitiatedNavigation) {
+       SimpleRendererInitiatedSameSiteNavigation) {
   const GURL kUrl1("http://www.chromium.org/");
   const GURL kUrl2("http://www.chromium.org/Home");
 
@@ -194,6 +194,51 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_FALSE(node->render_manager()->pending_frame_host());
 }
 
+// PlzNavigate: Test a complete renderer-initiated navigation that should be
+// cross-site but does not result in a SiteInstance swap because its
+// renderer-initiated.
+TEST_F(NavigatorTestWithBrowserSideNavigation,
+       SimpleRendererInitiatedCrossSiteNavigation) {
+  const GURL kUrl1("http://www.chromium.org/");
+  const GURL kUrl2("http://www.google.com");
+
+  contents()->NavigateAndCommit(kUrl1);
+  EXPECT_TRUE(main_test_rfh()->IsRenderFrameLive());
+  int32 site_instance_id_1 = main_test_rfh()->GetSiteInstance()->GetId();
+
+  // Start a renderer-initiated non-user-initiated navigation.
+  process()->sink().ClearMessages();
+  main_test_rfh()->SendRendererInitiatedNavigationRequest(kUrl2, false);
+  FrameTreeNode* node = main_test_rfh()->frame_tree_node();
+  NavigationRequest* request = GetNavigationRequestForFrameTreeNode(node);
+  ASSERT_TRUE(request);
+
+  // The navigation is immediately started as there's no need to wait for
+  // beforeUnload to be executed.
+  EXPECT_EQ(NavigationRequest::STARTED, request->state());
+  EXPECT_FALSE(request->begin_params().has_user_gesture);
+  EXPECT_EQ(kUrl2, request->common_params().url);
+  EXPECT_FALSE(request->browser_initiated());
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
+
+  // Have the current RenderFrameHost commit the navigation.
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  GetLoaderForNavigationRequest(request)
+      ->CallOnResponseStarted(response, MakeEmptyStream());
+  EXPECT_TRUE(DidRenderFrameHostRequestCommit(main_test_rfh()));
+  EXPECT_EQ(NavigationRequest::RESPONSE_STARTED, request->state());
+
+  // Commit the navigation.
+  main_test_rfh()->SendNavigate(0, kUrl2);
+  EXPECT_EQ(RenderFrameHostImpl::STATE_DEFAULT, main_test_rfh()->rfh_state());
+  EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
+  EXPECT_FALSE(GetNavigationRequestForFrameTreeNode(node));
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
+  EXPECT_FALSE(node->render_manager()->pending_frame_host());
+  // The SiteInstance did not change.

[nasko, 2015/03/16 16:22:42]

nit: Empty line before the comment.

[carlosk, 2015/03/16 17:25:00]

Done.

+  EXPECT_EQ(site_instance_id_1, main_test_rfh()->GetSiteInstance()->GetId());
+}
+
 // PlzNavigate: Test that a beforeUnload denial cancels the navigation.
 TEST_F(NavigatorTestWithBrowserSideNavigation,
        BeforeUnloadDenialCancelNavigation) {
@@ -542,10 +587,8 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
       GetLoaderForNavigationRequest(request1)->AsWeakPtr();
   EXPECT_TRUE(loader1);
 
-  // Confirm a speculative RenderFrameHost was created.
-  TestRenderFrameHost* speculative_rfh = GetSpeculativeRenderFrameHost(node);
-  ASSERT_TRUE(speculative_rfh);
-  int32 site_instance_id_1 = speculative_rfh->GetSiteInstance()->GetId();
+  // Confirm that a speculative RenderFrameHost was created.
+  ASSERT_TRUE(GetSpeculativeRenderFrameHost(node));
 
   // Now receive a renderer-initiated user-initiated request. It should replace
   // the current NavigationRequest.
@@ -559,28 +602,21 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   // Confirm that the first loader got destroyed.
   EXPECT_FALSE(loader1);
 
-  // Confirm that a new speculative RenderFrameHost was created.
-  speculative_rfh = GetSpeculativeRenderFrameHost(node);
-  ASSERT_TRUE(speculative_rfh);
-  int32 site_instance_id_2 = speculative_rfh->GetSiteInstance()->GetId();
-  EXPECT_NE(site_instance_id_1, site_instance_id_2);
+  // Confirm that the speculative RenderFrameHost was destroyed.
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   // Have the RenderFrameHost commit the navigation.
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(request2)
       ->CallOnResponseStarted(response, MakeEmptyStream());
-  EXPECT_TRUE(DidRenderFrameHostRequestCommit(speculative_rfh));
-  EXPECT_FALSE(DidRenderFrameHostRequestCommit(main_test_rfh()));
+  EXPECT_TRUE(DidRenderFrameHostRequestCommit(main_test_rfh()));
 
   // Commit the navigation.
-  speculative_rfh->SendNavigate(0, kUrl2);
+  main_test_rfh()->SendNavigate(0, kUrl2);
 
   // Confirm that the commit corresponds to the new request.
   ASSERT_TRUE(main_test_rfh());
   EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
-
-  // Confirm that the committed RenderFrameHost is the latest speculative one.
-  EXPECT_EQ(site_instance_id_2, main_test_rfh()->GetSiteInstance()->GetId());

[nasko, 2015/03/16 16:22:42]

Why did you remove this SiteInstance check? If we expect to reuse a
SiteInstance, why not change the check appropriately instead of removing it?

[carlosk, 2015/03/16 17:25:00]

This check was added to confirm that the RenderFrameHost that committed was the
speculative one instead of using unsafe pointer checks
(http://crbug.com/438651), and not to verify the SiteInstance was maintained. I
removed it because there's no other RFH involved in the navigation after this
change but the current one. Testing that a RFH doesn't have its SiteInstance
changed doesn't seem useful in this test.

 }
 
 // PlzNavigate: Test that a renderer-initiated user-initiated navigation is NOT
@@ -604,7 +640,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_EQ(kUrl1, request1->common_params().url);
   EXPECT_FALSE(request1->browser_initiated());
   EXPECT_TRUE(request1->begin_params().has_user_gesture);
-  EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   // Now receive a renderer-initiated non-user-initiated request. Nothing should
   // change.
@@ -615,18 +651,16 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_EQ(kUrl1, request2->common_params().url);
   EXPECT_FALSE(request2->browser_initiated());
   EXPECT_TRUE(request2->begin_params().has_user_gesture);
-  TestRenderFrameHost* speculative_rfh = GetSpeculativeRenderFrameHost(node);
-  ASSERT_TRUE(speculative_rfh);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   // Have the RenderFrameHost commit the navigation.
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(request2)
       ->CallOnResponseStarted(response, MakeEmptyStream());
-  EXPECT_TRUE(DidRenderFrameHostRequestCommit(speculative_rfh));
-  EXPECT_FALSE(DidRenderFrameHostRequestCommit(main_test_rfh()));
+  EXPECT_TRUE(DidRenderFrameHostRequestCommit(main_test_rfh()));
 
   // Commit the navigation.
-  speculative_rfh->SendNavigate(0, kUrl1);
+  main_test_rfh()->SendNavigate(0, kUrl1);
   EXPECT_EQ(kUrl1, contents()->GetLastCommittedURL());
 }
 
@@ -689,6 +723,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   // Initialization.
   contents()->NavigateAndCommit(kUrl0);
   FrameTreeNode* node = main_test_rfh()->frame_tree_node();
+  int32 site_instance_id_0 = main_test_rfh()->GetSiteInstance()->GetId();
 
   // Start a renderer-initiated non-user-initiated navigation to the 1st URL.
   process()->sink().ClearMessages();
@@ -698,7 +733,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_EQ(kUrl1, request1->common_params().url);
   EXPECT_FALSE(request1->browser_initiated());
   EXPECT_FALSE(request1->begin_params().has_user_gesture);
-  EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
   base::WeakPtr<TestNavigationURLLoader> loader1 =
       GetLoaderForNavigationRequest(request1)->AsWeakPtr();
   EXPECT_TRUE(loader1);
@@ -709,8 +744,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_EQ(kUrl2, request2->common_params().url);
   EXPECT_FALSE(request2->browser_initiated());
   EXPECT_FALSE(request2->begin_params().has_user_gesture);
-  TestRenderFrameHost* speculative_rfh = GetSpeculativeRenderFrameHost(node);
-  ASSERT_TRUE(speculative_rfh);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   // Confirm that the first loader got destroyed.
   EXPECT_FALSE(loader1);
@@ -719,12 +753,13 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(request2)
       ->CallOnResponseStarted(response, MakeEmptyStream());
-  EXPECT_TRUE(DidRenderFrameHostRequestCommit(speculative_rfh));
-  EXPECT_FALSE(DidRenderFrameHostRequestCommit(main_test_rfh()));
+  EXPECT_TRUE(DidRenderFrameHostRequestCommit(main_test_rfh()));
 
   // Commit the navigation.
-  speculative_rfh->SendNavigate(0, kUrl2);
+  main_test_rfh()->SendNavigate(0, kUrl2);
   EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
+  // The SiteInstance did not change.

[nasko, 2015/03/16 16:22:42]

nit: Empty line before comment.

[carlosk, 2015/03/16 17:25:00]

Done.

+  EXPECT_EQ(site_instance_id_0, main_test_rfh()->GetSiteInstance()->GetId());
 }
 
 // PlzNavigate: Test that a reload navigation is properly signaled to the
@@ -968,10 +1003,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, DataUrls) {
   EXPECT_EQ(NavigationRequest::RESPONSE_STARTED,
             navigation_request->state());
   EXPECT_FALSE(navigation_request->loader_for_testing());
-  speculative_rfh = GetSpeculativeRenderFrameHost(node);
-  ASSERT_TRUE(speculative_rfh);
-  speculative_rfh->SendNavigate(0, kUrl2);
-  EXPECT_EQ(main_test_rfh(), speculative_rfh);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 }
 
 }  // namespace content