Introduce cross_site_iframe_factory.html, and use it in a test

chrome/browser/metrics/metrics_memory_details_browsertest.cc

Index: chrome/browser/metrics/metrics_memory_details_browsertest.cc
diff --git a/chrome/browser/metrics/metrics_memory_details_browsertest.cc b/chrome/browser/metrics/metrics_memory_details_browsertest.cc
index d38bfda6c6ea11c221fc75ab99747acb8e6bde9d..4272a92cd13812adb71ca17adb770aea68f13a5b 100644
--- a/chrome/browser/metrics/metrics_memory_details_browsertest.cc
+++ b/chrome/browser/metrics/metrics_memory_details_browsertest.cc
@@ -6,9 +6,22 @@
 
 #include "base/bind_helpers.h"
 #include "base/message_loop/message_loop.h"
+#include "base/path_service.h"
 #include "base/test/histogram_tester.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+using base::Bucket;
+using testing::ContainerEq;
+using testing::ElementsAre;
 
 namespace {
 
@@ -18,10 +31,15 @@ class TestMemoryDetails : public MetricsMemoryDetails {
       : MetricsMemoryDetails(base::Bind(&base::DoNothing), nullptr) {}
 
   void StartFetchAndWait() {
+    uma_.reset(new base::HistogramTester());
     StartFetch(FROM_CHROME_ONLY);
     content::RunMessageLoop();
   }
 
+  // Returns a HistogramTester which observed the most recent call to
+  // StartFetchAndWait().
+  base::HistogramTester* uma() { return uma_.get(); }
+
  private:
   ~TestMemoryDetails() override {}
 
@@ -31,6 +49,8 @@ class TestMemoryDetails : public MetricsMemoryDetails {
     base::MessageLoop::current()->Quit();
   }
 
+  scoped_ptr<base::HistogramTester> uma_;
+
   DISALLOW_COPY_AND_ASSIGN(TestMemoryDetails);
 };
 
@@ -41,20 +61,241 @@ class MetricsMemoryDetailsBrowserTest : public InProcessBrowserTest {
   MetricsMemoryDetailsBrowserTest() {}
   ~MetricsMemoryDetailsBrowserTest() override {}
 
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+
+    // Add content/test/data so we can use cross_site_iframe_factory.html
+    base::FilePath test_data_dir;
+    CHECK(PathService::Get(base::DIR_SOURCE_ROOT, &test_data_dir));
+    embedded_test_server()->ServeFilesFromDirectory(
+        test_data_dir.AppendASCII("content/test/data/"));
+    ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());

[dcheng, 2015/07/30 23:35:04]

Out of curiosity, why CHECK() above and ASSERT_TRUE() here?

[ncarter (slow), 2015/08/04 23:51:21]

Done.

+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(MetricsMemoryDetailsBrowserTest);
 };
 
 IN_PROC_BROWSER_TEST_F(MetricsMemoryDetailsBrowserTest, TestMemoryDetails) {
-  base::HistogramTester histogram_tester;
-
   scoped_refptr<TestMemoryDetails> details(new TestMemoryDetails);
   details->StartFetchAndWait();
 
   // Memory.Browser histogram should have a single non-0 sample recorded.
-  histogram_tester.ExpectTotalCount("Memory.Browser", 1);
+  details->uma()->ExpectTotalCount("Memory.Browser", 1);
   scoped_ptr<base::HistogramSamples> samples(
-      histogram_tester.GetHistogramSamplesSinceCreation("Memory.Browser"));
+      details->uma()->GetHistogramSamplesSinceCreation("Memory.Browser"));
   ASSERT_TRUE(samples);
   EXPECT_NE(0, samples->sum());
 }
+
+IN_PROC_BROWSER_TEST_F(MetricsMemoryDetailsBrowserTest, TestSiteIsolation) {
+  // Page with 14 nested oopifs across 9 sites (a.com through i.com).
+  // None of these are https.
+  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))");

[dcheng, 2015/07/30 23:35:04]

This is pretty neat, but the query string is a bit difficult for humans to read,
especially if it's deeply nested.

Out of curiosity, what do you think of a C++ builder approach? For example:

TestFrameBuilder()
  .addFrame("b", 
      TestFrameBuilder().addFrame("c").addFrame("d"))
  .addFrame("e").BuildURL()

might get you a page like:

,------------------------------.
| a.com                        |
|,-------------------.,-------.|
|| b.com             || e.com ||
||,-------. ,-------.|`-------'|
||| c.com | | d.com ||         |
||`-------'-'-------'|         |
|`-------------------'         |
`------------------------------'

BuildURL() can dump out base64 JSON or something silly like that, so the parser
logic in JS can be made a lot shorter.

[ncarter (slow), 2015/07/31 17:16:01]

I don't think the builder is necessary, at least not right away. Let me argue
both sides:

Argument against:
=================

For our existing test pages, you have to look at the HTML to know what the frame
tree structure actually is -- so I'd argue that having even a terse
representation of the structure in the URL is moving the ball forward forward.
With syntax highlighting, things actually look a little better than they do in
rietveld: the arguments inside the string literal don't blend together with the
surrounding c++ code so much. And when you're writing a test, it's pretty deeply
satisfying to be able to add a a new cross-site iframe with just two characters.

Without any additional infrastructure, you have the option to add indentation to
the string (since whitespace is ignored), like this:
"a.com(b.com(a.com(b.com,"
"                  c.com,"
"                  d.com,"
"                  e.com,"
"                  f.com,"
"                  g.com,"
"                  h.com)),
"      c.com,"
"      d.com,"
"      e.com,
"      i.com(f.com))"

However, my eyeballs prefer the compact form. If I want a visualization of the
structure, I can just load the page.

Argument for:
=============

Having said the above, I do see some upside to adding a builder. If we expand
the grammar at all so that it supports any kind of operator or additional
arguments (e.g. you want "iframe sandbox" or using title1.html as your leaf
node, or options for https, or "extension scheme"), I think a builder will start
to be be useful at that point.

Another advantage to having a C++ builder would be to provide a place where we
could ensure that the host resolver rule and the other test boilerplate has been
configured properly. I worry that such setup conventions (especially the host
resolver (and eventually, cert verifier) mucking) is an impediment to people
actually using these facilities from whichever test fixture.
The downside to a base64/json scheme is that it totally breaks crafting the test
page under file:// preview, which is actually a really nice feature of this. If
we isolated file origins (or had a mode to do that), this would be really useful
for local debugging.

The other reason I prefer what I've implemented, is that it's implemented
already :)

[dcheng, 2015/08/04 00:14:41]

I don't see myself using the ability to preview it on a file:// URL much, as I'd
expect it to Just Work =). That being said, I'm not a huge fan of the C++
builder stuff either: though I think it's much better in terms of readability,
it's too verbose.

[ncarter (slow), 2015/08/04 23:51:21]

If we wind up adding any kind of per-node options to this thing, the builder
will make sense. So I'm open to adding it in that case, but I'm pretty attached
to keeping the URLs human readable. The readable URLs help not just with local
preview, but also with running under a debugger and printf debugging.

+  ui_test_utils::NavigateToURL(browser(), abcdefghi_url);
+
+  // Get the metrics.
+  scoped_refptr<TestMemoryDetails> details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+
+  EXPECT_THAT(
+      details->uma()->GetAllSamples("SiteIsolation.BrowsingInstanceCount"),
+      ElementsAre(Bucket(1, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.CurrentRendererProcessCount"),
+              ElementsAre(Bucket(1, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountEstimate"),
+              ElementsAre(Bucket(9, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountLowerBound"),
+              ElementsAre(Bucket(9, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountNoLimit"),
+              ElementsAre(Bucket(9, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesTotalProcessCountEstimate"),
+              ElementsAre(Bucket(11, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountEstimate"),
+              ElementsAre(Bucket(1, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountLowerBound"),
+              ElementsAre(Bucket(1, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountNoLimit"),
+              ElementsAre(Bucket(1, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesTotalProcessCountEstimate"),
+              ElementsAre(Bucket(3, 1)));
+
+  // Navigate to a different, disjoint set of 7 sites.
+  GURL pqrstuv_url = embedded_test_server()->GetURL(
+      "p.com",
+      "/cross_site_iframe_factory.html?p(q(r),r(s),s(t),t(q),u(u),v(p))");
+  ui_test_utils::NavigateToURL(browser(), pqrstuv_url);
+
+  details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+
+  EXPECT_THAT(
+      details->uma()->GetAllSamples("SiteIsolation.BrowsingInstanceCount"),
+      ElementsAre(Bucket(1, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.CurrentRendererProcessCount"),
+              ElementsAre(Bucket(1, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountEstimate"),
+              ElementsAre(Bucket(7, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountLowerBound"),
+              ElementsAre(Bucket(7, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountNoLimit"),
+              ElementsAre(Bucket(7, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesTotalProcessCountEstimate"),
+              ElementsAre(Bucket(9, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountEstimate"),
+              ElementsAre(Bucket(1, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountLowerBound"),
+              ElementsAre(Bucket(1, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountNoLimit"),
+              ElementsAre(Bucket(1, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesTotalProcessCountEstimate"),
+              ElementsAre(Bucket(3, 1)));
+
+  // Open a second tab (different browsing instance) with 4 sites (a through d).
+  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();
+
+  EXPECT_THAT(
+      details->uma()->GetAllSamples("SiteIsolation.BrowsingInstanceCount"),
+      ElementsAre(Bucket(2, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.CurrentRendererProcessCount"),
+              ElementsAre(Bucket(2, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountEstimate"),
+              ElementsAre(Bucket(11, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountLowerBound"),
+              ElementsAre(Bucket(11, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountNoLimit"),
+              ElementsAre(Bucket(11, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesTotalProcessCountEstimate"),
+              ElementsAre(Bucket(13, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountEstimate"),
+              ElementsAre(Bucket(2, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountLowerBound"),
+              ElementsAre(Bucket(1, 1)));  // TODO(nick): This should be 2.
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountNoLimit"),
+              ElementsAre(Bucket(2, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesTotalProcessCountEstimate"),
+              ElementsAre(Bucket(4, 1)));
+
+  // Open a third tab (different browsing instance) with the same 4 sites.
+  AddTabAtIndex(2, abcd_url, ui::PAGE_TRANSITION_TYPED);
+
+  details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+
+  EXPECT_THAT(
+      details->uma()->GetAllSamples("SiteIsolation.BrowsingInstanceCount"),
+      ElementsAre(Bucket(3, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.CurrentRendererProcessCount"),
+              ElementsAre(Bucket(3, 1)));
+  // Could be 11 if subframe processes were reused across browsing instances.
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountEstimate"),
+              ElementsAre(Bucket(15, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountLowerBound"),
+              ElementsAre(Bucket(11, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountNoLimit"),
+              ElementsAre(Bucket(15, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesTotalProcessCountEstimate"),
+              ElementsAre(Bucket(17, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountEstimate"),
+              ElementsAre(Bucket(3, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountLowerBound"),
+              ElementsAre(Bucket(1, 1)));  // TODO(nick): This should be 3.
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountNoLimit"),
+              ElementsAre(Bucket(3, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesTotalProcessCountEstimate"),
+              ElementsAre(Bucket(5, 1)));
+
+  // From the third tab, window.open() a fourth tab in the same browsing
+  // instance, to a page using the same four sites "a-d" as third tab, plus an
+  // additional site "e". The estimated process counts should increase by one
+  // (not five) from the previous scenario, as the new tab can reuse the four
+  // processes already in the BrowsingInstance.
+  GURL dcbae_url = embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?d(c(b(a(e))))");
+  ui_test_utils::UrlLoadObserver load_complete(
+      dcbae_url, content::NotificationService::AllSources());
+  ASSERT_EQ(3, browser()->tab_strip_model()->count());
+  ASSERT_TRUE(content::ExecuteScript(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      "window.open('" + dcbae_url.spec() + "');"));
+  ASSERT_EQ(4, browser()->tab_strip_model()->count());
+  load_complete.Wait();
+
+  details = new TestMemoryDetails();
+  details->StartFetchAndWait();
+
+  EXPECT_THAT(
+      details->uma()->GetAllSamples("SiteIsolation.BrowsingInstanceCount"),
+      ElementsAre(Bucket(3, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.CurrentRendererProcessCount"),
+              ElementsAre(Bucket(3, 1)));
+  // Could be 11 if subframe processes were reused across browsing instances.
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountEstimate"),
+              ElementsAre(Bucket(16, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountLowerBound"),
+              ElementsAre(Bucket(12, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesProcessCountNoLimit"),
+              ElementsAre(Bucket(16, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateAllSitesTotalProcessCountEstimate"),
+              ElementsAre(Bucket(18, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountEstimate"),
+              ElementsAre(Bucket(3, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountLowerBound"),
+              ElementsAre(Bucket(1, 1)));  // TODO(nick): This should be 3.
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesProcessCountNoLimit"),
+              ElementsAre(Bucket(3, 1)));
+  EXPECT_THAT(details->uma()->GetAllSamples(
+                  "SiteIsolation.IsolateHttpsSitesTotalProcessCountEstimate"),
+              ElementsAre(Bucket(5, 1)));
+}