Fix process reuse for dedicated processes when over process limit.

content/browser/site_instance_impl.h

 // 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.
 
 #ifndef CONTENT_BROWSER_SITE_INSTANCE_IMPL_H_
 #define CONTENT_BROWSER_SITE_INSTANCE_IMPL_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 131 matching lines @@
   static GURL GetEffectiveURL(BrowserContext* browser_context,
                               const GURL& url);
 
   // Returns true if pages loaded from |url| ought to be handled only by a
   // renderer process isolated from other sites. If --site-per-process is on the
   // command line, this is true for all sites. In other site isolation modes,
   // only a subset of sites will require dedicated processes.
   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;
 
   // Create a new SiteInstance.  Only BrowsingInstance should call this
   // directly; clients should use Create() or GetRelatedSiteInstance() instead.
   explicit SiteInstanceImpl(BrowsingInstance* browsing_instance);
 
   ~SiteInstanceImpl() override;
 
   // RenderProcessHostObserver implementation.
   void RenderProcessHostDestroyed(RenderProcessHost* host) override;
   void RenderProcessWillExit(RenderProcessHost* host) override;
   void RenderProcessExited(RenderProcessHost* host,
                            base::TerminationStatus status,
                            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(
       BrowserContext* browser_context,
       bool is_for_guests_only);
 
   // An object used to construct RenderProcessHosts.
   static const RenderProcessHostFactory* g_render_process_host_factory_;
 
   // The next available SiteInstance ID.
@@ skipping 25 matching lines @@
   ProcessReusePolicy process_reuse_policy_;
 
   base::ObserverList<Observer, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(SiteInstanceImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_SITE_INSTANCE_IMPL_H_