Fix process reuse for dedicated processes when over process limit.

content/browser/site_instance_impl.h

Index: content/browser/site_instance_impl.h
diff --git a/content/browser/site_instance_impl.h b/content/browser/site_instance_impl.h
index 001790fcb376970e72470c98ab8ec1ce527d7de3..8f67579afb1082504795c5476ef6602ac9b4b8b5 100644
--- a/content/browser/site_instance_impl.h
+++ b/content/browser/site_instance_impl.h
@@ -149,6 +149,14 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   static bool DoesSiteRequireDedicatedProcess(BrowserContext* browser_context,
                                               const GURL& url);
 
+  // Returns true unless a process for |site_url| cannot be locked to just that

[Charlie Reis, 2017/06/17 23:13:53]

nit: s/unless/if/, s/cannot/should/
(Otherwise it's too hard to tell if the comment and method name agree.)

[alexmos, 2017/06/19 20:03:59]

Done.

+  // site. Returning true here implies that |site_url| requires a dedicated

[Charlie Reis, 2017/06/17 23:13:53]

nit: also implies

[alexmos, 2017/06/19 20:03:59]

Done.

+  // process. However, the converse does not hold: if |site_url| requires a

[Charlie Reis, 2017/06/17 23:13:53]

I'm finding this pretty hard to read, since "lock to origin" and "requires
dedicated process" sound synonymous.  Can you explain the difference in the
comment a bit more?

[alexmos, 2017/06/19 20:03:59]

Done, and also see below.

+  // dedicated process, this might still return false for certain special
+  // cases, most of which are temporary bug workarounds.

[Charlie Reis, 2017/06/17 23:13:53]

"Temporary bug workarounds" is confusing me a bit.  I thought WebUI and
extensions were the cases that required "dedicated" processes but weren't locked
to origin, but those don't sound like temporary bug workarounds.  (I thought
that at least the extension case was about letting extensions share with each
other but not other types of processes.)  Are there plans to change that?

[alexmos, 2017/06/19 20:03:59]

I was mostly thinking about this comment on
ContentBrowserClient::ShouldLockToOrigin when I wrote this:

  // Returns true unless the effective URL is part of a site that cannot live in
  // a process restricted to just that site.  This is only called if site
  // isolation is enabled for this URL, and is a bug workaround.
  //
  // TODO(nick): Remove this function once https://crbug.com/160576 is fixed,
  // and origin lock can be applied to all URLs.
  virtual bool ShouldLockToOrigin(BrowserContext* browser_context,
                                  const GURL& effective_url);

The other two parts of it in content/ (guests and chrome://) also have TODOs and
sound like we can eventually remove them.  But overall, I agree that it's not
clear how "temporary" some of these are (e.g., extension process reuse case), so
I've rephrased this -- let me know if that's better.

[Charlie Reis, 2017/06/28 00:08:37]

Huh.  How would we apply origin lock to all URLs?  Maybe the idea there was to
allow a process to be locked to multiple origins, and keep track of them all via
origin lock?
Yes, thanks.  The new comment works for me.

[alexmos, 2017/06/28 00:33:26]

I assumed this just meant "origin lock can be applied to all URLs that require a
dedicated process", given that ContentBrowserClient::ShouldLockToOrigin() is
only called when RequiresDedicatedProcess() is true in LockToOrigin().  But
maybe we should double-check with Nick.

+  static bool ShouldLockToOrigin(BrowserContext* browser_context,
+                                 GURL site_url);
+
  private:
   friend class BrowsingInstance;
   friend class SiteInstanceTestBrowserClient;
@@ -167,7 +175,7 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
                            int exit_code) override;
 
   // Used to restrict a process' origin access rights.
-  void LockToOrigin();
+  void LockToOriginIfNeeded();
 
   // This gets the render process to use for default subframe site instances.
   RenderProcessHost* GetDefaultSubframeProcessHost(