PlzNavigate: have renderer-initiated navigations be same-process

content/browser/frame_host/navigator_impl_unittest.cc

 // Copyright 2014 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 "base/command_line.h"
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigation_request.h"
@@ skipping 136 matching lines @@
   // After a navigation is finished no speculative RenderFrameHost should
   // exist.
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   // With PlzNavigate enabled a pending RenderFrameHost should never exist.
   EXPECT_FALSE(node->render_manager()->pending_frame_host());
 }
 
 // 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");
 
   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();
@@ skipping 19 matching lines @@
   main_test_rfh()->SendNavigate(0, kUrl2);
   EXPECT_EQ(RenderFrameHostImpl::STATE_DEFAULT, main_test_rfh()->rfh_state());
   EXPECT_EQ(SiteInstanceImpl::GetSiteForURL(browser_context(), kUrl2),
             main_test_rfh()->GetSiteInstance()->GetSiteURL());
   EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
   EXPECT_FALSE(GetNavigationRequestForFrameTreeNode(node));
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
   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) {
   const GURL kUrl1("http://www.google.com/");
   const GURL kUrl2("http://www.chromium.org/");
 
   contents()->NavigateAndCommit(kUrl1);
 
   // Start a new navigation.
   FrameTreeNode* node = main_test_rfh()->frame_tree_node();
@@ skipping 328 matching lines @@
   RequestNavigation(node, kUrl1);
   main_test_rfh()->SendBeforeUnloadACK(true);
   NavigationRequest* request1 = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(request1);
   EXPECT_EQ(kUrl1, request1->common_params().url);
   EXPECT_TRUE(request1->browser_initiated());
   base::WeakPtr<TestNavigationURLLoader> loader1 =
       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.
   main_test_rfh()->SendRendererInitiatedNavigationRequest(kUrl2, true);
   NavigationRequest* request2 = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(request2);
   EXPECT_EQ(kUrl2, request2->common_params().url);
   EXPECT_FALSE(request2->browser_initiated());
   EXPECT_TRUE(request2->begin_params().has_user_gesture);
 
   // 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
 // canceled if a renderer-initiated non-user-initiated request is issued in the
 // meantime.
 TEST_F(NavigatorTestWithBrowserSideNavigation,
        RendererNonUserInitiatedNavigationDoesntCancelRendererUserInitiated) {
   const GURL kUrl0("http://www.wikipedia.org/");
   const GURL kUrl1("http://www.chromium.org/");
   const GURL kUrl2("http://www.google.com/");
 
   // Initialization.
   contents()->NavigateAndCommit(kUrl0);
   FrameTreeNode* node = main_test_rfh()->frame_tree_node();
 
   // Start a renderer-initiated user-initiated navigation to the 1st URL.
   process()->sink().ClearMessages();
   main_test_rfh()->SendRendererInitiatedNavigationRequest(kUrl1, true);
   NavigationRequest* request1 = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(request1);
   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.
   main_test_rfh()->SendRendererInitiatedNavigationRequest(kUrl2, false);
   NavigationRequest* request2 = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(request2);
   EXPECT_EQ(request1, request2);
   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());
 }
 
 // PlzNavigate: Test that a browser-initiated navigation is NOT canceled if a
 // renderer-initiated non-user-initiated request is issued in the meantime.
 TEST_F(NavigatorTestWithBrowserSideNavigation,
        RendererNonUserInitiatedNavigationDoesntCancelBrowserInitiated) {
   const GURL kUrl0("http://www.wikipedia.org/");
   const GURL kUrl1("http://www.chromium.org/");
   const GURL kUrl2("http://www.google.com/");
@@ skipping 51 matching lines @@
   FrameTreeNode* node = main_test_rfh()->frame_tree_node();
 
   // Start a renderer-initiated non-user-initiated navigation to the 1st URL.
   process()->sink().ClearMessages();
   main_test_rfh()->SendRendererInitiatedNavigationRequest(kUrl1, false);
   NavigationRequest* request1 = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(request1);
   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);
 
   // Now receive a 2nd similar request that should replace the current one.
   main_test_rfh()->SendRendererInitiatedNavigationRequest(kUrl2, false);
   NavigationRequest* request2 = GetNavigationRequestForFrameTreeNode(node);
   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);
 
   // 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);
   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
 // RenderFrame when the navigation can commit. A speculative RenderFrameHost
 // should not be created at any step.
 TEST_F(NavigatorTestWithBrowserSideNavigation, Reload) {
   const GURL kUrl("http://www.google.com/");
   contents()->NavigateAndCommit(kUrl);
 
   FrameTreeNode* node = main_test_rfh()->frame_tree_node();
@@ skipping 223 matching lines @@
 
   // Do a renderer-initiated navigation to a data url. The request should not be
   // sent to the IO thread, nor committed.
   TestRenderFrameHost* main_rfh = main_test_rfh();
   main_rfh->SendRendererInitiatedNavigationRequest(kUrl2, true);
   navigation_request = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(navigation_request);
   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