PlzNavigate: implement process reuse for ServiceWorkers

content/browser/renderer_host/render_process_host_impl.cc

Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index 02f2bf99b06e234512fd311e96e72b0f3838a808..3870025b02cf754dd469e6407fcf415a24a8a660 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -564,6 +564,66 @@ class WorkerURLLoaderFactoryProviderImpl
   scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_;
 };
 
+// The following object is used to keep track of which sites
+// RenderProcessHosts are hosting frames and expecting navigations to.
+// There are two of them per BrowserContext, one for frames and one for
+// navigations.
+const void* const kFrameSiteCountPerProcessTrackerKey =

[Charlie Reis, 2017/05/15 03:41:52]

I think pending and committed are probably better terms than frames and
expecting navigations.  We already use those terms elsewhere, plus we're not
keeping a record of which frames the sites are in (as I initially thought from
the name).

[clamy, 2017/05/16 14:50:46]

Done.

+    "FrameSiteCountPerProcessTrackerKey";
+const void* const kNavigationSiteCountPerProcessTrackerKey =
+    "NavigationSiteCountPerProcessTrackerKey";
+class SiteCountPerProcessTracker : public base::SupportsUserData::Data,

[clamy, 2017/05/04 16:01:01]

This class + data structure looks a bit weird. This is because of the split
RenderProcessHostImpl/MockRenderProcessHost which prevents from storing this
kind of state in the RenderProcessHostImpl.

[nasko, 2017/05/05 05:22:00]

Yuck! Yet another reason to hate MockRenderProcessHost. I wonder if both classes
can just inherit a this as a base class.

[Charlie Reis, 2017/05/15 03:41:52]

If we really want per-RPH state, we should find a way to make it happen without
this indirect approach.

However, it seems to me that we don't really need that anyway.  The query API
seems to be, "hey BrowserContext, give me the processes being used for this
particular site."  We might be able to structure the data that way to make the
lookup easier (i.e., without needing to iterate over the processes).

That is, rather than:
  map( process -> map(site -> count) )

  for each process:
    if process has site, add it to candidates
  return candidates

We instead do:
  map( site -> map( process -> count) )

  look up site
  return its set of processes (i.e., keys of the map)

Then it makes sense to have the map associated with BrowserContext, and we don't
need to do any iteration over the entire list of processes for lookup.  It would
probably be about the same complexity for adds and removes, as well.

The tradeoff is that we couldn't easily ask a process which sites it contains
(e.g., for security checks).  But the truth is, we can't really use this data
structure for such security checks anyway, at least as it stands.  We're
removing the old site when a new navigation commits, even though the old site's
unload handler may continue to run in the background (for cross-process
navigations).  That means the browser process may see actions from the old site
after this data structure no longer has a record of it being there.  That's fine
for process-per-site logic, but not for determining whether the process is
allowed to access data from the old site.  (In fact, we might want to mention
this limitation in comments on the data structure declaration.)

Does that sound like a worthwhile change?

[clamy, 2017/05/15 15:21:15]

This is how I had written it initially. However, while it makes querying
simpler, it makes the RenderProcessHostDestroyed function below more complex. We
now have to iterate over all sites in the map to remove the rph from it, instead
of just erasing the map of sites belonging to the process. I reasoned that we
were more likely to call RenderProcessHostDestroyed than
FindRenderProcessesForSite, so it made sense to make RenderProcessHostDestroyed
the simpler function. I can change it back the other way if you want.

[Charlie Reis, 2017/05/15 23:46:18]

Interesting!  Yes, I hadn't thought about the process exit case.

Just trying to think of some alternatives, to help us avoid the awkwardness of
storing per-process data with BrowserContext SupportsUserData.

1) It doesn't look like there's an easy way to get to all the frames for a given
RenderProcessHost.  It knows its set of widgets, but you can't easily get the
frames for a widget.  Otherwise this would let us directly remove the process
from the relevant sites when it exits.

2) Could we avoid updating the data structure when the process exits, and
instead filter out non-live RenderProcessHosts when doing lookups?  The
RenderFrameHosts for the process are still around.  (Weird, though-- we seem to
be clearing last_committed_url_ but not last_committed_origin_ when the process
exits.  That doesn't seem great.  We should probably be consistent.)

3) Kind of like #2, what if we did clear the last_committed_site_ on RFH when
the process exits, and *that's* where we could update the data structure?

How bad would #3 be?  We can keep it as process -> site -> count if needed, but
if we can make site -> process -> count work, it seems like it would be nicer
for lookup, make more sense to have associated with BrowserContext, and
discourage people from misusing it for security checks (given the unload issue).

[clamy, 2017/05/16 14:50:46]

I've reversed the map as asked, and remove the observer part of the class. I was
a bit worried of storing stale pointers, so I moved to storing ids instead,
allowing to dcheck that we do get a RPH when we want to use it. Wdyt?

+                                   public RenderProcessHostObserver {
+ public:
+  SiteCountPerProcessTracker() {}
+  ~SiteCountPerProcessTracker() override {}
+
+  void AddCountForSiteForProcess(RenderProcessHost* render_process_host,
+                                 std::string site) {

[nasko, 2017/05/05 05:22:00]

Why accept the site as a string and not a GURL?

[clamy, 2017/05/16 14:50:46]

Done.

+    std::map<std::string, int>& counts_per_site = map_[render_process_host];

[nasko, 2017/05/05 05:22:00]

I assume you are using operator[] intentionally here to add an entry to the map
if one does not exist. Correct?

[clamy, 2017/05/05 15:10:11]

Yes. We want to create one if we don't have one, and in that case [] looks
cleaner IMO.

+    if (counts_per_site.empty())
+      render_process_host->AddObserver(this);
+    ++counts_per_site[site];
+  }
+
+  void RemoveCountForSiteForProcess(RenderProcessHost* render_process_host,
+                                    std::string site) {
+    std::map<std::string, int>& counts_per_site = map_[render_process_host];

[nasko, 2017/05/05 05:22:00]

Shouldn't this be map_.find()?

[clamy, 2017/05/16 14:50:46]

Done.

+    --counts_per_site[site];
+    DCHECK(counts_per_site[site] >= 0);
+    if (counts_per_site[site] == 0)
+      counts_per_site.erase(site);
+    if (counts_per_site.empty()) {
+      map_.erase(render_process_host);
+      render_process_host->RemoveObserver(this);
+    }
+  }
+
+  void RenderProcessHostDestroyed(RenderProcessHost* host) override {
+    map_.erase(host);
+  }
+
+  void FindRenderProcessesForSite(
+      std::string site,
+      std::set<RenderProcessHost*>* foreground_processes,
+      std::set<RenderProcessHost*>* background_processes) {
+    for (auto site_counts_per_process : map_) {
+      if (site_counts_per_process.second.count(site) > 0) {
+        RenderProcessHost* host = site_counts_per_process.first;
+        if (host->VisibleWidgetCount())
+          foreground_processes->insert(host);
+        else
+          background_processes->insert(host);
+      }
+    }
+  }
+
+ private:
+  using SiteCountPerProcessMap =
+      std::map<RenderProcessHost*, std::map<std::string, int>>;
+  SiteCountPerProcessMap map_;
+};
+
 }  // namespace
 
 RendererMainThreadFactoryFunction g_renderer_main_thread_factory = NULL;
@@ -1684,6 +1744,90 @@ void RenderProcessHostImpl::set_render_process_host_factory(
   g_render_process_host_factory_ = rph_factory;
 }
 
+// static
+void RenderProcessHostImpl::AddFrameWithSite(
+    BrowserContext* browser_context,
+    RenderProcessHost* render_process_host,
+    const GURL& site_url) {
+  std::string site = SiteInstance::GetSiteForURL(browser_context, site_url)
+                         .possibly_invalid_spec();

[nasko, 2017/05/05 05:22:00]

Why possibly_invalid_spec()? Shouldn't an non-empty GURL returned from this
method be correctly formatted GURL?

[clamy, 2017/05/16 14:50:46]

It's now taking a GURL.

+  if (site.empty())
+    return;
+
+  SiteCountPerProcessTracker* tracker =
+      static_cast<SiteCountPerProcessTracker*>(
+          browser_context->GetUserData(kFrameSiteCountPerProcessTrackerKey));
+  if (!tracker) {
+    tracker = new SiteCountPerProcessTracker();
+    browser_context->SetUserData(kFrameSiteCountPerProcessTrackerKey,
+                                 base::WrapUnique(tracker));
+  }
+  tracker->AddCountForSiteForProcess(render_process_host, site);
+}
+
+// static
+void RenderProcessHostImpl::RemoveFrameWithSite(
+    BrowserContext* browser_context,
+    RenderProcessHost* render_process_host,
+    const GURL& site_url) {
+  std::string site = SiteInstance::GetSiteForURL(browser_context, site_url)
+                         .possibly_invalid_spec();
+  if (site.empty())
+    return;
+
+  SiteCountPerProcessTracker* tracker =
+      static_cast<SiteCountPerProcessTracker*>(
+          browser_context->GetUserData(kFrameSiteCountPerProcessTrackerKey));
+  if (!tracker) {
+    tracker = new SiteCountPerProcessTracker();
+    browser_context->SetUserData(kFrameSiteCountPerProcessTrackerKey,
+                                 base::WrapUnique(tracker));
+  }
+  tracker->RemoveCountForSiteForProcess(render_process_host, site);
+}
+
+// static
+void RenderProcessHostImpl::AddExpectedNavigationToSite(
+    BrowserContext* browser_context,
+    RenderProcessHost* render_process_host,
+    const GURL& site_url) {
+  std::string site = SiteInstance::GetSiteForURL(browser_context, site_url)
+                         .possibly_invalid_spec();
+  if (site.empty())
+    return;
+
+  SiteCountPerProcessTracker* tracker =
+      static_cast<SiteCountPerProcessTracker*>(browser_context->GetUserData(
+          kNavigationSiteCountPerProcessTrackerKey));
+  if (!tracker) {
+    tracker = new SiteCountPerProcessTracker();
+    browser_context->SetUserData(kNavigationSiteCountPerProcessTrackerKey,
+                                 base::WrapUnique(tracker));
+  }
+  tracker->AddCountForSiteForProcess(render_process_host, site);
+}
+
+// static
+void RenderProcessHostImpl::RemoveExpectedNavigationToSite(
+    BrowserContext* browser_context,
+    RenderProcessHost* render_process_host,
+    const GURL& site_url) {
+  std::string site = SiteInstance::GetSiteForURL(browser_context, site_url)
+                         .possibly_invalid_spec();
+  if (site.empty())
+    return;
+
+  SiteCountPerProcessTracker* tracker =
+      static_cast<SiteCountPerProcessTracker*>(browser_context->GetUserData(
+          kNavigationSiteCountPerProcessTrackerKey));
+  if (!tracker) {
+    tracker = new SiteCountPerProcessTracker();
+    browser_context->SetUserData(kNavigationSiteCountPerProcessTrackerKey,
+                                 base::WrapUnique(tracker));
+  }
+  tracker->RemoveCountForSiteForProcess(render_process_host, site);
+}
+
 bool RenderProcessHostImpl::IsForGuestsOnly() const {
   return is_for_guests_only_;
 }
@@ -2796,6 +2940,10 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
       DCHECK(SiteIsolationPolicy::IsTopDocumentIsolationEnabled());
       render_process_host =
           GetDefaultSubframeProcessHost(browser_context, site_instance);
+    case SiteInstanceImpl::ProcessReusePolicy::
+        REUSE_CHECKING_FRAMES_AND_NAVIGATIONS:
+      render_process_host =
+          FindReusableProcessHostForSite(browser_context, site_url);
       break;
     default:
       break;
@@ -3168,6 +3316,57 @@ RenderProcessHost* RenderProcessHostImpl::GetDefaultSubframeProcessHost(
   return holder->GetProcessHost(site_instance);
 }
 
+// static
+RenderProcessHost* RenderProcessHostImpl::FindReusableProcessHostForSite(
+    BrowserContext* browser_context,
+    const GURL& site_url) {
+  std::string site = SiteInstance::GetSiteForURL(browser_context, site_url)
+                         .possibly_invalid_spec();
+  if (site.empty())
+    return nullptr;
+
+  std::set<RenderProcessHost*> eligible_foreground_hosts;
+  std::set<RenderProcessHost*> eligible_background_hosts;
+
+  // Add the RenderProcessHosts hosting a frame rendering site to the list of

[nasko, 2017/05/05 05:22:00]

nit: "... hosting a frame for |site| to ..."

[clamy, 2017/05/16 14:50:46]

Done.

+  // eligible RenderProcessHosts.
+  SiteCountPerProcessTracker* frame_tracker =
+      static_cast<SiteCountPerProcessTracker*>(
+          browser_context->GetUserData(kFrameSiteCountPerProcessTrackerKey));
+  if (frame_tracker) {
+    frame_tracker->FindRenderProcessesForSite(site, &eligible_foreground_hosts,
+                                              &eligible_background_hosts);
+  }
+
+  // Add the RenderProcessHosts expecting a navigation to site to the list of
+  // eligible RenderProcessHosts.
+  SiteCountPerProcessTracker* navigation_tracker =

[clamy, 2017/05/04 16:01:01]

if we want to implement process-per-site using this mechanism, we can just not
execute the following block.

+      static_cast<SiteCountPerProcessTracker*>(browser_context->GetUserData(
+          kNavigationSiteCountPerProcessTrackerKey));
+  if (frame_tracker) {
+    navigation_tracker->FindRenderProcessesForSite(
+        site, &eligible_foreground_hosts, &eligible_background_hosts);
+  }
+
+  if (!eligible_foreground_hosts.empty()) {
+    int index = rand() % eligible_foreground_hosts.size();

[nasko, 2017/05/05 05:22:00]

base::RandInt

[clamy, 2017/05/16 14:50:46]

Done.

+    auto iterator = eligible_foreground_hosts.begin();
+    for (int i = 0; i < index; ++i)
+      ++iterator;
+    return (*iterator);
+  }
+
+  if (!eligible_background_hosts.empty()) {
+    int index = rand() % eligible_background_hosts.size();

[nasko, 2017/05/05 05:22:00]

base::RandInt

[clamy, 2017/05/16 14:50:46]

Done.

+    auto iterator = eligible_background_hosts.begin();
+    for (int i = 0; i < index; ++i)
+      ++iterator;
+    return (*iterator);
+  }
+
+  return nullptr;
+}
+
 #if BUILDFLAG(ENABLE_WEBRTC)
 void RenderProcessHostImpl::OnRegisterAecDumpConsumer(int id) {
   BrowserThread::PostTask(