Remove usage of SSLStatus in RenderFrameImpl.

Remove usage of SSLStatus in RenderFrameImpl.

Per discussion, this optimization isn't necessary anymore since we don't have a UI that shows frame specific errors.

BUG=598073
Committed: https://crrev.com/6ce51b7351ed32a0b6c82f5a83d83641a1b5babd
Committed: https://crrev.com/aebe8907589f73cd66311a81f84c15b5a40d12bc
Cr-Original-Commit-Position: refs/heads/master@{#409855}
Cr-Commit-Position: refs/heads/master@{#410418}

[jam, 2016-07-28 23:18:44]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 23:19:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-07-28 23:34:08]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 23:34:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 23:58:53]

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

[commit-bot: I haz the power, 2016-07-28 23:58:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-08-03 06:24:24]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 06:24:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 06:32:11]

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

[commit-bot: I haz the power, 2016-08-03 06:32:12]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jam, 2016-08-03 16:30:25]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 16:31:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 17:31:08]

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

[commit-bot: I haz the power, 2016-08-03 17:31:09]

Dry run: This issue passed the CQ dry run.

[jam, 2016-08-03 17:33:02]

Patchset #3 (id:40001) has been deleted

[jam, 2016-08-03 19:33:44]

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

[jam, 2016-08-03 19:33:46]

btw as a tangent, while debugging this code I found a lot of code paths for
checking that mixed content was displayed. It appears that this is only used by
the devtools warnings.
1) is this true?
2) if so, just checking that we still want to output this warning to developers
given that we don't warn the user about it?

https://codereview.chromium.org/2191113002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2191113002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:810:
url::Origin(GURL(main_ds->request().url())));
I've kept this check because otherwise three SSL browser tests
(SSLUITest.TestRefNavigation, SSLUITest.TestBadFrameNavigation,
SSLUITest.TestUnsafeContentsWithUserException) were flaking. The reason was
their last call to CheckAuthenticationBrokenState that checked for
AuthState::NONE. The favicon loading for the page was sending a
FrameHostMsg_DidDisplayContentWithCertificateErrors which made the auth_state to
be sometimes DISPLAYED_INSECURE_CONTENT if the load for the favicon finished
before the CheckAuthenticationBrokenState call.

Alternatively I can change the CheckAuthenticationBrokenState calls to not look
at the auth state for the last call in these three tests. I had that locally but
then changed my mind and left this check instead of removing the method
altogether. WDYT?

[estark, 2016-08-03 22:01:48]

On 2016/08/03 19:33:46, jam wrote:
> btw as a tangent, while debugging this code I found a lot of code paths for
> checking that mixed content was displayed. It appears that this is only used
by
> the devtools warnings.
> 1) is this true?
> 2) if so, just checking that we still want to output this warning to
developers
> given that we don't warn the user about it?

I think the answer to #2 is "yes" but would you be able to give me an example of
one of the code paths you're talking about?

Reviewing now!

> 
>
https://codereview.chromium.org/2191113002/diff/60001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/2191113002/diff/60001/content/renderer/render...
> content/renderer/render_frame_impl.cc:810:
> url::Origin(GURL(main_ds->request().url())));
> I've kept this check because otherwise three SSL browser tests
> (SSLUITest.TestRefNavigation, SSLUITest.TestBadFrameNavigation,
> SSLUITest.TestUnsafeContentsWithUserException) were flaking. The reason was
> their last call to CheckAuthenticationBrokenState that checked for
> AuthState::NONE. The favicon loading for the page was sending a
> FrameHostMsg_DidDisplayContentWithCertificateErrors which made the auth_state
to
> be sometimes DISPLAYED_INSECURE_CONTENT if the load for the favicon finished
> before the CheckAuthenticationBrokenState call.
> 
> Alternatively I can change the CheckAuthenticationBrokenState calls to not
look
> at the auth state for the last call in these three tests. I had that locally
but
> then changed my mind and left this check instead of removing the method
> altogether. WDYT?

[estark, 2016-08-03 22:31:38]

https://codereview.chromium.org/2191113002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2191113002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:810:
url::Origin(GURL(main_ds->request().url())));
On 2016/08/03 19:33:45, jam wrote:
> I've kept this check because otherwise three SSL browser tests
> (SSLUITest.TestRefNavigation, SSLUITest.TestBadFrameNavigation,
> SSLUITest.TestUnsafeContentsWithUserException) were flaking. The reason was
> their last call to CheckAuthenticationBrokenState that checked for
> AuthState::NONE. The favicon loading for the page was sending a
> FrameHostMsg_DidDisplayContentWithCertificateErrors which made the auth_state
to
> be sometimes DISPLAYED_INSECURE_CONTENT if the load for the favicon finished
> before the CheckAuthenticationBrokenState call.
> 
> Alternatively I can change the CheckAuthenticationBrokenState calls to not
look
> at the auth state for the last call in these three tests. I had that locally
but
> then changed my mind and left this check instead of removing the method
> altogether. WDYT?

Theoretically, checking only the origin could get us in to trouble here. Suppose
that https://foo.com has a certificate error that only happens on some requests
(for example only if you happen to hit certain front-end servers or something).
Suppose you click through a certificate warning on https://foo.com -- that
decision gets remembered for a week. Then later you go visit https://foo.com
again and happen to not get the certificate error, but the page loads a
same-origin subresource that does happen to get the certificate error. With this
check, the browser wouldn't get notified of the same-origin subresource
certificate error, so the user wouldn't get notified that there is broken-HTTPS
content on the page.

I fully acknowledge that this is a very theoretical situation that would
probably never happen in real life. But it makes me a little nervous, so I guess
I'd prefer changing the tests.

[estark, 2016-08-03 22:52:55]

(Also, tangentially related, I think some of the reason this is all so hairy is
that the browser tries to treat broken-https subresources the same as plain-http
mixed content subresources, and that's a big hack. I filed
https://crbug.com/634171 for myself to clean that up.)

[jam, 2016-08-03 23:52:22]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 23:52:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-08-03 23:55:44]

On 2016/08/03 22:01:48, estark wrote:
> On 2016/08/03 19:33:46, jam wrote:
> > btw as a tangent, while debugging this code I found a lot of code paths for
> > checking that mixed content was displayed. It appears that this is only used
> by
> > the devtools warnings.
> > 1) is this true?
> > 2) if so, just checking that we still want to output this warning to
> developers
> > given that we don't warn the user about it?
> 
> I think the answer to #2 is "yes" but would you be able to give me an example
of
> one of the code paths you're talking about?

I just meant the FrameHostMsg_DidDisplayInsecureContent IPC and all the code
path that follow in chrome/browser/ssl & components/security_state. But this
point is moot since you say we need this for devtools.

https://codereview.chromium.org/2191113002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2191113002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:810:
url::Origin(GURL(main_ds->request().url())));
On 2016/08/03 22:31:37, estark wrote:
> On 2016/08/03 19:33:45, jam wrote:
> > I've kept this check because otherwise three SSL browser tests
> > (SSLUITest.TestRefNavigation, SSLUITest.TestBadFrameNavigation,
> > SSLUITest.TestUnsafeContentsWithUserException) were flaking. The reason was
> > their last call to CheckAuthenticationBrokenState that checked for
> > AuthState::NONE. The favicon loading for the page was sending a
> > FrameHostMsg_DidDisplayContentWithCertificateErrors which made the
auth_state
> to
> > be sometimes DISPLAYED_INSECURE_CONTENT if the load for the favicon finished
> > before the CheckAuthenticationBrokenState call.
> > 
> > Alternatively I can change the CheckAuthenticationBrokenState calls to not
> look
> > at the auth state for the last call in these three tests. I had that locally
> but
> > then changed my mind and left this check instead of removing the method
> > altogether. WDYT?
> 
> Theoretically, checking only the origin could get us in to trouble here.
Suppose
> that https://foo.com has a certificate error that only happens on some
requests
> (for example only if you happen to hit certain front-end servers or
something).
> Suppose you click through a certificate warning on https://foo.com -- that
> decision gets remembered for a week. Then later you go visit https://foo.com
> again and happen to not get the certificate error, but the page loads a
> same-origin subresource that does happen to get the certificate error. With
this
> check, the browser wouldn't get notified of the same-origin subresource
> certificate error, so the user wouldn't get notified that there is
broken-HTTPS
> content on the page.
> 
> I fully acknowledge that this is a very theoretical situation that would
> probably never happen in real life. But it makes me a little nervous, so I
guess
> I'd prefer changing the tests.

Thanks for the example, I updated the test and removed the method.

[commit-bot: I haz the power, 2016-08-04 00:57:12]

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

[commit-bot: I haz the power, 2016-08-04 00:57:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-08-04 01:28:16]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[jam, 2016-08-04 01:28:29]

Patchset #5 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-08-04 01:28:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 03:09:19]

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

[commit-bot: I haz the power, 2016-08-04 03:09:20]

Dry run: This issue passed the CQ dry run.

[estark, 2016-08-04 04:31:17]

lgtm with a question inline about the MAYBE_

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

https://codereview.chromium.org/2191113002/diff/120001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:158: // Useful when a favicon load may
or may not have finised loading, to avoid
nit: typo, finised -> finished

https://codereview.chromium.org/2191113002/diff/120001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:160:
DONT_CHECK_DISPLAYED_INSECURE_CONTENT = 1 << 4,
Just a note for posterity, this shouldn't be necessary if I clean up this big
hack where we treat broken-https subresources the same as plain-http mixed
content in the browser (https://crbug.com/634171). If you are making changes
before you submit and want to add a TODO(estark) here, I wouldn't mind that :)

https://codereview.chromium.org/2191113002/diff/120001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:1842: IN_PROC_BROWSER_TEST_F(SSLUITest,
MAYBE_TestRefNavigation) {
That's odd -- if MAYBE_ wasn't here before, maybe we don't need the MAYBE_?

[estark, 2016-08-04 04:35:36]

On 2016/08/03 23:55:44, jam wrote:
> On 2016/08/03 22:01:48, estark wrote:
> > On 2016/08/03 19:33:46, jam wrote:
> > > btw as a tangent, while debugging this code I found a lot of code paths
for
> > > checking that mixed content was displayed. It appears that this is only
used
> > by
> > > the devtools warnings.
> > > 1) is this true?
> > > 2) if so, just checking that we still want to output this warning to
> > developers
> > > given that we don't warn the user about it?
> > 
> > I think the answer to #2 is "yes" but would you be able to give me an
example
> of
> > one of the code paths you're talking about?
> 
> I just meant the FrameHostMsg_DidDisplayInsecureContent IPC and all the code
> path that follow in chrome/browser/ssl & components/security_state. But this
> point is moot since you say we need this for devtools.

Ah, that is actually used elsewhere besides devtools. Whenever the page displays
an insecure subresource on an HTTPS page, the DidDisplayInsecureContent message
tells the browser to remove the lock icon. (It ends up flowing to
SecurityStateModel which uses the presence of insecure content to downgrade the
overall security level of the page, somewhere around
https://cs.chromium.org/chromium/src/components/security_state/security_state...)

> 
>
https://codereview.chromium.org/2191113002/diff/60001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/2191113002/diff/60001/content/renderer/render...
> content/renderer/render_frame_impl.cc:810:
> url::Origin(GURL(main_ds->request().url())));
> On 2016/08/03 22:31:37, estark wrote:
> > On 2016/08/03 19:33:45, jam wrote:
> > > I've kept this check because otherwise three SSL browser tests
> > > (SSLUITest.TestRefNavigation, SSLUITest.TestBadFrameNavigation,
> > > SSLUITest.TestUnsafeContentsWithUserException) were flaking. The reason
was
> > > their last call to CheckAuthenticationBrokenState that checked for
> > > AuthState::NONE. The favicon loading for the page was sending a
> > > FrameHostMsg_DidDisplayContentWithCertificateErrors which made the
> auth_state
> > to
> > > be sometimes DISPLAYED_INSECURE_CONTENT if the load for the favicon
finished
> > > before the CheckAuthenticationBrokenState call.
> > > 
> > > Alternatively I can change the CheckAuthenticationBrokenState calls to not
> > look
> > > at the auth state for the last call in these three tests. I had that
locally
> > but
> > > then changed my mind and left this check instead of removing the method
> > > altogether. WDYT?
> > 
> > Theoretically, checking only the origin could get us in to trouble here.
> Suppose
> > that https://foo.com has a certificate error that only happens on some
> requests
> > (for example only if you happen to hit certain front-end servers or
> something).
> > Suppose you click through a certificate warning on https://foo.com -- that
> > decision gets remembered for a week. Then later you go visit https://foo.com
> > again and happen to not get the certificate error, but the page loads a
> > same-origin subresource that does happen to get the certificate error. With
> this
> > check, the browser wouldn't get notified of the same-origin subresource
> > certificate error, so the user wouldn't get notified that there is
> broken-HTTPS
> > content on the page.
> > 
> > I fully acknowledge that this is a very theoretical situation that would
> > probably never happen in real life. But it makes me a little nervous, so I
> guess
> > I'd prefer changing the tests.
> 
> Thanks for the example, I updated the test and removed the method.

[estark, 2016-08-04 04:46:08]

(Last message, sorry)
Just wanted to record some more detail for the historical record about this
change.

This change removes a band-aid that was intended to stop the browser from
treating subresources on a page with certificate errors the same as mixed
content. As of fairly recently, browser UI doesn't actually need to distinguish
these two cases, because when the page was loaded with certificate errors,
browser UI only talks about the certificate errors from the main resource and
doesn't care about mixed content or other "lesser" security problems. So I think
it's okay to remove this band-aid now.

However I'm planning to do some follow-up work so that the browser can
distinguish broken-https subresources from mixed content, and I'm tracking that
in https://crbug.com/634171.

[jam, 2016-08-04 17:41:53]

On 2016/08/04 04:35:36, estark wrote:
> On 2016/08/03 23:55:44, jam wrote:
> > On 2016/08/03 22:01:48, estark wrote:
> > > On 2016/08/03 19:33:46, jam wrote:
> > > > btw as a tangent, while debugging this code I found a lot of code paths
> for
> > > > checking that mixed content was displayed. It appears that this is only
> used
> > > by
> > > > the devtools warnings.
> > > > 1) is this true?
> > > > 2) if so, just checking that we still want to output this warning to
> > > developers
> > > > given that we don't warn the user about it?
> > > 
> > > I think the answer to #2 is "yes" but would you be able to give me an
> example
> > of
> > > one of the code paths you're talking about?
> > 
> > I just meant the FrameHostMsg_DidDisplayInsecureContent IPC and all the code
> > path that follow in chrome/browser/ssl & components/security_state. But this
> > point is moot since you say we need this for devtools.
> 
> Ah, that is actually used elsewhere besides devtools. Whenever the page
displays
> an insecure subresource on an HTTPS page, the DidDisplayInsecureContent
message
> tells the browser to remove the lock icon. (It ends up flowing to
> SecurityStateModel which uses the presence of insecure content to downgrade
the
> overall security level of the page, somewhere around
>
https://cs.chromium.org/chromium/src/components/security_state/security_state...)

ah I see, thanks I missed that this affects the UI.

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

https://codereview.chromium.org/2191113002/diff/120001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:158: // Useful when a favicon load may
or may not have finised loading, to avoid
On 2016/08/04 04:31:17, estark wrote:
> nit: typo, finised -> finished

Done.

https://codereview.chromium.org/2191113002/diff/120001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:160:
DONT_CHECK_DISPLAYED_INSECURE_CONTENT = 1 << 4,
On 2016/08/04 04:31:17, estark wrote:
> Just a note for posterity, this shouldn't be necessary if I clean up this big
> hack where we treat broken-https subresources the same as plain-http mixed
> content in the browser (https://crbug.com/634171). If you are making changes
> before you submit and want to add a TODO(estark) here, I wouldn't mind that :)

Done.

https://codereview.chromium.org/2191113002/diff/120001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:1842: IN_PROC_BROWSER_TEST_F(SSLUITest,
MAYBE_TestRefNavigation) {
On 2016/08/04 04:31:17, estark wrote:
> That's odd -- if MAYBE_ wasn't here before, maybe we don't need the MAYBE_?

true, I should have checked flakiness results.

http://chromium-try-flakes.appspot.com/search?q=SSLUITest.TestRefNavigation
doesn't show any hits, so I've undone this and closed the bug.

[jam, 2016-08-04 17:44:05]

The CQ bit was checked by jam@chromium.org

[jam, 2016-08-04 17:44:05]

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

[commit-bot: I haz the power, 2016-08-04 17:45:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 19:05:01]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-08-04 19:08:28]

Description was changed from

==========
Remove usage of SSLStatus in RenderFrameImpl.

Per discussion, this optimization isn't necessary anymore since we don't have a
UI that shows frame specific errors.

BUG=598073
==========

to

==========
Remove usage of SSLStatus in RenderFrameImpl.

Per discussion, this optimization isn't necessary anymore since we don't have a
UI that shows frame specific errors.

BUG=598073

Committed: https://crrev.com/6ce51b7351ed32a0b6c82f5a83d83641a1b5babd
Cr-Commit-Position: refs/heads/master@{#409855}
==========

[commit-bot: I haz the power, 2016-08-04 19:08:29]

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

[jam, 2016-08-05 00:47:07]

A revert of this CL (patchset #6 id:140001) has been created in
https://codereview.chromium.org/2214293003/ by jam@chromium.org.

The reason for reverting is: Causing test flakiness.

BUG=634543.

[jam, 2016-08-05 00:54:23]

Description was changed from

==========
Remove usage of SSLStatus in RenderFrameImpl.

Per discussion, this optimization isn't necessary anymore since we don't have a
UI that shows frame specific errors.

BUG=598073

Committed: https://crrev.com/6ce51b7351ed32a0b6c82f5a83d83641a1b5babd
Cr-Commit-Position: refs/heads/master@{#409855}
==========

to

==========
Remove usage of SSLStatus in RenderFrameImpl.

Per discussion, this optimization isn't necessary anymore since we don't have a
UI that shows frame specific errors.

BUG=598073

Committed: https://crrev.com/6ce51b7351ed32a0b6c82f5a83d83641a1b5babd
Cr-Commit-Position: refs/heads/master@{#409855}
==========

[jam, 2016-08-05 18:29:20]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 18:29:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 19:04:57]

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

[commit-bot: I haz the power, 2016-08-05 19:04:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-08-05 19:39:36]

Patchset #8 (id:180001) has been deleted

[jam, 2016-08-05 19:39:44]

Patchset #8 (id:200001) has been deleted

[jam, 2016-08-05 19:43:02]

Patchset #8 (id:220001) has been deleted

[jam, 2016-08-05 19:43:45]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 19:44:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-08-05 19:44:36]

ptal, it looks like there were other CheckAuthenticationBrokenState calls that
would flake depending on when the favicon loaded. I've switched the tests to use
a network interceptor that just hangs favicon requests to fix this for all
cases.

[estark, 2016-08-05 21:11:21]

lgtm

[jam, 2016-08-05 21:27:08]

The CQ bit was unchecked by jam@chromium.org

[jam, 2016-08-05 21:27:15]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2016-08-05 21:27:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 22:16:21]

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

[commit-bot: I haz the power, 2016-08-05 22:16:23]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jam, 2016-08-05 22:17:13]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2016-08-05 22:17:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 23:27:41]

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

[commit-bot: I haz the power, 2016-08-05 23:27:43]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jam, 2016-08-08 16:55:28]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2016-08-08 16:55:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 20:12:43]

Description was changed from

==========
Remove usage of SSLStatus in RenderFrameImpl.

Per discussion, this optimization isn't necessary anymore since we don't have a
UI that shows frame specific errors.

BUG=598073

Committed: https://crrev.com/6ce51b7351ed32a0b6c82f5a83d83641a1b5babd
Cr-Commit-Position: refs/heads/master@{#409855}
==========

to

==========
Remove usage of SSLStatus in RenderFrameImpl.

Per discussion, this optimization isn't necessary anymore since we don't have a
UI that shows frame specific errors.

BUG=598073

Committed: https://crrev.com/6ce51b7351ed32a0b6c82f5a83d83641a1b5babd
Cr-Commit-Position: refs/heads/master@{#409855}
==========

[commit-bot: I haz the power, 2016-08-08 20:12:44]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2016-08-08 20:16:37]

Description was changed from

==========
Remove usage of SSLStatus in RenderFrameImpl.

Per discussion, this optimization isn't necessary anymore since we don't have a
UI that shows frame specific errors.

BUG=598073

Committed: https://crrev.com/6ce51b7351ed32a0b6c82f5a83d83641a1b5babd
Cr-Commit-Position: refs/heads/master@{#409855}
==========

to

==========
Remove usage of SSLStatus in RenderFrameImpl.

Per discussion, this optimization isn't necessary anymore since we don't have a
UI that shows frame specific errors.

BUG=598073

Committed: https://crrev.com/6ce51b7351ed32a0b6c82f5a83d83641a1b5babd
Committed: https://crrev.com/aebe8907589f73cd66311a81f84c15b5a40d12bc
Cr-Original-Commit-Position: refs/heads/master@{#409855}
Cr-Commit-Position: refs/heads/master@{#410418}
==========

[commit-bot: I haz the power, 2016-08-08 20:16:39]

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