Lock hosted apps to their underlying web origin.

Previously, hosted apps were exempt from LockToOrigin() even in
--site-per-process mode.  That meant that hosted apps were not subject
to enforcements such as not allowing access to cookies, passwords, or
local storage of other sites.  Worse, it meant that hosted apps could
arbitrarily share a process (e.g., when over process limit), even if
they covered different web sites with --site-per-process.

This CL starts locking hosted apps to their underlying web origin.  If
a frame commits a navigation to URL http://foo.com, which is part of a
hosted app X's web extent, the process for that frame will be locked
to http://foo.com.  Note that the SiteInstance for this frame will
still use a site URL based on the effective URL (i.e.,
chrome-extension://<ext_id_for_X>/), but the origin lock will not be
based on effective URLs.  This requires plumbing to compute the origin
lock as a site URL that does not use an effective URL, and to plumb it
into various places that make process model decisions, such as
RPHI::IsSuitableHost().

Bug: 811939, 794315, 791796
Change-Id: Icc9b3c0a04253e581ea35953f3c566308305db59
Reviewed-on: https://chromium-review.googlesource.com/959346
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583895}

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

@@ -428,28 +428,29 @@ bool ChromeContentBrowserClientExtensionsPart::DoesSiteRequireDedicatedProcess(
 bool ChromeContentBrowserClientExtensionsPart::ShouldLockToOrigin(
     content::BrowserContext* browser_context,
     const GURL& effective_site_url) {
-  // https://crbug.com/160576 workaround: Origin lock to the chrome-extension://
-  // scheme for a hosted app would kill processes on legitimate requests for the
-  // app's cookies.
-  if (effective_site_url.SchemeIs(extensions::kExtensionScheme)) {
-    const Extension* extension =
-        ExtensionRegistry::Get(browser_context)
+  if (!effective_site_url.SchemeIs(kExtensionScheme))
+    return true;
+
+  const Extension* extension = ExtensionRegistry::Get(browser_context)
                                    ->enabled_extensions()
                                    .GetExtensionOrAppByURL(effective_site_url);
-    if (extension && extension->is_hosted_app())
-      return false;
+  if (!extension)
+    return true;
+
+  // Hosted apps should be locked to their web origin. See
+  // https://crbug.com/794315.
+  if (extension->is_hosted_app())
+    return true;
 
-    // Extensions are allowed to share processes, even in --site-per-process
-    // currently. See https://crbug.com/600441#c1 for some background on the
-    // intersection of extension process reuse and site isolation.
+  // Other extensions are allowed to share processes, even in
+  // --site-per-process currently. See https://crbug.com/600441#c1 for some
+  // background on the intersection of extension process reuse and site
+  // isolation.
   //
   // TODO(nick): Fix this, possibly by revamping the extensions process model
   // so that sharing is determined by privilege level, as described in
-    // https://crbug.com/766267
-    if (extension)
+  // https://crbug.com/766267.
   return false;
-  }
-  return true;
 }
 
 // static

chrome/browser/ui/extensions/hosted_app_browsertest.cc

@@ -43,6 +43,7 @@
 #include "chrome/common/web_application_info.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/security_interstitials/core/controller_client.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/interstitial_page_delegate.h"
 #include "content/public/browser/notification_service.h"
@@ -2007,6 +2008,278 @@ IN_PROC_BROWSER_TEST_P(HostedAppIsolatedOriginTest,
       web_contents->GetMainFrame()->GetProcess()->GetID()));
 }
 
+class HostedAppSitePerProcessTest : public HostedAppProcessModelTest {
+ public:
+  HostedAppSitePerProcessTest() {}
+  ~HostedAppSitePerProcessTest() override {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ASSERT_TRUE(embedded_test_server()->InitializeAndListen());
+    content::IsolateAllSitesForTesting(command_line);
+  }
+};
+
+// Check that two different cross-site hosted apps won't share a process even
+// when over process limit, when in --site-per-process mode.  See
+// https://crbug.com/811939.
+IN_PROC_BROWSER_TEST_P(HostedAppSitePerProcessTest,
+                       DoNotShareProcessWhenOverProcessLimit) {
+  // Set the process limit to 1.
+  content::RenderProcessHost::SetMaxRendererProcessCount(1);
+
+  // Set up and launch a hosted app covering foo.com.
+  GURL foo_app_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
+  constexpr const char kHostedAppManifest[] =
+      R"( { "name": "Hosted App With SitePerProcess Test",
+            "version": "1",
+            "manifest_version": 2,
+            "app": {
+              "launch": {
+                "web_url": "%s"
+              },
+              "urls": ["http://%s/"]
+            }
+          } )";
+  {
+    extensions::TestExtensionDir test_app_dir;
+    test_app_dir.WriteManifest(base::StringPrintf(
+        kHostedAppManifest, foo_app_url.spec().c_str(), "foo.com"));
+    SetupApp(test_app_dir.UnpackedPath());
+  }
+  content::WebContents* foo_contents =
+      app_browser_->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(content::WaitForLoadStop(foo_contents));
+
+  // Set up and launch a hosted app covering bar.com.
+  GURL bar_app_url(embedded_test_server()->GetURL("bar.com", "/title1.html"));
+  {
+    extensions::TestExtensionDir test_app_dir;
+    test_app_dir.WriteManifest(base::StringPrintf(
+        kHostedAppManifest, bar_app_url.spec().c_str(), "bar.com"));
+    SetupApp(test_app_dir.UnpackedPath());
+  }
+  content::WebContents* bar_contents =
+      app_browser_->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(content::WaitForLoadStop(bar_contents));
+
+  EXPECT_NE(foo_contents, bar_contents);
+  EXPECT_NE(foo_contents->GetMainFrame()->GetSiteInstance(),
+            bar_contents->GetMainFrame()->GetSiteInstance());
+  EXPECT_EQ(foo_app_url, foo_contents->GetLastCommittedURL());
+  EXPECT_EQ(bar_app_url, bar_contents->GetLastCommittedURL());
+
+  // Under --site-per-process the two apps should load in separate processes,
+  // even when over process limit.
+  EXPECT_NE(foo_contents->GetMainFrame()->GetProcess(),
+            bar_contents->GetMainFrame()->GetProcess());
+}
+
+// Check that when a hosted app covers multiple sites in its web extent, these
+// sites do not share a process in site-per-process mode. See
+// https://crbug.com/791796.
+IN_PROC_BROWSER_TEST_P(HostedAppSitePerProcessTest,
+                       DoNotShareProcessForDifferentSitesCoveredBySameApp) {
+  // Set up a hosted app covering http://foo.com and http://bar.com, and launch
+  // the app with a foo.com URL in a new window.
+  GURL foo_app_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
+  constexpr const char kHostedAppManifest[] =
+      R"( { "name": "Hosted App With SitePerProcess Test",
+            "version": "1",
+            "manifest_version": 2,
+            "app": {
+              "launch": {
+                "web_url": "%s"
+              },
+              "urls": ["http://%s/", "http://%s/"]
+            }
+          } )";
+  {
+    extensions::TestExtensionDir test_app_dir;
+    test_app_dir.WriteManifest(base::StringPrintf(
+        kHostedAppManifest, foo_app_url.spec().c_str(), "foo.com", "bar.com"));
+    SetupApp(test_app_dir.UnpackedPath());
+  }
+  content::WebContents* foo_contents =
+      app_browser_->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(content::WaitForLoadStop(foo_contents));
+  EXPECT_EQ(foo_app_url, foo_contents->GetLastCommittedURL());
+
+  // Now navigate original window to a bar.com app URL.
+  GURL bar_app_url(embedded_test_server()->GetURL("bar.com", "/title2.html"));
+  ui_test_utils::NavigateToURL(browser(), bar_app_url);
+  content::WebContents* bar_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(bar_app_url, bar_contents->GetLastCommittedURL());
+  EXPECT_NE(foo_contents, bar_contents);
+
+  // Ensure the two pages don't share a process despite being from the same
+  // app, since they are from different sites.
+  EXPECT_NE(foo_contents->GetMainFrame()->GetSiteInstance(),
+            bar_contents->GetMainFrame()->GetSiteInstance());
+  auto* foo_process =
+      foo_contents->GetMainFrame()->GetSiteInstance()->GetProcess();
+  auto* bar_process =
+      bar_contents->GetMainFrame()->GetSiteInstance()->GetProcess();
+  EXPECT_NE(foo_process, bar_process);
+
+  // Ensure each process only has access to its site's data.
+  auto* policy = content::ChildProcessSecurityPolicy::GetInstance();
+  EXPECT_TRUE(
+      policy->CanAccessDataForOrigin(foo_process->GetID(), foo_app_url));
+  EXPECT_FALSE(
+      policy->CanAccessDataForOrigin(foo_process->GetID(), bar_app_url));
+  EXPECT_FALSE(
+      policy->CanAccessDataForOrigin(bar_process->GetID(), foo_app_url));
+  EXPECT_TRUE(
+      policy->CanAccessDataForOrigin(bar_process->GetID(), bar_app_url));
+
+  // Both processes should still be app processes.
+  auto* process_map = extensions::ProcessMap::Get(browser()->profile());
+  EXPECT_TRUE(process_map->Contains(foo_process->GetID()));
+  EXPECT_TRUE(process_map->Contains(bar_process->GetID()));
+}
+
+// Check background page scriptability for a hosted app that covers multiple
+// sites in its web extent.  When a hosted app page opens a background page,
+// only same-site parts of the app should be able to script that background
+// page. This behavior should be the same with and without --site-per-process.
+IN_PROC_BROWSER_TEST_P(HostedAppProcessModelTest,
+                       BackgroundPageWithAppCoveringDifferentSites) {
+  // Set up a hosted app covering http://foo.com and http://bar.com.
+  GURL foo_app_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
+  constexpr const char kHostedAppManifest[] =
+      R"( { "name": "Hosted App With SitePerProcess Test",
+            "version": "1",
+            "manifest_version": 2,
+            "app": {
+              "launch": {
+                "web_url": "%s"
+              },
+              "urls": ["http://foo.com/", "http://bar.com/"]
+            },
+            "permissions": ["background"]
+          } )";
+  {
+    extensions::TestExtensionDir test_app_dir;
+    test_app_dir.WriteManifest(
+        base::StringPrintf(kHostedAppManifest, foo_app_url.spec().c_str()));
+    SetupApp(test_app_dir.UnpackedPath());
+  }
+
+  // Set up three unrelated hosted app tabs in the main browser window:
+  // foo.com, bar.com, and another one at foo.com.
+  ui_test_utils::NavigateToURL(browser(), foo_app_url);
+  content::WebContents* foo_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(foo_app_url, foo_contents->GetLastCommittedURL());
+
+  GURL bar_app_url(embedded_test_server()->GetURL("bar.com", "/title2.html"));
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), bar_app_url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  content::WebContents* bar_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(bar_app_url, bar_contents->GetLastCommittedURL());
+  EXPECT_NE(foo_contents, bar_contents);
+
+  GURL foo_app_url2(embedded_test_server()->GetURL("foo.com", "/title3.html"));
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), foo_app_url2, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  content::WebContents* foo_contents2 =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(foo_app_url2, foo_contents2->GetLastCommittedURL());
+  EXPECT_NE(foo_contents, foo_contents2);
+  EXPECT_NE(bar_contents, foo_contents2);
+  ASSERT_EQ(3, browser()->tab_strip_model()->count());
+
+  // The two foo.com tabs should be in the same process even though they are
+  // unrelated, since hosted apps use the process-per-site process model.
+  auto* foo_process = foo_contents->GetMainFrame()->GetProcess();
+  EXPECT_EQ(foo_process, foo_contents2->GetMainFrame()->GetProcess());
+  EXPECT_FALSE(
+      foo_contents->GetMainFrame()->GetSiteInstance()->IsRelatedSiteInstance(
+          foo_contents2->GetMainFrame()->GetSiteInstance()));
+
+  // The bar.com tab should be in the same process as the foo.com tabs only if
+  // we are not in --site-per-process mode.  --site-per-process should override
+  // the process-per-site behavior.
+  auto* bar_process = bar_contents->GetMainFrame()->GetProcess();
+  EXPECT_EQ(foo_process == bar_process,
+            !content::AreAllSitesIsolatedForTesting());
+
+  // Ensure all tabs are in app processes.
+  auto* process_map = extensions::ProcessMap::Get(browser()->profile());
+  EXPECT_TRUE(process_map->Contains(foo_process->GetID()));
+  EXPECT_TRUE(process_map->Contains(bar_process->GetID()));
+
+  // Open a background page from the first foo.com window.
+  {
+    content::TestNavigationObserver background_page_observer(nullptr);
+    background_page_observer.StartWatchingNewWebContents();
+    EXPECT_TRUE(content::ExecuteScript(
+        foo_contents,
+        "window.bg = window.open('/empty.html', 'bg', 'background');"));
+    background_page_observer.Wait();
+    EXPECT_EQ(embedded_test_server()->GetURL("foo.com", "/empty.html"),
+              background_page_observer.last_navigation_url());
+
+    // The background page shouldn't show up in the tab strip.
+    ASSERT_EQ(3, browser()->tab_strip_model()->count());
+  }
+
+  // Script the background page from the first foo.com window and set a dummy
+  // value.
+  EXPECT_TRUE(content::ExecuteScript(foo_contents,
+                                     "bg.document.body.innerText = 'foo'"));
+
+  // Ensure that the second foo.com page can script the same background page
+  // and retrieve the value.
+  EXPECT_EQ("foo",
+            content::EvalJs(foo_contents2,
+                            "window.open('', 'bg').document.body.innerText"));
+
+  // Ensure that the bar.com page cannot script this background page, since it
+  // is cross-origin from it. The window lookup via window.open('', bg') should
+  // be disallowed, resulting in a new popup instead, and the innerText value
+  // from that should be empty.
+  EXPECT_EQ("",
+            content::EvalJs(bar_contents,
+                            "window.open('', 'bg').document.body.innerText"));
+
+  // Open another bar.com app URL in an unrelated tab.  This should share a
+  // process with the first bar.com tab, due to hosted apps using
+  // process-per-site.
+  GURL bar_app_url2(embedded_test_server()->GetURL("bar.com", "/title3.html"));
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), bar_app_url2, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  content::WebContents* bar_contents2 =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(bar_app_url2, bar_contents2->GetLastCommittedURL());
+  EXPECT_EQ(bar_process, bar_contents2->GetMainFrame()->GetProcess());
+  EXPECT_FALSE(
+      bar_contents->GetMainFrame()->GetSiteInstance()->IsRelatedSiteInstance(
+          bar_contents2->GetMainFrame()->GetSiteInstance()));
+
+  // Ensure bar.com tabs can open and script their open background page.
+  {
+    content::TestNavigationObserver background_page_observer(nullptr);
+    background_page_observer.StartWatchingNewWebContents();
+    EXPECT_TRUE(content::ExecuteScript(
+        bar_contents,
+        "window.bg = window.open('/empty.html', 'bg2', 'background');"));
+    background_page_observer.Wait();
+    EXPECT_EQ(embedded_test_server()->GetURL("bar.com", "/empty.html"),
+              background_page_observer.last_navigation_url());
+  }
+  EXPECT_TRUE(content::ExecuteScript(bar_contents,
+                                     "bg.document.body.innerText = 'bar'"));
+  EXPECT_EQ("bar",
+            content::EvalJs(bar_contents2,
+                            "window.open('', 'bg2').document.body.innerText"));
+}
+
 INSTANTIATE_TEST_CASE_P(/* no prefix */,
                         HostedAppTest,
                         ::testing::Combine(kAppTypeValues, ::testing::Bool()));
@@ -2024,3 +2297,9 @@ INSTANTIATE_TEST_CASE_P(
     HostedAppIsolatedOriginTest,
     ::testing::Combine(::testing::Values(AppType::HOSTED_APP),
                        ::testing::Bool()));
+
+INSTANTIATE_TEST_CASE_P(
+    /* no prefix */,
+    HostedAppSitePerProcessTest,
+    ::testing::Combine(::testing::Values(AppType::HOSTED_APP),
+                       ::testing::Bool()));

content/browser/browsing_instance.cc

@@ -21,8 +21,7 @@ BrowsingInstance::BrowsingInstance(BrowserContext* browser_context)
 }
 
 bool BrowsingInstance::HasSiteInstance(const GURL& url) {
-  std::string site =
-      SiteInstanceImpl::GetSiteForURL(browser_context_, url)
+  std::string site = SiteInstance::GetSiteForURL(browser_context_, url)
                          .possibly_invalid_spec();
 
   return site_instance_map_.find(site) != site_instance_map_.end();
@@ -30,7 +29,7 @@ bool BrowsingInstance::HasSiteInstance(const GURL& url) {
 
 scoped_refptr<SiteInstanceImpl> BrowsingInstance::GetSiteInstanceForURL(
     const GURL& url) {
-  std::string site = SiteInstanceImpl::GetSiteForURL(browser_context_, url)
+  std::string site = SiteInstance::GetSiteForURL(browser_context_, url)
                          .possibly_invalid_spec();
 
   SiteInstanceMap::iterator i = site_instance_map_.find(site);

content/browser/child_process_security_policy_impl.cc

@@ -1120,7 +1120,7 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(int child_id,
   // TODO(creis): We must pass the valid browser_context to convert hosted apps
   // URLs. Currently, hosted apps cannot set cookies in this mode. See
   // http://crbug.com/160576.
-  GURL site_url = SiteInstanceImpl::GetSiteForURL(nullptr, url);
+  GURL site_url = SiteInstance::GetSiteForURL(nullptr, url);
 
   base::AutoLock lock(lock_);
   SecurityStateMap::iterator state = security_state_.find(child_id);
@@ -1153,7 +1153,7 @@ void ChildProcessSecurityPolicyImpl::LockToOrigin(int child_id,
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // "gurl" can be currently empty in some cases, such as file://blah.
-  DCHECK(SiteInstanceImpl::GetSiteForURL(nullptr, gurl) == gurl);
+  DCHECK_EQ(SiteInstanceImpl::DetermineProcessLockURL(nullptr, gurl), gurl);
   base::AutoLock lock(lock_);
   SecurityStateMap::iterator state = security_state_.find(child_id);
   DCHECK(state != security_state_.end());

content/browser/frame_host/navigator_impl_unittest.cc

@@ -133,7 +133,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_TRUE(main_test_rfh()->navigation_request());
   main_test_rfh()->SendNavigate(entry_id, true, kUrl);
   EXPECT_TRUE(main_test_rfh()->is_active());
-  EXPECT_EQ(SiteInstanceImpl::GetSiteForURL(browser_context(), kUrl),
+  EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrl),
             main_test_rfh()->GetSiteInstance()->GetSiteURL());
   EXPECT_EQ(kUrl, contents()->GetLastCommittedURL());
 
@@ -187,7 +187,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   // Commit the navigation.
   navigation->Commit();
   EXPECT_TRUE(main_test_rfh()->is_active());
-  EXPECT_EQ(SiteInstanceImpl::GetSiteForURL(browser_context(), kUrl2),
+  EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrl2),
             main_test_rfh()->GetSiteInstance()->GetSiteURL());
   EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
@@ -878,7 +878,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   // Receive the beforeUnload ACK.
   main_test_rfh()->SendBeforeUnloadACK(true);
   EXPECT_EQ(speculative_rfh, GetSpeculativeRenderFrameHost(node));
-  EXPECT_EQ(SiteInstanceImpl::GetSiteForURL(browser_context(), kUrl),
+  EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrl),
             speculative_rfh->GetSiteInstance()->GetSiteURL());
   int32_t site_instance_id = speculative_rfh->GetSiteInstance()->GetId();
   int64_t navigation_id =
@@ -923,7 +923,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_NE(init_site_instance_id, site_instance_id);
   EXPECT_EQ(init_site_instance_id, main_test_rfh()->GetSiteInstance()->GetId());
   EXPECT_NE(speculative_rfh, main_test_rfh());
-  EXPECT_EQ(SiteInstanceImpl::GetSiteForURL(browser_context(), kUrl),
+  EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrl),
             speculative_rfh->GetSiteInstance()->GetSiteURL());
 
   // Receive the beforeUnload ACK.
@@ -960,7 +960,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
 
   // Once commit happens the speculative RenderFrameHost is updated to match the
   // known final SiteInstance.
-  EXPECT_EQ(SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlRedirect),
+  EXPECT_EQ(SiteInstance::GetSiteForURL(browser_context(), kUrlRedirect),
             speculative_rfh->GetSiteInstance()->GetSiteURL());
   int32_t redirect_site_instance_id =
       speculative_rfh->GetSiteInstance()->GetId();

content/browser/frame_host/render_frame_host_manager.cc

@@ -1218,7 +1218,8 @@ RenderFrameHostManager::DetermineSiteInstanceForURL(
     // thus use the correct process.
     bool use_process_per_site =
         RenderProcessHost::ShouldUseProcessPerSite(browser_context, dest_url) &&
-        RenderProcessHostImpl::GetProcessHostForSite(browser_context, dest_url);
+        RenderProcessHostImpl::GetSoleProcessHostForURL(browser_context,
+                                                        dest_url);
     if (current_instance_impl->HasRelatedSiteInstance(dest_url) ||
         use_process_per_site) {
       return SiteInstanceDescriptor(browser_context, dest_url,

content/browser/isolated_origin_browsertest.cc

@@ -568,10 +568,12 @@ IN_PROC_BROWSER_TEST_F(IsolatedOriginTest, ProcessLimit) {
   // 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));
+                                            const GURL& url) {
+    GURL site_url(SiteInstance::GetSiteForURL(browser_context, url));
+    GURL lock_url(
+        SiteInstanceImpl::DetermineProcessLockURL(browser_context, url));
+    return RenderProcessHostImpl::IsSuitableHost(process, browser_context,
+                                                 site_url, lock_url);
   };
   EXPECT_TRUE(is_suitable_host(foo_process, foo_url));
   EXPECT_FALSE(is_suitable_host(foo_process, isolated_foo_url));

content/browser/renderer_host/render_process_host_impl.cc

@@ -908,11 +908,11 @@ class SiteProcessCountTracker : public base::SupportsUserData::Data,
       map_.erase(site_url);
   }
 
-  void FindRenderProcessesForSite(
-      const GURL& site_url,
+  void FindRenderProcessesForSiteInstance(
+      SiteInstanceImpl* site_instance,
       std::set<RenderProcessHost*>* foreground_processes,
       std::set<RenderProcessHost*>* background_processes) {
-    auto result = map_.find(site_url);
+    auto result = map_.find(site_instance->GetSiteURL());
     if (result == map_.end())
       return;
 
@@ -934,8 +934,10 @@ class SiteProcessCountTracker : public base::SupportsUserData::Data,
       // allow such hosts to be reused.  See https://crbug.com/780661.
       if (!host->MayReuseHost() ||
           !RenderProcessHostImpl::IsSuitableHost(
-              host, host->GetBrowserContext(), site_url))
+              host, host->GetBrowserContext(), site_instance->GetSiteURL(),
+              site_instance->lock_url())) {
         continue;
+      }
 
       if (host->VisibleClientCount())
         foreground_processes->insert(host);
@@ -1030,13 +1032,13 @@ class UnmatchedServiceWorkerProcessTracker
       public RenderProcessHostObserver {
  public:
   // Registers |render_process_host| as having an unmatched service worker for
-  // |site_url|.
-  static void Register(BrowserContext* browser_context,
-                       RenderProcessHost* render_process_host,
-                       const GURL& site_url) {
-    DCHECK(!site_url.is_empty());
+  // |site_instance|.
+  static void Register(RenderProcessHost* render_process_host,
+                       SiteInstanceImpl* site_instance) {
+    BrowserContext* browser_context = site_instance->GetBrowserContext();
+    DCHECK(!site_instance->GetSiteURL().is_empty());
     if (!ShouldTrackProcessForSite(browser_context, render_process_host,
-                                   site_url))
+                                   site_instance->GetSiteURL()))
       return;
 
     UnmatchedServiceWorkerProcessTracker* tracker =
@@ -1048,14 +1050,15 @@ class UnmatchedServiceWorkerProcessTracker
       browser_context->SetUserData(kUnmatchedServiceWorkerProcessTrackerKey,
                                    base::WrapUnique(tracker));
     }
-    tracker->RegisterProcessForSite(render_process_host, site_url);
+    tracker->RegisterProcessForSite(render_process_host, site_instance);
   }
 
-  // Find a process with an unmatched service worker for |site_url| and removes
-  // the process from the tracker if it exists.
-  static RenderProcessHost* MatchWithSite(BrowserContext* browser_context,
-                                          const GURL& site_url) {
-    if (!ShouldFindReusableProcessHostForSite(browser_context, site_url))
+  // Find a process with an unmatched service worker for |site_instance| and
+  // removes the process from the tracker if it exists.
+  static RenderProcessHost* MatchWithSite(SiteInstanceImpl* site_instance) {
+    BrowserContext* browser_context = site_instance->GetBrowserContext();
+    if (!ShouldFindReusableProcessHostForSite(browser_context,
+                                              site_instance->GetSiteURL()))
       return nullptr;
 
     UnmatchedServiceWorkerProcessTracker* tracker =
@@ -1064,7 +1067,7 @@ class UnmatchedServiceWorkerProcessTracker
                 kUnmatchedServiceWorkerProcessTrackerKey));
     if (!tracker)
       return nullptr;
-    return tracker->TakeFreshestProcessForSite(site_url);
+    return tracker->TakeFreshestProcessForSite(site_instance);
   }
 
   UnmatchedServiceWorkerProcessTracker() {}
@@ -1088,14 +1091,17 @@ class UnmatchedServiceWorkerProcessTracker
   }
 
  private:
-  void RegisterProcessForSite(RenderProcessHost* host, const GURL& site_url) {
+  void RegisterProcessForSite(RenderProcessHost* host,
+                              SiteInstanceImpl* site_instance) {
     if (!HasProcess(host))
       host->AddObserver(this);
-    site_process_set_.insert(SiteProcessIDPair(site_url, host->GetID()));
+    site_process_set_.insert(
+        SiteProcessIDPair(site_instance->GetSiteURL(), host->GetID()));
   }
 
-  RenderProcessHost* TakeFreshestProcessForSite(const GURL& site_url) {
-    RenderProcessHost* host = FindFreshestProcessForSite(site_url);
+  RenderProcessHost* TakeFreshestProcessForSite(
+      SiteInstanceImpl* site_instance) {
+    RenderProcessHost* host = FindFreshestProcessForSite(site_instance);
     if (!host)
       return nullptr;
 
@@ -1103,8 +1109,10 @@ class UnmatchedServiceWorkerProcessTracker
     // |site_url|, for example if it was used for a ServiceWorker for a
     // nonexistent extension URL.  See https://crbug.com/782349 and
     // https://crbug.com/780661.
+    GURL site_url(site_instance->GetSiteURL());
     if (!host->MayReuseHost() || !RenderProcessHostImpl::IsSuitableHost(
-                                     host, host->GetBrowserContext(), site_url))
+                                     host, host->GetBrowserContext(), site_url,
+                                     site_instance->lock_url()))
       return nullptr;
 
     site_process_set_.erase(SiteProcessIDPair(site_url, host->GetID()));
@@ -1113,7 +1121,9 @@ class UnmatchedServiceWorkerProcessTracker
     return host;
   }
 
-  RenderProcessHost* FindFreshestProcessForSite(const GURL& site_url) const {
+  RenderProcessHost* FindFreshestProcessForSite(
+      SiteInstanceImpl* site_instance) const {
+    GURL site_url(site_instance->GetSiteURL());
     for (const auto& site_process_pair : base::Reversed(site_process_set_)) {
       if (site_process_pair.first == site_url)
         return RenderProcessHost::FromID(site_process_pair.second);
@@ -1155,6 +1165,28 @@ void GetNetworkChangeManager(
   GetNetworkService()->GetNetworkChangeManager(std::move(request));
 }
 
+std::string DetermineKeyForSiteProcessMap(BrowserContext* browser_context,
+                                          const GURL& site_url,
+                                          const GURL& lock_url) {
+  // TODO(alexmos): See if |site_url| or |lock_url| can actually be invalid.
+  std::string key = site_url.possibly_invalid_spec();
+
+  // When process-per-site is combined with effective URLs, which is the case
+  // for hosted apps, we can't use the site URL (which is an effective URL) as
+  // an index to |map_|. This is because processes are locked to web sites
+  // that are resolved without effective URLs, and when multiple sites map to
+  // the same effective URL (e.g., when a hosted app spans multiple sites),
+  // one process cannot be used for all of them when in
+  // --site-per-process mode. Instead, augment the site URL with the process
+  // lock, so that all same-site parts of the app are placed in one process
+  // and maintain process-per-site behavior. See https://crbug.com/791796.
+  if (SiteInstanceImpl::ShouldLockToOrigin(browser_context, site_url) &&
+      site_url != lock_url) {
+    key += '|' + lock_url.possibly_invalid_spec();
+  }
+  return key;
+}
+
 }  // namespace
 
 // Held by the RPH and used to control an (unowned) ConnectionFilterImpl from
@@ -3498,7 +3530,8 @@ void RenderProcessHostImpl::FilterURL(RenderProcessHost* rph,
 // static
 bool RenderProcessHostImpl::IsSuitableHost(RenderProcessHost* host,
                                            BrowserContext* browser_context,
-                                           const GURL& site_url) {
+                                           const GURL& site_url,
+                                           const GURL& lock_url) {
   if (run_renderer_in_process()) {
     DCHECK_EQ(host->GetBrowserContext(), browser_context)
         << " Single-process mode does not support multiple browser contexts.";
@@ -3523,10 +3556,11 @@ bool RenderProcessHostImpl::IsSuitableHost(RenderProcessHost* host,
   if (!host->InSameStoragePartition(dest_partition))
     return false;
 
-  // Check WebUI bindings and origin locks.
+  // Check WebUI bindings and origin locks.  Note that |lock_url| may differ
+  // from |site_url| if an effective URL is used.
   auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
   bool host_has_web_ui_bindings = policy->HasWebUIBindings(host->GetID());
-  auto lock_state = policy->CheckOriginLock(host->GetID(), site_url);
+  auto lock_state = policy->CheckOriginLock(host->GetID(), lock_url);
   if (host->HostHasNotBeenUsed()) {
     // If the host hasn't been used, it won't have the expected WebUI bindings
     // or origin locks just *yet* - skip the checks in this case.  One example
@@ -3639,8 +3673,7 @@ bool RenderProcessHost::ShouldTryToUseExistingProcessHost(
 
 // static
 RenderProcessHost* RenderProcessHostImpl::GetExistingProcessHost(
-    BrowserContext* browser_context,
-    const GURL& site_url) {
+    SiteInstanceImpl* site_instance) {
   // First figure out which existing renderers we can use.
   std::vector<RenderProcessHost*> suitable_renderers;
   suitable_renderers.reserve(g_all_hosts.Get().size());
@@ -3648,8 +3681,9 @@ RenderProcessHost* RenderProcessHostImpl::GetExistingProcessHost(
   iterator iter(AllHostsIterator());
   while (!iter.IsAtEnd()) {
     if (iter.GetCurrentValue()->MayReuseHost() &&
-        RenderProcessHostImpl::IsSuitableHost(iter.GetCurrentValue(),
-                                              browser_context, site_url)) {
+        RenderProcessHostImpl::IsSuitableHost(
+            iter.GetCurrentValue(), site_instance->GetBrowserContext(),
+            site_instance->GetSiteURL(), site_instance->lock_url())) {
       // The spare is always considered before process reuse.
       DCHECK_NE(iter.GetCurrentValue(),
                 g_spare_render_process_host_manager.Get()
@@ -3703,19 +3737,32 @@ bool RenderProcessHost::ShouldUseProcessPerSite(BrowserContext* browser_context,
 }
 
 // static
-RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSite(
+RenderProcessHost* RenderProcessHostImpl::GetSoleProcessHostForURL(
     BrowserContext* browser_context,
     const GURL& url) {
+  GURL site_url = SiteInstance::GetSiteForURL(browser_context, url);
+  GURL lock_url =
+      SiteInstanceImpl::DetermineProcessLockURL(browser_context, url);
+  return GetSoleProcessHostForSite(browser_context, site_url, lock_url);
+}
+
+// static
+RenderProcessHost* RenderProcessHostImpl::GetSoleProcessHostForSite(
+    BrowserContext* browser_context,
+    const GURL& site_url,
+    const GURL& lock_url) {
   // Look up the map of site to process for the given browser_context.
   SiteProcessMap* map = GetSiteProcessMapForBrowserContext(browser_context);
 
+  std::string key =
+      DetermineKeyForSiteProcessMap(browser_context, site_url, lock_url);
+
   // See if we have an existing process with appropriate bindings for this site.
   // If not, the caller should create a new process and register it.  Note that
   // IsSuitableHost expects a site URL rather than the full |url|.
-  GURL site_url = SiteInstance::GetSiteForURL(browser_context, url);
-  RenderProcessHost* host = map->FindProcess(site_url.possibly_invalid_spec());
+  RenderProcessHost* host = map->FindProcess(key);
   if (host && (!host->MayReuseHost() ||
-               !IsSuitableHost(host, browser_context, site_url))) {
+               !IsSuitableHost(host, browser_context, site_url, lock_url))) {
     // The registered process does not have an appropriate set of bindings for
     // the url.  Remove it from the map so we can register a better one.
     RecordAction(
@@ -3727,25 +3774,27 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSite(
   return host;
 }
 
-void RenderProcessHostImpl::RegisterProcessHostForSite(
+void RenderProcessHostImpl::RegisterSoleProcessHostForSite(
     BrowserContext* browser_context,
     RenderProcessHost* process,
-    const GURL& url) {
+    SiteInstanceImpl* site_instance) {
   // Look up the map of site to process for the given browser_context.
   SiteProcessMap* map = GetSiteProcessMapForBrowserContext(browser_context);
 
+  GURL site_url = site_instance->GetSiteURL();
   // Only register valid, non-empty sites.  Empty or invalid sites will not
   // use process-per-site mode.  We cannot check whether the process has
   // appropriate bindings here, because the bindings have not yet been granted.
-  std::string site =
-      SiteInstance::GetSiteForURL(browser_context, url).possibly_invalid_spec();
-  if (!site.empty())
-    map->RegisterProcess(site, process);
+  if (site_url.possibly_invalid_spec().empty())
+    return;
+
+  std::string key = DetermineKeyForSiteProcessMap(browser_context, site_url,
+                                                  site_instance->lock_url());
+  map->RegisterProcess(key, process);
 }
 
 // static
 RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
-    BrowserContext* browser_context,
     SiteInstanceImpl* site_instance) {
   const GURL site_url = site_instance->GetSiteURL();
   SiteInstanceImpl::ProcessReusePolicy process_reuse_policy =
@@ -3754,11 +3803,13 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
   RenderProcessHost* render_process_host = nullptr;
 
   bool is_unmatched_service_worker = site_instance->is_for_service_worker();
+  BrowserContext* browser_context = site_instance->GetBrowserContext();
 
   // First, attempt to reuse an existing RenderProcessHost if necessary.
   switch (process_reuse_policy) {
     case SiteInstanceImpl::ProcessReusePolicy::PROCESS_PER_SITE:
-      render_process_host = GetProcessHostForSite(browser_context, site_url);
+      render_process_host = GetSoleProcessHostForSite(
+          browser_context, site_url, site_instance->lock_url());
       break;
     case SiteInstanceImpl::ProcessReusePolicy::USE_DEFAULT_SUBFRAME_PROCESS:
       DCHECK(SiteIsolationPolicy::IsTopDocumentIsolationEnabled());
@@ -3768,7 +3819,7 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
       break;
     case SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE:
       render_process_host =
-          FindReusableProcessHostForSite(browser_context, site_url);
+          FindReusableProcessHostForSiteInstance(site_instance);
       UMA_HISTOGRAM_BOOLEAN(
           "SiteIsolation.ReusePendingOrCommittedSite.CouldReuse",
           render_process_host != nullptr);
@@ -3786,8 +3837,8 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
   if (!render_process_host &&
       !(process_reuse_policy == SiteInstanceImpl::ProcessReusePolicy::DEFAULT &&
         site_instance->is_for_service_worker())) {
-    render_process_host = UnmatchedServiceWorkerProcessTracker::MatchWithSite(
-        browser_context, site_url);
+    render_process_host =
+        UnmatchedServiceWorkerProcessTracker::MatchWithSite(site_instance);
   }
 
   // See if the spare RenderProcessHost can be used.
@@ -3803,16 +3854,16 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
   // If not (or if none found), see if we should reuse an existing process.
   if (!render_process_host &&
       ShouldTryToUseExistingProcessHost(browser_context, site_url)) {
-    render_process_host = GetExistingProcessHost(browser_context, site_url);
+    render_process_host = GetExistingProcessHost(site_instance);
   }
 
   // If we found a process to reuse, sanity check that it is suitable for
   // hosting |site_url|. For example, if |site_url| requires a dedicated
   // process, we should never pick a process used by, or locked to, a different
   // site.
-  if (render_process_host &&
-      !RenderProcessHostImpl::IsSuitableHost(render_process_host,
-                                             browser_context, site_url)) {
+  if (render_process_host && !RenderProcessHostImpl::IsSuitableHost(
+                                 render_process_host, browser_context, site_url,
+                                 site_instance->lock_url())) {
     ChildProcessSecurityPolicyImpl* policy =
         ChildProcessSecurityPolicyImpl::GetInstance();
     base::debug::SetCrashKeyString(bad_message::GetRequestedSiteURLKey(),
@@ -3846,8 +3897,8 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
     spare_process_manager.PrepareForFutureRequests(browser_context);
 
   if (is_unmatched_service_worker) {
-    UnmatchedServiceWorkerProcessTracker::Register(
-        browser_context, render_process_host, site_url);
+    UnmatchedServiceWorkerProcessTracker::Register(render_process_host,
+                                                   site_instance);
   }
 
   // Make sure the chosen process is in the correct StoragePartition for the
@@ -4334,9 +4385,11 @@ RenderProcessHost* RenderProcessHostImpl::GetDefaultSubframeProcessHost(
 }
 
 // static
-RenderProcessHost* RenderProcessHostImpl::FindReusableProcessHostForSite(
-    BrowserContext* browser_context,
-    const GURL& site_url) {
+RenderProcessHost*
+RenderProcessHostImpl::FindReusableProcessHostForSiteInstance(
+    SiteInstanceImpl* site_instance) {
+  BrowserContext* browser_context = site_instance->GetBrowserContext();
+  GURL site_url(site_instance->GetSiteURL());
   if (!ShouldFindReusableProcessHostForSite(browser_context, site_url))
     return nullptr;
 
@@ -4349,8 +4402,8 @@ RenderProcessHost* RenderProcessHostImpl::FindReusableProcessHostForSite(
       static_cast<SiteProcessCountTracker*>(
           browser_context->GetUserData(kPendingSiteProcessCountTrackerKey));
   if (pending_tracker) {
-    pending_tracker->FindRenderProcessesForSite(
-        site_url, &eligible_foreground_hosts, &eligible_background_hosts);
+    pending_tracker->FindRenderProcessesForSiteInstance(
+        site_instance, &eligible_foreground_hosts, &eligible_background_hosts);
   }
 
   if (eligible_foreground_hosts.empty()) {
@@ -4360,8 +4413,9 @@ RenderProcessHost* RenderProcessHostImpl::FindReusableProcessHostForSite(
         static_cast<SiteProcessCountTracker*>(
             browser_context->GetUserData(kCommittedSiteProcessCountTrackerKey));
     if (committed_tracker) {
-      committed_tracker->FindRenderProcessesForSite(
-          site_url, &eligible_foreground_hosts, &eligible_background_hosts);
+      committed_tracker->FindRenderProcessesForSiteInstance(
+          site_instance, &eligible_foreground_hosts,
+          &eligible_background_hosts);
     }
   }
 

content/browser/renderer_host/render_process_host_impl.h

@@ -266,11 +266,16 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // Implementation of FilterURL below that can be shared with the mock class.
   static void FilterURL(RenderProcessHost* rph, bool empty_allowed, GURL* url);
 
-  // Returns true if |host| is suitable for launching a new view with |site_url|
-  // in the given |browser_context|.
+  // Returns true if |host| is suitable for rendering a page in the given
+  // |browser_context|, where the page would utilize |site_url| as its
+  // SiteInstance site URL, and its process would be locked to |lock_url|.
+  // |site_url| and |lock_url| may differ in cases where an effective URL is
+  // not the actual site that the process is locked to, which happens for
+  // hosted apps.
   static bool IsSuitableHost(RenderProcessHost* host,
                              BrowserContext* browser_context,
-                             const GURL& site_url);
+                             const GURL& site_url,
+                             const GURL& lock_url);
 
   // Returns an existing RenderProcessHost for |url| in |browser_context|,
   // if one exists.  Otherwise a new RenderProcessHost should be created and
@@ -278,25 +283,45 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // This should only be used for process-per-site mode, which can be enabled
   // globally with a command line flag or per-site, as determined by
   // SiteInstanceImpl::ShouldUseProcessPerSite.
-  static RenderProcessHost* GetProcessHostForSite(
+  // Important: |url| should be a full URL and *not* a site URL.
+  static RenderProcessHost* GetSoleProcessHostForURL(
       BrowserContext* browser_context,
       const GURL& url);
 
-  // Registers the given |process| to be used for any instance of |url|
-  // within |browser_context|.
+  // Variant of the above that takes in a SiteInstance site URL and the
+  // process's origin lock URL, when they are known.
+  static RenderProcessHost* GetSoleProcessHostForSite(
+      BrowserContext* browser_context,
+      const GURL& site_url,
+      const GURL& lock_url);
+
+  // Registers the given |process| to be used for all sites identified by
+  // |site_instance| within |browser_context|.
   // This should only be used for process-per-site mode, which can be enabled
   // globally with a command line flag or per-site, as determined by
   // SiteInstanceImpl::ShouldUseProcessPerSite.
-  static void RegisterProcessHostForSite(
-      BrowserContext* browser_context,
+  static void RegisterSoleProcessHostForSite(BrowserContext* browser_context,
                                              RenderProcessHost* process,
-      const GURL& url);
+                                             SiteInstanceImpl* site_instance);
 
   // Returns a suitable RenderProcessHost to use for |site_instance|. Depending
   // on the SiteInstance's ProcessReusePolicy and its url, this may be an
   // existing RenderProcessHost or a new one.
+  //
+  // This is the main entrypoint into the process assignment logic, which
+  // handles all cases.  These cases include:
+  // - process-per-site: see
+  //   RegisterSoleProcessHostForSite/GetSoleProcessHostForSite.
+  // - TDI: see GetDefaultSubframeProcessHost.
+  // - REUSE_PENDING_OR_COMMITTED reuse policy (for ServiceWorkers and OOPIFs):
+  //   see FindReusableProcessHostForSiteInstance.
+  // - normal process reuse when over process limit:  see
+  //   GetExistingProcessHost.
+  // - using the spare RenderProcessHost when possible: see
+  //   MaybeTakeSpareRenderProcessHost.
+  // - process creation when an existing process couldn't be found: see
+  //   CreateRenderProcessHost.
   static RenderProcessHost* GetProcessHostForSiteInstance(
-      BrowserContext* browser_context,
       SiteInstanceImpl* site_instance);
 
   // Should be called when |browser_context| is used in a navigation.
@@ -552,20 +577,19 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // Get an existing RenderProcessHost associated with the given browser
   // context, if possible.  The renderer process is chosen randomly from
   // suitable renderers that share the same context and type (determined by the
-  // site url).
+  // site url of |site_instance|).
   // Returns nullptr if no suitable renderer process is available, in which case
   // the caller is free to create a new renderer.
   static RenderProcessHost* GetExistingProcessHost(
-      content::BrowserContext* browser_context,
-      const GURL& site_url);
+      SiteInstanceImpl* site_instance);
   FRIEND_TEST_ALL_PREFIXES(RenderProcessHostUnitTest,
                            GuestsAreNotSuitableHosts);
 
-  // Returns a RenderProcessHost that is rendering |site_url| in one of its
-  // frames, or that is expecting a navigation to |site_url|.
-  static RenderProcessHost* FindReusableProcessHostForSite(
-      BrowserContext* browser_context,
-      const GURL& site_url);
+  // Returns a RenderProcessHost that is rendering a URL corresponding to
+  // |site_instance| in one of its frames, or that is expecting a navigation to
+  // that SiteInstance.
+  static RenderProcessHost* FindReusableProcessHostForSiteInstance(
+      SiteInstanceImpl* site_instance);
 
   void CreateMediaStreamDispatcherHost(
       MediaStreamManager* media_stream_manager,

content/browser/renderer_host/render_process_host_unittest.cc

@@ -42,12 +42,16 @@ TEST_F(RenderProcessHostUnitTest, GuestsAreNotSuitableHosts) {
   MockRenderProcessHost guest_host(browser_context());
   guest_host.set_is_for_guests_only(true);
 
+  scoped_refptr<SiteInstanceImpl> site_instance =
+      SiteInstanceImpl::CreateForURL(browser_context(), test_url);
   EXPECT_FALSE(RenderProcessHostImpl::IsSuitableHost(
-      &guest_host, browser_context(), test_url));
+      &guest_host, browser_context(), site_instance->GetSiteURL(),
+      site_instance->lock_url()));
   EXPECT_TRUE(RenderProcessHostImpl::IsSuitableHost(
-      process(), browser_context(), test_url));
-  EXPECT_EQ(process(), RenderProcessHostImpl::GetExistingProcessHost(
-                           browser_context(), test_url));
+      process(), browser_context(), site_instance->GetSiteURL(),
+      site_instance->lock_url()));
+  EXPECT_EQ(process(),
+            RenderProcessHostImpl::GetExistingProcessHost(site_instance.get()));
 }
 
 #if !defined(OS_ANDROID) && !defined(OS_CHROMEOS)

content/browser/site_instance_impl.cc

@@ -104,7 +104,8 @@ bool SiteInstanceImpl::HasProcess() const {
       browsing_instance_->browser_context();
   if (has_site_ &&
       RenderProcessHost::ShouldUseProcessPerSite(browser_context, site_) &&
-      RenderProcessHostImpl::GetProcessHostForSite(browser_context, site_)) {
+      RenderProcessHostImpl::GetSoleProcessHostForSite(browser_context, site_,
+                                                       lock_url_)) {
     return true;
   }
 
@@ -133,8 +134,7 @@ RenderProcessHost* SiteInstanceImpl::GetProcess() {
       process_reuse_policy_ = ProcessReusePolicy::DEFAULT;
     }
 
-    process_ = RenderProcessHostImpl::GetProcessHostForSiteInstance(
-        browser_context, this);
+    process_ = RenderProcessHostImpl::GetProcessHostForSiteInstance(this);
 
     CHECK(process_);
     process_->AddObserver(this);
@@ -144,8 +144,8 @@ RenderProcessHost* SiteInstanceImpl::GetProcess() {
     // at this time, we will register it in SetSite().)
     if (process_reuse_policy_ == ProcessReusePolicy::PROCESS_PER_SITE &&
         has_site_) {
-      RenderProcessHostImpl::RegisterProcessHostForSite(browser_context,
-                                                        process_, site_);
+      RenderProcessHostImpl::RegisterSoleProcessHostForSite(browser_context,
+                                                            process_, this);
     }
 
     TRACE_EVENT2("navigation", "SiteInstanceImpl::GetProcess",
@@ -182,8 +182,10 @@ void SiteInstanceImpl::SetSite(const GURL& url) {
   // URL is invalid.
   has_site_ = true;
   BrowserContext* browser_context = browsing_instance_->browser_context();
-  site_ = GetSiteForURL(browser_context, url);
+  site_ =
+      GetSiteForURL(browser_context, url, true /* should_use_effective_urls */);
   original_url_ = url;
+  lock_url_ = DetermineProcessLockURL(browser_context, url);
 
   // Now that we have a site, register it with the BrowsingInstance.  This
   // ensures that we won't create another SiteInstance for this site within
@@ -203,8 +205,8 @@ void SiteInstanceImpl::SetSite(const GURL& url) {
 
     // Ensure the process is registered for this site if necessary.
     if (should_use_process_per_site) {
-      RenderProcessHostImpl::RegisterProcessHostForSite(
-          browser_context, process_, site_);
+      RenderProcessHostImpl::RegisterSoleProcessHostForSite(browser_context,
+                                                            process_, this);
     }
   }
 }
@@ -262,9 +264,13 @@ bool SiteInstanceImpl::HasWrongProcessForURL(const GURL& url) {
 
   // If the site URL is an extension (e.g., for hosted apps or WebUI) but the
   // process is not (or vice versa), make sure we notice and fix it.
-  GURL site_url = GetSiteForURL(browsing_instance_->browser_context(), url);
+  GURL site_url =
+      SiteInstance::GetSiteForURL(browsing_instance_->browser_context(), url);
+  GURL origin_lock =
+      DetermineProcessLockURL(browsing_instance_->browser_context(), url);
   return !RenderProcessHostImpl::IsSuitableHost(
-      GetProcess(), browsing_instance_->browser_context(), site_url);
+      GetProcess(), browsing_instance_->browser_context(), site_url,
+      origin_lock);
 }
 
 scoped_refptr<SiteInstanceImpl>
@@ -415,12 +421,32 @@ bool SiteInstanceImpl::IsSameWebSite(BrowserContext* browser_context,
 
 // static
 GURL SiteInstance::GetSiteForURL(BrowserContext* browser_context,
-                                 const GURL& real_url) {
+                                 const GURL& url) {
+  // By default, GetSiteForURL will resolve |real_url| to an effective URL
+  // before computing its site.
+  return SiteInstanceImpl::GetSiteForURL(browser_context, url,
+                                         true /* should_use_effective_urls */);
+}
+
+// static
+GURL SiteInstanceImpl::DetermineProcessLockURL(BrowserContext* browser_context,
+                                               const GURL& url) {
+  // For the process lock URL, convert |url| to a site without resolving |url|
+  // to an effective URL.
+  return SiteInstanceImpl::GetSiteForURL(browser_context, url,
+                                         false /* should_use_effective_urls */);
+}
+
+GURL SiteInstanceImpl::GetSiteForURL(BrowserContext* browser_context,
+                                     const GURL& real_url,
+                                     bool should_use_effective_urls) {
   // TODO(fsamuel, creis): For some reason appID is not recognized as a host.
   if (real_url.SchemeIs(kGuestScheme))
     return real_url;
 
-  GURL url = SiteInstanceImpl::GetEffectiveURL(browser_context, real_url);
+  GURL url = should_use_effective_urls
+                 ? SiteInstanceImpl::GetEffectiveURL(browser_context, real_url)
+                 : real_url;
   url::Origin origin = url::Origin::Create(url);
 
   // Isolated origins should use the full origin as their site URL. A subdomain
@@ -522,7 +548,7 @@ bool SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
     return true;
 
   // Always require a dedicated process for isolated origins.
-  GURL site_url = GetSiteForURL(browser_context, url);
+  GURL site_url = SiteInstance::GetSiteForURL(browser_context, url);
   auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
   if (policy->IsIsolatedOrigin(url::Origin::Create(site_url)))
     return true;
@@ -605,7 +631,7 @@ void SiteInstanceImpl::LockToOriginIfNeeded() {
 
   ChildProcessSecurityPolicyImpl* policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
-  auto lock_state = policy->CheckOriginLock(process_->GetID(), site_);
+  auto lock_state = policy->CheckOriginLock(process_->GetID(), lock_url());
   if (ShouldLockToOrigin(GetBrowserContext(), site_)) {
     // Sanity check that this won't try to assign an origin lock to a <webview>
     // process, which can't be locked.
@@ -617,7 +643,7 @@ void SiteInstanceImpl::LockToOriginIfNeeded() {
         // strong protection. If only some sites are isolated, we need
         // additional logic to prevent the non-isolated sites from requesting
         // resources for isolated sites. https://crbug.com/509125
-        process_->LockToOrigin(site_);
+        process_->LockToOrigin(lock_url());
         break;
       }
       case CheckOriginLockResult::HAS_WRONG_LOCK:
@@ -628,7 +654,7 @@ void SiteInstanceImpl::LockToOriginIfNeeded() {
         base::debug::SetCrashKeyString(
             bad_message::GetKilledProcessOriginLockKey(),
             policy->GetOriginLock(process_->GetID()).spec());
-        CHECK(false) << "Trying to lock a process to " << site_
+        CHECK(false) << "Trying to lock a process to " << lock_url()
                      << " but the process is already locked to "
                      << policy->GetOriginLock(process_->GetID());
         break;

content/browser/site_instance_impl.h

@@ -116,11 +116,29 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   // May be empty if this SiteInstance does not have a |site_|.
   const GURL& original_url() { return original_url_; }
 
+  // Returns the URL which should be used in a LockToOrigin call for this
+  // SiteInstance's process.
+  const GURL& lock_url() { return lock_url_; }
+
   // True if |url| resolves to an effective URL that is different from |url|.
   // See GetEffectiveURL().  This will be true for hosted apps as well as NTP
   // URLs.
   static bool HasEffectiveURL(BrowserContext* browser_context, const GURL& url);
 
+  // Returns the site for the given URL, which includes only the scheme and
+  // registered domain.  Returns an empty GURL if the URL has no host.
+  // |use_effective_urls| specifies whether to resolve |url| to an effective
+  // URL (via ContentBrowserClient::GetEffectiveURL()) before determining the
+  // site.
+  static GURL GetSiteForURL(BrowserContext* context,
+                            const GURL& url,
+                            bool use_effective_urls);
+
+  // Returns the URL to which a process should be locked for the given URL.
+  // This is computed similarly to the site URL (see GetSiteForURL), but
+  // without resolving effective URLs.
+  static GURL DetermineProcessLockURL(BrowserContext* context, const GURL& url);
+
   // Returns the SiteInstance, related to this one, that should be used
   // for subframes when an oopif is required, but a dedicated process is not.
   // This SiteInstance will be created if it doesn't already exist. There is
@@ -275,6 +293,12 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   // The URL which was used to set the |site_| for this SiteInstance.
   GURL original_url_;
 
+  // The URL to use when locking a process to this SiteInstance's site via
+  // LockToOrigin().  This is the same as |site_| except for cases involving
+  // effective URLs, such as hosted apps.  In those cases, this URL is a site
+  // URL that is computed without the use of effective URLs.
+  GURL lock_url_;
+
   // The ProcessReusePolicy to use when creating a RenderProcessHost for this
   // SiteInstance.
   ProcessReusePolicy process_reuse_policy_;

content/browser/site_instance_impl_unittest.cc

@@ -258,51 +258,51 @@ TEST_F(SiteInstanceTest, SetSite) {
 TEST_F(SiteInstanceTest, GetSiteForURL) {
   // Pages are irrelevant.
   GURL test_url = GURL("http://www.google.com/index.html");
-  GURL site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  GURL site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("http://google.com"), site_url);
   EXPECT_EQ("http", site_url.scheme());
   EXPECT_EQ("google.com", site_url.host());
 
   // Ports are irrelevant.
   test_url = GURL("https://www.google.com:8080");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("https://google.com"), site_url);
 
   // Punycode is canonicalized.
   test_url = GURL("http://☃snowperson☃.net:333/");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("http://xn--snowperson-di0gka.net"), site_url);
 
   // Username and password are stripped out.
   test_url = GURL("ftp://username:password@ftp.chromium.org/files/README");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("ftp://chromium.org"), site_url);
 
   // Literal IP addresses of any flavor are okay.
   test_url = GURL("http://127.0.0.1/a.html");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("http://127.0.0.1"), site_url);
   EXPECT_EQ("127.0.0.1", site_url.host());
 
   test_url = GURL("http://2130706433/a.html");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("http://127.0.0.1"), site_url);
   EXPECT_EQ("127.0.0.1", site_url.host());
 
   test_url = GURL("http://[::1]:2/page.html");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("http://[::1]"), site_url);
   EXPECT_EQ("[::1]", site_url.host());
 
   // Hostnames without TLDs are okay.
   test_url = GURL("http://foo/a.html");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("http://foo"), site_url);
   EXPECT_EQ("foo", site_url.host());
 
   // File URLs should include the scheme.
   test_url = GURL("file:///C:/Downloads/");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("file:"), site_url);
   EXPECT_EQ("file", site_url.scheme());
   EXPECT_FALSE(site_url.has_host());
@@ -311,7 +311,7 @@ TEST_F(SiteInstanceTest, GetSiteForURL) {
   // maps *all* file://... URLs into "file://" origin) such file URLs still need
   // to map into "file:" site URL.  See also https://crbug.com/776160.
   test_url = GURL("file://server/path");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("file:"), site_url);
   EXPECT_EQ("file", site_url.scheme());
   EXPECT_FALSE(site_url.has_host());
@@ -319,7 +319,7 @@ TEST_F(SiteInstanceTest, GetSiteForURL) {
   // Data URLs should include the whole URL, except for the hash, when Site
   // Isolation is enabled.  Otherwise they just include the scheme.
   test_url = GURL("data:text/html,foo");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   if (AreAllSitesIsolatedForTesting())
     EXPECT_EQ(test_url, site_url);
   else
@@ -327,7 +327,7 @@ TEST_F(SiteInstanceTest, GetSiteForURL) {
   EXPECT_EQ("data", site_url.scheme());
   EXPECT_FALSE(site_url.has_host());
   test_url = GURL("data:text/html,foo#bar");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_FALSE(site_url.has_ref());
   if (AreAllSitesIsolatedForTesting()) {
     EXPECT_NE(test_url, site_url);
@@ -338,7 +338,7 @@ TEST_F(SiteInstanceTest, GetSiteForURL) {
 
   // Javascript URLs should include the scheme.
   test_url = GURL("javascript:foo();");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("javascript:"), site_url);
   EXPECT_EQ("javascript", site_url.scheme());
   EXPECT_FALSE(site_url.has_host());
@@ -347,12 +347,12 @@ TEST_F(SiteInstanceTest, GetSiteForURL) {
   test_url = GURL(
       "blob:gopher://www.ftp.chromium.org/"
       "4d4ff040-6d61-4446-86d3-13ca07ec9ab9");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("gopher://chromium.org"), site_url);
 
   // Blob URLs with file origin also extract the site from the origin.
   test_url = GURL("blob:file:///1029e5a4-2983-4b90-a585-ed217563acfeb");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("file:"), site_url);
   EXPECT_EQ("file", site_url.scheme());
   EXPECT_FALSE(site_url.has_host());
@@ -361,13 +361,13 @@ TEST_F(SiteInstanceTest, GetSiteForURL) {
   // when Site Isolation is enabled, except for the hash.  Otherwise they just
   // include the scheme.
   test_url = GURL("blob:null/1029e5a4-2983-4b90-a585-ed217563acfeb");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   if (AreAllSitesIsolatedForTesting())
     EXPECT_EQ(test_url, site_url);
   else
     EXPECT_EQ(GURL("blob:"), site_url);
   test_url = GURL("blob:null/1029e5a4-2983-4b90-a585-ed217563acfeb#foo");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_FALSE(site_url.has_ref());
   if (AreAllSitesIsolatedForTesting()) {
     EXPECT_NE(test_url, site_url);
@@ -380,12 +380,12 @@ TEST_F(SiteInstanceTest, GetSiteForURL) {
   test_url = GURL(
       "blob:http://www.example.appspot.com:44/"
       "4d4ff040-6d61-4446-86d3-13ca07ec9ab9");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("http://example.appspot.com"), site_url);
 
   // The site of filesystem URLs is determined by the inner URL.
   test_url = GURL("filesystem:http://www.google.com/foo/bar.html?foo#bar");
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(GURL("http://google.com"), site_url);
 
   // Guest URLs are special and need to have the path in the site as well,
@@ -393,12 +393,68 @@ TEST_F(SiteInstanceTest, GetSiteForURL) {
   std::string guest_url(kGuestScheme);
   guest_url.append("://abc123/path");
   test_url = GURL(guest_url);
-  site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
+  site_url = SiteInstance::GetSiteForURL(nullptr, test_url);
   EXPECT_EQ(test_url, site_url);
 
   DrainMessageLoop();
 }
 
+// Test that process lock URLs are computed without using effective URLs.
+TEST_F(SiteInstanceTest, ProcessLockDoesNotUseEffectiveURL) {
+  GURL test_url("https://some.app.foo.com/");
+  GURL nonapp_site_url("https://foo.com/");
+  GURL app_url("https://app.com/");
+  EffectiveURLContentBrowserClient modified_client(test_url, app_url);
+  ContentBrowserClient* regular_client =
+      SetBrowserClientForTesting(&modified_client);
+  std::unique_ptr<TestBrowserContext> browser_context(new TestBrowserContext());
+
+  // Sanity check that GetSiteForURL's |use_effective_urls| option works
+  // properly.
+  {
+    GURL site_url = SiteInstanceImpl::GetSiteForURL(
+        nullptr, test_url, false /* use_effective_urls */);
+    EXPECT_EQ(nonapp_site_url, site_url);
+
+    site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url,
+                                               true /* use_effective_urls */);
+    EXPECT_EQ(app_url, site_url);
+  }
+
+  // New SiteInstance in a new BrowsingInstance with a predetermined URL.
+  {
+    scoped_refptr<SiteInstanceImpl> site_instance =
+        SiteInstanceImpl::CreateForURL(browser_context.get(), test_url);
+    EXPECT_EQ(app_url, site_instance->GetSiteURL());
+    EXPECT_EQ(nonapp_site_url, site_instance->lock_url());
+  }
+
+  // New related SiteInstance from an existing SiteInstance with a
+  // predetermined URL.
+  {
+    scoped_refptr<SiteInstanceImpl> bar_site_instance =
+        SiteInstanceImpl::CreateForURL(browser_context.get(),
+                                       GURL("https://bar.com/"));
+    scoped_refptr<SiteInstance> site_instance =
+        bar_site_instance->GetRelatedSiteInstance(test_url);
+    EXPECT_EQ(app_url, site_instance->GetSiteURL());
+    EXPECT_EQ(nonapp_site_url,
+              static_cast<SiteInstanceImpl*>(site_instance.get())->lock_url());
+  }
+
+  // New SiteInstance with a lazily assigned site URL.
+  {
+    scoped_refptr<SiteInstanceImpl> site_instance =
+        SiteInstanceImpl::Create(browser_context.get());
+    EXPECT_FALSE(site_instance->HasSite());
+    site_instance->SetSite(test_url);
+    EXPECT_EQ(app_url, site_instance->GetSiteURL());
+    EXPECT_EQ(nonapp_site_url, site_instance->lock_url());
+  }
+
+  SetBrowserClientForTesting(regular_client);
+}
+
 // Test of distinguishing URLs from different sites.  Most of this logic is
 // tested in RegistryControlledDomainTest.  This test focuses on URLs with
 // different schemes or ports.
@@ -837,8 +893,10 @@ TEST_F(SiteInstanceTest, NoProcessPerSiteForEmptySite) {
   EXPECT_TRUE(instance->GetSiteURL().is_empty());
   host.reset(instance->GetProcess());
 
-  EXPECT_FALSE(RenderProcessHostImpl::GetProcessHostForSite(
+  EXPECT_FALSE(RenderProcessHostImpl::GetSoleProcessHostForURL(
       browser_context.get(), GURL()));
+  EXPECT_FALSE(RenderProcessHostImpl::GetSoleProcessHostForSite(
+      browser_context.get(), GURL(), GURL()));
 
   DrainMessageLoop();
 }

content/public/browser/site_instance.h

@@ -170,7 +170,9 @@ class CONTENT_EXPORT SiteInstance : public base::RefCounted<SiteInstance> {
                             const GURL& dest_url);
 
   // Returns the site for the given URL, which includes only the scheme and
-  // registered domain.  Returns an empty GURL if the URL has no host.
+  // registered domain.  Returns an empty GURL if the URL has no host. Prior to
+  // determining the site, |url| is resolved to an effective URL via
+  // ContentBrowserClient::GetEffectiveURL().
   static GURL GetSiteForURL(BrowserContext* context, const GURL& url);
 
  protected: