PlzNavigate: Speculatively spawns a renderer process for navigations.

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 37a0f243d6dbd01a074760fd51caeba647bfa7db..23d17f8ac068b90e3dc0545f800503ddd3c969eb 100644
--- a/content/browser/frame_host/navigator_impl_unittest.cc
+++ b/content/browser/frame_host/navigator_impl_unittest.cc
@@ -171,6 +171,8 @@ class NavigatorTest : public RenderViewHostImplTestHarness {
 // BeginNavigation.
 // Note that all PlzNavigate methods on the browser side require the use of the
 // flag kEnableBrowserSideNavigation.
+// TODO(carlosk): Will fix this ASAP but it currently crashes with my latest
+// changes from speculative renderer creation.
 TEST_F(NavigatorTest, BrowserSideNavigationBeginNavigation) {
   const GURL kUrl1("http://www.google.com/");
   const GURL kUrl2("http://www.chromium.org/");
@@ -301,6 +303,8 @@ TEST_F(NavigatorTest, BrowserSideNavigationNoContent) {
 
 // PlzNavigate: Test that a new RenderFrameHost is created when doing a cross
 // site navigation.
+// TODO(carlosk): Will fix this ASAP but it currently fails because it relies on
+// the pending_* members of RFHM which are not necessarily used by PlzNavigate.

[clamy, 2014/11/19 15:03:29]

In the new version, we should just check that the new main RFH is different from
the old one (stored in rfh).

[carlosk, 2014/11/19 17:24:28]

Acknowledged.

[carlosk, 2014/11/21 14:36:33]

I did that and fixed all but one of the failing tests.

NavigatorTestWithBrowserSideNavigation.BeginNavigation is crashing in a CHECK
inside RFHM due to an uninitialized RenderViewHost. I think it has something to
do with that test trying to navigate a subframe and the speculative RFH code is
not handling that case correctly.

[nasko, 2014/11/25 00:19:10]

Subframes will never navigate cross-process without --site-per-process flag.

[carlosk, 2014/11/28 13:08:17]

Done.

 TEST_F(NavigatorTest, BrowserSideNavigationCrossSiteNavigation) {
   const GURL kUrl1("http://www.chromium.org/");
   const GURL kUrl2("http://www.google.com/");
@@ -374,6 +378,8 @@ TEST_F(NavigatorTest, BrowserSideNavigationRedirectCrossSite) {
 
 // PlzNavigate: Test that a navigation is cancelled if another request has been
 // issued in the meantime.
+// TODO(carlosk): Will fix this ASAP but it currently fails because it relies on
+// the pending_* members of RFHM which are not necessarily used by PlzNavigate.
 TEST_F(NavigatorTest, BrowserSideNavigationReplacePendingNavigation) {
   const GURL kUrl0("http://www.wikipedia.org/");
   const GURL kUrl0_site = SiteInstance::GetSiteForURL(browser_context(), kUrl0);
@@ -477,4 +483,56 @@ TEST_F(NavigatorTest, BrowserSideNavigationReload) {
             main_request->common_params().navigation_type);
 }
 
+// PlzNavigate: Confirms that a speculative renderer process is used when
+// navigation is committed to the same SiteInstance as the one for the initial
+// URL.
+// TODO(carlosk): move this test to render_frame_host_manager_unittest.cc once
+// the proper test fixtures are made available (TestNavigationURLLoader).
+TEST_F(NavigatorTest, BrowserSideNavigationSpeculativeRendererWorksSimple) {
+  const GURL kUrlInit("http://wikipedia.org/");
+  contents()->NavigateAndCommit(kUrlInit);
+
+  EnableBrowserSideNavigation();
+  TestRenderFrameHost* mrfh = main_test_rfh();
+  FrameTreeNode* node = mrfh->frame_tree_node();
+  RenderFrameHostManager* rfhm = node->render_manager();
+  TestRenderFrameHost* srfh =
+      static_cast<TestRenderFrameHost*>(rfhm->speculative_render_frame_host());

[nasko, 2014/11/19 01:00:23]

Why do you need to static cast? You aren't calling any test specific methods on
this.

[carlosk, 2014/11/19 17:24:28]

I added the cast when I was figuring out how to invoke the correct PlzNavigate
methods but it was finally not needed and I forgot to remove it. Removed now.

[carlosk, 2014/11/21 14:36:33]

Done.

+  ASSERT_FALSE(srfh);
+
+  const GURL kUrlNav("http://google.com/");
+  SendRequestNavigation(node, kUrlNav);
+  contents()->GetMainFrame()->SendBeginNavigationWithURL(kUrlNav);
+  srfh =
+      static_cast<TestRenderFrameHost*>(rfhm->speculative_render_frame_host());
+  ASSERT_TRUE(srfh);
+  EXPECT_NE(srfh, mrfh);
+  EXPECT_TRUE(srfh->GetProcess()->HasConnection());
+
+  const GURL kUrlFinal("http://www.google.com/about/");
+  NavigationRequest* main_request = GetNavigationRequestForFrameTreeNode(node);
+  CommonNavigationParams& common_params = main_request->common_params();
+  common_params.url = kUrlFinal;
+  common_params.transition = ui::PAGE_TRANSITION_CLIENT_REDIRECT;

[nasko, 2014/11/19 01:00:23]

In order for client redirect to occur, you need to have a document committed,
right? I don't see a commit for google.com at this point.

[clamy, 2014/11/19 15:03:29]

Unless you are specifically testing a client redirect, you may want to change it
to PAGE_TRANSITION_TYPED (since we are faking a browser initiated navigation
here).

[carlosk, 2014/11/19 17:24:28]

In this case I was guesstimating that to maintain consistency a faked redirect
would be the correct transition because the initial URL differs from the final
one.

We are faking that the initial one is typed indeed but not the second one
otherwise it would cancel the former.

Would changing it to PAGE_TRANSITION_SERVER_REDIRECT make this consistent?

[nasko, 2014/11/19 19:01:29]

Why are you even faking it? If you want to test that server redirects behave
properly, it should be a separate test case. This test case is called *Simple,
so I would expect it to test a regular request that commits fine without
anything special.

[carlosk, 2014/11/21 14:36:33]

Agreed and done. I made this test to test the actual simplest case of navigating
from site 1 to site 2. Redirects are left for another future test.

+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  GetLoaderForNavigationRequest(main_request)
+      ->CallOnResponseStarted(response, MakeEmptyStream());
+  EXPECT_EQ(srfh, main_test_rfh());
+  EXPECT_FALSE(rfhm->speculative_render_frame_host());
+
+  // And just for completeness invokes OnDidCommitProvisionalLoad which
+  // shouldn't change anything in RFHM.
+  FrameHostMsg_DidCommitProvisionalLoad_Params params;
+  params.page_id = 1;
+  params.url = kUrlFinal;
+  params.transition = ui::PAGE_TRANSITION_CLIENT_REDIRECT;
+  params.was_within_same_page = false;
+  params.is_post = false;
+  params.post_id = -1;
+  params.page_state = PageState::CreateForTesting(kUrlFinal, false, 0, 0);
+  main_test_rfh()->SendNavigateWithParams(&params);
+  EXPECT_EQ(srfh, main_test_rfh());
+  EXPECT_FALSE(rfhm->speculative_render_frame_host());
+}
+
 }  // namespace content