Fix UMA stat for expiration of certificate memory decisions

Fix UMA stat for expiration of certificate memory decisions

In our certificate memory decisions experiment, an UMA stat has been
incorrectly recording when a decision is made after expiration for users
in the "forget decisions at session end" group. As a fix, this CL adds a
GUID to represent the current browsing session to the
ChromeSSLHostStateDelegate and records that identifier whenever a
decision is made. Then, in a future session, Chrome can tell if the
decision has expired if the GUID doesn't match the current session's
GUID.

A good question to ask might be, "why use a GUID to represent the
browsing session rather than simply iterating over all the current
entries and marking them as expired?" Well, I'm glad you asked!
Unfortunately, content settings does not allow one to iterate over the
values of *compound* content settings; it only works for simply content
settings, which certificate decisions most certainly are not. While this
could be added, it would be a fair amount work for what amounts to a
temporary problem, as this will all go away once the experiment is
finished.

BUG=418631
Committed: https://crrev.com/a040213b7020119ffe8f434ad948fe6adb37f1dc
Cr-Commit-Position: refs/heads/master@{#297671}

[jww, 2014-09-27 00:53:47]

jww@chromium.org changed reviewers:
+ palmer@chromium.org

[jww, 2014-09-27 00:53:48]

This is a minor fix for an incorrectly counted UMA stat in the cert memory
experiment.

[jww, 2014-09-27 02:09:29]

jww@chromium.org changed reviewers:
+ felt@chromium.org
- palmer@chromium.org

[jww, 2014-09-27 02:10:31]

On 2014/09/27 00:53:48, jww wrote:
> This is a minor fix for an incorrectly counted UMA stat in the cert memory
> experiment.

On second thought, I think felt@ makes more sense as a reviewer as she's already
looked at this code. palmer@, if you magnanimous and making felt@'s job a bit
easier, I'm happy to have your review instead :-)

[felt, 2014-09-30 01:50:59]

I don't entirely understand. Can you explain why the old version is incorrect?

https://codereview.chromium.org/610063002/diff/40001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right):

https://codereview.chromium.org/610063002/diff/40001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_delegate.h:139: //
FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END groups.
can you associate this TODO with a crbug? either 418631 (and keep it open) or a
new bug?

[jww, 2014-09-30 17:51:52]

On 2014/09/30 01:50:59, felt wrote:
> I don't entirely understand. Can you explain why the old version is incorrect?
Hm, sorry that it's not clear. In the old version, every time the browser
restarts for users in the FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END group,
it clears all user certificate decisions. While this provides correct *behavior*
for the users, it means that we can't tell when a user makes a new decision on a
certificate that they previously had made a decision on (because we've already
cleared their previous decisions, so there's no record of their old decisions).
This CL adds

Any thoughts on how I can make this clearer in the description or comments?
> 
>
https://codereview.chromium.org/610063002/diff/40001/chrome/browser/ssl/chrom...
> File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right):
> 
>
https://codereview.chromium.org/610063002/diff/40001/chrome/browser/ssl/chrom...
> chrome/browser/ssl/chrome_ssl_host_state_delegate.h:139: //
> FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END groups.
> can you associate this TODO with a crbug? either 418631 (and keep it open) or
a
> new bug?

I'll add a reference to 418631 in the comments, but why do we have to leave it
open? Can't we mark it as 'fixed' once this is committed? It should still be
accessible after that, right?

[jww, 2014-09-30 17:54:37]

On 2014/09/30 17:51:52, jww wrote:
> On 2014/09/30 01:50:59, felt wrote:
> > I don't entirely understand. Can you explain why the old version is
incorrect?
> Hm, sorry that it's not clear. In the old version, every time the browser
> restarts for users in the FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END group,
> it clears all user certificate decisions. While this provides correct
*behavior*
> for the users, it means that we can't tell when a user makes a new decision on
a
> certificate that they previously had made a decision on (because we've already
> cleared their previous decisions, so there's no record of their old
decisions).
> This CL adds
Yay for incomplete sentences :-P Just to finish:

This CL changes it so that when the browser restarts, we don't clear the
decisions even for users in the FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END
group. Instead, Chrome just marks what session the decision was made in with a
GUID. Then, when the session restarts, it will be a different GUID, so Chrome
now knows that the session has changed and the decision should be ignored.
However, it also means that we know that a previous decision was made, so we now
have that additional information for the UMA stats.
> 
> Any thoughts on how I can make this clearer in the description or comments?
> > 
> >
>
https://codereview.chromium.org/610063002/diff/40001/chrome/browser/ssl/chrom...
> > File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right):
> > 
> >
>
https://codereview.chromium.org/610063002/diff/40001/chrome/browser/ssl/chrom...
> > chrome/browser/ssl/chrome_ssl_host_state_delegate.h:139: //
> > FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END groups.
> > can you associate this TODO with a crbug? either 418631 (and keep it open)
or
> a
> > new bug?
> 
> I'll add a reference to 418631 in the comments, but why do we have to leave it
> open? Can't we mark it as 'fixed' once this is committed? It should still be
> accessible after that, right?

[felt, 2014-10-01 02:24:08]

On 2014/09/30 17:51:52, jww wrote:
> On 2014/09/30 01:50:59, felt wrote:
> > I don't entirely understand. Can you explain why the old version is
incorrect?
> Hm, sorry that it's not clear. In the old version, every time the browser
> restarts for users in the FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END group,
> it clears all user certificate decisions. While this provides correct
*behavior*
> for the users, it means that we can't tell when a user makes a new decision on
a
> certificate that they previously had made a decision on (because we've already
> cleared their previous decisions, so there's no record of their old
decisions).
> This CL adds
> 
> Any thoughts on how I can make this clearer in the description or comments?
> > 
> >
>
https://codereview.chromium.org/610063002/diff/40001/chrome/browser/ssl/chrom...
> > File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right):
> > 
> >
>
https://codereview.chromium.org/610063002/diff/40001/chrome/browser/ssl/chrom...
> > chrome/browser/ssl/chrome_ssl_host_state_delegate.h:139: //
> > FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END groups.
> > can you associate this TODO with a crbug? either 418631 (and keep it open)
or
> a
> > new bug?
> 
> I'll add a reference to 418631 in the comments, but why do we have to leave it
> open? Can't we mark it as 'fixed' once this is committed? It should still be
> accessible after that, right?

Open bug because people don't remember to fix closed bugs :)

[felt, 2014-10-01 02:42:36]

https://codereview.chromium.org/610063002/diff/60001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right):

https://codereview.chromium.org/610063002/diff/60001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:260:
current_expiration_guid_(base::GenerateGUID()) {
This comment is not actionable, it's just a note for posterity:

ChromeSSLHostStateDelegateFactory is a singleton that matches one
ChromeSSLHostStateDelegate to each profile. That means it's fine to generate
base::GenerateGUID() here; it will only be done once per session, since
ChromeSSLHostStateDelegate will only be instantiated once per session per
profile.

https://codereview.chromium.org/610063002/diff/60001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right):

https://codereview.chromium.org/610063002/diff/60001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:245: // The
certificate has never been seen before, so it should be UNKONWN and
nit: UNKNOWN misspelled

https://codereview.chromium.org/610063002/diff/60001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:267: // decisions
after restart, we wait until after a restart to see if the decision
nit: Sleevi has instructed me to never put "we" in comments. He has a point:
"we" is ambiguous. Can you rephrase?

[jww, 2014-10-01 16:33:20]

https://codereview.chromium.org/610063002/diff/60001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right):

https://codereview.chromium.org/610063002/diff/60001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:245: // The
certificate has never been seen before, so it should be UNKONWN and
On 2014/10/01 02:42:36, felt wrote:
> nit: UNKNOWN misspelled

Done.

https://codereview.chromium.org/610063002/diff/60001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:267: // decisions
after restart, we wait until after a restart to see if the decision
On 2014/10/01 02:42:36, felt wrote:
> nit: Sleevi has instructed me to never put "we" in comments. He has a point:
> "we" is ambiguous. Can you rephrase?

Done.

[felt, 2014-10-01 16:43:08]

lgtm

[jww, 2014-10-01 16:46:55]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-10-01 16:49:40]

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

[commit-bot: I haz the power, 2014-10-01 17:38:22]

Committed patchset #5 (id:80001) as b63886b3d08d04e2019e87f7f5dd9531f2833d6d

[commit-bot: I haz the power, 2014-10-01 17:39:08]

Patchset 5 (id:??) landed as
https://crrev.com/a040213b7020119ffe8f434ad948fe6adb37f1dc
Cr-Commit-Position: refs/heads/master@{#297671}