Add UMA histogram for the number of SiteInstances per BrowsingInstance.

chrome/browser/site_details.cc

 // Copyright 2013 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.
 
 #include "chrome/browser/site_details.h"
 
+#include "base/containers/hash_tables.h"

[ncarter (slow), 2015/12/02 18:16:49]

This #include belongs in the .h file.

[nasko, 2015/12/02 19:06:56]

Done.

 #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)
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #endif
@@ skipping 81 matching lines @@
 
   // 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;
 }
 
+content::SiteInstance* DeterminePrimarySiteInstance(
+    content::SiteInstance* instance,
+    SiteData* site_data) {
+  // Find the BrowsingInstance this WebContents belongs to by iterating over
+  // the "primary" SiteInstances of each BrowsingInstance we've seen so far.
+  SiteInstance* primary = nullptr;
+  for (auto& already_collected_instance : site_data->instances) {
+    if (instance->IsRelatedSiteInstance(already_collected_instance.first)) {
+      primary = already_collected_instance.first;
+      already_collected_instance.second.insert(instance);
+      break;

[ncarter (slow), 2015/12/02 18:16:49]

Just return here. That'll let you get rid of the |primary| local var altogether,
and shave a couple lines off the function.

[nasko, 2015/12/02 19:06:56]

Done.

+    }
+  }
+  if (!primary) {
+    // Remember this as the "primary" SiteInstance of a new BrowsingInstance.
+    primary = instance;
+    site_data->instances.insert(
+        std::make_pair(instance, std::set<SiteInstance*>()));

[ncarter (slow), 2015/12/02 18:16:49]

Shouldn't the new std::set contain |instance| initially? Seems like a subsequent
calls to DeterminePrimarySiteInstance will insert it, so that seems
inconsistent. (Probably this isn't a bug in practice because
DeterminePrimarySiteInstance always happens twice on the main frame)

See my next comment, however.

[nasko, 2015/12/02 19:06:56]

Acknowledged.

+  }
+
+  return primary;
+}
+
 void CollectCurrentSnapshot(SiteData* site_data, RenderFrameHost* frame) {
   if (frame->GetParent()) {
     if (frame->GetSiteInstance() != frame->GetParent()->GetSiteInstance())
       site_data->out_of_process_frames++;
   }
+
+  DeterminePrimarySiteInstance(frame->GetSiteInstance(), site_data);

[ncarter (slow), 2015/12/02 18:16:49]

We've already determined the primary site instance for the webcontents's
browsinginstance; if we plumbed it in here, this operation could become:

site_data->instances[primary].insert(frame->GetSiteInstance())

[nasko, 2015/12/02 19:06:56]

Unfortunately not. Due to crbug.com/522302 not all SiteInstances in a FrameTree
are in the same BrowsingInstance. We should be counting reality until we fix
this.

 }
 
 }  // namespace
 
 IsolationScenario::IsolationScenario() : policy(ISOLATE_ALL_SITES) {}
 
 IsolationScenario::~IsolationScenario() {}
 
 SiteData::SiteData() : out_of_process_frames(0) {
   for (int i = 0; i <= ISOLATION_SCENARIO_LAST; i++)
     scenarios[i].policy = static_cast<IsolationScenarioType>(i);
 }
 
 SiteData::~SiteData() {}
 
 SiteDetails::SiteDetails() {}
 
 SiteDetails::~SiteDetails() {}
 
 void SiteDetails::CollectSiteInfo(WebContents* contents,
                                   SiteData* site_data) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  // 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();
-  SiteInstance* primary = NULL;
-  for (SiteInstance* already_collected_instance : site_data->instances) {
-    if (instance->IsRelatedSiteInstance(already_collected_instance)) {
-      primary = already_collected_instance;
-      break;
-    }
-  }
-  if (!primary) {
-    // Remember this as the "primary" SiteInstance of a new BrowsingInstance.
-    primary = instance;
-    site_data->instances.push_back(instance);
-  }
+  SiteInstance* primary =
+      DeterminePrimarySiteInstance(contents->GetSiteInstance(), site_data);
 
   // Now keep track of how many sites we have in this BrowsingInstance (and
   // overall), including sites in iframes.
   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)));
   }
 
@@ skipping 18 matching lines @@
        i != site_data_map.end(); ++i) {
     for (const IsolationScenario& scenario : i->second.scenarios) {
       num_sites[scenario.policy] += scenario.sites.size();
       for (auto& browsing_instance : scenario.browsing_instance_site_map) {
         num_isolated_site_instances[scenario.policy] +=
             browsing_instance.second.size();
       }
     }
     num_browsing_instances += i->second.scenarios[ISOLATE_ALL_SITES]
                                   .browsing_instance_site_map.size();
+    for (const auto& site : i->second.instances) {
+      UMA_HISTOGRAM_COUNTS_100("SiteIsolation.SiteInstancesPerBrowsingInstance",
+                               site.second.size());
+    }
     num_oopifs += i->second.out_of_process_frames;
   }
 
   // Predict the number of processes needed when isolating all sites, when
   // isolating only HTTPS sites, and when isolating extensions.
   int process_count_lower_bound[ISOLATION_SCENARIO_LAST + 1];
   int process_count_upper_bound[ISOLATION_SCENARIO_LAST + 1];
   int process_count_estimate[ISOLATION_SCENARIO_LAST + 1];
   for (int policy = 0; policy <= ISOLATION_SCENARIO_LAST; policy++) {
     process_count_lower_bound[policy] = num_sites[policy];
@@ skipping 52 matching lines @@
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateExtensionsProcessCountLowerBound",
       process_count_lower_bound[ISOLATE_EXTENSIONS]);
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateExtensionsProcessCountEstimate",
       process_count_estimate[ISOLATE_EXTENSIONS]);
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateExtensionsTotalProcessCountEstimate",
       process_count_estimate[ISOLATE_EXTENSIONS] + non_renderer_process_count);
 }