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..155b568e3513b23922d88e52fed48f305cbf6a14 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,52 @@ 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());
+
+  // 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());
+  // As the SiteInstance didn't change, the site URL is still the same as

[clamy, 2015/03/06 15:57:50]

Maybe "The SiteInstance did not change, therefore its site URL is still the same
as the one used to create it before." But to me, a more interesting test (since
the site URL is a bit limitated) would be to compare the original SiteInstance
ID with the one at the end and they should match, hence making it same-site.

[carlosk, 2015/03/06 16:34:08]

I updated the comment.

The information I think is important to convey here is that even though this
last navigation was made to an apparent "http://google.com" site URL, the
SiteInstance is still pointing to "http://chromium.org". Switching to a site_id
comparison would not present that fact as clearly.

[clamy, 2015/03/06 16:58:15]

I would argue that the important information is that the SiteInstance did not
change. Yes its site did not change, because once you bind a SiteInstance to a
site, the site cannot change anymore (see
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/si...).
However this is more of a SiteInstance implementation detail than an essential
feature to test for.

Secondly all tests dealing specifically with the swapping or not of
SiteInstances use the SiteInstance ID, if only because using Site URLs is not a
reliable way to do SiteInstances comparison.

[carlosk, 2015/03/06 17:44:45]

You and I know how about the immutability of SiteInstance's site URL after it's
set. But I'm suggesting this test would help whoever doesn't know that yet (like
last-year-me for instance).

I agree that the important fact is that the SiteInstance didn't change but just
checking for site Ids doesn't make it clear that the site URL didn't change
either. IMO this is the more surprising fact for a newcomer who's looking at
these tests for the 1st time (likely before looking at SiteInstance::SetSite
documentation, which is not directly referenced from here).
Indeed they do, but for a different reason: they are testing for RFH instance
correctness. I replaced all direct RFH pointer comparisons from these tests with
site Id comparisons because they were safer (see http://crbug.com/438651). This
is not my objective with this check.

So to try and also resolve your concern, I'm adding the site id comparison check
along with the site URL check. WDYT?

[clamy, 2015/03/16 13:51:18]

Thanks for adding the site instance ID check. My concern with keeping the url
CHECK is that people reading the test may misinterpret it as "This is how we
should behave" instead of "This is how we currently behave due to implementation
details". Therefore I would like to see it removed.

[carlosk, 2015/03/16 14:08:03]

Agreed and removed.

+  // before.
+  EXPECT_EQ(SiteInstanceImpl::GetSiteForURL(browser_context(), kUrl1),
+            main_test_rfh()->GetSiteInstance()->GetSiteURL());
+}
+
 // PlzNavigate: Test that a beforeUnload denial cancels the navigation.
 TEST_F(NavigatorTestWithBrowserSideNavigation,
        BeforeUnloadDenialCancelNavigation) {
@@ -542,10 +588,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 +603,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());
 }
 
 // PlzNavigate: Test that a renderer-initiated user-initiated navigation is NOT
@@ -604,7 +641,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 +652,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());
 }
 
@@ -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,15 @@ 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());
+  // As the SiteInstance didn't change, the site URL is still the same as
+  // before.
+  EXPECT_EQ(SiteInstanceImpl::GetSiteForURL(browser_context(), kUrl0),
+            main_test_rfh()->GetSiteInstance()->GetSiteURL());
 }
 
 // PlzNavigate: Test that a reload navigation is properly signaled to the
@@ -968,10 +1005,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