Fix process reuse for dedicated processes when over process limit.

content/browser/isolated_origin_browsertest.cc

Index: content/browser/isolated_origin_browsertest.cc
diff --git a/content/browser/isolated_origin_browsertest.cc b/content/browser/isolated_origin_browsertest.cc
index d44a1ba3ee54e0235c566966fa6e90aced6e6740..a832fad7d4c9981a5d8a5f96dfef0eaff0fed091 100644
--- a/content/browser/isolated_origin_browsertest.cc
+++ b/content/browser/isolated_origin_browsertest.cc
@@ -353,4 +353,117 @@ IN_PROC_BROWSER_TEST_F(IsolatedOriginTest, Cookies) {
   EXPECT_EQ("foo=bar", cookie);
 }
 
+// Check that isolated origins won't be placed into processes for other sites
+// when over the process limit.
+IN_PROC_BROWSER_TEST_F(IsolatedOriginTest, ProcessLimit) {
+  // Set the process limit to 1.
+  RenderProcessHost::SetMaxRendererProcessCount(1);
+
+  // Navigate to an unisolated foo.com URL with an iframe.
+  GURL foo_url(
+      embedded_test_server()->GetURL("www.foo.com", "/page_with_iframe.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), foo_url));
+  FrameTreeNode* root = web_contents()->GetFrameTree()->root();
+  RenderProcessHost* foo_process = root->current_frame_host()->GetProcess();
+  FrameTreeNode* child = root->child_at(0);
+
+  // Navigate iframe to an isolated origin.
+  GURL isolated_foo_url(
+      embedded_test_server()->GetURL("isolated.foo.com", "/title2.html"));
+  NavigateIframeToURL(web_contents(), "test_iframe", isolated_foo_url);

[alexmos, 2017/06/12 17:35:07]

Interesting note: without this CL, this actually hangs.  This is because we
first attempted the transfer but then, because of (incorrectly) trying to reuse
the existing www.foo.com process, we hit the is_transfer_to_same path:
   bool is_transfer_to_same =
        is_transfer &&
        entry.transferred_global_request_id().child_id ==
            dest_render_frame_host->GetProcess()->GetID();

because we compare process IDs and not SiteInstances (the SiteInstances are
different but process ID is the same due to the reuse).  This causes us to not
send the Navigate IPC.  I wonder if this can come up in other cases as well, and
whether we should avoid comparing process IDs in the is_transfer_to_same check
due to this.

[Charlie Reis, 2017/06/12 23:08:17]

Wow, nice find-- that could definitely come up in other cases.  It's perhaps not
as common as it could be since most sites don't trigger a cross-process redirect
and those that do (e.g., CWS) tend to require a different process.  But I could
see it happening for a redirect from one extension to another when we're over
the limit and the extensions end up sharing a process.  Can you file a bug?

[alexmos, 2017/06/14 22:39:04]

Done - filed https://crbug.com/733023.

+
+  // Ensure that the subframe was rendered in a new process.
+  EXPECT_NE(child->current_frame_host()->GetProcess(), foo_process);
+
+  // Sanity-check IsSuitableHost values for the current processes.
+  BrowserContext* browser_context = web_contents()->GetBrowserContext();
+  auto is_suitable_host = [browser_context](RenderProcessHost* process,
+                                            GURL url) {
+    return RenderProcessHostImpl::IsSuitableHost(
+        process, browser_context,
+        SiteInstance::GetSiteForURL(browser_context, url));
+  };
+  EXPECT_TRUE(is_suitable_host(foo_process, foo_url));
+  EXPECT_FALSE(is_suitable_host(foo_process, isolated_foo_url));
+  EXPECT_TRUE(is_suitable_host(child->current_frame_host()->GetProcess(),
+                               isolated_foo_url));
+  EXPECT_FALSE(
+      is_suitable_host(child->current_frame_host()->GetProcess(), foo_url));
+
+  // Open a new, unrelated tab and navigate it to isolated.foo.com.  This
+  // should use a new, unrelated SiteInstance that reuses the existing isolated
+  // origin process from first tab's subframe.
+  Shell* new_shell = CreateBrowser();
+  EXPECT_TRUE(NavigateToURL(new_shell, isolated_foo_url));
+  scoped_refptr<SiteInstance> isolated_foo_instance(
+      new_shell->web_contents()->GetMainFrame()->GetSiteInstance());
+  RenderProcessHost* isolated_foo_process = isolated_foo_instance->GetProcess();
+  EXPECT_NE(child->current_frame_host()->GetSiteInstance(),
+            isolated_foo_instance);

[Charlie Reis, 2017/06/12 23:08:17]

Can you also check that isolated_foo_instance and the previous subframe
SiteInstance are not related, as the comment mentions?

[alexmos, 2017/06/14 22:39:04]

Done.

+  // TODO(alexmos): with --site-per-process, this won't currently reuse the
+  // subframe process, because the new SiteInstance will initialize its
+  // process while it still has no site (during CreateBrowser()), and since
+  // dedicated processes can't currently be reused for a SiteInstance with no
+  // site, this creates a new process.  The subsequent navigation to
+  // |isolated_foo_url| stays in that new process without consulting whether it
+  // can now reuse a different process.  This should be fixed; see
+  // https://crbug.com/513036.   Without --site-per-process, this works because
+  // the site-less SiteInstance is allowed to reuse the first tab's foo.com
+  // process (which isn't dedicated), and then it swaps to the isolated.foo.com
+  // process during navigation.
+  if (!AreAllSitesIsolatedForTesting())
+    EXPECT_EQ(child->current_frame_host()->GetProcess(), isolated_foo_process);

[alexmos, 2017/06/12 17:35:07]

I decided to deal with this for a followup to keep this CL simpler.  The
sequence of events is:
- start with all processes hosting only dedicated sites
- create new tab with site-less SiteInstance
- call GetProcess() on that SiteInstance.  This creates a new process since none
of the dedicated processes can host it.
- navigate new tab to a site that has an existing dedicated process.  Even
though process reuse is possible, we hit the checks that say that a site-less
SiteInstance that sets a site should stay in the same process.

It doesn't matter much in practice, as opening a new tab has the NTP and never
hits this path.  Potential fixes could be to checking for process reuse when a
site-less SI sets a site, or to allow dedicated processes to host uninitialized
SiteInstances (they should transfer out if necessary when they navigate).

[Charlie Reis, 2017/06/12 23:08:17]

For potential fix #1, what would we do at the time of setting the site?  That's
at commit time, right?  (That would be too late to transfer to an existing
process.)

For potential fix #2, I'm not sure I understand-- would we randomly put a blank
SiteInstance into a dedicated process and hope that's the site we end up on? 
That seems unlikely to succeed in most cases, but maybe I'm missing the idea.

I could see setting up a transfer once we know we're going to commit a URL that
needs a dedicated (existing) process.  Or rather, letting PlzNavigate switch to
an RFH in that process, since there's not much point in fixing it in the
non-PlzNavigate case.  Either way, I agree there's probably no rush.

[alexmos, 2017/06/14 22:39:04]

Yeah, agreed that commit-time SetSite is too late (without PlzNavigate).
Yes, the idea was to allow a blank SiteInstance into a dedicated process and
then transfer it out if it turns out to be for a different site.  This does seem
somewhat fragile though.
I agree this will be easier with PlzNavigate, so maybe we can just wait until
PlzNavigate ships before tackling this, since there's no rush.

+
+  // Navigate iframe on the first tab to a non-isolated site.  This should swap
+  // processes so that it does not reuse the isolated origin's process.
+  NavigateIframeToURL(
+      web_contents(), "test_iframe",
+      embedded_test_server()->GetURL("www.foo.com", "/title1.html"));
+  EXPECT_EQ(foo_process, child->current_frame_host()->GetProcess());
+  EXPECT_NE(isolated_foo_process, child->current_frame_host()->GetProcess());
+
+  // Navigate iframe back to isolated origin.  See that it reuses the
+  // |new_shell| process.
+  NavigateIframeToURL(web_contents(), "test_iframe", isolated_foo_url);
+  EXPECT_NE(foo_process, child->current_frame_host()->GetProcess());
+  EXPECT_EQ(isolated_foo_process, child->current_frame_host()->GetProcess());
+
+  // Navigate iframe to a different isolated origin.  Ensure that this creates
+  // a third process.
+  GURL isolated_bar_url(
+      embedded_test_server()->GetURL("isolated.bar.com", "/title3.html"));
+  NavigateIframeToURL(web_contents(), "test_iframe", isolated_bar_url);
+  RenderProcessHost* isolated_bar_process =
+      child->current_frame_host()->GetProcess();
+  EXPECT_NE(foo_process, isolated_bar_process);
+  EXPECT_NE(isolated_foo_process, isolated_bar_process);
+
+  // The new process should only be suitable to host isolated.bar.com, not
+  // regular web URLs or other isolated origins.
+  EXPECT_TRUE(is_suitable_host(isolated_bar_process, isolated_bar_url));
+  EXPECT_FALSE(is_suitable_host(isolated_bar_process, foo_url));
+  EXPECT_FALSE(is_suitable_host(isolated_bar_process, isolated_foo_url));
+
+  // Navigate second tab (currently at isolated.foo.com) to the
+  // second isolated origin, and see that it switches processes.
+  EXPECT_TRUE(NavigateToURL(new_shell, isolated_bar_url));
+  EXPECT_NE(foo_process,
+            new_shell->web_contents()->GetMainFrame()->GetProcess());
+  EXPECT_NE(isolated_foo_process,
+            new_shell->web_contents()->GetMainFrame()->GetProcess());
+  EXPECT_EQ(isolated_bar_process,
+            new_shell->web_contents()->GetMainFrame()->GetProcess());
+
+  // Navigate second tab to a non-isolated URL and see that it goes back into
+  // the www.foo.com process, and that it does not share processes with any
+  // isolated origins.
+  EXPECT_TRUE(NavigateToURL(new_shell, foo_url));
+  EXPECT_EQ(foo_process,
+            new_shell->web_contents()->GetMainFrame()->GetProcess());
+  EXPECT_NE(isolated_foo_process,
+            new_shell->web_contents()->GetMainFrame()->GetProcess());
+  EXPECT_NE(isolated_bar_process,
+            new_shell->web_contents()->GetMainFrame()->GetProcess());
+}
+
 }  // namespace content