Attach a SecurityStyle to each request in ResourceLoader

Attach a SecurityStyle to each request in ResourceLoader

This CL adds a SecurityStyle to the serialized security information that
is sent with each request from the browser to the renderer. The
SecurityStyle describes the individual resource, not any bigger-picture
concerns like mixed content. The per-request SecurityStyle will be
displayed in DevTools to help developers diagnose SSL issues on
subresources.

BUG=502118, 445234
Committed: https://crrev.com/5318895a4dd623caf5d152461684935c6e874e12
Cr-Commit-Position: refs/heads/master@{#340762}

Committed: https://crrev.com/294fbd93323cef34063eb85ba2768d5fcc9b3299
Cr-Commit-Position: refs/heads/master@{#341375}

[estark, 2015-07-21 22:08:58]

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

[estark, 2015-07-21 22:08:58]

creis, do you think you could please take a look? This is the first part of
attaching security styles to resources as discussed in email thread "question
about computing a SecurityStyle for a resource"). Thanks!

[Charlie Reis, 2015-07-21 22:18:00]

creis@chromium.org changed reviewers:
+ davidben@chromium.org

[Charlie Reis, 2015-07-21 22:18:01]

I'm generally not a good reviewer for ResourceLoader or SSLManager, so I'll
suggest davidben@ instead.  Are there specific things you wanted me to take a
look at?

[estark, 2015-07-21 22:25:16]

Ok, thanks Charlie. David, do you think you'll have time for this one?

If not, maybe I can rope in some combination of palmer + avi... (avi because he
was doing some work on SSL status deserialization recently, palmer as a
//content/browser/ssl OWNER and someone who sits next to me so easy to explain
context)

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:124: if (!UpdateEntry(entry))
If you're wondering why this change is necessary... before, the assignment on
line 120 meant that entry->GetSSL().security_style was UNKNOWN before calling
UpdateEntry(). (Since |details.ssl_status| comes from the deserialized renderer
information, which always had a style of UNKNOWN before this CL.) So
UpdateEntry() would assign a SecurityStyle, and then observe that the SSLStatus
had changed so it was time to call DidChangeVisibleSSLState().

Now that the deserialized info contains a security style, we need to make sure
that DidChangeVisibleSSLState() gets called when navigating.

[estark, 2015-07-21 22:44:10]

CC'ing lgarron since he's also working on this devtools project

[estark, 2015-07-22 00:13:30]

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

[estark, 2015-07-22 00:13:30]

Adding palmer@ because he expressed interest. (Thanks, palmer!) I haven't
assigned reviewers for the subsequent CLs in this series yet in case comments on
this one cause me to change stuff on them, but feel free to add yourself to any
of them.

[davidben, 2015-07-22 20:56:58]

https://codereview.chromium.org/1244863003/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1244863003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2954: // Back to verify that the good
security style is seen again.
Optional: the good -> the previous good? Makes it clearer that you're not
expecting a good security style from the bad HTTPS page, but from the valid
HTTPS URL from earlier.

https://codereview.chromium.org/1244863003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2960:
expired_url.ReplaceComponents(replace_expired);
Is the base URL being expired_url important, rather than using
https_test_server? Not a huge deal, but it seems you're just interested in the
name mismatch, and not that it's expired.

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:124: if (!UpdateEntry(entry))
On 2015/07/21 22:25:16, estark wrote:
> If you're wondering why this change is necessary... before, the assignment on
> line 120 meant that entry->GetSSL().security_style was UNKNOWN before calling
> UpdateEntry(). (Since |details.ssl_status| comes from the deserialized
renderer
> information, which always had a style of UNKNOWN before this CL.) So
> UpdateEntry() would assign a SecurityStyle, and then observe that the
SSLStatus
> had changed so it was time to call DidChangeVisibleSSLState().
> 
> Now that the deserialized info contains a security style, we need to make sure
> that DidChangeVisibleSSLState() gets called when navigating.

This probably wants a comment. It took me a while to realize that what you were
doing is to fire NotifyDidChangeVisibleSSLState for line 120 if UpdateEntry
*didn't* already fire it for you. (It's also not quite right if entry is NULL or
if details.is_main_frame is true, right? Though we probably don't care much
there.)

Another possibility is to call policy()->UpdateEntry directly so that
DidChangeVisibleSSLState isn't automatically notified. And then the outer
function can coalesce and notify just once; the entirety of UpdateEntry seems to
just exist to do the automatic notifying anyway.

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

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_policy.cc:185: if
(!net::IsCertStatusMinorError(ssl.cert_status))
Nit: You can fold this to

// Minor blah blah
if (net::IsCertStatusError(...) &&
    !net::IsCertStatusMinorError(...)) {
  return ...
}

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_policy.cc:260:
GetSecurityStyleForResource(entry->GetURL(), entry->GetSSL());
Do you know if this code can ever run, now that security_style flows through the
renderer instead? Maybe for the initial state of things?

[Checked codesearch and it doesn't appear possible to update cert_id or
cert_status and not security_style, which would be one way this might change
behavior. Before, UpdateEntry would pick up the implications for security_style
while this new code won't. But it doesn't seem that ever happens, so it should
be fine. Not updating them in tandem seems scary.]

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_policy.h (right):

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_policy.h:44: // |web_contents| is the WebContents
associated with this entry.
(Either's fine, but code can freely assume WebContentses are backed by
WebContentsImpls. There's, sadly, a few cases where tests break this, but it
should be true for WebContents.)

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:17: return true;
Can a renderer ever legally send this one now? I guess tightening that is a
sufficiently more violent change that probably it should be separate.

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:83: ssl_status->security_style =
static_cast<SecurityStyle>(security_style);
NOTE: We are now trusting the renderer to set security_style, which means the
browser process can't completely explode if security_style is inconsistent with
the rest of the SSLStatus. (This is probably fine?)

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
File content/common/ssl_status_serialization.h (right):

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
content/common/ssl_status_serialization.h:17: CONTENT_EXPORT std::string
SerializeSecurityInfo(
Why not just take an SSLStatus as input?

[palmer, 2015-07-22 22:30:00]

https://codereview.chromium.org/1244863003/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1244863003/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:87: for
(net::SignedCertificateTimestampAndStatusList::const_iterator iter =
Nit: Could use auto here.

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:24: case
content::SECURITY_STYLE_AUTHENTICATED:
Nit: Maybe just implement these as fall-throughs?

[estark, 2015-07-22 22:56:55]

Thanks David and Chris.

https://codereview.chromium.org/1244863003/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1244863003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2954: // Back to verify that the good
security style is seen again.
On 2015/07/22 20:56:57, David Benjamin wrote:
> Optional: the good -> the previous good? Makes it clearer that you're not
> expecting a good security style from the bad HTTPS page, but from the valid
> HTTPS URL from earlier.

Done.

https://codereview.chromium.org/1244863003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2960:
expired_url.ReplaceComponents(replace_expired);
On 2015/07/22 20:56:57, David Benjamin wrote:
> Is the base URL being expired_url important, rather than using
> https_test_server? Not a huge deal, but it seems you're just interested in the
> name mismatch, and not that it's expired.

Done. Using https_test_server is probably much better because it means the test
doesn't rely on Chrome prioritizing COMMON_NAME_INVALID over DATE_INVALID.

https://codereview.chromium.org/1244863003/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1244863003/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:87: for
(net::SignedCertificateTimestampAndStatusList::const_iterator iter =
On 2015/07/22 22:30:00, palmer wrote:
> Nit: Could use auto here.

Done.

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:124: if (!UpdateEntry(entry))
On 2015/07/22 20:56:57, David Benjamin wrote:
> On 2015/07/21 22:25:16, estark wrote:
> > If you're wondering why this change is necessary... before, the assignment
on
> > line 120 meant that entry->GetSSL().security_style was UNKNOWN before
calling
> > UpdateEntry(). (Since |details.ssl_status| comes from the deserialized
> renderer
> > information, which always had a style of UNKNOWN before this CL.) So
> > UpdateEntry() would assign a SecurityStyle, and then observe that the
> SSLStatus
> > had changed so it was time to call DidChangeVisibleSSLState().
> > 
> > Now that the deserialized info contains a security style, we need to make
sure
> > that DidChangeVisibleSSLState() gets called when navigating.
> 
> This probably wants a comment. It took me a while to realize that what you
were
> doing is to fire NotifyDidChangeVisibleSSLState for line 120 if UpdateEntry
> *didn't* already fire it for you. (It's also not quite right if entry is NULL
or
> if details.is_main_frame is true, right? Though we probably don't care much
> there.)
> 
> Another possibility is to call policy()->UpdateEntry directly so that
> DidChangeVisibleSSLState isn't automatically notified. And then the outer
> function can coalesce and notify just once; the entirety of UpdateEntry seems
to
> just exist to do the automatic notifying anyway.

Done. (Changed it to call policy()->UpdateEntry() directly and added a comment.)

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

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_policy.cc:185: if
(!net::IsCertStatusMinorError(ssl.cert_status))
On 2015/07/22 20:56:57, David Benjamin wrote:
> Nit: You can fold this to
> 
> // Minor blah blah
> if (net::IsCertStatusError(...) &&
>     !net::IsCertStatusMinorError(...)) {
>   return ...
> }

Done.

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_policy.cc:260:
GetSecurityStyleForResource(entry->GetURL(), entry->GetSSL());
On 2015/07/22 20:56:57, David Benjamin wrote:
> Do you know if this code can ever run, now that security_style flows through
the
> renderer instead? Maybe for the initial state of things?

I think the only way it can run right now is 1. a misbehaving renderer (which,
as you pointed out elsewhere, we should probably disallow), or 2. if it is
somehow possible for the first committed navigation to not be a |is_main_frame|
navigation. The latter sounds like a weird thing but I can't find any evidence
that it's impossible.

> 
> [Checked codesearch and it doesn't appear possible to update cert_id or
> cert_status and not security_style, which would be one way this might change
> behavior. Before, UpdateEntry would pick up the implications for
security_style
> while this new code won't. But it doesn't seem that ever happens, so it should
> be fine. Not updating them in tandem seems scary.]

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_policy.h (right):

https://codereview.chromium.org/1244863003/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_policy.h:44: // |web_contents| is the WebContents
associated with this entry.
On 2015/07/22 20:56:58, David Benjamin wrote:
> (Either's fine, but code can freely assume WebContentses are backed by
> WebContentsImpls. There's, sadly, a few cases where tests break this, but it
> should be true for WebContents.)

Yeah, I just changed it here to avoid an unnecessary cast.

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:17: return true;
On 2015/07/22 20:56:58, David Benjamin wrote:
> Can a renderer ever legally send this one now? I guess tightening that is a
> sufficiently more violent change that probably it should be separate.

Agree on both counts. I added that to crbug.com/508232 and can do it in a
follow-up CL. The next CL in this series allows the embedder to override the
security style for a resource, and I think I should tighten that to not allow
the embedder to set UNKNOWN as well.

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:24: case
content::SECURITY_STYLE_AUTHENTICATED:
On 2015/07/22 22:30:00, palmer wrote:
> Nit: Maybe just implement these as fall-throughs?

Done (I think -- can you please check that I did what you meant?)

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:83: ssl_status->security_style =
static_cast<SecurityStyle>(security_style);
On 2015/07/22 20:56:58, David Benjamin wrote:
> NOTE: We are now trusting the renderer to set security_style, which means the
> browser process can't completely explode if security_style is inconsistent
with
> the rest of the SSLStatus. (This is probably fine?)

I took a quick look through and this seems fine. I added a comment to the header
file to document it. There are definitely places where the browser would end up
somewhat confused (e.g. displaying confused security UI), but I don't think
that's fundamentally worse off than we currently are where the renderer can set
the rest of the fields. (As you said, the thing to worry about is probably the
browser exploding, which it doesn't look like it will.)

crbug.com/508232 is the general fix for this.

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
File content/common/ssl_status_serialization.h (right):

https://codereview.chromium.org/1244863003/diff/40001/content/common/ssl_stat...
content/common/ssl_status_serialization.h:17: CONTENT_EXPORT std::string
SerializeSecurityInfo(
On 2015/07/22 20:56:58, David Benjamin wrote:
> Why not just take an SSLStatus as input?

Done.

[estark, 2015-07-24 17:36:22]

ping?

[davidben, 2015-07-24 22:21:52]

lgtm, although note that the first comment is actually rather important. :-)

https://codereview.chromium.org/1244863003/diff/80001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/1244863003/diff/80001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:23: return true;
Doesn't this just always return true now? Probably you want return true on line
21 and return false on line 23. (Worth a unit test?)

https://codereview.chromium.org/1244863003/diff/80001/content/common/ssl_stat...
File content/common/ssl_status_serialization.h (right):

https://codereview.chromium.org/1244863003/diff/80001/content/common/ssl_stat...
content/common/ssl_status_serialization.h:17: // |ssl_status.content_status| in
the serialized info.
[Oh, I didn't realize one field was omitted. Meh, this is probably still
cleaner. I'm likely going to need to split up this struct a bit for PlzNavigate
later anyway. I'm thinking browser-side gets a scoped_refptr<X509Certificate>
and it only turns into cert_id when we send it to a renderer.
https://crbug.com/513315

Probably it makes sense to eventually pull content_status outside of SSLStatus
and either have it be a dedicated field of the NavigationEntry or we have
different structs for resource-level security status and the combined page-level
stuff, similar to what was going on with SSLInfo.]

[estark, 2015-07-25 00:38:00]

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

[estark, 2015-07-25 00:38:01]

Thanks, David!

msw: could you please review //chrome/browser/ui/browser_browsertest.cc?

https://codereview.chromium.org/1244863003/diff/80001/content/common/ssl_stat...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/1244863003/diff/80001/content/common/ssl_stat...
content/common/ssl_status_serialization.cc:23: return true;
On 2015/07/24 22:21:52, David Benjamin wrote:
> Doesn't this just always return true now? Probably you want return true on
line
> 21 and return false on line 23. (Worth a unit test?)

Ahhh, oops, done + unit test.

https://codereview.chromium.org/1244863003/diff/80001/content/common/ssl_stat...
File content/common/ssl_status_serialization.h (right):

https://codereview.chromium.org/1244863003/diff/80001/content/common/ssl_stat...
content/common/ssl_status_serialization.h:17: // |ssl_status.content_status| in
the serialized info.
On 2015/07/24 22:21:52, David Benjamin wrote:
> [Oh, I didn't realize one field was omitted. Meh, this is probably still
> cleaner. I'm likely going to need to split up this struct a bit for
PlzNavigate
> later anyway. I'm thinking browser-side gets a scoped_refptr<X509Certificate>
> and it only turns into cert_id when we send it to a renderer.
> https://crbug.com/513315
> 
> Probably it makes sense to eventually pull content_status outside of SSLStatus
> and either have it be a dedicated field of the NavigationEntry or we have
> different structs for resource-level security status and the combined
page-level
> stuff, similar to what was going on with SSLInfo.]

Yeah, I definitely agree, it's very confusing right now to have |content_status|
-- which only applies to a NavigationEntry -- lumped together with the rest of
the stuff that describes an individual request.

I can possibly do this as a follow-up depending on how quickly you think you'll
get to the PlzNavigate-related split.

[palmer, 2015-07-27 22:25:45]

LGTM with a question.

https://codereview.chromium.org/1244863003/diff/120001/content/common/ssl_sta...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/1244863003/diff/120001/content/common/ssl_sta...
content/common/ssl_status_serialization.cc:73: ssl_status->security_style =
static_cast<SecurityStyle>(security_style);
Is it necessary to validate this cast/conversion?

[estark, 2015-07-27 22:49:38]

Thanks palmer.

https://codereview.chromium.org/1244863003/diff/120001/content/common/ssl_sta...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/1244863003/diff/120001/content/common/ssl_sta...
content/common/ssl_status_serialization.cc:73: ssl_status->security_style =
static_cast<SecurityStyle>(security_style);
On 2015/07/27 22:25:45, palmer wrote:
> Is it necessary to validate this cast/conversion?

That's what I meant to do with CheckSecurityStyle() on line 68 -- or do you mean
something else?

[palmer, 2015-07-27 22:57:20]

https://codereview.chromium.org/1244863003/diff/120001/content/common/ssl_sta...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/1244863003/diff/120001/content/common/ssl_sta...
content/common/ssl_status_serialization.cc:73: ssl_status->security_style =
static_cast<SecurityStyle>(security_style);
> That's what I meant to do with CheckSecurityStyle() on line 68 -- or do you
mean
> something else?

Nope, I'm just a goat. In fact I even saw it; it just left my mind between
reading like 68 and 73. :|

[estark, 2015-07-27 22:57:58]

https://codereview.chromium.org/1244863003/diff/120001/content/common/ssl_sta...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/1244863003/diff/120001/content/common/ssl_sta...
content/common/ssl_status_serialization.cc:73: ssl_status->security_style =
static_cast<SecurityStyle>(security_style);
On 2015/07/27 22:57:20, palmer wrote:
> > That's what I meant to do with CheckSecurityStyle() on line 68 -- or do you
> mean
> > something else?
> 
> Nope, I'm just a goat. In fact I even saw it; it just left my mind between
> reading like 68 and 73. :| 

goat af!

[msw, 2015-07-28 18:40:12]

chrome/browser/ui/browser_browsertest.cc lgtm with a nit.

https://codereview.chromium.org/1244863003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1244863003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_browsertest.cc:2967:
content::Source<NavigationController>(&browser()
nit: use |web_contents->GetController()| here?

[estark, 2015-07-28 19:37:52]

Thanks msw.

https://codereview.chromium.org/1244863003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1244863003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_browsertest.cc:2967:
content::Source<NavigationController>(&browser()
On 2015/07/28 18:40:12, msw wrote:
> nit: use |web_contents->GetController()| here?

Done.

[estark, 2015-07-28 19:37:57]

The CQ bit was checked by estark@chromium.org

[estark, 2015-07-28 19:37:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, palmer@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1244863003/#ps160001
(title: "msw comment")

[commit-bot: I haz the power, 2015-07-28 19:38:20]

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

[commit-bot: I haz the power, 2015-07-28 20:56:28]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-07-28 20:57:05]

Patchset 9 (id:??) landed as
https://crrev.com/5318895a4dd623caf5d152461684935c6e874e12
Cr-Commit-Position: refs/heads/master@{#340762}

[estark, 2015-07-28 22:39:06]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1259253009/ by estark@chromium.org.

The reason for reverting is: SecurityStyleChanged browser test is flaky after
this change: http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests.

[estark, 2015-07-29 04:43:24]

Patchset #12 (id:220001) has been deleted

[estark, 2015-07-30 01:25:13]

Patchset #13 (id:260001) has been deleted

[estark, 2015-07-30 01:25:24]

Patchset #12 (id:240001) has been deleted

[estark, 2015-07-30 05:52:24]

Patchset #15 (id:340001) has been deleted

[estark, 2015-07-31 00:31:09]

Patchset #22 (id:480001) has been deleted

[estark, 2015-07-31 15:05:18]

Patchset #22 (id:500001) has been deleted

[estark, 2015-07-31 15:05:25]

Patchset #21 (id:460001) has been deleted

[estark, 2015-07-31 15:05:32]

Patchset #20 (id:420022) has been deleted

[estark, 2015-07-31 15:05:39]

Patchset #19 (id:430001) has been deleted

[estark, 2015-07-31 15:05:47]

Patchset #18 (id:410001) has been deleted

[estark, 2015-07-31 15:05:57]

Patchset #17 (id:240002) has been deleted

[estark, 2015-07-31 15:06:10]

Patchset #16 (id:380001) has been deleted

[estark, 2015-07-31 15:06:16]

Patchset #15 (id:360001) has been deleted

[estark, 2015-07-31 15:06:22]

Patchset #14 (id:320001) has been deleted

[estark, 2015-07-31 15:06:32]

Patchset #13 (id:300001) has been deleted

[estark, 2015-07-31 15:06:37]

Patchset #12 (id:280001) has been deleted

[estark, 2015-07-31 17:31:54]

On 2015/07/28 22:39:06, estark wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/1259253009/ by mailto:estark@chromium.org.
> 
> The reason for reverting is: SecurityStyleChanged browser test is flaky after
> this change:
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests.

Sadly, I haven't yet been able to get to the root cause of this browser test
flakiness. (I can only reproduce it on the bot, not locally, which has made it
hard to investigate.) I've basically been doing a lot of printf debugging on the
bot, and the test seems to get stuck inside the SSL handshake, before getting to
DoVerifyCert(), when navigating to the broken HTTPS site. For some reason, using
a separate SpawnedTestServer for the broken HTTPS site seems to fix it.

It's hard to be sure, but it seems like there may be a SpawnedTestServer or net
stack bug that's causing this, since it's hard to imagine (for me, at least) how
this change could be causing a connection to get stuck inside the SSL handshake.
I'm going to re-land this patch with the workaround in the browser test, though
I'll keep an eye on the bots for flakiness and filed crbug.com/515906 to
investigate further.

[estark, 2015-07-31 17:32:54]

The CQ bit was checked by estark@chromium.org

[estark, 2015-07-31 17:32:54]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
msw@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1244863003/#ps520001
(title: "split out browser test and use another SpawnedTestServer")

[commit-bot: I haz the power, 2015-07-31 17:34:27]

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

[commit-bot: I haz the power, 2015-07-31 18:36:31]

Committed patchset #12 (id:520001)

[commit-bot: I haz the power, 2015-07-31 18:37:38]

Patchset 12 (id:??) landed as
https://crrev.com/294fbd93323cef34063eb85ba2768d5fcc9b3299
Cr-Commit-Position: refs/heads/master@{#341375}