Add UMA histogram for the number of SiteInstances per BrowsingInstance.

BUG=542921

Review URL: https://codereview.chromium.org/1492033002

Cr-Commit-Position: refs/heads/master@{#362988}

chrome/browser/site_details.cc

@@ -105,11 +105,32 @@ void CollectForScenario(std::map<RenderFrameHost*, GURL>* frame_urls,
   (*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.
+  for (auto& existing_site_instance : site_data->instances) {
+    if (instance->IsRelatedSiteInstance(existing_site_instance.first)) {
+      existing_site_instance.second.insert(instance);
+      return existing_site_instance.first;
+    }
+  }
+
+  // Add |instance| as the "primary" SiteInstance of a new BrowsingInstance.
+  site_data->instances[instance].clear();
+  site_data->instances[instance].insert(instance);
+
+  return instance;
+}
+
 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);
 }
 
 }  // namespace
@@ -132,21 +153,8 @@ 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.
@@ -185,6 +193,10 @@ void SiteDetails::UpdateHistograms(
     }
     num_browsing_instances += i->second.scenarios[ISOLATE_ALL_SITES]
                                   .browsing_instance_site_map.size();
+    for (const auto& site_instance : i->second.instances) {
+      UMA_HISTOGRAM_COUNTS_100("SiteIsolation.SiteInstancesPerBrowsingInstance",
+                               site_instance.second.size());
+    }
     num_oopifs += i->second.out_of_process_frames;
   }
 

chrome/browser/site_details.h

@@ -11,7 +11,12 @@
 #include "content/public/browser/web_contents.h"
 
 // Maps an ID representing each BrowsingInstance to a set of site URLs.
-typedef base::hash_map<int32, std::set<GURL>> BrowsingInstanceSiteMap;
+using BrowsingInstanceSiteMap = base::hash_map<int32, std::set<GURL>>;
+
+// Maps a SiteInstance to a set of all SiteInstances in the same
+// BrowsingInstance.
+using SiteInstanceMap =
+    base::hash_map<content::SiteInstance*, std::set<content::SiteInstance*>>;
 
 // This enum represents various alternative process model policies that we want
 // to evaluate. We'll estimate the process cost of each scenario.
@@ -44,7 +49,9 @@ struct SiteData {
   IsolationScenario scenarios[ISOLATION_SCENARIO_LAST + 1];
 
   // Global list of all SiteInstances, used for de-duping related instances.
-  std::vector<content::SiteInstance*> instances;
+  // It also keeps a set of all SiteInstances in the BrowsingInstance identified
+  // by the SiteInstance used as the key.
+  SiteInstanceMap instances;
 
   // A count of all RenderFrameHosts, which are in a different SiteInstance from
   // their parents.

chrome/browser/site_details_browsertest.cc

@@ -1035,3 +1035,163 @@ IN_PROC_BROWSER_TEST_F(SiteDetailsBrowserTest, VerifyFieldTrialGroup) {
 
   EXPECT_TRUE(IsInTrialGroup("SiteIsolationExtensionsActive", group));
 }
+
+// Verifies that the UMA counter for SiteInstances in a BrowsingInstance is
+// correct when using tabs with web pages.
+IN_PROC_BROWSER_TEST_F(SiteDetailsBrowserTest,
+                       VerifySiteInstanceCountInBrowsingInstance) {
+  // Page with 14 nested oopifs across 9 sites (a.com through i.com).
+  GURL abcdefghi_url = embedded_test_server()->GetURL(
+      "a.com",
+      "/cross_site_iframe_factory.html?a(b(a(b,c,d,e,f,g,h)),c,d,e,i(f))");
+  ui_test_utils::NavigateToURL(browser(), abcdefghi_url);
+
+  // Get the metrics.
+  scoped_refptr<TestMemoryDetails> details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+  if (content::AreAllSitesIsolatedForTesting()) {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(9, 1)));
+  } else {
+    // Since there are no extensions involved, the results in the default case
+    // and extensions::IsIsolateExtensionsEnabled() are the same.
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 1)));
+  }
+
+  // Open another tab through window.open(), which will be in the same
+  // BrowsingInstance.
+  GURL dcbae_url = embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?d(c(b(j(k))))");
+  ui_test_utils::UrlLoadObserver load_complete(
+      dcbae_url, content::NotificationService::AllSources());
+  ASSERT_EQ(1, browser()->tab_strip_model()->count());
+  ASSERT_TRUE(content::ExecuteScript(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      "window.open('" + dcbae_url.spec() + "');"));
+  ASSERT_EQ(2, browser()->tab_strip_model()->count());
+  load_complete.Wait();
+
+  details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+  if (content::AreAllSitesIsolatedForTesting()) {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(11, 1)));
+  } else {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 1)));
+  }
+
+  // Open a tab, which will be in a different BrowsingInstance.
+  GURL abcd_url = embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b(c(d())))");
+  AddTabAtIndex(1, abcd_url, ui::PAGE_TRANSITION_TYPED);
+
+  details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+  if (content::AreAllSitesIsolatedForTesting()) {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(4, 1), Bucket(11, 1)));
+  } else {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 2)));
+  }
+}
+
+// Verifies that the UMA counter for SiteInstances in a BrowsingInstance is
+// correct when extensions and web pages are mixed together.
+IN_PROC_BROWSER_TEST_F(
+    SiteDetailsBrowserTest,
+    VerifySiteInstanceCountInBrowsingInstanceWithExtensions) {
+  // Open two a.com tabs (with cross site http iframes). IsolateExtensions mode
+  // should have no effect so far, since there are no frames straddling the
+  // extension/web boundary.
+  GURL tab_url = embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b,c,d(e))");
+  ui_test_utils::NavigateToURL(browser(), tab_url);
+  WebContents* tab = browser()->tab_strip_model()->GetWebContentsAt(0);
+  scoped_refptr<TestMemoryDetails> details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+  if (content::AreAllSitesIsolatedForTesting()) {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(5, 1)));
+  } else {
+    // Since there are no extensions loaded yet, the results in the default case
+    // and extensions::IsIsolateExtensionsEnabled() are the same.
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 1)));
+  }
+
+  // Load an extension without a background page, which will avoid creating a
+  // BrowsingInstance for it.
+  const Extension* extension1 = CreateExtension("Extension One", false);
+
+  // Navigate the tab's first iframe to a resource of the extension. The
+  // extension iframe will be put in a separate BrowsingInstance (see
+  // https://crbug.com/522302) unless in the default process model.
+  content::NavigateIframeToURL(
+      tab, "child-0", extension1->GetResourceURL("/blank_iframe.html"));
+  details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+  if (content::AreAllSitesIsolatedForTesting()) {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 1), Bucket(4, 1)));
+  } else if (extensions::IsIsolateExtensionsEnabled()) {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 2)));
+  } else {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 1)));
+  }
+
+  // Now load an extension with a background page. This will result in a
+  // BrowsingInstance for the background page.
+  const Extension* extension2 = CreateExtension("Extension One", true);
+  details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+  if (content::AreAllSitesIsolatedForTesting()) {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 2), Bucket(4, 1)));
+  } else if (extensions::IsIsolateExtensionsEnabled()) {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 3)));
+  } else {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 2)));
+  }
+
+  // Navigate the second iframe of the tab to the second extension. This will
+  // create a new BrowsingInstance again due to https://crbug.com/522302 for
+  // --site-per-process and --isolate-extensions.
+  content::NavigateIframeToURL(
+      tab, "child-1", extension2->GetResourceURL("/blank_iframe.html"));
+  details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+  if (content::AreAllSitesIsolatedForTesting()) {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 3), Bucket(3, 1)));
+  } else if (extensions::IsIsolateExtensionsEnabled()) {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 4)));
+  } else {
+    EXPECT_THAT(details->uma()->GetAllSamples(
+                    "SiteIsolation.SiteInstancesPerBrowsingInstance"),
+                ElementsAre(Bucket(1, 2)));
+  }
+}

tools/metrics/histograms/histograms.xml

@@ -45048,6 +45048,14 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="SiteIsolation.SiteInstancesPerBrowsingInstance">
+  <owner>nasko@chromium.org</owner>
+  <summary>
+    The count of SiteInstances in a single BrowsingInstance. Recorded once per
+    UMA ping.
+  </summary>
+</histogram>
+
 <histogram name="SiteIsolation.XSD.DataLength" units="byte">
   <owner>creis@chromium.org</owner>
   <summary>