Introduce support for origins that require process isolation.

content/browser/site_instance_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/site_instance_impl.h"
 
+#include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "content/browser/browsing_instance.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/frame_host/debug_urls.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host_factory.h"
@@ skipping 381 matching lines @@
   // If either URL is invalid, they aren't part of the same site.
   if (!src_url.is_valid() || !dest_url.is_valid())
     return false;
 
   // If the destination url is just a blank page, we treat them as part of the
   // same site.
   GURL blank_page(url::kAboutBlankURL);
   if (dest_url == blank_page)
     return true;
 
+  // If either URL has an isolated origin, compare origins rather than sites.
+  url::Origin src_origin(src_url);

[Charlie Reis, 2017/05/05 23:18:51]

Fun.  src_url is an effective URL, so it may be a hosted app's
chrome-extension:// equivalent.  That means isolating an origin which is within
a hosted app has no effect, which is probably not what we want.

Then again, is it safe for isolated origins to always take precedence over
GetEffectiveURL (which is also used for NTP)?  I think so?

Also worth checking if anyone converts to effective URL *before* passing into
this method, just in case.

[alexmos, 2017/05/16 17:26:37]

Thanks for pointing this out.  I agree that isolated origins should take
precedence over hosted apps.  I fixed the order so that isolated origins are
checked before GetEffectiveURL, both here and in GetSiteForURL.  Also added a
section about this to the design doc and a test to hosted_app_browsertest.cc.

Does seem like anyone converts to effective URLs before calling this.   The only
other call to GetEffectiveURL besides IsSameWebSite, GetSiteForURL, and tests,
is in GetSiteInstanceForNavigation, where it converts two URLs fed into
ShouldSwapBrowsingInstancesForNavigation.  But that doesn't call into here or to
GetSiteForURL.

This should play well with NTP as long as we don't try to specify something like
chrome-search://foo (or whatever else would conflict with NTP) as an isolated
origin.

An alternate fix would be to make SiteInstanceImpl::GetEffectiveURL always
return |url| whenever it corresponds to an isolated origin (before forwarding to
ChromeContentBrowserClient).  That might be a more robust fix, as it's
centralized and might be less vulnerable to incorrect ordering in future
refactors.  WDYT?

[Charlie Reis, 2017/05/19 00:10:18]

I like that idea-- seems like it will help us be more consistent overall.

+  url::Origin dest_origin(dest_url);
+  if (SiteInstanceImpl::IsIsolatedOrigin(src_origin) ||
+      SiteInstanceImpl::IsIsolatedOrigin(dest_origin))
+    return src_origin == dest_origin;
+
   // If the schemes differ, they aren't part of the same site.
+  //
+  // Note that this happens after the isolated origin check, since blob or
+  // filesystem URLs will fail this check even though they might have the
+  // same origin.
   if (src_url.scheme() != dest_url.scheme())
     return false;
 
   return net::registry_controlled_domains::SameDomainOrHost(
       src_url,
       dest_url,
       net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
 }
 
 // static
 GURL SiteInstance::GetSiteForURL(BrowserContext* browser_context,
                                  const GURL& real_url) {
   // 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);
+
+  // Isolated origins should use the full origin as their site URL.
   url::Origin origin(url);
+  if (SiteInstanceImpl::IsIsolatedOrigin(origin))
+    return origin.GetURL();
 
   // If the url has a host, then determine the site.
   if (!origin.host().empty()) {
     // Only keep the scheme and registered domain of |origin|.
     std::string domain = net::registry_controlled_domains::GetDomainAndRegistry(
         origin.host(),
         net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
     std::string site = origin.scheme();
     site += url::kStandardSchemeSeparator;
     site += domain.empty() ? origin.host() : domain;
@@ skipping 18 matching lines @@
 }
 
 // static
 bool SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
     BrowserContext* browser_context,
     const GURL& url) {
   // If --site-per-process is enabled, site isolation is enabled everywhere.
   if (SiteIsolationPolicy::UseDedicatedProcessesForAllSites())
     return true;
 
+  // For now, always require a dedicated process for isolated origins.
+  // TODO(alexmos): revisit this for Isolate-Me.
+  GURL site_url = GetSiteForURL(browser_context, url);
+  if (IsIsolatedOrigin(url::Origin(site_url)))
+    return true;
+
   // Let the content embedder enable site isolation for specific URLs. Use the
   // canonical site url for this check, so that schemes with nested origins
   // (blob and filesystem) work properly.
-  GURL site_url = GetSiteForURL(browser_context, url);
   if (GetContentClient()->IsSupplementarySiteIsolationModeEnabled() &&
       GetContentClient()->browser()->DoesSiteRequireDedicatedProcess(
           browser_context, site_url)) {
     return true;
   }
 
   return false;
 }
 
+// static
+void SiteInstanceImpl::AddIsolatedOrigin(const url::Origin& origin) {

[Charlie Reis, 2017/05/05 23:18:51]

Might be worth putting a UI thread check in each of the static methods?

[alexmos, 2017/05/16 17:26:37]

That was a really good idea, and it made me realize that this isn't really a
valid assumption. :/  ChildProcessSecurityPolicy needs this data for cookie
access checks on the IO thread via CanAccessDataForOrigin -> GetSiteForURL.

Per out offline discussions, I moved this to ChildProcessSecurityPolicyImpl.

+  DCHECK(!origin.unique());
+  DCHECK(!IsIsolatedOrigin(origin));
+
+  GetIsolatedOrigins()->insert(origin);
+}
+
+void SiteInstanceImpl::AddIsolatedOriginsFromCommandLine(
+    const std::string& origin_list) {
+  for (const base::StringPiece& origin_piece :
+       base::SplitStringPiece(origin_list, ",", base::TRIM_WHITESPACE,
+                              base::SPLIT_WANT_NONEMPTY)) {
+    url::Origin origin((GURL(origin_piece)));
+    if (!origin.unique())
+      SiteInstanceImpl::AddIsolatedOrigin(origin);
+  }
+}
+
+// static
+bool SiteInstanceImpl::IsIsolatedOrigin(const url::Origin& origin) {
+  return GetIsolatedOrigins()->find(origin) != GetIsolatedOrigins()->end();
+}
+
+// static
+SiteInstanceImpl::IsolatedOriginSet* SiteInstanceImpl::GetIsolatedOrigins() {
+  CR_DEFINE_STATIC_LOCAL(IsolatedOriginSet, isolated_origins, ());
+  return &isolated_origins;
+}
+
 void SiteInstanceImpl::RenderProcessHostDestroyed(RenderProcessHost* host) {
   DCHECK_EQ(process_, host);
   process_->RemoveObserver(this);
   process_ = nullptr;
 }
 
 void SiteInstanceImpl::RenderProcessWillExit(RenderProcessHost* host) {
   // TODO(nick): http://crbug.com/575400 - RenderProcessWillExit might not serve
   // any purpose here.
   for (auto& observer : observers_)
@@ skipping 35 matching lines @@
             browsing_instance_->browser_context(), site_))
       return;
 
     ChildProcessSecurityPolicyImpl* policy =
         ChildProcessSecurityPolicyImpl::GetInstance();
     policy->LockToOrigin(process_->GetID(), site_);
   }
 }
 
 }  // namespace content