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 96afc97d3479fbf69b0575ba16bdf879d1bfb3eb..f50512bf43a4e8cc2e5c15226052f87ac33a9a59 100644
--- a/content/browser/frame_host/navigator_impl_unittest.cc
+++ b/content/browser/frame_host/navigator_impl_unittest.cc
@@ -79,6 +79,10 @@ class NavigatorTestWithBrowserSideNavigation
     return static_cast<NavigatorImpl*>(frame_tree_node->navigator())
         ->GetNavigationRequestForNodeForTesting(frame_tree_node);
   }
+
+  RenderFrameHost* GetSpeculativeRenderFrameHost(RenderFrameHostManager* rfhm) {
+    return rfhm->speculative_render_frame_host_.get();
+  }
 };
 
 // PlzNavigate: Test that a proper NavigationRequest is created by
@@ -228,15 +232,15 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, CrossSiteNavigation) {
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(main_request)->CallOnResponseStarted(

[Charlie Reis, 2014/12/04 01:11:30]

I'm a bit confused by what this means.  Before, it created a pending RFH but
hadn't committed it.  Now, the new RFH has already committed.

Where are we in the navigation timeline?  Has the new NavigationEntry committed?
 Maybe it would help to add that stage to the test if not.

[clamy, 2014/12/05 17:16:19]

The new RFH is only committed after this call (this simulates the IO thread
signaling us that a navigation is ready to be committed). The navigation entry
is still committed after receiving the DidCommitProvisionalLoad IPC. This may be
a problem, and I think we should move the commit of the NavigationEntry at the
same time as the RFH swap in (ie in response to
NavigatorImpl::CommitNavigation).

[Charlie Reis, 2014/12/05 19:06:23]

Oh, I get it.  CallOnResponseStarted used to create the pending RFH and it
wasn't committed until DidCommitProvisionalLoad.  As a result of this CL, we
commit the speculative RFH at this time.
This makes me very nervous.  I agree that it's not great to commit the RFH
earlier than the NavigationEntry, but it's also very bad to say that a
NavigationEntry has committed when it hasn't.  There's huge potential for URL
spoofs in that case.

The problem is that it's still too hard to guarantee that we'll see the commit
we're expecting.  Even if we ignore the fact that the renderer process might
crash or be slow, there's just a whole lot of content+Blink code between
OnCommitNavigation and DidCommitProvisionalLoad in the renderer.  Without
auditing that, I don't know if we have a 100% guarantee we'll get a commit of
the URL we were expecting, or if something might alter it.

Maybe it's worth auditing that code to see if we can get a guarantee?

[carlosk, 2014/12/09 07:55:41]

I can do the audit but I'd need more details on what I should be looking for. If
I understood what you said it seems the main thing would be to verify that
there's no way the navigation could be cancelled in between the RFH being made
active and the call to DidNavigate, right? And what else?

Another option would be to simply move the RFH swapping back to
did-commit-provisional-load time if that makes more sense and keeps the RFH in
sync with the other involved components.

[Charlie Reis, 2014/12/10 22:37:44]

Canceled or otherwise altered (e.g., committing a different URL than expected,
or other changes to the DidCommit parameters).
Yes, I'm in favor of not making the change to commit time in this CL.

[carlosk, 2014/12/16 01:53:47]

Ack. As discussed let's leave that for when we remove regular navigation code
and have a clear code path.
Done.

       response, MakeEmptyStream());
-  RenderFrameHostImpl* pending_rfh =
-      node->render_manager()->pending_frame_host();
-  ASSERT_TRUE(pending_rfh);
-  EXPECT_NE(pending_rfh, rfh);
-  EXPECT_TRUE(pending_rfh->IsRenderFrameLive());
-  EXPECT_TRUE(pending_rfh->render_view_host()->IsRenderViewLive());
+  RenderFrameHostImpl* final_rfh = main_test_rfh();
+  ASSERT_TRUE(final_rfh);
+  EXPECT_NE(final_rfh, rfh);
+  EXPECT_TRUE(final_rfh->IsRenderFrameLive());
+  EXPECT_TRUE(final_rfh->render_view_host()->IsRenderViewLive());
 }
 
-// PlzNavigate: Test that redirects are followed.
+// PlzNavigate: Test that redirects are followed and the speculative renderer
+// logic behaves as expected.
 TEST_F(NavigatorTestWithBrowserSideNavigation, RedirectCrossSite) {
   const GURL kUrl1("http://www.chromium.org/");
   const GURL kUrl2("http://www.google.com/");
@@ -245,12 +249,14 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, RedirectCrossSite) {
   RenderFrameHostImpl* rfh = main_test_rfh();
   EXPECT_EQ(RenderFrameHostImpl::STATE_DEFAULT, rfh->rfh_state());
   FrameTreeNode* node = main_test_rfh()->frame_tree_node();
+  RenderFrameHostManager* rfhm = node->render_manager();
 
   // Navigate to a URL on the same site.
   SendRequestNavigation(node, kUrl1);
   main_test_rfh()->SendBeginNavigationWithURL(kUrl1);
   NavigationRequest* main_request = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(main_request);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(rfhm));
 
   // It then redirects to another site.
   net::RedirectInfo redirect_info;
@@ -258,27 +264,29 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, RedirectCrossSite) {
   redirect_info.new_method = "GET";
   redirect_info.new_url = kUrl2;
   redirect_info.new_first_party_for_cookies = kUrl2;
- scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(main_request)->CallOnRequestRedirected(
       redirect_info, response);
 
   // The redirect should have been followed.
   EXPECT_EQ(1, GetLoaderForNavigationRequest(main_request)->redirect_count());
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(rfhm));

[Charlie Reis, 2014/12/04 01:11:30]

Why is there no speculative RFH after it redirects cross site?  Is that planned
for later?  (Seems like we might as well create one while we're waiting for the
new response.)

If we're not planning to create one in this CL, then a TODO here might be
worthwhile.

[carlosk, 2014/12/09 07:55:41]

Indeed that is a task for later on. The idea is to monitor redirects --- that
now are conveniently tracked at the browser side -- to update the speculative
RFH accordingly. But we have to be careful with this as we'll possibly be
creating quite some churn by creating and destroying RFH in bursts.

Also this made me think that the question regarding the name choice of
"speculative" might be the right one (see respective comment in RFHM) as when we
actually implement this that all we will have are speculative RFH until the last
redirect.

[Charlie Reis, 2014/12/10 22:37:44]

Later is fine.  Worth noting that we wouldn't get a "burst" for multiple
redirects, at least.  We'll get at most two speculative RFHs per navigation
(once when it starts, and once after all the server redirects are done).

[carlosk, 2014/12/16 01:53:47]

So if I understand correctly when server redirects happen we only get the final
result? I guess we could still get some more when jumping from one server to the
next?

[Charlie Reis, 2015/01/12 23:30:51]

[Just following up on an old thread.  This probably doesn't matter anymore.]

It was designed so that we don't choose a new process until all the server
redirects have finished.  There's at most one speculative RFH at the time the
navigation is begun, and one more after all the server redirects.

However, client redirects are a different story.  Those actually commit a real
page in a renderer process, and then immediately decide to navigate somewhere
else.  There could be an arbitrarily long chain of client redirects each
requiring a process swap (up to a limit when Chrome says the redirect chain is
too long).

[carlosk, 2015/01/13 15:54:12]

Understood. To confirm: the server redirects are "invisible" even when we hop
from domain to domain (ie goo.gl -> bit.ly -> tinyurl.com -> ...)?

[Charlie Reis, 2015/01/13 18:48:21]

Invisible to RFHM and the UI thread, correct.  The network stack handles them
and lets the us know when the page is ready to commit.

[carlosk, 2015/01/14 10:09:46]

Acknowledged.

 
   // Then it commits.
   response = new ResourceResponse;
   GetLoaderForNavigationRequest(main_request)->CallOnResponseStarted(
       response, MakeEmptyStream());
-  RenderFrameHostImpl* pending_rfh =
-      node->render_manager()->pending_frame_host();
-  ASSERT_TRUE(pending_rfh);
-  EXPECT_NE(pending_rfh, rfh);
-  EXPECT_TRUE(pending_rfh->IsRenderFrameLive());
-  EXPECT_TRUE(pending_rfh->render_view_host()->IsRenderViewLive());
+  RenderFrameHostImpl* final_rfh = main_test_rfh();
+  ASSERT_TRUE(final_rfh);
+  EXPECT_NE(final_rfh, rfh);
+  EXPECT_TRUE(final_rfh->IsRenderFrameLive());
+  EXPECT_TRUE(final_rfh->render_view_host()->IsRenderViewLive());
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(rfhm));
 }
 
-// PlzNavigate: Test that a navigation is cancelled if another request has been
-// issued in the meantime.
+// PlzNavigate: Test that a navigation is canceled if another request has been
+// issued in the meantime. Also confirms that the speculative renderer is
+// correctly updated in the process.
 TEST_F(NavigatorTestWithBrowserSideNavigation, ReplacePendingNavigation) {
   const GURL kUrl0("http://www.wikipedia.org/");
   const GURL kUrl0_site = SiteInstance::GetSiteForURL(browser_context(), kUrl0);
@@ -289,6 +297,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, ReplacePendingNavigation) {
   // Initialization.
   contents()->NavigateAndCommit(kUrl0);
   FrameTreeNode* node = main_test_rfh()->frame_tree_node();
+  RenderFrameHostManager* rfhm = node->render_manager();
   EXPECT_EQ(kUrl0_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
 
   // Request navigation to the 1st URL.
@@ -300,6 +309,11 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, ReplacePendingNavigation) {
   base::WeakPtr<TestNavigationURLLoader> loader1 =
       GetLoaderForNavigationRequest(request1)->AsWeakPtr();
 
+  // Confirms a speculative RFH was created

[Charlie Reis, 2014/12/04 01:11:30]

nit: Please end sentences with a period, here and below.  Also "Confirm" rather
than "Confirms" (as elsewhere in this test).

[carlosk, 2014/12/09 07:55:41]

Done.

+  ASSERT_TRUE(GetSpeculativeRenderFrameHost(rfhm));
+  int32 first_site_id =

[Charlie Reis, 2014/12/04 01:11:30]

nit: site_instance_id1 and site_instance_id2 would be clearer names, since sites
don't have IDs.

[carlosk, 2014/12/09 07:55:41]

Done.

+      GetSpeculativeRenderFrameHost(rfhm)->GetSiteInstance()->GetId();

[Charlie Reis, 2014/12/04 01:11:30]

Let's also verify that GetSiteURL() is kUrl1_site (similar to the kUrl2_site
check below).

[carlosk, 2014/12/09 07:55:41]

Done.

+
   // Request navigation to the 2nd URL; the NavigationRequest must have been
   // replaced by a new one with a different URL.
   SendRequestNavigation(node, kUrl2);
@@ -311,23 +325,32 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, ReplacePendingNavigation) {
   // Confirm that the first loader got destroyed.
   EXPECT_FALSE(loader1);
 
+  // Confirms that a new speculative RFH was created
+  ASSERT_TRUE(GetSpeculativeRenderFrameHost(rfhm));
+  int32 second_site_id =
+      GetSpeculativeRenderFrameHost(rfhm)->GetSiteInstance()->GetId();
+  EXPECT_NE(first_site_id, second_site_id);
+
   // Confirm that the commit corresponds to the new request.
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(request2)->CallOnResponseStarted(
       response, MakeEmptyStream());
-  RenderFrameHostImpl* pending_rfh =
-      node->render_manager()->pending_frame_host();
-  ASSERT_TRUE(pending_rfh);
-  EXPECT_EQ(kUrl2_site, pending_rfh->GetSiteInstance()->GetSiteURL());
+  ASSERT_TRUE(main_test_rfh());
+  EXPECT_EQ(kUrl2_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
+
+  // Confirms that the committed RFH is the new speculative one
+  EXPECT_EQ(second_site_id, main_test_rfh()->GetSiteInstance()->GetId());
 }
 
 // PlzNavigate: Test that a reload navigation is properly signaled to the
-// renderer when the navigation can commit.
+// renderer when the navigation can commit. Speculative renderers 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();
+  RenderFrameHostManager* rfhm = node->render_manager();
   SendRequestNavigationWithParameters(
       node, kUrl, Referrer(), ui::PAGE_TRANSITION_LINK,
       NavigationController::RELOAD);
@@ -338,9 +361,12 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, Reload) {
   ASSERT_TRUE(main_request != NULL);
   EXPECT_EQ(FrameMsg_Navigate_Type::RELOAD,
             main_request->common_params().navigation_type);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(rfhm));
+
   int page_id = contents()->GetMaxPageIDForSiteInstance(
                     main_test_rfh()->GetSiteInstance()) + 1;
   main_test_rfh()->SendNavigate(page_id, kUrl);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(rfhm));
 
   // Now do a shift+reload.
   SendRequestNavigationWithParameters(
@@ -352,6 +378,104 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, Reload) {
   ASSERT_TRUE(main_request != NULL);
   EXPECT_EQ(FrameMsg_Navigate_Type::RELOAD_IGNORING_CACHE,
             main_request->common_params().navigation_type);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(rfhm));
+}
+
+// PlzNavigate: Confirms that a speculative RenderFrameHost is used when
+// navigating from one site to the another.
+TEST_F(NavigatorTestWithBrowserSideNavigation,
+       SpeculativeRendererWorksBaseCase) {
+  // Navigate to an initial site.
+  const GURL kUrlInit("http://wikipedia.org/");
+  contents()->NavigateAndCommit(kUrlInit);
+  FrameTreeNode* node = main_test_rfh()->frame_tree_node();
+  RenderFrameHostManager* rfhm = node->render_manager();
+  ASSERT_FALSE(GetSpeculativeRenderFrameHost(rfhm));
+
+  // Begin navigating to another site.
+  const GURL kUrl("http://google.com/");
+  SendRequestNavigation(node, kUrl);
+  contents()->GetMainFrame()->SendBeginNavigationWithURL(kUrl);
+  ASSERT_TRUE(GetSpeculativeRenderFrameHost(rfhm));
+  EXPECT_NE(GetSpeculativeRenderFrameHost(rfhm), main_test_rfh());
+  EXPECT_EQ(
+      GetSpeculativeRenderFrameHost(rfhm)->GetSiteInstance()->GetSiteURL(),

[Charlie Reis, 2014/12/04 01:11:30]

nit: Switch argument order.  (EXPECT_EQ(expected, actual))

[carlosk, 2014/12/09 07:55:40]

Done.

+      SiteInstanceImpl::GetSiteForURL(browser_context(), kUrl));
+  EXPECT_TRUE(
+      GetSpeculativeRenderFrameHost(rfhm)->GetProcess()->HasConnection());

[Charlie Reis, 2014/12/04 01:11:30]

We're in a unit test (with MockRenderProcessHosts), so this can't fail.  Might
as well leave it out.

[clamy, 2014/12/05 17:16:19]

This could be replaced by a call to RFH::IsRenderFrameLive(), and we would still
test that we initialized the speculative rfh. 

[carlosk, 2014/12/09 07:55:41]

Acknowledged and done.

+  int32 site_id =

[Charlie Reis, 2014/12/04 01:11:30]

Same nit as above.

[carlosk, 2014/12/09 07:55:41]

Done.

+      GetSpeculativeRenderFrameHost(rfhm)->GetSiteInstance()->GetId();
+
+  // Commit.

[Charlie Reis, 2014/12/04 01:11:30]

I'm still finding it very confusing to call this "commit," which sounds a lot
like DidCommit.  Here (and in general), let's try to clarify it (e.g., "Ask the
renderer to commit the response").

[carlosk, 2014/12/09 07:55:40]

It seemed to me that "commit" was the usually accepted name for this step in the
navigation process. I updated the comment to be more explicit but PTAL.

+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  GetLoaderForNavigationRequest(GetNavigationRequestForFrameTreeNode(node))
+      ->CallOnResponseStarted(response, MakeEmptyStream());
+  EXPECT_EQ(site_id, main_test_rfh()->GetSiteInstance()->GetId());
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(rfhm));
+
+  // And just for completeness invoke OnDidCommitProvisionalLoad which
+  // shouldn't change anything in RFHM.
+  FrameHostMsg_DidCommitProvisionalLoad_Params params;
+  params.page_id = 1;
+  params.url = kUrl;
+  params.was_within_same_page = false;
+  params.is_post = false;
+  params.post_id = -1;
+  params.page_state = PageState::CreateForTesting(kUrl, false, 0, 0);
+  main_test_rfh()->SendNavigateWithParams(&params);
+  EXPECT_EQ(site_id, main_test_rfh()->GetSiteInstance()->GetId());
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(rfhm));
+}
+
+// PlzNavigate: Confirms that a speculative RenderFrameHost is thrown away when
+// the final URL's site differ from the initial one due to redirects.

[Charlie Reis, 2014/12/04 01:11:30]

nit: differs

[carlosk, 2014/12/09 07:55:41]

Done.

+TEST_F(NavigatorTestWithBrowserSideNavigation,
+       SpeculativeRendererDiscardedAfterRedirectToAnotherSite) {
+  // Navigate to an initial site.
+  const GURL kUrlInit("http://wikipedia.org/");
+  contents()->NavigateAndCommit(kUrlInit);
+  FrameTreeNode* node = main_test_rfh()->frame_tree_node();
+  RenderFrameHostManager* rfhm = node->render_manager();
+  ASSERT_FALSE(GetSpeculativeRenderFrameHost(rfhm));
+
+  // Begin navigating to another site.
+  const GURL kUrl("http://google.com/");
+  SendRequestNavigation(node, kUrl);
+  contents()->GetMainFrame()->SendBeginNavigationWithURL(kUrl);
+  int32 site_id =
+      GetSpeculativeRenderFrameHost(rfhm)->GetSiteInstance()->GetId();
+  ASSERT_TRUE(GetSpeculativeRenderFrameHost(rfhm));
+  EXPECT_NE(GetSpeculativeRenderFrameHost(rfhm), main_test_rfh());
+  EXPECT_TRUE(
+      GetSpeculativeRenderFrameHost(rfhm)->GetProcess()->HasConnection());

[Charlie Reis, 2014/12/04 01:11:30]

Same as above.

[carlosk, 2014/12/09 07:55:41]

Done.

+  EXPECT_EQ(
+      GetSpeculativeRenderFrameHost(rfhm)->GetSiteInstance()->GetSiteURL(),
+      SiteInstanceImpl::GetSiteForURL(browser_context(), kUrl));
+
+  // It then redirects to yet another site.
+  NavigationRequest* main_request = GetNavigationRequestForFrameTreeNode(node);
+  ASSERT_TRUE(main_request);
+  const GURL kUrlRedirect("https://www.google.com/");
+  net::RedirectInfo redirect_info;
+  redirect_info.status_code = 302;
+  redirect_info.new_method = "GET";
+  redirect_info.new_url = kUrlRedirect;
+  redirect_info.new_first_party_for_cookies = kUrlRedirect;
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  GetLoaderForNavigationRequest(main_request)
+      ->CallOnRequestRedirected(redirect_info, response);
+  ASSERT_TRUE(GetSpeculativeRenderFrameHost(rfhm));
+  EXPECT_EQ(site_id,

[Charlie Reis, 2014/12/04 01:11:30]

The test says the redirect is expected to discard the speculative RFH.  Why is
it still around with the old SiteInstance?

Also, as mentioned earlier, it seems like we should go ahead and create a new
speculative RFH for the new site.

[carlosk, 2014/12/09 07:55:41]

As we're not *yet* monitoring redirects the speculative RFH is still the old,
now stale one. So this is correct, for now. It's here as a mark/failure point
for when we begin monitoring redirects.

+            GetSpeculativeRenderFrameHost(rfhm)->GetSiteInstance()->GetId());
+
+  // Commit.

[Charlie Reis, 2014/12/04 01:11:30]

See comment in previous test.

[carlosk, 2014/12/09 07:55:41]

Done.

+  response = new ResourceResponse;
+  GetLoaderForNavigationRequest(main_request)
+      ->CallOnResponseStarted(response, MakeEmptyStream());
+  EXPECT_NE(site_id, main_test_rfh()->GetSiteInstance()->GetId());
+  EXPECT_EQ(main_test_rfh()->GetSiteInstance()->GetSiteURL(),
+            SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlRedirect));
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(rfhm));
 }
 
 }  // namespace content