Add cookie prefix metrics

Add cookie prefix metrics

Measure the number of naturally occurring cookies that begin with
$Secure- or $Host- so that we know if cookie prefixes will break real
sites.

BUG=557083
Committed: https://crrev.com/23f41ce59f05597c1797b935f607635420fea97e
Cr-Commit-Position: refs/heads/master@{#360969}

[estark, 2015-11-20 00:24:05]

estark@chromium.org changed reviewers:
+ mkwst@chromium.org

[estark, 2015-11-20 00:24:06]

mkwst, PTAL

[Mike West, 2015-11-20 05:24:56]

LGTM % a nit and a suggestion.

https://codereview.chromium.org/1455693007/diff/40001/net/cookies/canonical_c...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/1455693007/diff/40001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:255: prefix !=
CanonicalCookie::COOKIE_PREFIX_NONE &&
Instead of doing this check here, perhaps you could fold it into
`IsCookiePrefixValid`? If that method simply returned `true` for NONE, I think
this would be a bit simpler.

https://codereview.chromium.org/1455693007/diff/40001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:491: return parsed_cookie.IsSecure() &&
url.SchemeIsCryptographic();
It would be nice to measure not only how many `$Secure-` and `$Host-` cookies we
see in the wild, but also what percentage of those cookies we'd block under
these rules. I suspect it's pretty much all of them, but maybe it's none of
them?

Basically, that's the question I think we'd ask ourselves in a month when we
have some data. It would be nice to gather it now.

[estark, 2015-11-20 14:37:45]

estark@chromium.org changed reviewers:
+ mpearson@chromium.org

[estark, 2015-11-20 14:37:46]

Thanks Mike.

mpearson, could you please review histograms.xml?

https://codereview.chromium.org/1455693007/diff/40001/net/cookies/canonical_c...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/1455693007/diff/40001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:255: prefix !=
CanonicalCookie::COOKIE_PREFIX_NONE &&
On 2015/11/20 05:24:55, Mike West wrote:
> Instead of doing this check here, perhaps you could fold it into
> `IsCookiePrefixValid`? If that method simply returned `true` for NONE, I think
> this would be a bit simpler.

Done.

https://codereview.chromium.org/1455693007/diff/40001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:491: return parsed_cookie.IsSecure() &&
url.SchemeIsCryptographic();
On 2015/11/20 05:24:55, Mike West wrote:
> It would be nice to measure not only how many `$Secure-` and `$Host-` cookies
we
> see in the wild, but also what percentage of those cookies we'd block under
> these rules. I suspect it's pretty much all of them, but maybe it's none of
> them?
> 
> Basically, that's the question I think we'd ask ourselves in a month when we
> have some data. It would be nice to gather it now.

Done.

[Mike West, 2015-11-20 14:43:58]

Still LGTM, even though it's 6:30 in the morning and I can't believe you're
awake and writing sane code.

https://codereview.chromium.org/1455693007/diff/60001/net/cookies/canonical_c...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/1455693007/diff/60001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:484: CanonicalCookie::COOKIE_PREFIX_LAST);
It might be easier to compare numbers if these were all in one histogram. That
is, "Of all the cookies we saw, X had this prefix and would have been blocked. Y
had this prefix and would have been allowed. Z had no prefix."

I'm totally fine with gathering that information out of two histograms; it's
easy, just marginally more annoying than having all the data in one place.

[Mark P, 2015-11-20 17:58:53]

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5236: +   
https://tools.ietf.org/html/draft-west-cookie-prefixes.
All histograms should clearly say when they are emitted.

[estark, 2015-11-20 18:19:32]

https://codereview.chromium.org/1455693007/diff/60001/net/cookies/canonical_c...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/1455693007/diff/60001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:484: CanonicalCookie::COOKIE_PREFIX_LAST);
On 2015/11/20 14:43:58, Mike West wrote:
> It might be easier to compare numbers if these were all in one histogram. That
> is, "Of all the cookies we saw, X had this prefix and would have been blocked.
Y
> had this prefix and would have been allowed. Z had no prefix."
> 
> I'm totally fine with gathering that information out of two histograms; it's
> easy, just marginally more annoying than having all the data in one place.

It might be because I started trying to write code at 6am but doing it in the
same histogram seemed like it led to slightly uglier code, so I decided to go
for cleaner code at the expense of slightly more annoying histogram.

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5236: +   
https://tools.ietf.org/html/draft-west-cookie-prefixes.
On 2015/11/20 17:58:53, Mark P wrote:
> All histograms should clearly say when they are emitted.

Done, PTAL.

[Mike West, 2015-11-20 18:20:46]

On 2015/11/20 at 18:19:32, estark wrote:
>
https://codereview.chromium.org/1455693007/diff/60001/net/cookies/canonical_c...
> net/cookies/canonical_cookie.cc:484: CanonicalCookie::COOKIE_PREFIX_LAST);
> On 2015/11/20 14:43:58, Mike West wrote:
> > It might be easier to compare numbers if these were all in one histogram.
That
> > is, "Of all the cookies we saw, X had this prefix and would have been
blocked. Y
> > had this prefix and would have been allowed. Z had no prefix."
> > 
> > I'm totally fine with gathering that information out of two histograms; it's
> > easy, just marginally more annoying than having all the data in one place.
> 
> It might be because I started trying to write code at 6am but doing it in the
same histogram seemed like it led to slightly uglier code, so I decided to go
for cleaner code at the expense of slightly more annoying histogram.

I trust you. Land it once Mark is happy. :)

[Mark P, 2015-11-20 19:01:42]

the two comments below say the same thing.  I wrote one, then it didn't appear
when I hit publish, so I (sigh) wrote it again, then hit publish and now they're
both there.  Oh well.

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5236: +   
https://tools.ietf.org/html/draft-west-cookie-prefixes.
On 2015/11/20 18:19:32, estark wrote:
> On 2015/11/20 17:58:53, Mark P wrote:
> > All histograms should clearly say when they are emitted.
> 
> Done, PTAL.

Not done in the latest patchset.

For example, is this recorded every time a cookie is read?  Every time the
cookie is updated?  The first time a cookie is created?  When a cookie is sent
to a server?  Once per pageload, regardless of how many times the cookie was
updated on the pageload?  etc.

ditto in the histogram below

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5236: +   
https://tools.ietf.org/html/draft-west-cookie-prefixes.
On 2015/11/20 18:19:32, estark wrote:
> On 2015/11/20 17:58:53, Mark P wrote:
> > All histograms should clearly say when they are emitted.
> 
> Done, PTAL.

Not done in the latest patchset.

You need to say when the histogram is emitted.  For instance, is it emitted
every time the cookie is read?  Every time it is updated?  When it is first
created?  Every time it is sent to a server?  Once per pageload in which the
cookie is touched, regardless of how many times the cookie is touched on that
pageload?  etc.

ditto in the histogram below

[estark, 2015-11-20 19:18:28]

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5236: +   
https://tools.ietf.org/html/draft-west-cookie-prefixes.
On 2015/11/20 19:01:42, Mark P wrote:
> On 2015/11/20 18:19:32, estark wrote:
> > On 2015/11/20 17:58:53, Mark P wrote:
> > > All histograms should clearly say when they are emitted.
> > 
> > Done, PTAL.
> 
> Not done in the latest patchset.
> 
> You need to say when the histogram is emitted.  For instance, is it emitted
> every time the cookie is read?  Every time it is updated?  When it is first
> created?  Every time it is sent to a server?  Once per pageload in which the
> cookie is touched, regardless of how many times the cookie is touched on that
> pageload?  etc.
> 
> ditto in the histogram below

Sorry but I'm having trouble understanding what isn't clear in what I wrote in
patch set #5: "Number of times a cookie is set..." To me that seems to describe
exactly when the histogram is emitted: whenever we set a cookie with the
described properties, we emit the histogram. Would either of the following
options be more clear to you?

- "Number of times a cookie is set in the cookie store..."
- "Number of times a cookie is set in the cookie store via a Set-Cookie header
or setting document.cookie..."

[Mark P, 2015-11-20 20:13:43]

histograms.xml lgtm

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1455693007/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5236: +   
https://tools.ietf.org/html/draft-west-cookie-prefixes.
On 2015/11/20 19:18:27, estark wrote:
> On 2015/11/20 19:01:42, Mark P wrote:
> > On 2015/11/20 18:19:32, estark wrote:
> > > On 2015/11/20 17:58:53, Mark P wrote:
> > > > All histograms should clearly say when they are emitted.
> > > 
> > > Done, PTAL.
> > 
> > Not done in the latest patchset.
> > 
> > You need to say when the histogram is emitted.  For instance, is it emitted
> > every time the cookie is read?  Every time it is updated?  When it is first
> > created?  Every time it is sent to a server?  Once per pageload in which the
> > cookie is touched, regardless of how many times the cookie is touched on
that
> > pageload?  etc.
> > 
> > ditto in the histogram below
> 
> Sorry but I'm having trouble understanding what isn't clear in what I wrote in
> patch set #5: "Number of times a cookie is set..." To me that seems to
describe
> exactly when the histogram is emitted: whenever we set a cookie with the
> described properties, we emit the histogram. Would either of the following
> options be more clear to you?
> 
> - "Number of times a cookie is set in the cookie store..."
> - "Number of times a cookie is set in the cookie store via a Set-Cookie header
> or setting document.cookie..."

Oops, somehow I missed the "is set" section.

[estark, 2015-11-20 21:24:27]

Thanks!

[estark, 2015-11-20 21:25:27]

The CQ bit was checked by estark@chromium.org

[estark, 2015-11-20 21:25:28]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1455693007/#ps80001
(title: "mpearson comment")

[commit-bot: I haz the power, 2015-11-20 21:26:54]

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

[commit-bot: I haz the power, 2015-11-20 21:30:19]

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

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

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[estark, 2015-11-20 21:36:54]

The CQ bit was checked by estark@chromium.org

[estark, 2015-11-20 21:36:54]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1455693007/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-20 21:37:08]

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

[commit-bot: I haz the power, 2015-11-21 00:59:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-21 01:00:01]

Patchset 6 (id:??) landed as
https://crrev.com/23f41ce59f05597c1797b935f607635420fea97e
Cr-Commit-Position: refs/heads/master@{#360969}