Add constructor to create SSLStatus from SSLInfo

Add constructor to create SSLStatus from SSLInfo

This constructor is now used from several places that set SSLStatus
fields themselves, to reduce the risk of such places forgetting to set
fields. This was causing bugs on WebsiteSettings for SSL interstitials
(which were not setting the connection_status field, among others).

BUG=529456
Committed: https://crrev.com/ced641eccd2a2f9e32ba173641ac6ddc68999f5c
Cr-Commit-Position: refs/heads/master@{#348519}

[estark, 2015-09-08 22:13:36]

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

[estark, 2015-09-08 22:13:36]

meacer@, could you please take a look?

[meacer, 2015-09-08 22:19:39]

https://codereview.chromium.org/1332633002/diff/1/chrome/browser/ssl/ssl_brow...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1332633002/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:2366: 
I think you should do a WaitForInterstitialAttach here, otherwise the test might
be flaky. Same goes for other tests in this file, but no need to worry about
them for now.

https://codereview.chromium.org/1332633002/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:2378: 
nit: Remove blank line, or move it above the previous line so that
after_interstitial_ssl_status code is grouped.

[estark, 2015-09-08 22:53:59]

https://codereview.chromium.org/1332633002/diff/1/chrome/browser/ssl/ssl_brow...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1332633002/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:2366: 
On 2015/09/08 22:19:39, Mustafa Emre Acer wrote:
> I think you should do a WaitForInterstitialAttach here, otherwise the test
might
> be flaky. Same goes for other tests in this file, but no need to worry about
> them for now.

Done.

https://codereview.chromium.org/1332633002/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:2378: 
On 2015/09/08 22:19:39, Mustafa Emre Acer wrote:
> nit: Remove blank line, or move it above the previous line so that
> after_interstitial_ssl_status code is grouped.

Done.

[meacer, 2015-09-08 23:05:54]

LGTM for SSL interstitial, but looks like we might want a stricter fix for other
interstitials.

https://codereview.chromium.org/1332633002/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/1332633002/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:302: entry->GetSSL().connection_status =
ssl_info_.connection_status;
Looks like BadClockBlockingPage doesn't set this either. Should it? In fact any
time we add a new field to SSL status we'll need to go fix all OverrideEntry
instances. WDYT about doing something like this?:

entry->GetSSL() = MakeSSLStatus(ssl_info, cert_id, AUTH_BROKEN)

[estark, 2015-09-08 23:19:58]

https://codereview.chromium.org/1332633002/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/1332633002/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:302: entry->GetSSL().connection_status =
ssl_info_.connection_status;
On 2015/09/08 23:05:54, Mustafa Emre Acer wrote:
> Looks like BadClockBlockingPage doesn't set this either. Should it? In fact
any
> time we add a new field to SSL status we'll need to go fix all OverrideEntry
> instances.

Ouch, good point. BadClockBlockingPage does need to set this as well.

> WDYT about doing something like this?:
> 
> entry->GetSSL() = MakeSSLStatus(ssl_info, cert_id, AUTH_BROKEN)

That seems reasonable. Do you think MakeSSLStatus() should go in ssl_status.h?

[meacer, 2015-09-08 23:23:19]

On 2015/09/08 23:19:58, estark wrote:
>
https://codereview.chromium.org/1332633002/diff/20001/chrome/browser/ssl/ssl_...
> File chrome/browser/ssl/ssl_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/1332633002/diff/20001/chrome/browser/ssl/ssl_...
> chrome/browser/ssl/ssl_blocking_page.cc:302: entry->GetSSL().connection_status
=
> ssl_info_.connection_status;
> On 2015/09/08 23:05:54, Mustafa Emre Acer wrote:
> > Looks like BadClockBlockingPage doesn't set this either. Should it? In fact
> any
> > time we add a new field to SSL status we'll need to go fix all OverrideEntry
> > instances.
> 
> Ouch, good point. BadClockBlockingPage does need to set this as well.
> 
> > WDYT about doing something like this?:
> > 
> > entry->GetSSL() = MakeSSLStatus(ssl_info, cert_id, AUTH_BROKEN)
> 
> That seems reasonable. Do you think MakeSSLStatus() should go in ssl_status.h?

Sure. Though I suppose a single method won't suffice and you'll need to write
one or two overrides of that method.

[estark, 2015-09-09 03:37:05]

meacer, PTAL at the latest patchset (+ updated CL description)? I added a new
content::SSLStatus constructor. In addition to fixing the connection_status bug,
it also fixes the same bug with signed_certificate_timestamp_ids.

There is still a fair amount of code duplication between
SSLBlockingPage::OverrideEntry() and BadClockBlockingPage::OverrideEntry(), but
I think it's less error-prone than it was before before they are no longer
enumerating the fields of SSLStatus. What do you think?

[meacer, 2015-09-09 18:26:08]

Looks good, though looks like one of the tests failed on the trybot. Once you
got that fixed, I'll l g t m.

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/bad_...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/bad_...
chrome/browser/ssl/bad_clock_blocking_page.cc:281: int cert_id =
content::CertStore::GetInstance()->StoreCert(
How about consting process_id and cert_id too, since you have const int sct_id
below?

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/bad_...
chrome/browser/ssl/bad_clock_blocking_page.cc:293: }
Hah, I remember trying to do something like this in a very old CL which I never
got to land
(https://codereview.chromium.org/403933002/diff/180001/content/browser/loader/...)

Not that you need to change anything here.

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:295: int cert_id =
content::CertStore::GetInstance()->StoreCert(
nit: const?

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2382: LOG(ERROR) << "Interstitial
status: " << interstitial_ssl_status.security_style << ", " <<
interstitial_ssl_status.cert_id << ", " << interstitial_ssl_status.cert_status
<< ", " << interstitial_ssl_status.security_bits << ", " <<
interstitial_ssl_status.connection_status << ", " <<
interstitial_ssl_status.content_status;
I see you're done debugging on the trybots :) Don't forget to remove the log
statements.

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2395: scoped_ptr<base::SimpleTestClock>
mock_clock(new base::SimpleTestClock());
nit: This could just be |base::SimpleTestClock mock_clock;| right?

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2415: // and proceed through it.
nit: Seems like this part is extra on top of the previous test. Can you add a
second line to the outer comment mentioning this?

https://codereview.chromium.org/1332633002/diff/80001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_policy.cc (right):

https://codereview.chromium.org/1332633002/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_policy.cc:182: net::CertStatus cert_status) {
Seems like these can fit in a single line? ("Minimize use of vertical
whitespace." clause from style guide)

https://codereview.chromium.org/1332633002/diff/80001/content/public/common/s...
File content/public/common/ssl_status.cc (right):

https://codereview.chromium.org/1332633002/diff/80001/content/public/common/s...
content/public/common/ssl_status.cc:23: SignedCertificateTimestampIDStatusList
signed_certificate_timestamp_ids,
You can pass signed_certificate_timestamp_ids as const ref

https://codereview.chromium.org/1332633002/diff/80001/content/public/common/s...
File content/public/common/ssl_status.h (right):

https://codereview.chromium.org/1332633002/diff/80001/content/public/common/s...
content/public/common/ssl_status.h:44: bool Equals(const SSLStatus& status)
const {
Off topic: Odd, I'm wondering why this isn't an operator overload for == (there
probably is a good reason)

[estark, 2015-09-10 14:32:05]

Fixed the tests on the trybots, but in a sort of weird way. Turns out the cert
ids aren't guaranteed to be the same just because the certs are. So in the tests
I'm now comparing the actual certs for equality, then zeroing out the cert_id
fields and comparing the rest of the SSLStatus. WDYT?

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/bad_...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/bad_...
chrome/browser/ssl/bad_clock_blocking_page.cc:281: int cert_id =
content::CertStore::GetInstance()->StoreCert(
On 2015/09/09 18:26:07, Mustafa Emre Acer wrote:
> How about consting process_id and cert_id too, since you have const int sct_id
> below?

Done.

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:295: int cert_id =
content::CertStore::GetInstance()->StoreCert(
On 2015/09/09 18:26:07, Mustafa Emre Acer wrote:
> nit: const?

Done.

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2382: LOG(ERROR) << "Interstitial
status: " << interstitial_ssl_status.security_style << ", " <<
interstitial_ssl_status.cert_id << ", " << interstitial_ssl_status.cert_status
<< ", " << interstitial_ssl_status.security_bits << ", " <<
interstitial_ssl_status.connection_status << ", " <<
interstitial_ssl_status.content_status;
On 2015/09/09 18:26:07, Mustafa Emre Acer wrote:
> I see you're done debugging on the trybots :) Don't forget to remove the log
> statements.

Done.

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2395: scoped_ptr<base::SimpleTestClock>
mock_clock(new base::SimpleTestClock());
On 2015/09/09 18:26:07, Mustafa Emre Acer wrote:
> nit: This could just be |base::SimpleTestClock mock_clock;| right?

Done.

https://codereview.chromium.org/1332633002/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2415: // and proceed through it.
On 2015/09/09 18:26:07, Mustafa Emre Acer wrote:
> nit: Seems like this part is extra on top of the previous test. Can you add a
> second line to the outer comment mentioning this?

Done.

https://codereview.chromium.org/1332633002/diff/80001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_policy.cc (right):

https://codereview.chromium.org/1332633002/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_policy.cc:182: net::CertStatus cert_status) {
On 2015/09/09 18:26:07, Mustafa Emre Acer wrote:
> Seems like these can fit in a single line? ("Minimize use of vertical
> whitespace." clause from style guide)

'git cl format' seems to like it this way. (They do go over 80 chars in a single
line.)

https://codereview.chromium.org/1332633002/diff/80001/content/public/common/s...
File content/public/common/ssl_status.cc (right):

https://codereview.chromium.org/1332633002/diff/80001/content/public/common/s...
content/public/common/ssl_status.cc:23: SignedCertificateTimestampIDStatusList
signed_certificate_timestamp_ids,
On 2015/09/09 18:26:08, Mustafa Emre Acer wrote:
> You can pass signed_certificate_timestamp_ids as const ref

Done.

https://codereview.chromium.org/1332633002/diff/80001/content/public/common/s...
File content/public/common/ssl_status.h (right):

https://codereview.chromium.org/1332633002/diff/80001/content/public/common/s...
content/public/common/ssl_status.h:44: bool Equals(const SSLStatus& status)
const {
On 2015/09/09 18:26:08, Mustafa Emre Acer wrote:
> Off topic: Odd, I'm wondering why this isn't an operator overload for ==
(there
> probably is a good reason)

Yeah I wondered that too... no idea.

[meacer, 2015-09-10 17:55:22]

Looks good. Two more nits.

https://codereview.chromium.org/1332633002/diff/100001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1332633002/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:251: EXPECT_TRUE(cert1);
nit: EXPECT_TRUE(cert1 && cert2)?

https://codereview.chromium.org/1332633002/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:257: two_without_cert_id.cert_id = 0;
nit: I'm wondering if these should be done in SSLStatus::Equals. You could
simply skip cert_id check there and add a comment explaining why.

Actually, you could go one step further and fix SSLStatus::Equals altogether by
moving the RetrieveCert code above to that function.

(I'm trying to patch your CL and see if doing that will break any tests, but
it's taking too long and I wanted to send my comments earlier)

[estark, 2015-09-10 17:59:28]

https://codereview.chromium.org/1332633002/diff/100001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1332633002/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:257: two_without_cert_id.cert_id = 0;
On 2015/09/10 17:55:22, Mustafa Emre Acer wrote:
> nit: I'm wondering if these should be done in SSLStatus::Equals. You could
> simply skip cert_id check there and add a comment explaining why.
> 
> Actually, you could go one step further and fix SSLStatus::Equals altogether
by
> moving the RetrieveCert code above to that function.
> 
> (I'm trying to patch your CL and see if doing that will break any tests, but
> it's taking too long and I wanted to send my comments earlier)

The bummer is that SSLStatus is in content/public/common, and CertStore only
exists in content/public/browser. :( So I don't think it would be possible to
expose this kind of equality test to the renderer, because the CertStore doesn't
exist in the renderer.

[meacer, 2015-09-10 18:02:23]

Lgtm

https://codereview.chromium.org/1332633002/diff/100001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1332633002/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:257: two_without_cert_id.cert_id = 0;
On 2015/09/10 17:59:28, estark wrote:
> On 2015/09/10 17:55:22, Mustafa Emre Acer wrote:
> > nit: I'm wondering if these should be done in SSLStatus::Equals. You could
> > simply skip cert_id check there and add a comment explaining why.
> > 
> > Actually, you could go one step further and fix SSLStatus::Equals altogether
> by
> > moving the RetrieveCert code above to that function.
> > 
> > (I'm trying to patch your CL and see if doing that will break any tests, but
> > it's taking too long and I wanted to send my comments earlier)
> 
> The bummer is that SSLStatus is in content/public/common, and CertStore only
> exists in content/public/browser. :( So I don't think it would be possible to
> expose this kind of equality test to the renderer, because the CertStore
doesn't
> exist in the renderer.

Ah, right. I suppose we could make SSLStatus::Equals pure virtual method, and
then derive our own ChromeSSLStatus and implement that and so on to make this
even more complicated.

Let's go with this one instead :)

[estark, 2015-09-11 05:22:52]

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

[estark, 2015-09-11 05:22:53]

Thanks Mustafa!

avi: could you please review changes in content? Context: SSLBlockingPage had a
place where it was setting each of the fields on an SSLStatus, but it had
forgotten to set |connection_status|, which was causing a UI bug. To fix this I
introduced a new SSLStatus constructor to create an SSLStatus from a
net::SSLInfo, to make it harder for callers to forget to set SSLStatus fields
when creating them. I also fixed the fact that SSLStatus::Equals was neglecting
to check the |connection_status| fields for equality.

https://codereview.chromium.org/1332633002/diff/100001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1332633002/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:251: EXPECT_TRUE(cert1);
On 2015/09/10 17:55:22, Mustafa Emre Acer wrote:
> nit: EXPECT_TRUE(cert1 && cert2)?

Done.

[Avi (use Gerrit), 2015-09-11 18:03:20]

lgtm

[estark, 2015-09-11 18:07:01]

The CQ bit was checked by estark@chromium.org

[estark, 2015-09-11 18:07:01]

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

[commit-bot: I haz the power, 2015-09-11 18:07:51]

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

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

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

[commit-bot: I haz the power, 2015-09-11 19:30:55]

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

[estark, 2015-09-11 20:35:49]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-09-11 20:36:28]

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

[commit-bot: I haz the power, 2015-09-11 22:37:02]

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

[commit-bot: I haz the power, 2015-09-11 22:37:03]

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

[estark, 2015-09-12 01:11:03]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-09-12 01:11:44]

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

[commit-bot: I haz the power, 2015-09-12 02:17:41]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-12 02:18:13]

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

[commit-bot: I haz the power, 2015-09-23 12:27:42]

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