Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

We had a bug where we failed to notify that the SSL state has changed
when there are events that trigger a "not secure" warning on HTTP pages
(specifically, password and credit card fields). We were setting the
appropriate flags on SSLStatus, but, due to an early return, we were
never notifying the WebContents which meant the omnibox would never
redraw (among other things).

This bug was fixed somewhat unintentionally in
https://codereview.chromium.org/2408393003/. This CL adds a unit test
for it, and also includes some style fixes in SSLManager::UpdateEntry().

BUG=647560
Committed: https://crrev.com/79b7352086134008f856ac78753aeee3dc52a166
Cr-Commit-Position: refs/heads/master@{#424930}

[estark, 2016-10-11 21:25:17]

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

[commit-bot: I haz the power, 2016-10-11 21:25:39]

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

[estark, 2016-10-11 21:29:18]

estark@chromium.org changed reviewers:
+ elawrence@chromium.org, lgarron@chromium.org, nasko@chromium.org

[estark, 2016-10-11 21:29:18]

elawrence + lgarron, could you please review?

nasko: no need to review the whole change unless you want to, but I was hoping
you could take a quick look at ssl_manager_unittest.cc and tell if I'm using
RenderViewHostTestHarness appropriately.

Thanks!

[commit-bot: I haz the power, 2016-10-11 21:35:49]

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

[commit-bot: I haz the power, 2016-10-11 21:35:49]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[elawrence, 2016-10-12 15:22:47]

Yay, unit-tests!

I had one meaningful question around ssl_manager.cc:413 and a few style
questions as I learn how we do things.

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

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:124: if
(web_contents_impl->DisplayedInsecureContent())
Can the value of this change from line 123? If not, could we test just once,
e.g.

if (web_contents_impl->DisplayedInsecureContent())
 entry->GetSSL().content_status |= SSLStatus::DISPLAYED_INSECURE_CONTENT;
else 
 entry->GetSSL().content_status &= ~SSLStatus::DISPLAYED_INSECURE_CONTENT;

?

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:130: if
(web_contents_impl->DisplayedContentWithCertErrors()) {
ditto

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:138: // possibly have insecure content.  See
bug http://crbug.com/12423.
HTTPS for all URLs please. :)

Utterly trivial, but is there a reason that Googlers often use two spaces after
a sentence?

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:139: if (site_instance &&
ssl_host_state_delegate &&
Similar to earlier, is there a reason we shouldn't test once, e.g.

   if (site_instance && ssl_host_state_delegate) 
   {
      if (ssl_host_state_delegate->DidHostRunInsecureContent(Mixed))
content_status |= Ran_Insecure_content);

      if (ssl_host_state_delegate->DidHostRunInsecureContent(Errors))
content_status |= Ran_cert_errors);
   }

... potentially even pulling in the .host() and GetID() results into a local to
avoid calling multiple times?

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:413: if
(!entry->GetSSL().Equals(original_ssl_status)) {
Will this be reached much more often than it was previously? The very first
thing we do in this function is make a copy of the SSLStatus and then we toggle
a bit in the original. 

In the past, an early return meant that we wouldn't subsequently call
NotifyDidChangeVisibleState if the page wasn't HTTPS, but now we do.

Or is GetSSL().initialized always expected to be true upon entry to this
function, making line 384 redundant?

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager_unittest.cc (right):

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager_unittest.cc:83: }
Does it make sense to include a third test where we call both
OnCreditCardInputShownOnHttp AND OnPasswordInputShownOnHttp and verify that the
flags for both are set together?

[estark, 2016-10-12 16:19:17]

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

[commit-bot: I haz the power, 2016-10-12 16:19:27]

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

[estark, 2016-10-12 16:20:17]

Thanks, Eric!

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

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:124: if
(web_contents_impl->DisplayedInsecureContent())
On 2016/10/12 15:22:47, elawrence wrote:
> Can the value of this change from line 123? If not, could we test just once,
> e.g.
> 
> if (web_contents_impl->DisplayedInsecureContent())
>  entry->GetSSL().content_status |= SSLStatus::DISPLAYED_INSECURE_CONTENT;
> else 
>  entry->GetSSL().content_status &= ~SSLStatus::DISPLAYED_INSECURE_CONTENT;
> 
> ?

Done.

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:130: if
(web_contents_impl->DisplayedContentWithCertErrors()) {
On 2016/10/12 15:22:46, elawrence wrote:
> ditto

Done.

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:138: // possibly have insecure content.  See
bug http://crbug.com/12423.
On 2016/10/12 15:22:46, elawrence wrote:
> HTTPS for all URLs please. :)
> 
> Utterly trivial, but is there a reason that Googlers often use two spaces
after
> a sentence?

Neither of those are my fault, I was just moving old code! :P Fixed both. I feel
like I see two spaces in a lot of older code, but ISTR some style guide or
chromium-dev thread encouraging one space (which is also my personal
preference).

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:139: if (site_instance &&
ssl_host_state_delegate &&
On 2016/10/12 15:22:46, elawrence wrote:
> Similar to earlier, is there a reason we shouldn't test once, e.g.
> 
>    if (site_instance && ssl_host_state_delegate) 
>    {
>       if (ssl_host_state_delegate->DidHostRunInsecureContent(Mixed))
> content_status |= Ran_Insecure_content);
> 
>       if (ssl_host_state_delegate->DidHostRunInsecureContent(Errors))
> content_status |= Ran_cert_errors);
>    }
> 
> ... potentially even pulling in the .host() and GetID() results into a local
to
> avoid calling multiple times?

Done.

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:413: if
(!entry->GetSSL().Equals(original_ssl_status)) {
On 2016/10/12 15:22:46, elawrence wrote:
> Will this be reached much more often than it was previously? The very first
> thing we do in this function is make a copy of the SSLStatus and then we
toggle
> a bit in the original. 
> 
> In the past, an early return meant that we wouldn't subsequently call
> NotifyDidChangeVisibleState if the page wasn't HTTPS, but now we do.
> 
> Or is GetSSL().initialized always expected to be true upon entry to this
> function, making line 384 redundant?

Ohh, nice catch. I thought that in the common case it would already be
initialized during navigation
(https://cs.chromium.org/chromium/src/content/browser/loader/navigation_resour...),
but that is only so for HTTPS.

I think we should clean this up -- I'm not sure if this |initialized| thing is
even necessary -- but I vote for doing it in a separate CL. The cost here is at
most one notification per navigation which isn't too big a deal.

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager_unittest.cc (right):

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager_unittest.cc:83: }
On 2016/10/12 15:22:47, elawrence wrote:
> Does it make sense to include a third test where we call both
> OnCreditCardInputShownOnHttp AND OnPasswordInputShownOnHttp and verify that
the
> flags for both are set together?

Sure, done.

[elawrence, 2016-10-12 16:28:58]

Looks great to me.

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

https://codereview.chromium.org/2410023003/diff/20001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:413: if
(!entry->GetSSL().Equals(original_ssl_status)) {
On 2016/10/12 16:20:16, estark wrote:
> On 2016/10/12 15:22:46, elawrence wrote:
> > Will this be reached much more often than it was previously? The very first
> > thing we do in this function is make a copy of the SSLStatus and then we
> toggle
> > a bit in the original. 
> > 
> > In the past, an early return meant that we wouldn't subsequently call
> > NotifyDidChangeVisibleState if the page wasn't HTTPS, but now we do.
> > 
> > Or is GetSSL().initialized always expected to be true upon entry to this
> > function, making line 384 redundant?
> 
> Ohh, nice catch. I thought that in the common case it would already be
> initialized during navigation
>
(https://cs.chromium.org/chromium/src/content/browser/loader/navigation_resour...),
> but that is only so for HTTPS.
> 
> I think we should clean this up -- I'm not sure if this |initialized| thing is
> even necessary -- but I vote for doing it in a separate CL. The cost here is
at
> most one notification per navigation which isn't too big a deal.

Acknowledged.

[commit-bot: I haz the power, 2016-10-12 16:30:18]

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

[commit-bot: I haz the power, 2016-10-12 16:30:19]

Dry run: Exceeded global retry quota

[estark, 2016-10-12 22:10:48]

Description was changed from

==========
Notify WebContents when SSLStatus changes due to HTTP-bad detection

We had a bug where we failed to notify that the SSL state has changed
when there are events that trigger a "not secure" warning on HTTP pages
(specifically, password and credit card fields). We were setting the
appropriate flags on SSLStatus, but, due to an early return, we were
never notifying the WebContents which meant the omnibox would never
redraw (among other things).

This CL fixes the bug with a slight rejigger of
SSLManager::UpdateEntry(). Also added ssl_manager_unittests.cc so that
we have a place to write SSLManager unit tests now. Yay.

BUG=647560
==========

to

==========
Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

We had a bug where we failed to notify that the SSL state has changed
when there are events that trigger a "not secure" warning on HTTP pages
(specifically, password and credit card fields). We were setting the
appropriate flags on SSLStatus, but, due to an early return, we were
never notifying the WebContents which meant the omnibox would never
redraw (among other things).

This bug was fixed somewhat unintentionally in
https://codereview.chromium.org/2408393003/. This CL adds a unit test
for it, and also includes some style fixes in SSLManager::UpdateEntry().

BUG=647560
==========

[estark, 2016-10-12 22:10:56]

Description was changed from

==========
Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

We had a bug where we failed to notify that the SSL state has changed
when there are events that trigger a "not secure" warning on HTTP pages
(specifically, password and credit card fields). We were setting the
appropriate flags on SSLStatus, but, due to an early return, we were
never notifying the WebContents which meant the omnibox would never
redraw (among other things).

This bug was fixed somewhat unintentionally in
https://codereview.chromium.org/2408393003/. This CL adds a unit test
for it, and also includes some style fixes in SSLManager::UpdateEntry().

BUG=647560
==========

to

==========
Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

We had a bug where we failed to notify that the SSL state has changed
when there are events that trigger a "not secure" warning on HTTP pages
(specifically, password and credit card fields). We were setting the
appropriate flags on SSLStatus, but, due to an early return, we were
never notifying the WebContents which meant the omnibox would never
redraw (among other things).

This bug was fixed somewhat unintentionally in
https://codereview.chromium.org/2408393003/. This CL adds a unit test
for it, and also includes some style fixes in SSLManager::UpdateEntry().

BUG=647560
==========

[estark, 2016-10-12 22:11:55]

https://codereview.chromium.org/2408393003/ just landed, which somewhat
coincidentally fixes the same bug. Rebased on top of that, keeping the style
changes that Eric suggested and the new unit tests.

[estark, 2016-10-12 22:12:37]

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

[commit-bot: I haz the power, 2016-10-12 22:13:05]

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

[nasko, 2016-10-12 23:45:53]

LGTM with a nit.

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

https://codereview.chromium.org/2410023003/diff/60001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:374: // possibly have insecure content.  See
bug https://crbug.com/12423.
Nice!

https://codereview.chromium.org/2410023003/diff/60001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager_unittest.cc (right):

https://codereview.chromium.org/2410023003/diff/60001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager_unittest.cc:60:
->NavigateAndCommit(GURL("http://example.test"));
nit: You could just call NavigateAndCommit, right? It should be present on
RenderViewHostTestHarness itself and will allow you to drop the entire line
above.

[commit-bot: I haz the power, 2016-10-12 23:59:02]

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

[commit-bot: I haz the power, 2016-10-12 23:59:03]

Dry run: This issue passed the CQ dry run.

[estark, 2016-10-13 00:49:25]

https://codereview.chromium.org/2410023003/diff/60001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager_unittest.cc (right):

https://codereview.chromium.org/2410023003/diff/60001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager_unittest.cc:60:
->NavigateAndCommit(GURL("http://example.test"));
On 2016/10/12 23:45:53, nasko (slow) wrote:
> nit: You could just call NavigateAndCommit, right? It should be present on
> RenderViewHostTestHarness itself and will allow you to drop the entire line
> above.

Done, thanks!

[estark, 2016-10-13 00:49:50]

The CQ bit was checked by estark@chromium.org

[estark, 2016-10-13 00:49:51]

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

[commit-bot: I haz the power, 2016-10-13 00:50:04]

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-10-13 01:36:20]

Description was changed from

==========
Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

We had a bug where we failed to notify that the SSL state has changed
when there are events that trigger a "not secure" warning on HTTP pages
(specifically, password and credit card fields). We were setting the
appropriate flags on SSLStatus, but, due to an early return, we were
never notifying the WebContents which meant the omnibox would never
redraw (among other things).

This bug was fixed somewhat unintentionally in
https://codereview.chromium.org/2408393003/. This CL adds a unit test
for it, and also includes some style fixes in SSLManager::UpdateEntry().

BUG=647560
==========

to

==========
Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

We had a bug where we failed to notify that the SSL state has changed
when there are events that trigger a "not secure" warning on HTTP pages
(specifically, password and credit card fields). We were setting the
appropriate flags on SSLStatus, but, due to an early return, we were
never notifying the WebContents which meant the omnibox would never
redraw (among other things).

This bug was fixed somewhat unintentionally in
https://codereview.chromium.org/2408393003/. This CL adds a unit test
for it, and also includes some style fixes in SSLManager::UpdateEntry().

BUG=647560
==========

[commit-bot: I haz the power, 2016-10-13 01:36:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-13 01:38:33]

Description was changed from

==========
Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

We had a bug where we failed to notify that the SSL state has changed
when there are events that trigger a "not secure" warning on HTTP pages
(specifically, password and credit card fields). We were setting the
appropriate flags on SSLStatus, but, due to an early return, we were
never notifying the WebContents which meant the omnibox would never
redraw (among other things).

This bug was fixed somewhat unintentionally in
https://codereview.chromium.org/2408393003/. This CL adds a unit test
for it, and also includes some style fixes in SSLManager::UpdateEntry().

BUG=647560
==========

to

==========
Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

We had a bug where we failed to notify that the SSL state has changed
when there are events that trigger a "not secure" warning on HTTP pages
(specifically, password and credit card fields). We were setting the
appropriate flags on SSLStatus, but, due to an early return, we were
never notifying the WebContents which meant the omnibox would never
redraw (among other things).

This bug was fixed somewhat unintentionally in
https://codereview.chromium.org/2408393003/. This CL adds a unit test
for it, and also includes some style fixes in SSLManager::UpdateEntry().

BUG=647560

Committed: https://crrev.com/79b7352086134008f856ac78753aeee3dc52a166
Cr-Commit-Position: refs/heads/master@{#424930}
==========

[commit-bot: I haz the power, 2016-10-13 01:38:34]

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