Introduce support for origins that require process isolation.

content/browser/site_instance_impl.cc

Index: content/browser/site_instance_impl.cc
diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc
index 0dd62877d2ec961eacf4adb5fd4d41bd92ba66df..19df31e0d7d5ea2daff6113dbaace5b34c92a1b1 100644
--- a/content/browser/site_instance_impl.cc
+++ b/content/browser/site_instance_impl.cc
@@ -4,6 +4,7 @@
 
 #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"
@@ -405,7 +406,18 @@ bool SiteInstance::IsSameWebSite(BrowserContext* browser_context,
   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;
 
@@ -423,7 +435,11 @@ GURL SiteInstance::GetSiteForURL(BrowserContext* browser_context,
     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()) {
@@ -462,10 +478,15 @@ bool SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
   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)) {
@@ -475,6 +496,36 @@ bool SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
   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);