Introduce support for origins that require process isolation.

content/browser/child_process_security_policy_impl.h

Index: content/browser/child_process_security_policy_impl.h
diff --git a/content/browser/child_process_security_policy_impl.h b/content/browser/child_process_security_policy_impl.h
index 82f0e9be22c660dd30f0c11eb4c58e775405cf78..a8028a3fc5d95d14f9830d93630e4b5ceac8a1a9 100644
--- a/content/browser/child_process_security_policy_impl.h
+++ b/content/browser/child_process_security_policy_impl.h
@@ -19,6 +19,7 @@
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/common/resource_type.h"
 #include "storage/common/fileapi/file_system_types.h"
+#include "url/origin.h"
 
 class GURL;
 
@@ -170,12 +171,39 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
   // Returns true if sending system exclusive messages is allowed.
   bool CanSendMidiSysExMessage(int child_id);
 
+  // Add an origin to the list of origins that require process isolation.
+  // When making process model decisions for such origins, the full
+  // scheme+host+port tuple rather than scheme and eTLD+1 will be used.
+  // SiteInstances for these origins will also use the full origin as site URL.
+  //
+  // Note that |origin| must not be unique.  URLs that render with
+  // unique origins, such as data: URLs, are not supported.  Suborigins and

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

Might clarify what you mean by suborigins, since it sounds like bar.foo.com at
first.  Maybe there's a link we can use, or a more specific term?

[alexmos, 2017/05/24 00:19:56]

Ah, good call - I didn't realize there might be such a confusion.  I added a
link to the spec and an explicit comment that these are not subdomains.

+  // non-standard schemes are also not supported.  Sandboxed frames (e.g.,
+  // <iframe sandbox>) *are* supported, since process placement decisions will
+  // be based on the URLs such frames navigate to, and not the origin of
+  // committed documents (which might be unique).  If an isolated origin opens
+  // an about:blank popup, it will stay in the isolated origin's process.
+  // Nested URLs (filesystem: and blob:) retain process isolation behavior of
+  // their inner origin.
+  void AddIsolatedOrigin(const url::Origin& origin);
+
+  // Register a set of isolated origins as specified on the command line with
+  // the --isolate-origins flag.  |origin_list| is the flag's value, which
+  // contains the list of comma-separated scheme-host-port origins. See
+  // AddIsolatedOrigin for definition of an isolated origin.
+  void AddIsolatedOriginsFromCommandLine(const std::string& origin_list);
+
+  // Helper to check whether an origin requires origin-wide process isolation.
+  bool IsIsolatedOrigin(const url::Origin& origin);
+
  private:
   friend class ChildProcessSecurityPolicyInProcessBrowserTest;
   friend class ChildProcessSecurityPolicyTest;
   FRIEND_TEST_ALL_PREFIXES(ChildProcessSecurityPolicyInProcessBrowserTest,
                            NoLeak);
   FRIEND_TEST_ALL_PREFIXES(ChildProcessSecurityPolicyTest, FilePermissions);
+  FRIEND_TEST_ALL_PREFIXES(ChildProcessSecurityPolicyTest,
+                           IsolateOriginsFromCommandLine);
 
   class SecurityState;
 
@@ -260,6 +288,20 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
 
   FileSystemPermissionPolicyMap file_system_policy_map_;
 
+  // Protects access to isolated_origins_.  This has to be a separate lock from
+  // |lock_|, because it's possible to attempt access to isolated_origins_ via
+  // IsIsolatedOrigin() from outside ChildProcessSecurityPolicy while already
+  // holding |lock_|.  A scenario where this happens is
+  // CanAccessDataForOrigin() (grabs lock_) -> SiteInstance::GetSiteForURL() ->
+  // IsIsolatedOrigin().

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

This cycle is nasty, and I'm not sure this is a good way to deal with it. :(  I
considered handling isolated origins separately before calling GetSiteForURL,
but that didn't seem clean (it'd need to be done in
CPSPI::CanAccessDataForOrigin, where isolated_origins_ are, and not on
SecurityState, but it'd still need origin_lock_ to compare against which is on
SecurityState).  Or maybe we could move the isolated origin tracking into their
own class after all, to avoid tying their state synchronization to the rest of
CPSP.  Let me know what you think.

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

Ooh, this is tough.  Let's find a chance to chat about this, since it's
unfortunate to need another lock...

[Charlie Reis, 2017/05/19 16:54:07]

What if we moved the GetSiteForURL call from
SecurityState::CanAccessDataForOrigin out to
ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin, before acquiring lock_? 
Then we could document that both CanAccessDataForOrigin and LockToOrigin on
SecurityState expect site_urls, not arbitrary GURLs, and hopefully avoid the
need for an extra lock?

It looks like the only other use of SiteInstance in
ChildProcessSecurityPolicyImpl is another GetSiteForURL call within
LockToOrigin, but that's also done before lock_ is acquired, so it shouldn't be
a problem.

[alexmos, 2017/05/24 00:19:56]

Thanks for the suggestion!  I went ahead and followed it.  I initially thought
something like this would be problematic because we'll end up doing
CanAccessDataForOrigin over two transactions instead of one, opening the door to
races in between the lock getting acquired, but thinking about this more
carefully, this might actually be ok.

The scenario I was worried about was:
1. [IO] www.foo.com accesses a cookie and retrieves GetSiteForURL.  That queries
isolated origins and then returns "foo.com".
2. [UI] www.foo.com is added to the isolated origin list (e.g., as part of SB
clickthrough), and we load www.foo.com in a new process locked to a site URL of
"www.foo.com".
3. [IO] continuing step 1, we check whether origin_lock_ matches "foo.com"
(grabbed from step 1), and it might not if we end up using the origin_lock_ of
"www.foo.com" from step 2.

But I think steps 1-3 can't happen for the same process as step 2: we won't
change the origin lock on any existing processes, we'll only use the new origin
lock for new processes dedicated to the new isolated origin.  Even if we decide
to do something like reloading tabs containing the newly-isolated origin within
the same BI, they will reload in a new dedicated process.  So I think this is
fine. :)

+  base::Lock isolated_origins_lock_;
+
+  // Tracks origins for which the entire origin should be treated as a site
+  // when making process model decisions, rather than the origin's scheme and
+  // eTLD+1. Each of these origins requires a dedicated process.  This set is
+  // protected by |isolated_origins_lock_|.
+  std::set<url::Origin> isolated_origins_;
+
   DISALLOW_COPY_AND_ASSIGN(ChildProcessSecurityPolicyImpl);
 };