PlzNavigate: implement process reuse for ServiceWorkers

content/browser/renderer_host/render_process_host_unittest.cc

Index: content/browser/renderer_host/render_process_host_unittest.cc
diff --git a/content/browser/renderer_host/render_process_host_unittest.cc b/content/browser/renderer_host/render_process_host_unittest.cc
index a846c714b2d15978fdff482ab6db010f76491edb..21aefa44222ca4e779fcc0f81b663eee136d8c31 100644
--- a/content/browser/renderer_host/render_process_host_unittest.cc
+++ b/content/browser/renderer_host/render_process_host_unittest.cc
@@ -10,14 +10,19 @@
 
 #include "base/memory/ptr_util.h"
 #include "build/build_config.h"
+#include "content/common/frame_messages.h"
+#include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_constants.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_utils.h"
+#include "content/test/test_render_frame_host.h"
 #include "content/test/test_render_view_host.h"
+#include "content/test/test_web_contents.h"
+#include "third_party/WebKit/public/web/WebSandboxFlags.h"
 
 namespace content {
 
-class RenderProcessHostUnitTest : public RenderViewHostTestHarness {};
+class RenderProcessHostUnitTest : public RenderViewHostImplTestHarness {};
 
 // Tests that guest RenderProcessHosts are not considered suitable hosts when
 // searching for RenderProcessHost.
@@ -84,4 +89,226 @@ TEST_F(RenderProcessHostUnitTest, NoRendererProcessLimitOnAndroid) {
 }
 #endif
 
+// Tests that RenderProcessHost reuse considers committed sites correctly.
+TEST_F(RenderProcessHostUnitTest, ReuseCommittedSite) {
+  const GURL kUrl1("http://foo.com");
+  const GURL kUrl2("http://bar.com");
+
+  // At first, trying to get a RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should return a new process.
+  scoped_refptr<SiteInstanceImpl> site_instance =
+      SiteInstanceImpl::CreateForURL(browser_context(), kUrl1);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Have the main frame navigate to the first url. Getting a RenderProcessHost
+  // with the  REUSE_PENDING_OR_COMMITTED_SITE policy should now return the

[Charlie Reis, 2017/05/18 00:58:34]

nit: Remove extra space after "the"
Same below in a few places.

[clamy, 2017/05/22 16:59:52]

Done.

+  // process of the main RFH.
+  NavigateAndCommit(kUrl1);
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl1);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_EQ(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Navigate away. Getting a RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should again return a new process.
+  NavigateAndCommit(kUrl2);
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl1);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Now add a subframe that navigates to kUrl1. Getting a RenderProcessHost
+  // with the  REUSE_PENDING_OR_COMMITTED_SITE policy for kUrl1 should now
+  // return the process of the subframe RFH.
+  std::string unique_name("uniqueName0");
+  main_test_rfh()->OnCreateChildFrame(
+      process()->GetNextRoutingID(), blink::WebTreeScopeType::kDocument,
+      std::string(), unique_name, blink::WebSandboxFlags::kNone,
+      ParsedFeaturePolicyHeader(), FrameOwnerProperties());
+  TestRenderFrameHost* subframe = static_cast<TestRenderFrameHost*>(
+      contents()->GetFrameTree()->root()->child_at(0)->current_frame_host());
+  {
+    FrameHostMsg_DidCommitProvisionalLoad_Params params;
+    params.nav_entry_id = 0;
+    params.frame_unique_name = unique_name;
+    params.did_create_new_entry = false;
+    params.url = kUrl1;
+    params.transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
+    params.should_update_history = false;
+    params.gesture = NavigationGestureUser;
+    params.method = "GET";
+    params.page_state = PageState::CreateFromURL(kUrl1);
+    subframe->SendRendererInitiatedNavigationRequest(kUrl1, false);
+    subframe->PrepareForCommit();
+    subframe->SendNavigateWithParams(&params);
+  }
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl1);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_EQ(subframe->GetProcess(), site_instance->GetProcess());
+}
+
+// Tests that RenderProcessHost will not consider reusing a process that has
+// committed an error page.
+TEST_F(RenderProcessHostUnitTest, DoNotReuseError) {
+  const GURL kUrl1("http://foo.com");
+  const GURL kUrl2("http://bar.com");
+
+  // At first, trying to get a RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should return a new process.
+  scoped_refptr<SiteInstanceImpl> site_instance =
+      SiteInstanceImpl::CreateForURL(browser_context(), kUrl1);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Have the main frame navigate to the first url. Getting a RenderProcessHost
+  // with the  REUSE_PENDING_OR_COMMITTED_SITE policy should now return the
+  // process of the main RFH.
+  NavigateAndCommit(kUrl1);
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl1);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_EQ(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Navigate away. Getting a RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should again return a new process.
+  NavigateAndCommit(kUrl2);
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl1);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Navigate back and simulate an error. Getting a RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should return a new process.
+  web_contents()->GetController().GoBack();
+  TestRenderFrameHost* pending_rfh = contents()->GetPendingMainFrame();
+  if (!IsBrowserSideNavigationEnabled())
+    pending_rfh->SimulateNavigationStart(kUrl1);
+  pending_rfh->SimulateNavigationError(kUrl1, net::ERR_TIMED_OUT);
+  pending_rfh->SimulateNavigationErrorPageCommit();
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl1);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+}
+
+// Tests that RenderProcessHost reuse considers navigations correctly.
+TEST_F(RenderProcessHostUnitTest, ReuseNavigationProcess) {
+  // This is only applicable to PlzNavigate.
+  if (!IsBrowserSideNavigationEnabled())
+    return;
+
+  const GURL kUrl1("http://foo.com");
+  const GURL kUrl2("http://bar.com");
+
+  // At first, trying to get a RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should return a new process.
+  scoped_refptr<SiteInstanceImpl> site_instance =
+      SiteInstanceImpl::CreateForURL(browser_context(), kUrl1);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Start a navigation. Now Getting RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should return the current process.
+  main_test_rfh()->SimulateNavigationStart(kUrl1);
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl1);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_EQ(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Finish the navigation and start a new cross-site one. Getting
+  // RenderProcessHost with the  REUSE_PENDING_OR_COMMITTED_SITE policy should
+  // return the process of the speculative RenderFrameHost.
+  main_test_rfh()->PrepareForCommit();
+  main_test_rfh()->SendNavigate(0, true, kUrl1);
+  contents()->GetController().LoadURL(kUrl2, Referrer(),
+                                      ui::PAGE_TRANSITION_TYPED, std::string());
+  main_test_rfh()->SendBeforeUnloadACK(true);
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl2);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_EQ(contents()->GetPendingMainFrame()->GetProcess(),
+            site_instance->GetProcess());
+
+  // Remember the process id and cancel the navigation. Getting
+  // RenderProcessHost with the  REUSE_PENDING_OR_COMMITTED_SITE policy should
+  // no longer return the process of the speculative RenderFrameHost.
+  int speculative_process_host_id =
+      contents()->GetPendingMainFrame()->GetProcess()->GetID();
+  contents()->GetPendingMainFrame()->SimulateNavigationError(kUrl2,
+                                                             net::ERR_ABORTED);
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl2);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(speculative_process_host_id, site_instance->GetProcess()->GetID());
+}
+
+// Tests that RenderProcessHost reuse considers navigations correctly during
+// redirects.
+TEST_F(RenderProcessHostUnitTest, ReuseNavigationProcessRedirects) {
+  // This is only applicable to PlzNavigate.
+  if (!IsBrowserSideNavigationEnabled())
+    return;
+
+  const GURL kUrl("http://foo.com");
+  const GURL kRedirectUrl1("http://foo.com/redirect");
+  const GURL kRedirectUrl2("http://bar.com");
+
+  // At first, trying to get a RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should return a new process.
+  scoped_refptr<SiteInstanceImpl> site_instance =
+      SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Start a navigation. Now getting RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should return the current process.
+  main_test_rfh()->SimulateNavigationStart(kUrl);
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_EQ(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Simulate a same-site redirect. Getting RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should return the current process.
+  main_test_rfh()->SimulateRedirect(kRedirectUrl1);
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_EQ(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Simulate a cross-site redirect. Getting RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should not return the current
+  // process, whether for the old site or the new site.
+  main_test_rfh()->SimulateRedirect(kRedirectUrl2);
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+  site_instance =
+      SiteInstanceImpl::CreateForURL(browser_context(), kRedirectUrl2);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+
+  // Once the navigation is ready to commit, Getting RenderProcessHost with the
+  // REUSE_PENDING_OR_COMMITTED_SITE policy should not return the current
+  // process for the final site, but not the initial one.
+  main_test_rfh()->PrepareForCommit();
+  site_instance = SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+  site_instance =
+      SiteInstanceImpl::CreateForURL(browser_context(), kRedirectUrl2);
+  site_instance->set_process_reuse_policy(
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+  EXPECT_EQ(main_test_rfh()->GetProcess(), site_instance->GetProcess());
+}

[Charlie Reis, 2017/05/18 00:58:34]

These are great.  Should we also add a NTP test in chrome/ to make sure we're
doing the right thing for ServiceWorkers on the NTP?

[clamy, 2017/05/22 16:59:52]

I was thinking of doing this in a follow up patch, as it may need additional
modifications to the code.

[Charlie Reis, 2017/05/23 07:29:13]

What would that mean for the NTP in the meantime, for anyone using PlzNavigate? 
That the ServiceWorker would go into a different process than the NTP?  Is that
equivalent or worse than today's PlzNavigate NTP behavior?

I can see doing NTP support in a followup CL, but I wouldn't want to regress
things for long, especially near branch point.  Maybe we can set up the NTP CL
as a dependent CL to this one to get it ready to land as soon after this one as
we can?

[clamy, 2017/05/23 13:41:15]

This patch does not regress the situation in PlzNavigate. In fact, it should
improve the situation on all navigations that are not the NTP and don't
redirect.

For the NTP, we need the search code to overwrite the site URL for the NTP
ServiceWorker so that it becomes chrome-search://remote-ntp. Then the NTP issue
should be fixed. I'd rather land this change in a follow-up CL, as it is not
directly related to this one.

[Charlie Reis, 2017/05/24 04:50:54]

Acknowledged.

+
 }  // namespace content