Add SiteIsolation.IsolateExtensions metrics and tests.

Add SiteIsolation.IsolateExtensions metrics and tests.

These new UMA metrics estimate the process-count impact of a policy
wherein extension content and web content never share processes,
even if one contains the other in an iframe.

SiteDetailsBrowserTest: add tests for the new metrics.

WebContentsImpl: Fix a UMA metrics counting error where
<iframe src="about:blank"> was counted as a distinct site.
Now we'll only do that if we see "about:blank" in an
undifferentiated SiteInstance.

BUG=528049
Committed: https://crrev.com/37dfd9de7667e9d177edc27e208359cd0020fb26
Cr-Commit-Position: refs/heads/master@{#348690}

[ncarter (slow), 2015-09-04 19:32:12]

nick@chromium.org changed reviewers:
+ nasko@chromium.org

[ncarter (slow), 2015-09-04 19:32:12]

nasko, please review

[nasko, 2015-09-04 22:19:44]

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details.cc:94: for (IsolationScenario& scenario :
site_data->scenarios) {
nit: no need for {}

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details.cc:125: // isolating only HTTPS sites, and when
isolating
This sentence doesn't seem complete. Did you miss "extensions"?

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details.h (right):

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details.h:54: // Maps a BrowserContext to information about
the sites it contains.
Why not SiteInstances?

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:89: const Extension*
CreateExtension(const std::string& name,
Why create extension by writing it to the filesystem instead of loading one from
chrome/test directory?

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:496: // Navigate Tab2 to a top-level
page from extension3.
It is useful to state verbally what we expect to change and why, like you do in
the other cases so far.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:514: // Add a web iframe to the
chrome-extension:// page. This should create a new
You aren't really adding it to a page, rather navigating to an extensions page
with web iframe.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:541: 
No need for empty line, to stay consistent with code above.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:562: 
No need for empty line, to stay consistent with code above.

https://codereview.chromium.org/1313863006/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1313863006/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:174: // is unassigned. Do not
not otherwise depend on the siteinstance's site URL,
nit: double not, SiteInstance

https://codereview.chromium.org/1313863006/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:175: // since it's value
reflects the current process model, and this function
nit: its

[ncarter (slow), 2015-09-10 19:08:59]

ptal

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details.cc:94: for (IsolationScenario& scenario :
site_data->scenarios) {
On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> nit: no need for {}

The style guide allows both ways, right? I usually prefer the {'s for loops,
especially like here when we're at the end of an outer block.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details.cc:125: // isolating only HTTPS sites, and when
isolating
On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> This sentence doesn't seem complete. Did you miss "extensions"?

Done.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details.h (right):

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details.h:54: // Maps a BrowserContext to information about
the sites it contains.
On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> Why not SiteInstances?

Because SiteInstances reflect the active process model, and we're tracking stats
for alternative process models.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:89: const Extension*
CreateExtension(const std::string& name,
On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> Why create extension by writing it to the filesystem instead of loading one
from
> chrome/test directory?

First off: I'm using an established practice -- dynamic extensions like this are
not uncommon, and classes like TestExtensionDir and DictionaryBuilder exist
precisely to support that pattern.

It's kind of a long story how I got here, but since you asked: Originally I
wanted to use existing extensions, but all the existing ones that had iframes
were in the "apitest" test data directory, and referencing them from a
non-apitest seemed like it would be a kind of layering violation, in that people
editing the original apitests might be surprised to see random distant tests
depending on their test data.

Then, Devlin pointed me at
chrome/browser/extensions/active_script_controller_browsertest.cc as a model to
follow, and that used this model, so I built on top of it. It proved to be a
good idea, because doing it this way turned out to have two practical benefits I
didn't anticipate:

1. We are able to embed the iframe url in http_iframe.html directly in the
extension content (line 113 below) -- which was fortuitous, because it turns out
that you can't do ExecuteJavascript from an extension iframe by default, unless
you relax the CSP in the manifest (
https://developer.chrome.com/extensions/contentSecurityPolicy ).  And because
ExecuteJavascript doesn't work, NavigateIframeToURL doesn't work either, so it
winds up being pretty tough to point extension content at the testserver port,
so that you can get web content. So I could have made it work with a static
extension, but there would have been many extra hoops there -- I probably would
have had to use postMessage to deliver the URL, and manually mess around with
test navigation observers.

2. Another thing that would have required extra work is the fact that I needed
to load three extensions. Since extension1 and extension2 have different
behavior that probably would have required 2 manifests and 2 extension
directories -- fine. But I also, I think, would have had to create a copy for
extension3, since just loading extension2 the second time would generate the
same extension id (the ID of an unpacked extension is determined by the path of
the directory, per https://developer.chrome.com/extensions/overview ). When I'm
creating the directory dynamically, I get fresh IDs even when reloading, making
it trivial to add "extension4" if I needed it.

I'm also fond of this approach because it makes it easier to see at a glance the
differences between extension1 and extension2, and why they're different. You
can't add comments to an on-disk manifest file to clarify that. Here it ought to
be pretty clear that has_background_process controls the differences between the
behaviors.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:496: // Navigate Tab2 to a top-level
page from extension3.
On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> It is useful to state verbally what we expect to change and why, like you do
in
> the other cases so far.

Done.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:514: // Add a web iframe to the
chrome-extension:// page. This should create a new
On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> You aren't really adding it to a page, rather navigating to an extensions page
> with web iframe.

Done.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:541: 
On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> No need for empty line, to stay consistent with code above.

Done.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:562: 
On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> No need for empty line, to stay consistent with code above.

Done.

https://codereview.chromium.org/1313863006/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1313863006/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:174: // is unassigned. Do not
not otherwise depend on the siteinstance's site URL,
On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> nit: double not, SiteInstance

Done.

https://codereview.chromium.org/1313863006/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:175: // since it's value
reflects the current process model, and this function
On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> nit: its

Done.

[nasko, 2015-09-11 18:14:11]

LGTM

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details.cc:94: for (IsolationScenario& scenario :
site_data->scenarios) {
On 2015/09/10 19:08:58, ncarter wrote:
> On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> > nit: no need for {}
> 
> The style guide allows both ways, right? I usually prefer the {'s for loops,
> especially like here when we're at the end of an outer block.

Acknowledged.

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1313863006/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:89: const Extension*
CreateExtension(const std::string& name,
On 2015/09/10 19:08:59, ncarter wrote:
> On 2015/09/04 22:19:43, nasko (out until Sept 11) wrote:
> > Why create extension by writing it to the filesystem instead of loading one
> from
> > chrome/test directory?
> 
> First off: I'm using an established practice -- dynamic extensions like this
are
> not uncommon, and classes like TestExtensionDir and DictionaryBuilder exist
> precisely to support that pattern.
> 
> It's kind of a long story how I got here, but since you asked: Originally I
> wanted to use existing extensions, but all the existing ones that had iframes
> were in the "apitest" test data directory, and referencing them from a
> non-apitest seemed like it would be a kind of layering violation, in that
people
> editing the original apitests might be surprised to see random distant tests
> depending on their test data.
> 
> Then, Devlin pointed me at
> chrome/browser/extensions/active_script_controller_browsertest.cc as a model
to
> follow, and that used this model, so I built on top of it. It proved to be a
> good idea, because doing it this way turned out to have two practical benefits
I
> didn't anticipate:
> 
> 1. We are able to embed the iframe url in http_iframe.html directly in the
> extension content (line 113 below) -- which was fortuitous, because it turns
out
> that you can't do ExecuteJavascript from an extension iframe by default,
unless
> you relax the CSP in the manifest (
> https://developer.chrome.com/extensions/contentSecurityPolicy ).  And because
> ExecuteJavascript doesn't work, NavigateIframeToURL doesn't work either, so it
> winds up being pretty tough to point extension content at the testserver port,
> so that you can get web content. So I could have made it work with a static
> extension, but there would have been many extra hoops there -- I probably
would
> have had to use postMessage to deliver the URL, and manually mess around with
> test navigation observers.
> 
> 2. Another thing that would have required extra work is the fact that I needed
> to load three extensions. Since extension1 and extension2 have different
> behavior that probably would have required 2 manifests and 2 extension
> directories -- fine. But I also, I think, would have had to create a copy for
> extension3, since just loading extension2 the second time would generate the
> same extension id (the ID of an unpacked extension is determined by the path
of
> the directory, per https://developer.chrome.com/extensions/overview ). When
I'm
> creating the directory dynamically, I get fresh IDs even when reloading,
making
> it trivial to add "extension4" if I needed it.
> 
> I'm also fond of this approach because it makes it easier to see at a glance
the
> differences between extension1 and extension2, and why they're different. You
> can't add comments to an on-disk manifest file to clarify that. Here it ought
to
> be pretty clear that has_background_process controls the differences between
the
> behaviors.
> 

Thanks for the detailed answer!

[ncarter (slow), 2015-09-11 18:15:54]

nick@chromium.org changed reviewers:
+ jam@chromium.org

[ncarter (slow), 2015-09-11 18:15:54]

jam: approval needed

[ncarter (slow), 2015-09-11 18:16:17]

nick@chromium.org changed reviewers:
+ isherman@chromium.org

[ncarter (slow), 2015-09-11 18:16:18]

+isherman for histograms.xml

[Ilya Sherman, 2015-09-11 20:08:23]

histograms lgtm

https://codereview.chromium.org/1313863006/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1313863006/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:43374: +<histogram
name="SiteIsolation.IsolateExtensionsProcessCountEstimate">
nit: Maybe name this "UpperBound", to match "LowerBound" below?

[ncarter (slow), 2015-09-11 20:11:09]

https://codereview.chromium.org/1313863006/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1313863006/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:43374: +<histogram
name="SiteIsolation.IsolateExtensionsProcessCountEstimate">
On 2015/09/11 20:08:23, Ilya Sherman wrote:
> nit: Maybe name this "UpperBound", to match "LowerBound" below?

This is named Estimate to match the naming scheme of the other histograms in the
SiteIsolation category (e.g. SiteIsolation.IsolateHttpsSitesProcessCountEstimate
and others above/below this diff hunk).

[jam, 2015-09-11 20:56:29]

lgtm

[ncarter (slow), 2015-09-11 21:18:44]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2015-09-11 21:20:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313863006/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313863006/140001

[commit-bot: I haz the power, 2015-09-11 21:48:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-11 21:48:25]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ncarter (slow), 2015-09-14 18:18:13]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2015-09-14 18:19:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313863006/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313863006/140001

[commit-bot: I haz the power, 2015-09-14 19:54:11]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-14 19:54:53]

Patchset 8 (id:??) landed as
https://crrev.com/37dfd9de7667e9d177edc27e208359cd0020fb26
Cr-Commit-Position: refs/heads/master@{#348690}

[commit-bot: I haz the power, 2015-09-23 12:36:04]

Patchset 8 (id:??) landed as
https://crrev.com/37dfd9de7667e9d177edc27e208359cd0020fb26
Cr-Commit-Position: refs/heads/master@{#348690}