Several Site Details / Memory metrics fixes

chrome/browser/site_details.cc

Index: chrome/browser/site_details.cc
diff --git a/chrome/browser/site_details.cc b/chrome/browser/site_details.cc
index bf9d9478200dd518a863c3300248ff3232bb2eee..847bdd5e9e008119ffe74283d952223ea075e7ec 100644
--- a/chrome/browser/site_details.cc
+++ b/chrome/browser/site_details.cc
@@ -6,6 +6,7 @@
 
 #include "base/metrics/histogram.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 
 #if defined(ENABLE_EXTENSIONS)
@@ -14,17 +15,21 @@
 #include "extensions/common/extension.h"
 #endif
 
+using content::BrowserContext;
 using content::BrowserThread;
+using content::RenderFrameHost;
 using content::RenderProcessHost;
 using content::SiteInstance;
 using content::WebContents;
 
 namespace {
 
-bool ShouldIsolate(content::BrowserContext* browser_context,
-                   IsolationScenarioType policy,
+bool ShouldIsolate(BrowserContext* browser_context,
+                   IsolationScenario* scenario,

[Lei Zhang, 2015/10/30 07:17:49]

const IsolationScenario*

[ncarter (slow), 2015/10/30 18:13:24]

Done.

                    const GURL& site) {
-  switch (policy) {
+  switch (scenario->policy) {
+    case ISOLATE_NOTHING:
+      return false;
     case ISOLATE_ALL_SITES:
       return true;
     case ISOLATE_HTTPS_SITES:
@@ -51,26 +56,64 @@ bool ShouldIsolate(content::BrowserContext* browser_context,
   return true;
 }
 
+// Walk the frame tree and update |scenario|'s data for the given frame. Memoize

[Lei Zhang, 2015/10/30 07:17:49]

typo: Memoize

[ncarter (slow), 2015/10/30 18:13:24]

Not a typo. Memoize is what I meant. https://en.wikipedia.org/wiki/Memoization

+// each frame's computed URL in |frame_urls| so it can be reused when visiting
+// its children.
+void CollectForScenario(std::map<RenderFrameHost*, GURL>* frame_urls,
+                        SiteInstance* primary,
+                        IsolationScenario* scenario,
+                        RenderFrameHost* frame) {
+  BrowserContext* context = primary->GetBrowserContext();
+
+  GURL url = frame->GetLastCommittedURL();
+
+  // Treat about:blank url specially: use the URL on the SiteInstance if it is
+  // assigned. The rest of this computation must not depend on the
+  // SiteInstance's URL, since its value reflects the current process model, and
+  // this computation is simulating other process models.
+  if (url == GURL(url::kAboutBlankURL) &&
+      frame->GetSiteInstance()->GetSiteURL() != GURL()) {

[Lei Zhang, 2015/10/30 07:17:49]

Can you call GURL::is_empty() instead?

[ncarter (slow), 2015/10/30 18:13:24]

Done.

+    url = frame->GetSiteInstance()->GetSiteURL();
+  }
+
+  GURL site = SiteInstance::GetSiteForURL(context, url);
+  bool should_isolate = ShouldIsolate(context, scenario, site);
+
+  // Treat a subframe as part of its parent site if neither needs isolation.
+  if (frame->GetParent()) {
+    GURL parent_site = (*frame_urls)[frame->GetParent()];
+    if (!should_isolate && !ShouldIsolate(context, scenario, parent_site))

[Lei Zhang, 2015/10/30 07:17:49]

Move the |should_isolate| check up 2 lines?

[ncarter (slow), 2015/10/30 18:13:24]

Done.

+      site = parent_site;
+  }
+
+  bool process_per_site =
+      site.is_valid() &&
+      RenderProcessHost::ShouldUseProcessPerSite(context, site);
+
+  // If we don't need a dedicated process, and aren't living in a process-
+  // per-site process, we are nothing special: collapse our URL to a dummy
+  // site.
+  if (!process_per_site && !should_isolate)
+    site = GURL("http://");
+
+  // We model process-per-site by only inserting those sites into the first
+  // browsing instance in which they appear.
+  if (scenario->sites.insert(site).second || !process_per_site)
+    scenario->browsing_instance_site_map[primary->GetId()].insert(site);
+
+  // Record our result in |frame_urls| for use by children.
+  (*frame_urls)[frame] = site;
+}
+
 }  // namespace
 
 IsolationScenario::IsolationScenario() : policy(ISOLATE_ALL_SITES) {}
 
 IsolationScenario::~IsolationScenario() {}
 
-void IsolationScenario::CollectSiteInfoForScenario(SiteInstance* primary,
-                                                   const GURL& site) {
-  const GURL& isolated =
-      ShouldIsolate(primary->GetBrowserContext(), policy, site)
-          ? site
-          : GURL("http://");
-  sites.insert(isolated);
-  browsing_instance_site_map[primary->GetId()].insert(isolated);
-}
-
 SiteData::SiteData() {
-  scenarios[ISOLATE_ALL_SITES].policy = ISOLATE_ALL_SITES;
-  scenarios[ISOLATE_HTTPS_SITES].policy = ISOLATE_HTTPS_SITES;
-  scenarios[ISOLATE_EXTENSIONS].policy = ISOLATE_EXTENSIONS;
+  for (int i = 0; i <= ISOLATION_SCENARIO_LAST; i++)
+    scenarios[i].policy = static_cast<IsolationScenarioType>(i);
 }
 
 SiteData::~SiteData() {}
@@ -82,8 +125,6 @@ SiteDetails::~SiteDetails() {}
 void SiteDetails::CollectSiteInfo(WebContents* contents,
                                   SiteData* site_data) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  content::BrowserContext* browser_context = contents->GetBrowserContext();
-
   // Find the BrowsingInstance this WebContents belongs to by iterating over
   // the "primary" SiteInstances of each BrowsingInstance we've seen so far.
   SiteInstance* instance = contents->GetSiteInstance();
@@ -102,19 +143,11 @@ void SiteDetails::CollectSiteInfo(WebContents* contents,
 
   // Now keep track of how many sites we have in this BrowsingInstance (and
   // overall), including sites in iframes.
-  for (const GURL& site : contents->GetSitesInTab()) {
-    // Make sure we don't overcount process-per-site sites, like the NTP or
-    // extensions, by skipping over them if they're already logged for
-    // ISOLATE_ALL_SITES.
-    if (RenderProcessHost::ShouldUseProcessPerSite(browser_context, site) &&
-        site_data->scenarios[ISOLATE_ALL_SITES].sites.find(site) !=
-            site_data->scenarios[ISOLATE_ALL_SITES].sites.end()) {
-      continue;
-    }
-
-    for (IsolationScenario& scenario : site_data->scenarios) {
-      scenario.CollectSiteInfoForScenario(primary, site);
-    }
+  for (IsolationScenario& scenario : site_data->scenarios) {
+    std::map<RenderFrameHost*, GURL> memo;
+    contents->ForEachFrame(
+        base::Bind(&CollectForScenario, base::Unretained(&memo),
+                   base::Unretained(primary), base::Unretained(&scenario)));
   }
 }
 
@@ -160,6 +193,19 @@ void SiteDetails::UpdateHistograms(
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.BrowsingInstanceCount",
       num_browsing_instances);
+
+  // ISOLATE_NOTHING metrics.
+  UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateNothingProcessCountNoLimit",
+                           num_isolated_site_instances[ISOLATE_NOTHING]);
+  UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateNothingProcessCountLowerBound",
+                           process_count_lower_bound[ISOLATE_NOTHING]);
+  UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateNothingProcessCountEstimate",
+                           process_count_estimate[ISOLATE_NOTHING]);
+  UMA_HISTOGRAM_COUNTS_100(
+      "SiteIsolation.IsolateNothingTotalProcessCountEstimate",
+      process_count_estimate[ISOLATE_NOTHING] + non_renderer_process_count);
+
+  // ISOLATE_ALL_SITES metrics.
   UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateAllSitesProcessCountNoLimit",
                            num_isolated_site_instances[ISOLATE_ALL_SITES]);
   UMA_HISTOGRAM_COUNTS_100(
@@ -167,7 +213,11 @@ void SiteDetails::UpdateHistograms(
       process_count_lower_bound[ISOLATE_ALL_SITES]);
   UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateAllSitesProcessCountEstimate",
                            process_count_estimate[ISOLATE_ALL_SITES]);
+  UMA_HISTOGRAM_COUNTS_100(
+      "SiteIsolation.IsolateAllSitesTotalProcessCountEstimate",
+      process_count_estimate[ISOLATE_ALL_SITES] + non_renderer_process_count);
 
+  // ISOLATE_HTTPS_SITES metrics.
   UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateHttpsSitesProcessCountNoLimit",
                            num_isolated_site_instances[ISOLATE_HTTPS_SITES]);
   UMA_HISTOGRAM_COUNTS_100(
@@ -176,7 +226,11 @@ void SiteDetails::UpdateHistograms(
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateHttpsSitesProcessCountEstimate",
       process_count_estimate[ISOLATE_HTTPS_SITES]);
+  UMA_HISTOGRAM_COUNTS_100(
+      "SiteIsolation.IsolateHttpsSitesTotalProcessCountEstimate",
+      process_count_estimate[ISOLATE_HTTPS_SITES] + non_renderer_process_count);
 
+  // ISOLATE_EXTENSIONS metrics.
   UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateExtensionsProcessCountNoLimit",
                            num_isolated_site_instances[ISOLATE_EXTENSIONS]);
   UMA_HISTOGRAM_COUNTS_100(
@@ -185,14 +239,6 @@ void SiteDetails::UpdateHistograms(
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateExtensionsProcessCountEstimate",
       process_count_estimate[ISOLATE_EXTENSIONS]);
-
-  // Total process count:
-  UMA_HISTOGRAM_COUNTS_100(
-      "SiteIsolation.IsolateAllSitesTotalProcessCountEstimate",
-      process_count_estimate[ISOLATE_ALL_SITES] + non_renderer_process_count);
-  UMA_HISTOGRAM_COUNTS_100(
-      "SiteIsolation.IsolateHttpsSitesTotalProcessCountEstimate",
-      process_count_estimate[ISOLATE_HTTPS_SITES] + non_renderer_process_count);
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateExtensionsTotalProcessCountEstimate",
       process_count_estimate[ISOLATE_EXTENSIONS] + non_renderer_process_count);