Remember user decisions on invalid certificates behind a flag

Remember user decisions on invalid certificates behind a flag

Currently if a user clicks thorough an invalid certificate, the decision is
remembered for the rest of the session, but after session restart, the decision
is forgotten. This implements a flag that remembers the decision for a specified
length of time across session restarts. Users can specific in the flag how long
decisions should be remembered, or they can specify that it should still be
reset on restart.

BUG=262615
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287118

[jww, 2014-07-03 05:25:34]

This is the implementation of invalid certificate memory being time based, but
put behind a flag. The experiment will come later, after we have the revoke
button committed. felt@ and rsleevi, you seem like the most logical initial
reviewers, so please let me know what you think.

[felt, 2014-07-07 22:26:39]

https://codereview.chromium.org/369703002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/369703002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:14360: +      Remeber for three days
*Remember

https://codereview.chromium.org/369703002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:14541: +      Remember across session
restart, for the specificed length of time, user decisions to proceed through
SSL error interstitials.
I can't brain-parse that description string

https://codereview.chromium.org/369703002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:499: switches::kRememberCertErrorDecisions,
FYI: FieldTrials doesn't currently support flag=value flags, so you'll need to
use a different flag for each one when you set up a Field Trial.

https://codereview.chromium.org/369703002/diff/1/chrome/browser/browsing_data...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/browsing_data...
chrome/browser/browsing_data/browsing_data_remover.cc:409:
profile_->GetSSLHostStateDecisions()->Clear();
when this is called in Incognito, I assume this just clears the Incognito
decisions, not also the decisions from the parent profile that spawned the
Incognito profile?

https://codereview.chromium.org/369703002/diff/1/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_impl.cc:386: return
ChromeSSLHostStateDecisionsFactory::GetForProfile(this);
how do these changes play with incognito? I assume any saved state will last (at
max) the length of the incognito session?

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:26: // "old" style of
certificate decision memory.
Might you want to use -1 instead of 0 for the "default"/old case? One could
imagine that a policy of 0 should mean it expires at the end of the current day.

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:38: if
(!base::StringToInt64(switch_value, &expiration_delta)) {
should this be a DCHECK?

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:72: // the decisions are
expired, a new dictionary will be created
why do you need to support both cases for |create_entries|?

https://codereview.chromium.org/369703002/diff/1/content/browser/ssl/ssl_bloc...
File content/browser/ssl/ssl_blocking_proceed_decision_store.cc (right):

https://codereview.chromium.org/369703002/diff/1/content/browser/ssl/ssl_bloc...
content/browser/ssl/ssl_blocking_proceed_decision_store.cc:11: void
SSLBlockingProceedDecisionStore::UserDidProceed() {
what's going on here?

[jww, 2014-07-08 17:35:42]

https://codereview.chromium.org/369703002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/369703002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:14360: +      Remeber for three days
On 2014/07/07 22:26:38, felt wrote:
> *Remember

Done.

https://codereview.chromium.org/369703002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:14540: +    <message
name="IDS_FLAGS_REMEMBER_CERTIFICATE_ERROR_DECISIONS_DESCRIPTION"
desc="Description of the remeber certificate error decisions lab">
Same mistake. Also fixed.

https://codereview.chromium.org/369703002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:14541: +      Remember across session
restart, for the specificed length of time, user decisions to proceed through
SSL error interstitials.
On 2014/07/07 22:26:38, felt wrote:
> I can't brain-parse that description string

Hopefully simplified.

https://codereview.chromium.org/369703002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:499: switches::kRememberCertErrorDecisions,
On 2014/07/07 22:26:39, felt wrote:
> FYI: FieldTrials doesn't currently support flag=value flags, so you'll need to
> use a different flag for each one when you set up a Field Trial.

Isn't it simpler than that? In my previous experiment, I didn't even use flags
in the field trial portion, and I plan on the same basic implementation here.
Basically, have a field trial bucket for each flag, and check if the user is in
the bucket *or* if they've overriden the choice with a flag.

https://codereview.chromium.org/369703002/diff/1/chrome/browser/browsing_data...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/browsing_data...
chrome/browser/browsing_data/browsing_data_remover.cc:409:
profile_->GetSSLHostStateDecisions()->Clear();
On 2014/07/07 22:26:39, felt wrote:
> when this is called in Incognito, I assume this just clears the Incognito
> decisions, not also the decisions from the parent profile that spawned the
> Incognito profile?

Correct. I've updated the browser tests in
chrome_ssl_host_state_decisions_test.cc to verify this.

https://codereview.chromium.org/369703002/diff/1/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_impl.cc:386: return
ChromeSSLHostStateDecisionsFactory::GetForProfile(this);
On 2014/07/07 22:26:39, felt wrote:
> how do these changes play with incognito? I assume any saved state will last
(at
> max) the length of the incognito session?

Right. Incognito's behavior effectively remains unchanged. If you start an
inconginto session, it is populated with the cert decisions from it's parent
profile (which may be remembered from an earlier session), but any future
decisions are unique to the incognito profile.

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:26: // "old" style of
certificate decision memory.
On 2014/07/07 22:26:39, felt wrote:
> Might you want to use -1 instead of 0 for the "default"/old case? One could
> imagine that a policy of 0 should mean it expires at the end of the current
day.

I'm not sure I understand. The number is in seconds, so my impression would be
that, if anything, "0 seconds" would mean "no memory at all," it seems to make
more sense to me to repurpose it as "don't remember after the session ends."

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:38: if
(!base::StringToInt64(switch_value, &expiration_delta)) {
On 2014/07/07 22:26:39, felt wrote:
> should this be a DCHECK?

I don't think so. A user can manually specify a command line flag that is
invalid (e.g. has letters in it), and I don't think Chrome should fail on an
assert, even in Debug. Also, this is the pattern I've seen elsewhere.

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:72: // the decisions are
expired, a new dictionary will be created
On 2014/07/07 22:26:39, felt wrote:
> why do you need to support both cases for |create_entries|?

There are two uses of GetValidCertDecisionsDict. The first is a simple query of
the policy, and if the entry is missing, that just means that no judgement has
been rendered, thus we don't want to create a new entry for the dictionary. The
second case is when we're changing the cert policy. In that case, we don't
necessarily already have an entry, but we definitely want to create one a new
one. The two cases are identical in that they are fetching a dictionary that we
want to look at or modify properties of. The only difference is that, in one
case, if it doesn't exist, we want to create a new one, and the other we just
want to know that it doesn't exist.

https://codereview.chromium.org/369703002/diff/1/content/browser/ssl/ssl_bloc...
File content/browser/ssl/ssl_blocking_proceed_decision_store.cc (right):

https://codereview.chromium.org/369703002/diff/1/content/browser/ssl/ssl_bloc...
content/browser/ssl/ssl_blocking_proceed_decision_store.cc:11: void
SSLBlockingProceedDecisionStore::UserDidProceed() {
On 2014/07/07 22:26:39, felt wrote:
> what's going on here?

Yikes. That slipped in from an earlier implementation. Removing.

[felt, 2014-07-08 17:52:48]

https://codereview.chromium.org/369703002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:499: switches::kRememberCertErrorDecisions,
On 2014/07/08 17:35:41, jww wrote:
> On 2014/07/07 22:26:39, felt wrote:
> > FYI: FieldTrials doesn't currently support flag=value flags, so you'll need
to
> > use a different flag for each one when you set up a Field Trial.
> 
> Isn't it simpler than that? In my previous experiment, I didn't even use flags
> in the field trial portion, and I plan on the same basic implementation here.
> Basically, have a field trial bucket for each flag, and check if the user is
in
> the bucket *or* if they've overriden the choice with a flag.

If you want to track forcing flags, I believe you need to have a different flag
for each condition:
https://sites.google.com/a/google.com/chrome-data/finch-experiments/guide-for...

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:26: // "old" style of
certificate decision memory.
On 2014/07/08 17:35:41, jww wrote:
> On 2014/07/07 22:26:39, felt wrote:
> > Might you want to use -1 instead of 0 for the "default"/old case? One could
> > imagine that a policy of 0 should mean it expires at the end of the current
> day.
> 
> I'm not sure I understand. The number is in seconds, so my impression would be
> that, if anything, "0 seconds" would mean "no memory at all," it seems to make
> more sense to me to repurpose it as "don't remember after the session ends."

I agree, 0 seconds means "no memory at all." But here it means "remember
indefinitely until session ends", which seems like the opposite of "no memory at
all."

https://codereview.chromium.org/369703002/diff/1/content/browser/ssl/ssl_host...
File content/browser/ssl/ssl_host_state.h (right):

https://codereview.chromium.org/369703002/diff/1/content/browser/ssl/ssl_host...
content/browser/ssl/ssl_host_state.h:88: // The certificate decision store. It
may be NULl, depending on the browsing
nit: NULL

[felt, 2014-07-08 17:52:49]

https://codereview.chromium.org/369703002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:499: switches::kRememberCertErrorDecisions,
On 2014/07/08 17:35:41, jww wrote:
> On 2014/07/07 22:26:39, felt wrote:
> > FYI: FieldTrials doesn't currently support flag=value flags, so you'll need
to
> > use a different flag for each one when you set up a Field Trial.
> 
> Isn't it simpler than that? In my previous experiment, I didn't even use flags
> in the field trial portion, and I plan on the same basic implementation here.
> Basically, have a field trial bucket for each flag, and check if the user is
in
> the bucket *or* if they've overriden the choice with a flag.

If you want to track forcing flags, I believe you need to have a different flag
for each condition:
https://sites.google.com/a/google.com/chrome-data/finch-experiments/guide-for...

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:26: // "old" style of
certificate decision memory.
On 2014/07/08 17:35:41, jww wrote:
> On 2014/07/07 22:26:39, felt wrote:
> > Might you want to use -1 instead of 0 for the "default"/old case? One could
> > imagine that a policy of 0 should mean it expires at the end of the current
> day.
> 
> I'm not sure I understand. The number is in seconds, so my impression would be
> that, if anything, "0 seconds" would mean "no memory at all," it seems to make
> more sense to me to repurpose it as "don't remember after the session ends."

I agree, 0 seconds means "no memory at all." But here it means "remember
indefinitely until session ends", which seems like the opposite of "no memory at
all."

https://codereview.chromium.org/369703002/diff/1/content/browser/ssl/ssl_host...
File content/browser/ssl/ssl_host_state.h (right):

https://codereview.chromium.org/369703002/diff/1/content/browser/ssl/ssl_host...
content/browser/ssl/ssl_host_state.h:88: // The certificate decision store. It
may be NULl, depending on the browsing
nit: NULL

[jww, 2014-07-08 23:49:01]

https://codereview.chromium.org/369703002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:499: switches::kRememberCertErrorDecisions,
On 2014/07/08 17:52:48, felt wrote:
> On 2014/07/08 17:35:41, jww wrote:
> > On 2014/07/07 22:26:39, felt wrote:
> > > FYI: FieldTrials doesn't currently support flag=value flags, so you'll
need
> to
> > > use a different flag for each one when you set up a Field Trial.
> > 
> > Isn't it simpler than that? In my previous experiment, I didn't even use
flags
> > in the field trial portion, and I plan on the same basic implementation
here.
> > Basically, have a field trial bucket for each flag, and check if the user is
> in
> > the bucket *or* if they've overriden the choice with a flag.
> 
> If you want to track forcing flags, I believe you need to have a different
flag
> for each condition:
>
https://sites.google.com/a/google.com/chrome-data/finch-experiments/guide-for...

Got it. I've switched to pure flags style.

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/1/chrome/browser/ssl/chrome_ss...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:26: // "old" style of
certificate decision memory.
On 2014/07/08 17:52:49, felt wrote:
> On 2014/07/08 17:35:41, jww wrote:
> > On 2014/07/07 22:26:39, felt wrote:
> > > Might you want to use -1 instead of 0 for the "default"/old case? One
could
> > > imagine that a policy of 0 should mean it expires at the end of the
current
> > day.
> > 
> > I'm not sure I understand. The number is in seconds, so my impression would
be
> > that, if anything, "0 seconds" would mean "no memory at all," it seems to
make
> > more sense to me to repurpose it as "don't remember after the session ends."
> 
> I agree, 0 seconds means "no memory at all." But here it means "remember
> indefinitely until session ends", which seems like the opposite of "no memory
at
> all."

Fair enough. I've changed this to your suggestion.

https://codereview.chromium.org/369703002/diff/1/content/browser/ssl/ssl_host...
File content/browser/ssl/ssl_host_state.h (right):

https://codereview.chromium.org/369703002/diff/1/content/browser/ssl/ssl_host...
content/browser/ssl/ssl_host_state.h:88: // The certificate decision store. It
may be NULl, depending on the browsing
On 2014/07/08 17:52:49, felt wrote:
> nit: NULL

Done.

[Ryan Sleevi, 2014-07-08 23:53:29]

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:24: };
Duplicate definition as the .cc. What's up with that?

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:37: return 0;
Braces since you wrap?

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:47: << "memory length: "
<< switch_value;
Seems to be spammy if the user didn't pass the switch.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:60: const int
kDefaultSSLCertDecisionVersion = 1;
anal retentive nit: Prefer constants declared first, as per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
recommendation for classes

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:128: // session end (i.e.
the expiration delta is 0).
This feels like it's violating the cardinal rule of comments, in that it
describes what the code does, even though a few bits and pieces are smattered as
to why.

Perhaps a better structure for the comment is to list what the 'expected'
behaviours are, and let the reader validate the code matches said expectations?

// Check to see if the user's certificate decision has expired.
// - Expired and |create_entries| is DoNoCreateDicitionaryEntries, return NULL
// - Expired and |create_entries| is CreateDictionaryEntries, update the
expiration

Note that pipes around variables, not values, and you describe
expired+|DoNotCreateDictionaryEntries| twice, with two different behaviours.

In short, rework it all :)

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:136: // If
defaultSSLCertDecisionExpirationDelta_ is 0, that means that decisions
bad variable name - default_ssl_cert_decision_expiration_delta

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:202: url, url,
CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL));
std::string(), not ""

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:238: pattern, pattern,
CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL);
std::string() - and throughout rest of file.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:247: url, url,
CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL));
Why don't you DCHECK here, but you do on line 204?

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:276: url, url,
CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL));
ditto DCHECK

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:20: class
ChromeSSLHostStateDecisions : public content::SSLHostStateDecisions {
Document

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:45: void
SetClock(scoped_ptr<base::Clock> clock) {
IWYU: You need to include the base::Clock header
IWYU: You need to include base/memory/scoped_ptr.h

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:52:
DoNotCreateDictionaryEntries
Document

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:58:
net::CertPolicy::Judgment judgment);
Document

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:61:
CreateDictionaryEntriesDisposition create_entries);
Document

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:64: const base::TimeDelta
defaultSSLCertDecisionExpirationDelta_;
IWYU: base/time.h

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:67: friend class
RememberSSLHostStateDecisionsTest;
As in life, it's good to put our friends first. (This is a common Chromium
idiom, at least)

It serves as a reminder that everything that follows the "private" will be a
dirty rotten lie in some cases (hopefully only tests) :)

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:30: unsigned char
google_der[] = {
Use the net test data. Don't hardcode files into tests like this.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:116: virtual void
CleanUpOnMainThread() OVERRIDE{};
1) spaces between OVERRIDE and {}
2) Um, why are you overriding to begin with, if they're just empty?

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:132:
content::SSLHostStateDecisions* state = profile->GetSSLHostStateDecisions();
Seems like all of this could be done as a simple TEST_F, no IPBT.

That is, just test the decisions class for testing decisions.

If you are going to do any IPBT, it'd probably just to be sure that a regular
and incognito profile don't bleed together.

https://codereview.chromium.org/369703002/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:1266: const char kSSLBlockingBypassed[] =
"ssl.ssl_blocking_bypassed";
Does seem wrong to store this in prefs, potentially for a lot of sites the user
visits.

We're having this same debate in //net land

Let's not forget, every pref you add blocks Chrome's startup - even from
starting up other threads. This is a Bad Thing. Separate storage - even if it's
just a single Flat File, but that can be loaded after start-up (and only on a
bad cert?) is a Good Thing.

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_host_state.cc (right):

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_host_state.cc:27: ->CloseIdleConnections();
I really, really, really don't like this being called. Especially because "Idle"
connections has a particular meaning that may not be doing what you want.

I'd be happy to leave this out of the first run. Otherwise, flush the socket
pools if the user changes connection state (or, ideally, just flush the affected
sockets)

https://codereview.chromium.org/369703002/diff/40001/content/public/browser/s...
File content/public/browser/ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/40001/content/public/browser/s...
content/public/browser/ssl_host_state_decisions.h:20: : public
base::RefCountedThreadSafe<SSLHostStateDecisions> {
In general, I like to push back against introducing RefCounted classes, as they
generally lead to poor lifetime management.

Could you explain more why you can't have an explicit ownership transfer occur?

[jww, 2014-07-11 00:08:42]

Thanks for the great comments. I should have also mentioned that I'm planning on
submitting this CL for C++ Readability, so please feel free to emphasize style
nits; no offense will be taken :-)

sleevi, I've addressed most of your comments and have responded to a few inline.

willchan, please see my response to sleevi regarding CloseIdleConnections and
let me know if I'm off base or wrong.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:24: };
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> Duplicate definition as the .cc. What's up with that?

This was actually fixed in the latest patch set that I uploaded. It appears that
your review and my previous path upload appeared simultaneously. Real life
TOCTTOU!

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:37: return 0;
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> Braces since you wrap?

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:47: << "memory length: "
<< switch_value;
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> Seems to be spammy if the user didn't pass the switch.

It shouldn't reach this if the user didn't include a switch because of the
HasSwitch check on line 35; this was just to make sure it was a valid value.
However, this is now irrelevant in the new patch set upload, since per felt's
suggestion, I changed it from a value switch to an explicit switch to work
better with Finch experiments.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:60: const int
kDefaultSSLCertDecisionVersion = 1;
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> anal retentive nit: Prefer constants declared first, as per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> recommendation for classes

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:128: // session end (i.e.
the expiration delta is 0).
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> This feels like it's violating the cardinal rule of comments, in that it
> describes what the code does, even though a few bits and pieces are smattered
as
> to why.
> 
> Perhaps a better structure for the comment is to list what the 'expected'
> behaviours are, and let the reader validate the code matches said
expectations?
> 
> // Check to see if the user's certificate decision has expired.
> // - Expired and |create_entries| is DoNoCreateDicitionaryEntries, return NULL
> // - Expired and |create_entries| is CreateDictionaryEntries, update the
> expiration
> 
> Note that pipes around variables, not values, and you describe
> expired+|DoNotCreateDictionaryEntries| twice, with two different behaviours.
> 
> In short, rework it all :)

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:136: // If
defaultSSLCertDecisionExpirationDelta_ is 0, that means that decisions
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> bad variable name - default_ssl_cert_decision_expiration_delta

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:202: url, url,
CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL));
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> std::string(), not ""

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:238: pattern, pattern,
CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL);
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> std::string() - and throughout rest of file.

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:247: url, url,
CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL));
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> Why don't you DCHECK here, but you do on line 204?

Good catch. The DCHECK on 204 was from testing earlier, and really this should
all explicitly test for NULL.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:276: url, url,
CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL));
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> ditto DCHECK

See above.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:20: class
ChromeSSLHostStateDecisions : public content::SSLHostStateDecisions {
On 2014/07/08 23:53:28, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:45: void
SetClock(scoped_ptr<base::Clock> clock) {
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> IWYU: You need to include the base::Clock header
> IWYU: You need to include base/memory/scoped_ptr.h

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:52:
DoNotCreateDictionaryEntries
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:58:
net::CertPolicy::Judgment judgment);
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:61:
CreateDictionaryEntriesDisposition create_entries);
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:64: const base::TimeDelta
defaultSSLCertDecisionExpirationDelta_;
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> IWYU: base/time.h

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:67: friend class
RememberSSLHostStateDecisionsTest;
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> As in life, it's good to put our friends first. (This is a common Chromium
> idiom, at least)
> 
> It serves as a reminder that everything that follows the "private" will be a
> dirty rotten lie in some cases (hopefully only tests) :)

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:30: unsigned char
google_der[] = {
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> Use the net test data. Don't hardcode files into tests like this.

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:116: virtual void
CleanUpOnMainThread() OVERRIDE{};
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> 1) spaces between OVERRIDE and {}
I actually had a space initially, but git-cl format changed it.
> 2) Um, why are you overriding to begin with, if they're just empty?
Habit. Getting rid of them.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:132:
content::SSLHostStateDecisions* state = profile->GetSSLHostStateDecisions();
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> Seems like all of this could be done as a simple TEST_F, no IPBT.
> 
> That is, just test the decisions class for testing decisions.
> 
> If you are going to do any IPBT, it'd probably just to be sure that a regular
> and incognito profile don't bleed together.

I think you're right that these basic unit tests can probably not be IPBT, so
I'm changing them to be just TEST_F. However, the other tests below are all
about profiles and making sure things are maintained (or not) after restart, as
well as checking incognito bleeds, so those, I think, need to remain IPBT.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:132:
content::SSLHostStateDecisions* state = profile->GetSSLHostStateDecisions();
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> Seems like all of this could be done as a simple TEST_F, no IPBT.
> 
> That is, just test the decisions class for testing decisions.
> 
> If you are going to do any IPBT, it'd probably just to be sure that a regular
> and incognito profile don't bleed together.

Perhaps I don't quite understand how TEST_F works, but I think these need to all
be IPBT because the implementation relies on having a profile. That is, the
state forwards queries and changes into the profile, so if we're not in an IPBT,
I believe there won't be a profile, and none of this will work.

If I'm mistaken, though, could you point me to a good example that's similar
that I can use to model the test off of?

https://codereview.chromium.org/369703002/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:1266: const char kSSLBlockingBypassed[] =
"ssl.ssl_blocking_bypassed";
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> Does seem wrong to store this in prefs, potentially for a lot of sites the
user
> visits.
> 
> We're having this same debate in //net land
> 
> Let's not forget, every pref you add blocks Chrome's startup - even from
> starting up other threads. This is a Bad Thing. Separate storage - even if
it's
> just a single Flat File, but that can be loaded after start-up (and only on a
> bad cert?) is a Good Thing.

This was the model suggested by jschuh since it is a user choice. I think my two
thoughts as to why this is the right design is:
(a) This is not that common of a case. Most users will not have loads of these
sitting in their preferences.
(b) My understanding is that prefs will be improving quite a bit in the
not-too-distant future, so this should be less of an issue. I think they're
being reworking into a DB of some sort.

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_host_state.cc (right):

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_host_state.cc:27: ->CloseIdleConnections();
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> I really, really, really don't like this being called. Especially because
"Idle"
> connections has a particular meaning that may not be doing what you want.
> 
> I'd be happy to leave this out of the first run. Otherwise, flush the socket
> pools if the user changes connection state (or, ideally, just flush the
affected
> sockets)

This was suggested by willchan (who I'm CC'ing in case he has anything to add)
as the correct way to deal with problems that we were arising since the
connection pool keeps its own state about "user decisions." That is, if a
decision is changed because of timeout (or, soon, user choice), that is not
actually reflected immediately at the socket level, which has its own state, and
this led to some real bugs that that revoking the decision, you could still
reload the page in the same tab and bypass the interstitial. 

My understanding is that the only way to avoid this is by closing idle
connections. Obviously, this is high cost, but this should be a pretty
infrequent event. Is there another method that makes more sense for this kind of
flushing that we can use instead?

https://codereview.chromium.org/369703002/diff/40001/content/public/browser/s...
File content/public/browser/ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/40001/content/public/browser/s...
content/public/browser/ssl_host_state_decisions.h:20: : public
base::RefCountedThreadSafe<SSLHostStateDecisions> {
On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> In general, I like to push back against introducing RefCounted classes, as
they
> generally lead to poor lifetime management.
> 
> Could you explain more why you can't have an explicit ownership transfer
occur?

So I'm pretty sure ownership transfer is out of the question. Since this,
ultimately, is returned as a profile service (like SpecialStoragePolicy, which I
modeled this on), it's effectively a singleton owned by the profile.

I'm not completely sure on this, but I'm under the impression that it's
theoretically possible for the profile to be pulled out from underneath code
(such as the interstitial code) that might have grabbed a reference to this,
thus we can't treat it as a raw pointer either. Thus, a ref counted pointer
seemed like the only option.

[Ryan Sleevi, 2014-07-11 00:48:49]

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:132:
content::SSLHostStateDecisions* state = profile->GetSSLHostStateDecisions();
On 2014/07/11 00:08:41, jww wrote:
> Perhaps I don't quite understand how TEST_F works, but I think these need to
all
> be IPBT because the implementation relies on having a profile. That is, the
> state forwards queries and changes into the profile, so if we're not in an
IPBT,
> I believe there won't be a profile, and none of this will work.
> 
> If I'm mistaken, though, could you point me to a good example that's similar
> that I can use to model the test off of?

Correct, there won't be an associated Profile in a normal test fixture.

That said, I don't think you should be coupling *these* tests - which are about
basic functionality of the host decisions - to being a profile. There's nothing
profile-y about it (compared to the Incognito vs Regular tests, which I agree
should be IPBT)

That is, rather than lines 129-132, which bounce to a profile to get an
SSLHostStateDecisions, you should just be creating an SSLHostStateDecisions, and
running the various tests.

All of these tests (that is, 125-221) belong in content::SSLHostStateDecision's
tests, not Chrome's. Chrome's should only test the profile-relationships, but
content should test the core interface associations.

https://codereview.chromium.org/369703002/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:1266: const char kSSLBlockingBypassed[] =
"ssl.ssl_blocking_bypassed";
On 2014/07/11 00:08:41, jww wrote:
> This was the model suggested by jschuh since it is a user choice. I think my
two
> thoughts as to why this is the right design is:
> (a) This is not that common of a case. Most users will not have loads of these
> sitting in their preferences.
> (b) My understanding is that prefs will be improving quite a bit in the
> not-too-distant future, so this should be less of an issue. I think they're
> being reworking into a DB of some sort.

What do we do for content settings today - like which sites to enable images on
or disable them?

If that's prefs, then I can live with it.
If it's not (and I didn't think it was), we shouldn't be doing that here.

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_host_state.cc (right):

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_host_state.cc:27: ->CloseIdleConnections();
On 2014/07/11 00:08:42, jww wrote:
> This was suggested by willchan (who I'm CC'ing in case he has anything to add)
> as the correct way to deal with problems that we were arising since the
> connection pool keeps its own state about "user decisions." That is, if a
> decision is changed because of timeout (or, soon, user choice), that is not
> actually reflected immediately at the socket level, which has its own state,
and
> this led to some real bugs that that revoking the decision, you could still
> reload the page in the same tab and bypass the interstitial. 
> 
> My understanding is that the only way to avoid this is by closing idle
> connections. Obviously, this is high cost, but this should be a pretty
> infrequent event. Is there another method that makes more sense for this kind
of
> flushing that we can use instead?

I'm aware of the potential for bugs, I'm not aware of any 'real' bugs, but more
importantly, I think the discussion around this is worthy in and of itself its
own CL, with its own set of nuance.

Whether it should be tackled first or a follow-up is fine, but I don't think we
should be doing this in this CL.

https://codereview.chromium.org/369703002/diff/40001/content/public/browser/s...
File content/public/browser/ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/40001/content/public/browser/s...
content/public/browser/ssl_host_state_decisions.h:20: : public
base::RefCountedThreadSafe<SSLHostStateDecisions> {
On 2014/07/11 00:08:42, jww wrote:
> On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> > In general, I like to push back against introducing RefCounted classes, as
> they
> > generally lead to poor lifetime management.
> > 
> > Could you explain more why you can't have an explicit ownership transfer
> occur?
> 
> So I'm pretty sure ownership transfer is out of the question. Since this,
> ultimately, is returned as a profile service (like SpecialStoragePolicy, which
I
> modeled this on), it's effectively a singleton owned by the profile.

Except with RefCounting, you're always (potentially) doing an ownership
transfer. That's the real danger.

If not a transfer, a clear set of lifetimes and precedence should be
established.

> I'm not completely sure on this, but I'm under the impression that it's
> theoretically possible for the profile to be pulled out from underneath code
> (such as the interstitial code) that might have grabbed a reference to this,
> thus we can't treat it as a raw pointer either. Thus, a ref counted pointer
> seemed like the only option.

That certainly highlights a bad code smell to me then. It seems wrong to keep
this alive even if the profile is gone.

[jww, 2014-07-11 01:52:07]

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:132:
content::SSLHostStateDecisions* state = profile->GetSSLHostStateDecisions();
On 2014/07/11 00:48:49, Ryan Sleevi wrote:
> On 2014/07/11 00:08:41, jww wrote:
> > Perhaps I don't quite understand how TEST_F works, but I think these need to
> all
> > be IPBT because the implementation relies on having a profile. That is, the
> > state forwards queries and changes into the profile, so if we're not in an
> IPBT,
> > I believe there won't be a profile, and none of this will work.
> > 
> > If I'm mistaken, though, could you point me to a good example that's similar
> > that I can use to model the test off of?
> 
> Correct, there won't be an associated Profile in a normal test fixture.
> 
> That said, I don't think you should be coupling *these* tests - which are
about
> basic functionality of the host decisions - to being a profile. There's
nothing
> profile-y about it (compared to the Incognito vs Regular tests, which I agree
> should be IPBT)
> 
> That is, rather than lines 129-132, which bounce to a profile to get an
> SSLHostStateDecisions, you should just be creating an SSLHostStateDecisions,
and
> running the various tests.
> 
> All of these tests (that is, 125-221) belong in
content::SSLHostStateDecision's
> tests, not Chrome's. Chrome's should only test the profile-relationships, but
> content should test the core interface associations.

Sorry for my density, I still think that doesn't make sense. SSLHostState
actually doesn't have any more tests because it exists simply to forward queries
and changes to ChromeSSLHostStateDecisions, and without a profile it can't
forward anything since ChromeSSLHostStateDecisions exists as a singleton in each
profile.

I understand that it feels weird to put this unit-y tests inside the browser
test, except that all of the things we're testing here (querying, allowing
certs, etc.) are backed on a per-profile basis. For example, with QueryPolicy,
it's meaningless to talk about QueryPolicy anymore without a profile because the
querying works with content settings, which is build into the profile.

https://codereview.chromium.org/369703002/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:1266: const char kSSLBlockingBypassed[] =
"ssl.ssl_blocking_bypassed";
On 2014/07/11 00:48:49, Ryan Sleevi wrote:
> On 2014/07/11 00:08:41, jww wrote:
> > This was the model suggested by jschuh since it is a user choice. I think my
> two
> > thoughts as to why this is the right design is:
> > (a) This is not that common of a case. Most users will not have loads of
these
> > sitting in their preferences.
> > (b) My understanding is that prefs will be improving quite a bit in the
> > not-too-distant future, so this should be less of an issue. I think they're
> > being reworking into a DB of some sort.
> 
> What do we do for content settings today - like which sites to enable images
on
> or disable them?
> 
> If that's prefs, then I can live with it.
> If it's not (and I didn't think it was), we shouldn't be doing that here.

These are all stored in preferences. For example, I'm looking at my profile's
preferences file right now, and it has entries for all the sites where I allow
plugins to run (since I block by default).

My understanding is that in the general case, content settings defaults to store
things in the preferences file; I'm not if, with content settings, you can back
it by anything else. At any rate, that's why they're working to improve the
preferences storage mechanism.

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_host_state.cc (right):

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_host_state.cc:27: ->CloseIdleConnections();
On 2014/07/11 00:48:49, Ryan Sleevi wrote:
> On 2014/07/11 00:08:42, jww wrote:
> > This was suggested by willchan (who I'm CC'ing in case he has anything to
add)
> > as the correct way to deal with problems that we were arising since the
> > connection pool keeps its own state about "user decisions." That is, if a
> > decision is changed because of timeout (or, soon, user choice), that is not
> > actually reflected immediately at the socket level, which has its own state,
> and
> > this led to some real bugs that that revoking the decision, you could still
> > reload the page in the same tab and bypass the interstitial. 
> > 
> > My understanding is that the only way to avoid this is by closing idle
> > connections. Obviously, this is high cost, but this should be a pretty
> > infrequent event. Is there another method that makes more sense for this
kind
> of
> > flushing that we can use instead?
> 
> I'm aware of the potential for bugs, I'm not aware of any 'real' bugs, but
more
> importantly, I think the discussion around this is worthy in and of itself its
> own CL, with its own set of nuance.
> 
> Whether it should be tackled first or a follow-up is fine, but I don't think
we
> should be doing this in this CL.

Sorry, I'm confused about the action to take at this point. I was stating that
without CloseIdleConnections(), this behavior leads to inconsistent state
between the interstitial layer (SSLHostStateDecisions) and the network layer
(unfortunately I can't reference the networking code that does this off the top
of my head). I took the heavy hammer here to simply close the connections in the
pool (at least I thought that's what I was doing). Without this close, this code
doesn't work and it leads to a potentially nonsensical browsing experience where
a cert has expired and some tabs show an interstitial error and other tabs don't
(that's what I meant by 'bugs').

Is there another heavy hammer we can use to avoid this? I don't feel
particularly attached to the CloseIdleConnections call; I used it because I
thought it was the right one, and seemed to resolve the bugs.

https://codereview.chromium.org/369703002/diff/40001/content/public/browser/s...
File content/public/browser/ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/40001/content/public/browser/s...
content/public/browser/ssl_host_state_decisions.h:20: : public
base::RefCountedThreadSafe<SSLHostStateDecisions> {
On 2014/07/11 00:48:49, Ryan Sleevi wrote:
> On 2014/07/11 00:08:42, jww wrote:
> > On 2014/07/08 23:53:29, Ryan Sleevi wrote:
> > > In general, I like to push back against introducing RefCounted classes, as
> > they
> > > generally lead to poor lifetime management.
> > > 
> > > Could you explain more why you can't have an explicit ownership transfer
> > occur?
> > 
> > So I'm pretty sure ownership transfer is out of the question. Since this,
> > ultimately, is returned as a profile service (like SpecialStoragePolicy,
which
> I
> > modeled this on), it's effectively a singleton owned by the profile.
> 
> Except with RefCounting, you're always (potentially) doing an ownership
> transfer. That's the real danger.
> 
> If not a transfer, a clear set of lifetimes and precedence should be
> established.
> 
> > I'm not completely sure on this, but I'm under the impression that it's
> > theoretically possible for the profile to be pulled out from underneath code
> > (such as the interstitial code) that might have grabbed a reference to this,
> > thus we can't treat it as a raw pointer either. Thus, a ref counted pointer
> > seemed like the only option.
> 
> That certainly highlights a bad code smell to me then. It seems wrong to keep
> this alive even if the profile is gone.

Ha. Looking again, I don't ever even assign it to a scoped_refptr, so clearly
this needs to go. Sorry about that.

[Ryan Sleevi, 2014-07-11 19:43:27]

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:132:
content::SSLHostStateDecisions* state = profile->GetSSLHostStateDecisions();
On 2014/07/11 01:52:07, jww wrote:
> Sorry for my density, I still think that doesn't make sense. SSLHostState
> actually doesn't have any more tests because it exists simply to forward
queries
> and changes to ChromeSSLHostStateDecisions, and without a profile it can't
> forward anything since ChromeSSLHostStateDecisions exists as a singleton in
each
> profile.
> 
> I understand that it feels weird to put this unit-y tests inside the browser
> test, except that all of the things we're testing here (querying, allowing
> certs, etc.) are backed on a per-profile basis. For example, with QueryPolicy,
> it's meaningless to talk about QueryPolicy anymore without a profile because
the
> querying works with content settings, which is build into the profile.

Bah, I missed *Decisions rather than just SSLHostState (which is Profile-less).

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:143: example_url,
google_cert.get(), net::CERT_STATUS_DATE_INVALID));
Document the precondition you're testing on line 133.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:145:
state->AllowCert(
Document what you're doing, and then on line 147, what you're verifying was
done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:159:
state->AllowCert(
Document what you're doing, and what you're verifying was done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:173: state->DenyCert(
Document what you're doing, and what you're verifying was done (e.g. that you
can deny a cert, and it does not affect other URLs)

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:187:
EXPECT_TRUE(state->HasAllowedOrDeniedCert(www_google_url));
Seems like 187-193 should be their own, hermetic test.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:195:
EXPECT_FALSE(state->HasAllowedOrDeniedCert(google_url));
What is this testing that is different than what you just tested on 187-193?

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:202:
EXPECT_TRUE(state->HasAllowedOrDeniedCert(example_url));
How is this a different test than the previous two?

Should you perhaps be instead doing a test to ensure revoking for www_google_url
doesn't affect example_url? Or that revoking for google_url affects
www_google_url if content settings are supposed to inherit? Or that it doesn't,
if they're not.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:220: example_url,
google_cert.get(), net::CERT_STATUS_DATE_INVALID));
How does this actually test that Clear() worked, considering you just made them
all unknown on 187-208.

Here, Clear() could fail, and this test would still succeed.

I expect that you should roll 209-220 into its own test, precisely to test that
Clear() works.

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_host_state.cc (right):

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_host_state.cc:27: ->CloseIdleConnections();
On 2014/07/11 01:52:07, jww wrote:
> Sorry, I'm confused about the action to take at this point. I was stating that
> without CloseIdleConnections(), this behavior leads to inconsistent state
> between the interstitial layer (SSLHostStateDecisions) and the network layer
> (unfortunately I can't reference the networking code that does this off the
top
> of my head). I took the heavy hammer here to simply close the connections in
the
> pool (at least I thought that's what I was doing). Without this close, this
code
> doesn't work and it leads to a potentially nonsensical browsing experience
where
> a cert has expired and some tabs show an interstitial error and other tabs
don't
> (that's what I meant by 'bugs').
> 
> Is there another heavy hammer we can use to avoid this? I don't feel
> particularly attached to the CloseIdleConnections call; I used it because I
> thought it was the right one, and seemed to resolve the bugs.

Don't use the heavy hammer here.

Yes, it has the potential for bugs. This potential already exists. Don't try to
fix a bug and refactor/introduce features in the same CL.

The bug fix has a lot of nuance, hence I don't want it detracting from the CL,
other than acknowledging that yes, in theory, there's a bug here.

We boot connections far too often, and far too unnecessarily. It makes Chrome
completely unusuable in some situations - such as when you're behind the Great
Firewall - and it's a hammer that really, really, really needs to be a
screwdriver.

But that's a battle for a different CL :)

[jww, 2014-07-14 21:21:16]

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:132:
content::SSLHostStateDecisions* state = profile->GetSSLHostStateDecisions();
On 2014/07/11 19:43:27, Ryan Sleevi wrote:
> On 2014/07/11 01:52:07, jww wrote:
> > Sorry for my density, I still think that doesn't make sense. SSLHostState
> > actually doesn't have any more tests because it exists simply to forward
> queries
> > and changes to ChromeSSLHostStateDecisions, and without a profile it can't
> > forward anything since ChromeSSLHostStateDecisions exists as a singleton in
> each
> > profile.
> > 
> > I understand that it feels weird to put this unit-y tests inside the browser
> > test, except that all of the things we're testing here (querying, allowing
> > certs, etc.) are backed on a per-profile basis. For example, with
QueryPolicy,
> > it's meaningless to talk about QueryPolicy anymore without a profile because
> the
> > querying works with content settings, which is build into the profile.
> 
> Bah, I missed *Decisions rather than just SSLHostState (which is
Profile-less).

Acknowledged.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:143: example_url,
google_cert.get(), net::CERT_STATUS_DATE_INVALID));
On 2014/07/11 19:43:27, Ryan Sleevi wrote:
> Document the precondition you're testing on line 133.

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:145:
state->AllowCert(
On 2014/07/11 19:43:27, Ryan Sleevi wrote:
> Document what you're doing, and then on line 147, what you're verifying was
> done.

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:159:
state->AllowCert(
On 2014/07/11 19:43:27, Ryan Sleevi wrote:
> Document what you're doing, and what you're verifying was done.

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:173: state->DenyCert(
On 2014/07/11 19:43:27, Ryan Sleevi wrote:
> Document what you're doing, and what you're verifying was done (e.g. that you
> can deny a cert, and it does not affect other URLs)

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:187:
EXPECT_TRUE(state->HasAllowedOrDeniedCert(www_google_url));
On 2014/07/11 19:43:27, Ryan Sleevi wrote:
> Seems like 187-193 should be their own, hermetic test.

Done.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:195:
EXPECT_FALSE(state->HasAllowedOrDeniedCert(google_url));
On 2014/07/11 19:43:27, Ryan Sleevi wrote:
> What is this testing that is different than what you just tested on 187-193?

187-193 tests that HasAllowedOrDeniedCert works for a cert that has had a
decision made (and then revoked). This confirms that it also works for untouched
URLs, and that a revoke doesn't affect that state.

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:202:
EXPECT_TRUE(state->HasAllowedOrDeniedCert(example_url));
On 2014/07/11 19:43:27, Ryan Sleevi wrote:
> How is this a different test than the previous two?
> 
> Should you perhaps be instead doing a test to ensure revoking for
www_google_url
> doesn't affect example_url? Or that revoking for google_url affects
> www_google_url if content settings are supposed to inherit? Or that it
doesn't,
> if they're not.

Yeah, this was a silly test. I'm going to modify it to be your first suggestion
(revoking www_google_url doesn't affect example_url).

https://codereview.chromium.org/369703002/diff/20001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:220: example_url,
google_cert.get(), net::CERT_STATUS_DATE_INVALID));
On 2014/07/11 19:43:27, Ryan Sleevi wrote:
> How does this actually test that Clear() worked, considering you just made
them
> all unknown on 187-208.
> 
> Here, Clear() could fail, and this test would still succeed.
> 
> I expect that you should roll 209-220 into its own test, precisely to test
that
> Clear() works.

Done.

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_host_state.cc (right):

https://codereview.chromium.org/369703002/diff/20001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_host_state.cc:27: ->CloseIdleConnections();
On 2014/07/11 19:43:27, Ryan Sleevi wrote:
> On 2014/07/11 01:52:07, jww wrote:
> > Sorry, I'm confused about the action to take at this point. I was stating
that
> > without CloseIdleConnections(), this behavior leads to inconsistent state
> > between the interstitial layer (SSLHostStateDecisions) and the network layer
> > (unfortunately I can't reference the networking code that does this off the
> top
> > of my head). I took the heavy hammer here to simply close the connections in
> the
> > pool (at least I thought that's what I was doing). Without this close, this
> code
> > doesn't work and it leads to a potentially nonsensical browsing experience
> where
> > a cert has expired and some tabs show an interstitial error and other tabs
> don't
> > (that's what I meant by 'bugs').
> > 
> > Is there another heavy hammer we can use to avoid this? I don't feel
> > particularly attached to the CloseIdleConnections call; I used it because I
> > thought it was the right one, and seemed to resolve the bugs.
> 
> Don't use the heavy hammer here.
> 
> Yes, it has the potential for bugs. This potential already exists. Don't try
to
> fix a bug and refactor/introduce features in the same CL.
> 
> The bug fix has a lot of nuance, hence I don't want it detracting from the CL,
> other than acknowledging that yes, in theory, there's a bug here.
> 
> We boot connections far too often, and far too unnecessarily. It makes Chrome
> completely unusuable in some situations - such as when you're behind the Great
> Firewall - and it's a hammer that really, really, really needs to be a
> screwdriver.
> 
> But that's a battle for a different CL :)

Okay, sounds good. However, just to be explicit: there's no "potential" here,
this *is* introducing a known bug. It's not going to be a common one, and even
if experienced, it shouldn't be that big of a deal, but it will exist. I'll make
a comment in the the RevokeAllowAndDenyPreferences method explaining the
problem.

[Ryan Sleevi, 2014-07-14 21:30:02]

In future CLs (though I doubt i'll see many), it's generally helpful to avoid
rebasing+fixing in a single patchset. This prevents meaningful inter-patch
diffs.

A better approach is to
upload A
- hack hack hack fixes
upload B
- rebase
upload C

Since that allows the reviewer to review B:A and see that commentary was
addressed (or, if the order of B and C are inversed, they can at least see C:B
and see the changes you made).

Just $.02 :)

[jww, 2014-07-14 21:31:22]

Whoops, fair enough. My bad.


On Mon, Jul 14, 2014 at 2:30 PM, <rsleevi@chromium.org> wrote:

> In future CLs (though I doubt i'll see many), it's generally helpful to
> avoid
> rebasing+fixing in a single patchset. This prevents meaningful inter-patch
> diffs.
>
> A better approach is to
> upload A
> - hack hack hack fixes
> upload B
> - rebase
> upload C
>
> Since that allows the reviewer to review B:A and see that commentary was
> addressed (or, if the order of B and C are inversed, they can at least see
> C:B
> and see the changes you made).
>
> Just $.02 :)
>
> https://codereview.chromium.org/369703002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[radhikabhar, 2014-07-17 18:59:11]

The CQ bit was checked by radhikabhar@chromium.org

[radhikabhar, 2014-07-17 18:59:11]

The CQ bit was unchecked by radhikabhar@chromium.org

[felt, 2014-07-17 22:10:28]

On 2014/07/17 18:59:11, radhikabhar wrote:
> The CQ bit was unchecked by mailto:radhikabhar@chromium.org

radhika, please do not send other people's CLs to the CQ :)

[Ryan Sleevi, 2014-07-17 23:19:36]

High level: Why is GURL appropriate here? The URL portion of the request is
irrelevant. Really, it's the scheme/host/port that make sense, right?

However, the API implies (through it's use of GURL) that it's appropriate to
handle exceptions based on that. Needs more documentation to the effect as to
whether or not that's acceptable.

https://codereview.chromium.org/369703002/diff/120001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/369703002/diff/120001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:279: // this probably needs to
change.
I doubt it. WebView makes such responsibility the callers, IIRC.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:413:
profile_->GetSSLHostStateDecisions()->Clear();
Privacy question: What if the user tries to delete all visits to site within the
past hour/day?

Seems like you might still persist this information, and only nuke it if they
nuke ALL data. Doesn't seem like what users will want?

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:61:
switches::kRememberCertErrorDecisionsOneMonth)) {
Can we cap at a week? I'm a little nervous for a month, especially with SHA-1
being used for the security decision (we typically use the SHA-1 for an index,
but then do a full cert comparison)

Or use SHA-256.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:119: // and it should
resest only at session restart, so skip all of the expiration
typo, s/resest/reset

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:160: }
What about if the user sets their clock a week ahead, makes some entries, then
resets their clock?

What's the behaviour going to be? What should it be? Is it what users expect?

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:177: // If
should_remember_ssl_decisions_ is ForgetSSLExceptionDecisionsAtSessionEnd,
add ||

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:230: int policy_decision; 
        // Owned by dict
This comment doesn't make sense.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:23: // Tracks in the user
has allowed, denied, or not seen an exception for the
typo, s/in/if/

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:24: // specified cite, SSL
fingerprint, and error. If the user makes a decision,
typo, s/cite/site/

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:26: // (across session
restarts), based on Chrome flags.
s/Chrome/command line/ ?
Or perhaps
s/Chrome/preference/, if you're implemented in terms of prefs, rather than
directly flags?

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:27: class
ChromeSSLHostStateDecisions : public content::SSLHostStateDecisions {
Does this belong in namespace chrome? Do we do that? I thought we did that?
Maybe we don't.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:57: friend class
RememberSSLHostStateDecisionsTest;
Prefer one or the other. Feels weird to friend the fixture, since 58-60 cover
the specific tests, and friending the fixture won't affect derived tests (the
TEST_F), since each of those is a new derived class that inherits from your
fixture (hence the FRIEND_TEST_ALL_PREFIXES)

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:68: // Specifies whether
user SSL error decisions should be forgetten at the end
linebreak

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:69: // of this current
session (the "old style" of remembering decisions), or
s/"old style"/"old" style

Or just drop the quotes, it's cleaner that way.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:71: // length of time,
deteremined by default_ssl_cert_decision_expiration_delta_.
add |foo_| around the variable.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h (right):

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h:16: class
ChromeSSLHostStateDecisionsFactory
Document?

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:25: GURL
example_url("https://example.com");
Don't use static variables in tests. You're invoking static initializers here.

Use const char[] for the URLs, create GURLs on demand.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:27: const uint64
kDeltaOneDayInSeconds = 86400;
TYPES: uint64_t ?
COMPILE: UINT64_C(86400) (ensures proper suffix - ULL vs UI64, for example). SEe
https://code.google.com/p/chromium/codesearch#chromium/src/base/port.h&rcl=14...

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:38: // provided
per-profile.
Line break, then document what *this* test does.

If this is the documentation, then add simple one or two sentence explanations
for line 115 & 155

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:230:
IN_PROC_BROWSER_TEST_F(IncognitoSSLHostStateDecisionsTest, AfterRestart) {
Document a bit of the test

// Decisions made in an incognito profile should be forgotten after the profile
is closed or the user restarts the browser.

https://codereview.chromium.org/369703002/diff/120001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_host_state.h (right):

https://codereview.chromium.org/369703002/diff/120001/content/browser/ssl/ssl...
content/browser/ssl/ssl_host_state.h:36: NON_EXPORTED_BASE(public
base::NonThreadSafe) {
Future cleanup for brownie points/boredom busters/waiting inb etween reviews -
this class not deriving from base::NonThreadSafe but instead having a
base::ThreadChecker member and calling checker_.CalledOnValidThread() :)

[jww, 2014-07-21 23:39:34]

https://codereview.chromium.org/369703002/diff/120001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/369703002/diff/120001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:279: // this probably needs to
change.
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> I doubt it. WebView makes such responsibility the callers, IIRC.

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:413:
profile_->GetSSLHostStateDecisions()->Clear();
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> Privacy question: What if the user tries to delete all visits to site within
the
> past hour/day?
> 
> Seems like you might still persist this information, and only nuke it if they
> nuke ALL data. Doesn't seem like what users will want?

No, this deletes the cert memory if they delete the history for a site for any
length of time. I added a test to verify.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:61:
switches::kRememberCertErrorDecisionsOneMonth)) {
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> Can we cap at a week? I'm a little nervous for a month, especially with SHA-1
> being used for the security decision (we typically use the SHA-1 for an index,
> but then do a full cert comparison)
> 
> Or use SHA-256.

There's actually a good chance we want to do longer. parisa@ has suggested
something on the order of 3 months for some experimental groups.

With regards to SHA-256, are you suggested we switch the fingerprint to SHA-256?
I'm happy to make that change, but can we do that in a separate CL (especially
since this CL only implements opt-in behavior)?

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:119: // and it should
resest only at session restart, so skip all of the expiration
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> typo, s/resest/reset

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:160: }
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> What about if the user sets their clock a week ahead, makes some entries, then
> resets their clock?
> 
> What's the behaviour going to be? What should it be? Is it what users expect?

They will get a longer expiration time. That is, if they are set to expire after
a week, they set their clock forward a week, make an exception, then go back a
week, it will now be two weeks until their exception expires.

I don't have a lot of sympathy when people are resetting their clocks by such
dramatic amounts. Instead of tracking the expiration time, as this CL currently
does, we could track the start time and the expiration length, and then check
that the time isn't earlier than the start time, but this is really just a band
aid (imagine they wait a while before starting Chrome again and the time is now
passed the start time; then there's nothing we can do).

To me, this seems like an extreme corner case, and I'm not all that concerned.
Do you have a particular behavior you'd rather see?

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:177: // If
should_remember_ssl_decisions_ is ForgetSSLExceptionDecisionsAtSessionEnd,
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> add ||

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:230: int policy_decision; 
        // Owned by dict
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> This comment doesn't make sense.

Ha. Nope, it certainly does not. Removing.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:23: // Tracks in the user
has allowed, denied, or not seen an exception for the
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> typo, s/in/if/

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:24: // specified cite, SSL
fingerprint, and error. If the user makes a decision,
On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> typo, s/cite/site/

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:26: // (across session
restarts), based on Chrome flags.
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> s/Chrome/command line/ ?
> Or perhaps
> s/Chrome/preference/, if you're implemented in terms of prefs, rather than
> directly flags?

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:27: class
ChromeSSLHostStateDecisions : public content::SSLHostStateDecisions {
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> Does this belong in namespace chrome? Do we do that? I thought we did that?
> Maybe we don't.

It appears there's a variety of different ways its done. I was basing this on
src/chrome/browser/download/download_service.h and
src/chrome/browser/download/download_service_factory.h which don't used the
chrome:: namespace. Let me know if you feel strongly about using the chrome::
namespace, though.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:57: friend class
RememberSSLHostStateDecisionsTest;
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> Prefer one or the other. Feels weird to friend the fixture, since 58-60 cover
> the specific tests, and friending the fixture won't affect derived tests (the
> TEST_F), since each of those is a new derived class that inherits from your
> fixture (hence the FRIEND_TEST_ALL_PREFIXES)

Whoops, my misunderstanding (I thought both were needed).

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:68: // Specifies whether
user SSL error decisions should be forgetten at the end
On 2014/07/17 23:19:35, Ryan Sleevi wrote:
> linebreak

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:69: // of this current
session (the "old style" of remembering decisions), or
On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> s/"old style"/"old" style
> 
> Or just drop the quotes, it's cleaner that way.

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:71: // length of time,
deteremined by default_ssl_cert_decision_expiration_delta_.
On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> add |foo_| around the variable.

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h (right):

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h:16: class
ChromeSSLHostStateDecisionsFactory
On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> Document?

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:25: GURL
example_url("https://example.com");
On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> Don't use static variables in tests. You're invoking static initializers here.
> 
> Use const char[] for the URLs, create GURLs on demand.

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:27: const uint64
kDeltaOneDayInSeconds = 86400;
On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> TYPES: uint64_t ?
> COMPILE: UINT64_C(86400) (ensures proper suffix - ULL vs UI64, for example).
SEe
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/port.h&rcl=14...

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:38: // provided
per-profile.
On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> Line break, then document what *this* test does.
> 
> If this is the documentation, then add simple one or two sentence explanations
> for line 115 & 155

Done.

https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:230:
IN_PROC_BROWSER_TEST_F(IncognitoSSLHostStateDecisionsTest, AfterRestart) {
On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> Document a bit of the test
> 
> // Decisions made in an incognito profile should be forgotten after the
profile
> is closed or the user restarts the browser.

Done.

https://codereview.chromium.org/369703002/diff/120001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_host_state.h (right):

https://codereview.chromium.org/369703002/diff/120001/content/browser/ssl/ssl...
content/browser/ssl/ssl_host_state.h:36: NON_EXPORTED_BASE(public
base::NonThreadSafe) {
On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> Future cleanup for brownie points/boredom busters/waiting inb etween reviews -
> this class not deriving from base::NonThreadSafe but instead having a
> base::ThreadChecker member and calling checker_.CalledOnValidThread() :)

Acknowledged.

[jww, 2014-07-21 23:58:59]

On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> High level: Why is GURL appropriate here? The URL portion of the request is
> irrelevant. Really, it's the scheme/host/port that make sense, right?
> 
> However, the API implies (through it's use of GURL) that it's appropriate to
> handle exceptions based on that. Needs more documentation to the effect as to
> whether or not that's acceptable.
Sorry, forgot to respond to this. You are correct that the current
implementation ignores the path (that is, we only care about scheme/host/port).
The current API in ssl_host_state.h only takes a host, which is not sufficient
for our needs since content settings per website require a scheme and port.
Thus, GURLs seemed like the logical choice. If you feel differently, let me
know.

This does raise a potential bug, though, in that the calls to GetWebsiteSettings
we should really pass the GURL with a call to GetWithEmptyPath(), so I'll add
that in, as well as a comment.
> 
>
https://codereview.chromium.org/369703002/diff/120001/android_webview/browser...
> File android_webview/browser/aw_browser_context.cc (right):
> 
>
https://codereview.chromium.org/369703002/diff/120001/android_webview/browser...
> android_webview/browser/aw_browser_context.cc:279: // this probably needs to
> change.
> I doubt it. WebView makes such responsibility the callers, IIRC.
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/browsing...
> File chrome/browser/browsing_data/browsing_data_remover.cc (right):
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/browsing...
> chrome/browser/browsing_data/browsing_data_remover.cc:413:
> profile_->GetSSLHostStateDecisions()->Clear();
> Privacy question: What if the user tries to delete all visits to site within
the
> past hour/day?
> 
> Seems like you might still persist this information, and only nuke it if they
> nuke ALL data. Doesn't seem like what users will want?
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:61:
> switches::kRememberCertErrorDecisionsOneMonth)) {
> Can we cap at a week? I'm a little nervous for a month, especially with SHA-1
> being used for the security decision (we typically use the SHA-1 for an index,
> but then do a full cert comparison)
> 
> Or use SHA-256.
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:119: // and it should
> resest only at session restart, so skip all of the expiration
> typo, s/resest/reset
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:160: }
> What about if the user sets their clock a week ahead, makes some entries, then
> resets their clock?
> 
> What's the behaviour going to be? What should it be? Is it what users expect?
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:177: // If
> should_remember_ssl_decisions_ is ForgetSSLExceptionDecisionsAtSessionEnd,
> add ||
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:230: int
policy_decision; 
>         // Owned by dict
> This comment doesn't make sense.
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> File chrome/browser/ssl/chrome_ssl_host_state_decisions.h (right):
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:23: // Tracks in the user
> has allowed, denied, or not seen an exception for the
> typo, s/in/if/
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:24: // specified cite,
SSL
> fingerprint, and error. If the user makes a decision,
> typo, s/cite/site/
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:26: // (across session
> restarts), based on Chrome flags.
> s/Chrome/command line/ ?
> Or perhaps
> s/Chrome/preference/, if you're implemented in terms of prefs, rather than
> directly flags?
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:27: class
> ChromeSSLHostStateDecisions : public content::SSLHostStateDecisions {
> Does this belong in namespace chrome? Do we do that? I thought we did that?
> Maybe we don't.
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:57: friend class
> RememberSSLHostStateDecisionsTest;
> Prefer one or the other. Feels weird to friend the fixture, since 58-60 cover
> the specific tests, and friending the fixture won't affect derived tests (the
> TEST_F), since each of those is a new derived class that inherits from your
> fixture (hence the FRIEND_TEST_ALL_PREFIXES)
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:68: // Specifies whether
> user SSL error decisions should be forgetten at the end
> linebreak
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:69: // of this current
> session (the "old style" of remembering decisions), or
> s/"old style"/"old" style
> 
> Or just drop the quotes, it's cleaner that way.
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:71: // length of time,
> deteremined by default_ssl_cert_decision_expiration_delta_.
> add |foo_| around the variable.
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> File chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h (right):
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h:16: class
> ChromeSSLHostStateDecisionsFactory
> Document?
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:25: GURL
> example_url("https://example.com");
> Don't use static variables in tests. You're invoking static initializers here.
> 
> Use const char[] for the URLs, create GURLs on demand.
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:27: const uint64
> kDeltaOneDayInSeconds = 86400;
> TYPES: uint64_t ?
> COMPILE: UINT64_C(86400) (ensures proper suffix - ULL vs UI64, for example).
SEe
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/port.h&rcl=14...
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:38: // provided
> per-profile.
> Line break, then document what *this* test does.
> 
> If this is the documentation, then add simple one or two sentence explanations
> for line 115 & 155
> 
>
https://codereview.chromium.org/369703002/diff/120001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:230:
> IN_PROC_BROWSER_TEST_F(IncognitoSSLHostStateDecisionsTest, AfterRestart) {
> Document a bit of the test
> 
> // Decisions made in an incognito profile should be forgotten after the
profile
> is closed or the user restarts the browser.
> 
>
https://codereview.chromium.org/369703002/diff/120001/content/browser/ssl/ssl...
> File content/browser/ssl/ssl_host_state.h (right):
> 
>
https://codereview.chromium.org/369703002/diff/120001/content/browser/ssl/ssl...
> content/browser/ssl/ssl_host_state.h:36: NON_EXPORTED_BASE(public
> base::NonThreadSafe) {
> Future cleanup for brownie points/boredom busters/waiting inb etween reviews -
> this class not deriving from base::NonThreadSafe but instead having a
> base::ThreadChecker member and calling checker_.CalledOnValidThread() :)

[jww, 2014-07-28 17:34:29]

rsleevi, I believe I've responded to all your comments (including rebasing on
top of and using the new SHA-256 cert fingerprints), so let me know what you
think.

On 2014/07/21 23:58:59, jww wrote:
> On 2014/07/17 23:19:36, Ryan Sleevi wrote:
> > High level: Why is GURL appropriate here? The URL portion of the request is
> > irrelevant. Really, it's the scheme/host/port that make sense, right?
> > 
> > However, the API implies (through it's use of GURL) that it's appropriate to
> > handle exceptions based on that. Needs more documentation to the effect as
to
> > whether or not that's acceptable.
> Sorry, forgot to respond to this. You are correct that the current
> implementation ignores the path (that is, we only care about
scheme/host/port).
> The current API in ssl_host_state.h only takes a host, which is not sufficient
> for our needs since content settings per website require a scheme and port.
> Thus, GURLs seemed like the logical choice. If you feel differently, let me
> know.
> 
> This does raise a potential bug, though, in that the calls to
GetWebsiteSettings
> we should really pass the GURL with a call to GetWithEmptyPath(), so I'll add
> that in, as well as a comment.

[jww, 2014-07-30 00:56:38]

jam@, can you review the content and profile changes in this CL? This is the
experiment counterpart to the page info addition in
https://codereview.chromium.org/418133012/. Thanks!

rsleevi@ and felt@, in the latest update, I added the groups for the experiment.
Since the experiment groups are actually going to use field trial parameters, I
actually changed the command line flag back to using values rather than explicit
flags (just lends itself to being a bit more flexible).

[jww, 2014-07-30 19:58:06]

torne@chromium.org: Please review changes in android_webview/

markusheintz@chromium.org: Please review changes in components/

Thanks!

[Ryan Sleevi, 2014-07-31 00:31:28]

LGTM, mod copious nits.

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:359: friend class
DisableWebRtcEncryptionFlagTest;
Just like in life, put your friends first.

In particular, move this to line 300.

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:69: }
Is this switch necessary, given that we can force field trials from the
command-line?

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:279: return
static_cast<net::CertPolicy::Judgment>(policy_decision);
SECURITY: Seems like you should bounds check this, in case someone starts
fiddling with their prefs?

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:9: #include
"base/time/clock.h"
Can forward declare clock if you move line 56 to the .cc

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:19: 
can nuke the newlines on 17/19

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:62:
FRIEND_TEST_ALL_PREFIXES(RememberSSLHostStateDecisionsTest, AfterRestart);
IWYU:
#include "base/gtest_prod_util.h"

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h:30: //
BrowserContextKeyedBaseFactor methods:
1) Factor -> Factory?
2) These should all be protected, not private, right? What does it even mean to
override private virtuals? In general, prefer to keep the same-or-greater
visibility as the parent; reducing visibility in a derived class is like using
the bathroom in a glass house - it gives the semblance of privacy, even though
you're still exposed.

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:56:
reinterpret_cast<const char*>(google_der), sizeof(google_der)));
Not your fault, but bonus points if you want to use
https://code.google.com/p/chromium/codesearch#chromium/src/net/test/cert_test...
instead

(I'd like to get rid of test_certificate_data, because it's a pain to update
test data for - filed http://crbug.com/399122 )

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:171: // Verify the
rovaction of the www_google_url decision does not affect the
Revocation

https://codereview.chromium.org/369703002/diff/240001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_host_state.h (right):

https://codereview.chromium.org/369703002/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_host_state.h:85: BrowserContext* browser_context_;
Given that SSLHostState SupportsUserData, should this be hung off there?

It feels weird to store this as a member, since you never use it.

[jww, 2014-07-31 05:57:00]

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:359: friend class
DisableWebRtcEncryptionFlagTest;
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> Just like in life, put your friends first.
> 
> In particular, move this to line 300.

Whoops, that was actually due to a bad rebase (this is actually a duplicate and
is already on line 300). Fixed.

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:69: }
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> Is this switch necessary, given that we can force field trials from the
> command-line?

Yes, it's necessary because we provide flags in chrome://flags so users can
override the field trial if they choose (or to opt into a particular variant if
they aren't in a field trial).

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:279: return
static_cast<net::CertPolicy::Judgment>(policy_decision);
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> SECURITY: Seems like you should bounds check this, in case someone starts
> fiddling with their prefs?

Done.

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:9: #include
"base/time/clock.h"
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> Can forward declare clock if you move line 56 to the .cc

Done.

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:19: 
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> can nuke the newlines on 17/19

Done.

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions.h:62:
FRIEND_TEST_ALL_PREFIXES(RememberSSLHostStateDecisionsTest, AfterRestart);
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> IWYU:
> #include "base/gtest_prod_util.h"

Done.

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h:30: //
BrowserContextKeyedBaseFactor methods:
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> 1) Factor -> Factory?
Done.
> 2) These should all be protected, not private, right? What does it even mean
to
> override private virtuals? In general, prefer to keep the same-or-greater
> visibility as the parent; reducing visibility in a derived class is like using
> the bathroom in a glass house - it gives the semblance of privacy, even though
> you're still exposed.
I agree that it's weird, but I'm going to push back only because all the
examples that I looked at and used as outlines for this class *do* make these
methods private (see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
or
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sup...
for example, although I believe just about every other class that subclasses
BrowserContextKeyedServiceFactory does this, from what I can tell).

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:56:
reinterpret_cast<const char*>(google_der), sizeof(google_der)));
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> Not your fault, but bonus points if you want to use
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/test/cert_test...
> instead
> 
> (I'd like to get rid of test_certificate_data, because it's a pain to update
> test data for - filed http://crbug.com/399122 )

Done.

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:171: // Verify the
rovaction of the www_google_url decision does not affect the
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> Revocation

Done.

https://codereview.chromium.org/369703002/diff/240001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_host_state.h (right):

https://codereview.chromium.org/369703002/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_host_state.h:85: BrowserContext* browser_context_;
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> Given that SSLHostState SupportsUserData, should this be hung off there?
> 
> It feels weird to store this as a member, since you never use it.

This is leftover from when CloseIdleSockets was present and is now extraneous.
I'm removing it.

[markusheintz_, 2014-07-31 08:34:19]

On 2014/07/31 05:57:00, jww wrote:
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/chrome_c...
> File chrome/browser/chrome_content_browser_client.h (right):
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/chrome_c...
> chrome/browser/chrome_content_browser_client.h:359: friend class
> DisableWebRtcEncryptionFlagTest;
> On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> > Just like in life, put your friends first.
> > 
> > In particular, move this to line 300.
> 
> Whoops, that was actually due to a bad rebase (this is actually a duplicate
and
> is already on line 300). Fixed.
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> File chrome/browser/ssl/chrome_ssl_host_state_decisions.cc (right):
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:69: }
> On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> > Is this switch necessary, given that we can force field trials from the
> > command-line?
> 
> Yes, it's necessary because we provide flags in chrome://flags so users can
> override the field trial if they choose (or to opt into a particular variant
if
> they aren't in a field trial).
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.cc:279: return
> static_cast<net::CertPolicy::Judgment>(policy_decision);
> On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> > SECURITY: Seems like you should bounds check this, in case someone starts
> > fiddling with their prefs?
> 
> Done.
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> File chrome/browser/ssl/chrome_ssl_host_state_decisions.h (right):
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:9: #include
> "base/time/clock.h"
> On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> > Can forward declare clock if you move line 56 to the .cc
> 
> Done.
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:19: 
> On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> > can nuke the newlines on 17/19
> 
> Done.
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions.h:62:
> FRIEND_TEST_ALL_PREFIXES(RememberSSLHostStateDecisionsTest, AfterRestart);
> On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> > IWYU:
> > #include "base/gtest_prod_util.h"
> 
> Done.
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> File chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h (right):
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h:30: //
> BrowserContextKeyedBaseFactor methods:
> On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> > 1) Factor -> Factory?
> Done.
> > 2) These should all be protected, not private, right? What does it even mean
> to
> > override private virtuals? In general, prefer to keep the same-or-greater
> > visibility as the parent; reducing visibility in a derived class is like
using
> > the bathroom in a glass house - it gives the semblance of privacy, even
though
> > you're still exposed.
> I agree that it's weird, but I'm going to push back only because all the
> examples that I looked at and used as outlines for this class *do* make these
> methods private (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> or
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sup...
> for example, although I believe just about every other class that subclasses
> BrowserContextKeyedServiceFactory does this, from what I can tell).
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> File chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc (right):
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:56:
> reinterpret_cast<const char*>(google_der), sizeof(google_der)));
> On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> > Not your fault, but bonus points if you want to use
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/test/cert_test...
> > instead
> > 
> > (I'd like to get rid of test_certificate_data, because it's a pain to update
> > test data for - filed <crxref style="background-image:
url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/issue.png);">http://crbug.com/399122</crxref>
)
> 
> Done.
> 
>
https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
> chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc:171: // Verify the
> rovaction of the www_google_url decision does not affect the
> On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> > Revocation
> 
> Done.
> 
>
https://codereview.chromium.org/369703002/diff/240001/content/browser/ssl/ssl...
> File content/browser/ssl/ssl_host_state.h (right):
> 
>
https://codereview.chromium.org/369703002/diff/240001/content/browser/ssl/ssl...
> content/browser/ssl/ssl_host_state.h:85: BrowserContext* browser_context_;
> On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> > Given that SSLHostState SupportsUserData, should this be hung off there?
> > 
> > It feels weird to store this as a member, since you never use it.
> 
> This is leftover from when CloseIdleSockets was present and is now extraneous.
> I'm removing it.

Content Settings changes LGTM

[Torne, 2014-07-31 09:40:16]

https://codereview.chromium.org/369703002/diff/260001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/369703002/diff/260001/content/public/browser/...
content/public/browser/browser_context.h:155: // return NULL, except for testing
contexts.
You've written here that it's not valid to return NULL except for testing, but
the aw_browser_context implementation you added returns NULL. Are we going to
have to implement a real version of this function in android_webview, or is it
okay like this despite the comment? What is the actual effect of returning NULL
here?

[willchan no longer on Chromium, 2014-07-31 14:41:56]

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h (right):

https://codereview.chromium.org/369703002/diff/240001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h:30: //
BrowserContextKeyedBaseFactor methods:
On 2014/07/31 00:31:28, Ryan Sleevi wrote:
> 1) Factor -> Factory?
> 2) These should all be protected, not private, right? What does it even mean
to
> override private virtuals? In general, prefer to keep the same-or-greater
> visibility as the parent; reducing visibility in a derived class is like using
> the bathroom in a glass house - it gives the semblance of privacy, even though
> you're still exposed.

In answer to your question about reducing visibility from the base class, I
present this chromium-dev thread:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/CwzjcWd9cYM.
No comment on my personal opinion here.

In answer to your question about overriding private virtuals (but not about
reducing the visibility from the parent), I present Herb Sutter:

http://www.gotw.ca/publications/mill18.htm - "Guideline #2: Prefer to make
virtual functions private." and "Guideline #3: Only if derived classes need to
invoke the base implementation of a virtual function, make the virtual function
protected."

Overriding a private virtual function allows derived classes to override
specific hooks provided by base classes. But keeping it private means that only
the base class can control when this gets called.

class ThingThatFlies {
 public:
  void Fly() {
    PutOnAviatorSunglasses();
    FlyImpl();  // implemented by the derived class
    TakeOffAviatorSunglasses();
    PourSomeScotch();
  }

 private:
  // Derived classes can't call this impl. That's a good thing because
implementation inheritance is stupid.
  virtual void FlyImpl() { BoardAPlane(); }
};

class Bird : public ThingThatFlies {
 private:
  virtual void FlyImpl { FlapYoWingsLikeItsYoBirthday(); }
};

class Panda : public ThingThatFlies {
 private:
  virtual void FlyImpl { NOTREACHED() << "Pandas don't fly you insensitive
clod."; }
};

[jww, 2014-07-31 18:08:57]

https://codereview.chromium.org/369703002/diff/260001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/369703002/diff/260001/content/public/browser/...
content/public/browser/browser_context.h:155: // return NULL, except for testing
contexts.
On 2014/07/31 09:40:16, Torne wrote:
> You've written here that it's not valid to return NULL except for testing, but
> the aw_browser_context implementation you added returns NULL. Are we going to
> have to implement a real version of this function in android_webview, or is it
> okay like this despite the comment? What is the actual effect of returning
NULL
> here?

Great catch! Indeed, this was a problem, and I've just uploaded a fix. We
*should* allow NULL SSLHostStates, and that should imply a default "don't
remember anything" strategy, which is the current behavior of Android Web View
and Content Shell. In the Web View case, felt@ is toying with how we might
actually do interstitials, but that's a longer term plan, and definitely not
something to address in this CL.

[jam, 2014-07-31 22:01:09]

apologies for the delay. i've looked at content only. feel free to email me
directly or IM me when this is done and i'll look again

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
content/public/browser/browser_context.h:156: virtual SSLHostStateDecisions*
GetSSLHostStateDecisions() = 0;
per the other comment, this should be moved to a SetDelegate method on
SSLHostState

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
File content/public/browser/ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
content/public/browser/ssl_host_state_decisions.h:18: // storage strategy (if
any) for certificate decisions.
this sounds like it's required by the embedder, which is actually not true
looking at the code. so update the comment.

for background, that's the goal of the content api, which is for embedders to be
able to embed content easily, and then implement more interfaces to further
customize it

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
content/public/browser/ssl_host_state_decisions.h:19: class CONTENT_EXPORT
SSLHostStateDecisions {
for consistency, this should be something like SSLHostStateDelegate. you can add
SSLHostState in https://codereview.chromium.org/418133012/, and have a
SetDelegate method on it to take an implementation from the embedder

nit: no export needed

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
content/public/browser/ssl_host_state_decisions.h:21: virtual void
DenyCert(const GURL& url,
nit: please comment all these methods

[jww, 2014-07-31 23:53:10]

jam@, I haven't uploaded my new patch, and I'll wait to hear back on my one
question before I do so, to make sure we're on the same page.

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
content/public/browser/browser_context.h:156: virtual SSLHostStateDecisions*
GetSSLHostStateDecisions() = 0;
On 2014/07/31 22:01:08, jam wrote:
> per the other comment, this should be moved to a SetDelegate method on
> SSLHostState

Can you clarify what you mean? Since ChromeSSLHostStateDecisions is a keyed
browser service on BrowserContext (similar to SpecialStoragePolicy and
PushMessagingService), it seems like we need to have the
GetSSLHostStateDecisions()  method here as the other keyed browser services
have.

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
File content/public/browser/ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
content/public/browser/ssl_host_state_decisions.h:18: // storage strategy (if
any) for certificate decisions.
On 2014/07/31 22:01:08, jam wrote:
> this sounds like it's required by the embedder, which is actually not true
> looking at the code. so update the comment.
> 
> for background, that's the goal of the content api, which is for embedders to
be
> able to embed content easily, and then implement more interfaces to further
> customize it

Done.

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
content/public/browser/ssl_host_state_decisions.h:19: class CONTENT_EXPORT
SSLHostStateDecisions {
On 2014/07/31 22:01:08, jam wrote:
> for consistency, this should be something like SSLHostStateDelegate. you can
add
> SSLHostState in https://codereview.chromium.org/418133012/, and have a
> SetDelegate method on it to take an implementation from the embedder
I've changed it to SSLHostStateDecisionsDelegate since it's not a delegate for
*all* of SSLHostState, only the decisions portion.

I am a bit confused, though. I thought one of the points of the KeyedServices
was for providing singletons from the embedder, which this needs to be.

Is your suggestion that we create a static method on BrowserContext called
"CreateSSLHostStateDecisionsDelegate" or something like that, where we pass in
the SSLHostState and have it set the delegate? And if so, isn't that just
somewhat repeating the functionality of the KeyedServices?

Sorry for my confusion. I'm sure the answer is obvious, and it's just me being a
n00b to content :-)
> 
> nit: no export needed
Done.

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
content/public/browser/ssl_host_state_decisions.h:21: virtual void
DenyCert(const GURL& url,
On 2014/07/31 22:01:08, jam wrote:
> nit: please comment all these methods

Done.

[jww, 2014-08-01 00:21:35]

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
File content/public/browser/ssl_host_state_decisions.h (right):

https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
content/public/browser/ssl_host_state_decisions.h:19: class CONTENT_EXPORT
SSLHostStateDecisions {
On 2014/07/31 23:53:10, jww wrote:
> On 2014/07/31 22:01:08, jam wrote:
> > for consistency, this should be something like SSLHostStateDelegate. you can
> add
> > SSLHostState in https://codereview.chromium.org/418133012/, and have a
> > SetDelegate method on it to take an implementation from the embedder
> I've changed it to SSLHostStateDecisionsDelegate since it's not a delegate for
> *all* of SSLHostState, only the decisions portion.
> 
> I am a bit confused, though. I thought one of the points of the KeyedServices
> was for providing singletons from the embedder, which this needs to be.
> 
> Is your suggestion that we create a static method on BrowserContext called
> "CreateSSLHostStateDecisionsDelegate" or something like that, where we pass in
> the SSLHostState and have it set the delegate? And if so, isn't that just
> somewhat repeating the functionality of the KeyedServices?
> 
> Sorry for my confusion. I'm sure the answer is obvious, and it's just me being
a
> n00b to content :-)
> > 
> > nit: no export needed
> Done.

Okay, so I think I understand a bit better what you're suggesting. Basically,
implement something similar to how the DownloadManager works, in particular
GetDownloadManager on BrowserContext.

So the complication is that we won't need SSLHostState in content/public until
we finish https://codereview.chromium.org/418133012/, which is much lower
priority (it's sort of a nice addition to the experiment, but this CL, the
experiment itself, is of much higher priority).

Thus, my suggestion is, if you're cool with it, we hold off on making this a
proper delegate until we move SSLHostState to content/public, and we thus leave
SSLHostStateDecisions as is for now. Let me know if this doesn't make sense.

In either case, for now, I'll upload my patch with the other fixes.

[jww, 2014-08-01 00:37:12]

On 2014/08/01 00:21:35, jww wrote:
>
https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
> File content/public/browser/ssl_host_state_decisions.h (right):
> 
>
https://codereview.chromium.org/369703002/diff/340001/content/public/browser/...
> content/public/browser/ssl_host_state_decisions.h:19: class CONTENT_EXPORT
> SSLHostStateDecisions {
> On 2014/07/31 23:53:10, jww wrote:
> > On 2014/07/31 22:01:08, jam wrote:
> > > for consistency, this should be something like SSLHostStateDelegate. you
can
> > add
> > > SSLHostState in https://codereview.chromium.org/418133012/, and have a
> > > SetDelegate method on it to take an implementation from the embedder
> > I've changed it to SSLHostStateDecisionsDelegate since it's not a delegate
for
> > *all* of SSLHostState, only the decisions portion.
> > 
> > I am a bit confused, though. I thought one of the points of the
KeyedServices
> > was for providing singletons from the embedder, which this needs to be.
> > 
> > Is your suggestion that we create a static method on BrowserContext called
> > "CreateSSLHostStateDecisionsDelegate" or something like that, where we pass
in
> > the SSLHostState and have it set the delegate? And if so, isn't that just
> > somewhat repeating the functionality of the KeyedServices?
> > 
> > Sorry for my confusion. I'm sure the answer is obvious, and it's just me
being
> a
> > n00b to content :-)
> > > 
> > > nit: no export needed
> > Done.
> 
> Okay, so I think I understand a bit better what you're suggesting. Basically,
> implement something similar to how the DownloadManager works, in particular
> GetDownloadManager on BrowserContext.
> 
> So the complication is that we won't need SSLHostState in content/public until
> we finish https://codereview.chromium.org/418133012/, which is much lower
> priority (it's sort of a nice addition to the experiment, but this CL, the
> experiment itself, is of much higher priority).
> 
> Thus, my suggestion is, if you're cool with it, we hold off on making this a
> proper delegate until we move SSLHostState to content/public, and we thus
leave
> SSLHostStateDecisions as is for now. Let me know if this doesn't make sense.
> 
> In either case, for now, I'll upload my patch with the other fixes.

Arg, now I of course see the point that ssl_host_state_decisions.h is in
content/public :-P Okay, I'll get to work making SSLHostState content/public and
minimizing the interface.

[jam, 2014-08-01 03:15:13]

lgtm

[Torne, 2014-08-01 10:02:35]

android_webview LGTM

[jww, 2014-08-01 17:48:59]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-01 17:52:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/369703002/420001

[jww, 2014-08-01 18:00:49]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-01 18:01:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/369703002/440001

[jww, 2014-08-01 18:17:16]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-01 18:18:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/369703002/460001

[jww, 2014-08-01 21:47:47]

The CQ bit was unchecked by jww@chromium.org

[jww, 2014-08-01 21:47:47]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-01 21:53:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/369703002/460001

[jww, 2014-08-01 23:15:46]

The CQ bit was unchecked by jww@chromium.org

[jww, 2014-08-01 23:16:04]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-01 23:19:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/369703002/460001

[commit-bot: I haz the power, 2014-08-01 23:23:50]

Change committed as 287118

[benm (inactive), 2014-12-10 15:01:40]

benm@chromium.org changed reviewers:
+ benm@chromium.org

[benm (inactive), 2014-12-10 15:01:42]

https://codereview.chromium.org/369703002/diff/460001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_host_state.cc (right):

https://codereview.chromium.org/369703002/diff/460001/content/browser/ssl/ssl...
content/browser/ssl/ssl_host_state.cc:25: if (context)
should this check go up above? If context might be null, we are blindly derefing
it on line 23, right? Should this be rv->delegate we check?

[jww, 2014-12-10 23:20:06]

https://codereview.chromium.org/369703002/diff/460001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_host_state.cc (right):

https://codereview.chromium.org/369703002/diff/460001/content/browser/ssl/ssl...
content/browser/ssl/ssl_host_state.cc:25: if (context)
On 2014/12/10 15:01:42, benm wrote:
> should this check go up above? If context might be null, we are blindly
derefing
> it on line 23, right? Should this be rv->delegate we check?

Good catch, although this code was significantly refactored in
https://codereview.chromium.org/473643002 and this particular path doesn't exist
anymore.

[t.xheva_gmail.com, 2015-10-16 08:20:32]

Any Idea where I can find the flag Remember decisions to proceed through 
SSL errors for a specified length of time? I cannot find in Crome Version 
46.0.2490.71 m.


Thanks,

On Thursday, July 3, 2014 at 7:25:35 AM UTC+2, j...@chromium.org wrote:
>
> Reviewers: felt, Ryan Sleevi, 
>
> Message: 
> This is the implementation of invalid certificate memory being time based, 
>   
> but 
> put behind a flag. The experiment will come later, after we have the 
> revoke 
> button committed. felt@ and rsleevi, you seem like the most logical 
> initial 
> reviewers, so please let me know what you think. 
>
> Description: 
> Remember user decisions on invalid certificates behind a flag 
>
> Currently if a user clicks thorough an invalid certificate, the decision 
> is 
> remembered for the rest of the session, but after session restart, the   
> decision 
> is forgotten. This implements a flag that remembers the decision for a   
> specified 
> length of time across session restarts. Users can specific in the flag how 
>   
> long 
> decisions should be remembered, or they can specify that it should still 
> be 
> reset on restart. 
>
> BUG=262615 
>
> Please review this at https://codereview.chromium.org/369703002/ 
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src 
>
> Affected files (+1078, -238 lines): 
>    M chrome/app/generated_resources.grd 
>    M chrome/browser/about_flags.cc 
>    M chrome/browser/browsing_data/browsing_data_remover.cc 
>    M chrome/browser/content_settings/content_settings_default_provider.cc 
>    M chrome/browser/content_settings/content_settings_policy_provider.cc 
>    M chrome/browser/content_settings/content_settings_utils.cc 
>    M chrome/browser/content_settings/host_content_settings_map.cc 
>    M chrome/browser/profiles/off_the_record_profile_impl.h 
>    M chrome/browser/profiles/off_the_record_profile_impl.cc 
>    M chrome/browser/profiles/profile_impl.h 
>    M chrome/browser/profiles/profile_impl.cc 
>    A chrome/browser/ssl/chrome_ssl_host_state_decisions.h 
>    A chrome/browser/ssl/chrome_ssl_host_state_decisions.cc 
>    A chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h 
>    A chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.cc 
>    A chrome/browser/ssl/chrome_ssl_host_state_decisions_test.h 
>    A chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc 
>    M chrome/browser/ui/webui/options/content_settings_handler.cc 
>    M chrome/chrome_browser.gypi 
>    M chrome/chrome_tests.gypi 
>    M chrome/common/chrome_switches.h 
>    M chrome/common/chrome_switches.cc 
>    M chrome/common/content_settings_types.h 
>    M chrome/common/pref_names.h 
>    M chrome/common/pref_names.cc 
>    M chrome/test/base/testing_profile.h 
>    M chrome/test/base/testing_profile.cc 
>    M content/browser/download/download_manager_impl_unittest.cc 
>    A content/browser/ssl/ssl_blocking_proceed_decision_store.h 
>    A content/browser/ssl/ssl_blocking_proceed_decision_store.cc 
>    M content/browser/ssl/ssl_host_state.h 
>    M content/browser/ssl/ssl_host_state.cc 
>    D content/browser/ssl/ssl_host_state_unittest.cc 
>    M content/browser/ssl/ssl_policy.cc 
>    M content/browser/ssl/ssl_policy_backend.h 
>    M content/browser/ssl/ssl_policy_backend.cc 
>    M content/content_browser.gypi 
>    M content/content_tests.gypi 
>    M content/public/browser/browser_context.h 
>    A content/public/browser/ssl_host_state_decisions.h 
>    M content/public/test/test_browser_context.h 
>    M content/public/test/test_browser_context.cc 
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jww, 2015-10-16 22:38:36]

On 2015/10/16 08:20:32, t.xheva_gmail.com wrote:
> Any Idea where I can find the flag Remember decisions to proceed through 
> SSL errors for a specified length of time? I cannot find in Crome Version 
> 46.0.2490.71 m.
> 
> 
> Thanks,
> 
> On Thursday, July 3, 2014 at 7:25:35 AM UTC+2, mailto:j...@chromium.org wrote:
> >
> > Reviewers: felt, Ryan Sleevi, 
> >
> > Message: 
> > This is the implementation of invalid certificate memory being time based, 
> >   
> > but 
> > put behind a flag. The experiment will come later, after we have the 
> > revoke 
> > button committed. felt@ and rsleevi, you seem like the most logical 
> > initial 
> > reviewers, so please let me know what you think. 
> >
> > Description: 
> > Remember user decisions on invalid certificates behind a flag 
> >
> > Currently if a user clicks thorough an invalid certificate, the decision 
> > is 
> > remembered for the rest of the session, but after session restart, the   
> > decision 
> > is forgotten. This implements a flag that remembers the decision for a   
> > specified 
> > length of time across session restarts. Users can specific in the flag how 
> >   
> > long 
> > decisions should be remembered, or they can specify that it should still 
> > be 
> > reset on restart. 
> >
> > BUG=262615 
> >
> > Please review this at https://codereview.chromium.org/369703002/ 
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src 
> >
> > Affected files (+1078, -238 lines): 
> >    M chrome/app/generated_resources.grd 
> >    M chrome/browser/about_flags.cc 
> >    M chrome/browser/browsing_data/browsing_data_remover.cc 
> >    M chrome/browser/content_settings/content_settings_default_provider.cc 
> >    M chrome/browser/content_settings/content_settings_policy_provider.cc 
> >    M chrome/browser/content_settings/content_settings_utils.cc 
> >    M chrome/browser/content_settings/host_content_settings_map.cc 
> >    M chrome/browser/profiles/off_the_record_profile_impl.h 
> >    M chrome/browser/profiles/off_the_record_profile_impl.cc 
> >    M chrome/browser/profiles/profile_impl.h 
> >    M chrome/browser/profiles/profile_impl.cc 
> >    A chrome/browser/ssl/chrome_ssl_host_state_decisions.h 
> >    A chrome/browser/ssl/chrome_ssl_host_state_decisions.cc 
> >    A chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.h 
> >    A chrome/browser/ssl/chrome_ssl_host_state_decisions_factory.cc 
> >    A chrome/browser/ssl/chrome_ssl_host_state_decisions_test.h 
> >    A chrome/browser/ssl/chrome_ssl_host_state_decisions_test.cc 
> >    M chrome/browser/ui/webui/options/content_settings_handler.cc 
> >    M chrome/chrome_browser.gypi 
> >    M chrome/chrome_tests.gypi 
> >    M chrome/common/chrome_switches.h 
> >    M chrome/common/chrome_switches.cc 
> >    M chrome/common/content_settings_types.h 
> >    M chrome/common/pref_names.h 
> >    M chrome/common/pref_names.cc 
> >    M chrome/test/base/testing_profile.h 
> >    M chrome/test/base/testing_profile.cc 
> >    M content/browser/download/download_manager_impl_unittest.cc 
> >    A content/browser/ssl/ssl_blocking_proceed_decision_store.h 
> >    A content/browser/ssl/ssl_blocking_proceed_decision_store.cc 
> >    M content/browser/ssl/ssl_host_state.h 
> >    M content/browser/ssl/ssl_host_state.cc 
> >    D content/browser/ssl/ssl_host_state_unittest.cc 
> >    M content/browser/ssl/ssl_policy.cc 
> >    M content/browser/ssl/ssl_policy_backend.h 
> >    M content/browser/ssl/ssl_policy_backend.cc 
> >    M content/content_browser.gypi 
> >    M content/content_tests.gypi 
> >    M content/public/browser/browser_context.h 
> >    A content/public/browser/ssl_host_state_decisions.h 
> >    M content/public/test/test_browser_context.h 
> >    M content/public/test/test_browser_context.cc 
> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

The experiment has finished, so the flag was removed. Chrome now remembers
certificates for 1 week after an interstitial is clicked-through.