Trigger Dangerous indicator for unsafe subresources

Trigger Dangerous indicator for unsafe subresources

Previously, the Dangerous indicator was not firing for subresources
flagged by Safe Browsing as unsafe. This is because marking a
subresource as unsafe was not triggering an omnibox update. We want to
trigger an omnibox update whenever a URL is marked as
whitelisted (either pending, meaning that an interstitial is showing, or
whitelisted, meaning that an interstitial has been clicked through) in
SafeBrowsingUIManager.

To do so, this CL renames WebContentsImpl::DidChangeVisibleSSLState() to
DidChangeVisibleSecurityState() (since it's no longer just SSL
information that can affect the omnibox security UI) and moves it to
WebContents so that it can be called from SafeBrowsingUIManager.

BUG=659713
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f
Cr-Commit-Position: refs/heads/master@{#428223}

[estark, 2016-10-27 00:48:41]

Description was changed from

==========
Trigger Dangerous indicator for unsafe subresources

Previously, the Dangerous indicator was not firing for subresources
flagged by Safe Browsing as unsafe. This is because marking a
subresource as unsafe was not triggering an omnibox update. We want to
trigger an omnibox update whenever a URL is marked as
whitelisted (either pending, meaning that an interstitial is showing, or
whitelisted, meaning that an interstitial has been clicked through) in
SafeBrowsingUIManager.

To do so, this CL renames WebContentsImpl::DidChangeVisibleSSLState() to
DidChangeVisibleSecurityState() (since it's no longer just SSL
information that can affect the omnibox security UI) and moves it to
WebContents so that it can be called from SafeBrowsingUIManager.

BUG=659713
==========

to

==========
Trigger Dangerous indicator for unsafe subresources

Previously, the Dangerous indicator was not firing for subresources
flagged by Safe Browsing as unsafe. This is because marking a
subresource as unsafe was not triggering an omnibox update. We want to
trigger an omnibox update whenever a URL is marked as
whitelisted (either pending, meaning that an interstitial is showing, or
whitelisted, meaning that an interstitial has been clicked through) in
SafeBrowsingUIManager.

To do so, this CL renames WebContentsImpl::DidChangeVisibleSSLState() to
DidChangeVisibleSecurityState() (since it's no longer just SSL
information that can affect the omnibox security UI) and moves it to
WebContents so that it can be called from SafeBrowsingUIManager.

BUG=659713
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[estark, 2016-10-27 00:48:44]

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

[commit-bot: I haz the power, 2016-10-27 00:49:22]

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-27 00:53:51]

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

[commit-bot: I haz the power, 2016-10-27 00:54:30]

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-27 00:54:46]

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

[estark, 2016-10-27 00:54:46]

nparker, PTAL? Most of the files here are mechanical renames; the interesting
stuff is in chrome/browser/safe_browsing and content/browser/web_contents.
Thanks!

[commit-bot: I haz the power, 2016-10-27 01:14:17]

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

[commit-bot: I haz the power, 2016-10-27 01:14:18]

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

[estark, 2016-10-27 01:21:13]

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

[commit-bot: I haz the power, 2016-10-27 01:21: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-10-27 01:47:55]

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

[commit-bot: I haz the power, 2016-10-27 01:47:55]

Dry run: 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...)

[estark, 2016-10-27 18:48:18]

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

[commit-bot: I haz the power, 2016-10-27 18:49:09]

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

[Nathan Parker, 2016-10-27 18:49:32]

lgtm

Nice!

https://codereview.chromium.org/2444383007/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2444383007/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:394:
web_contents->DidChangeVisibleSecurityState();
Could this go in the "{Insert(whitelisted_url)..}" block so it doesn't trigger
an unnecessary change when the user clicks through (assuming that's correct)? 
It'd just be an optimization, so maybe simpler not to.

[commit-bot: I haz the power, 2016-10-27 19:59:33]

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

[commit-bot: I haz the power, 2016-10-27 19:59:34]

Dry run: This issue passed the CQ dry run.

[estark, 2016-10-27 20:06:08]

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

[estark, 2016-10-27 20:06:09]

jam: could you please review the chrome/browser/, components/, and /content
parts of this? The gist of it is renaming DidChangeVisibleSSLState to
DidChangeVisibleSecurityState and moving it to WebContents (from
WebContentsImpl) so that Safe Browsing can trigger security UI updates. Thanks!

https://codereview.chromium.org/2444383007/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2444383007/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:394:
web_contents->DidChangeVisibleSecurityState();
On 2016/10/27 18:49:32, Nathan Parker wrote:
> Could this go in the "{Insert(whitelisted_url)..}" block so it doesn't trigger
> an unnecessary change when the user clicks through (assuming that's correct)? 
> It'd just be an optimization, so maybe simpler not to.

I could be convinced either way, but I lean slightly towards leaving it as-is,
on the grounds that this class shouldn't be selective about what state changes
are interesting to the omnibox security indicator. For example, hypothetically
we could want to use a different icon for pending vs whitelisted urls, or
something like that.

[jam, 2016-10-27 22:42:33]

lgtm

[estark, 2016-10-27 23:51:07]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-10-27 23:51:32]

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-27 23:57:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-27 23:59:48]

Description was changed from

==========
Trigger Dangerous indicator for unsafe subresources

Previously, the Dangerous indicator was not firing for subresources
flagged by Safe Browsing as unsafe. This is because marking a
subresource as unsafe was not triggering an omnibox update. We want to
trigger an omnibox update whenever a URL is marked as
whitelisted (either pending, meaning that an interstitial is showing, or
whitelisted, meaning that an interstitial has been clicked through) in
SafeBrowsingUIManager.

To do so, this CL renames WebContentsImpl::DidChangeVisibleSSLState() to
DidChangeVisibleSecurityState() (since it's no longer just SSL
information that can affect the omnibox security UI) and moves it to
WebContents so that it can be called from SafeBrowsingUIManager.

BUG=659713
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Trigger Dangerous indicator for unsafe subresources

Previously, the Dangerous indicator was not firing for subresources
flagged by Safe Browsing as unsafe. This is because marking a
subresource as unsafe was not triggering an omnibox update. We want to
trigger an omnibox update whenever a URL is marked as
whitelisted (either pending, meaning that an interstitial is showing, or
whitelisted, meaning that an interstitial has been clicked through) in
SafeBrowsingUIManager.

To do so, this CL renames WebContentsImpl::DidChangeVisibleSSLState() to
DidChangeVisibleSecurityState() (since it's no longer just SSL
information that can affect the omnibox security UI) and moves it to
WebContents so that it can be called from SafeBrowsingUIManager.

BUG=659713
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f
Cr-Commit-Position: refs/heads/master@{#428223}
==========

[commit-bot: I haz the power, 2016-10-27 23:59:50]

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

[tommycli, 2016-10-28 22:21:15]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2461093002/ by tommycli@chromium.org.

The reason for reverting is: Breaks MSan tests:

First breaking build:
https://build.chromium.org/p/chromium.memory.full/builders/Linux%20MSan%20Tes....

[Lei Zhang, 2016-10-28 22:57:29]

On 2016/10/28 22:21:15, tommycli wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2461093002/ by mailto:tommycli@chromium.org.
> 
> The reason for reverting is: Breaks MSan tests:
> 
> First breaking build:
>
https://build.chromium.org/p/chromium.memory.full/builders/Linux%20MSan%20Tes....

FYI, r428441 landed after this. Although this reverted cleanly, r428441 used
VisibleSecurityStateChanged(), so the options here are:

1) r428441 also needs to be reverted
2) Fix the VisibleSecurityStateChanged call in the r428441 unit test, if
possible.
3) Revert the revert. Just fix the silly MSAN error. (Probably a one liner)

[estark, 2016-10-28 23:01:13]

On 2016/10/28 22:57:29, Lei Zhang wrote:
> On 2016/10/28 22:21:15, tommycli wrote:
> > A revert of this CL (patchset #4 id:60001) has been created in
> > https://codereview.chromium.org/2461093002/ by mailto:tommycli@chromium.org.
> > 
> > The reason for reverting is: Breaks MSan tests:
> > 
> > First breaking build:
> >
>
https://build.chromium.org/p/chromium.memory.full/builders/Linux%20MSan%20Tes....
> 
> FYI, r428441 landed after this. Although this reverted cleanly, r428441 used
> VisibleSecurityStateChanged(), so the options here are:
> 
> 1) r428441 also needs to be reverted
> 2) Fix the VisibleSecurityStateChanged call in the r428441 unit test, if
> possible.
> 3) Revert the revert. Just fix the silly MSAN error. (Probably a one liner)

I have a fix in https://codereview.chromium.org/2456383003/. I just putting it
through a CQ dry run because I figured there wasn't any rush... but maybe I
should just skip CQ checks on that and land it?

[DaleCurtis, 2016-10-28 23:03:14]

On 2016/10/28 at 23:01:13, estark wrote:
> On 2016/10/28 22:57:29, Lei Zhang wrote:
> > On 2016/10/28 22:21:15, tommycli wrote:
> > > A revert of this CL (patchset #4 id:60001) has been created in
> > > https://codereview.chromium.org/2461093002/ by
mailto:tommycli@chromium.org.
> > > 
> > > The reason for reverting is: Breaks MSan tests:
> > > 
> > > First breaking build:
> > >
> >
https://build.chromium.org/p/chromium.memory.full/builders/Linux%20MSan%20Tes....
> > 
> > FYI, r428441 landed after this. Although this reverted cleanly, r428441 used
> > VisibleSecurityStateChanged(), so the options here are:
> > 
> > 1) r428441 also needs to be reverted
> > 2) Fix the VisibleSecurityStateChanged call in the r428441 unit test, if
> > possible.
> > 3) Revert the revert. Just fix the silly MSAN error. (Probably a one liner)
> 
> I have a fix in https://codereview.chromium.org/2456383003/. I just putting it
through a CQ dry run because I figured there wasn't any rush... but maybe I
should just skip CQ checks on that and land it?

Already created and started https://codereview.chromium.org/2454393003/ --
thanks for using the CQ dry run though :)

[Lei Zhang, 2016-10-28 23:03:45]

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

[Lei Zhang, 2016-10-28 23:03:46]

https://codereview.chromium.org/2444383007/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager_unittest.cc (right):

https://codereview.chromium.org/2444383007/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager_unittest.cc:355:
MakeUnsafeResource(kBadURL, true /* is_subresource */);
I think the simple cause of the MSAN error is resource.is_subframe not being
initialized. Given the simple error, I recommended we do (3) - revert the
revert, and just init is_subframe and call it a day.

[estark, 2016-10-28 23:03:54]

On 2016/10/28 23:03:14, DaleCurtis wrote:
> On 2016/10/28 at 23:01:13, estark wrote:
> > On 2016/10/28 22:57:29, Lei Zhang wrote:
> > > On 2016/10/28 22:21:15, tommycli wrote:
> > > > A revert of this CL (patchset #4 id:60001) has been created in
> > > > https://codereview.chromium.org/2461093002/ by
> mailto:tommycli@chromium.org.
> > > > 
> > > > The reason for reverting is: Breaks MSan tests:
> > > > 
> > > > First breaking build:
> > > >
> > >
>
https://build.chromium.org/p/chromium.memory.full/builders/Linux%20MSan%20Tes....
> > > 
> > > FYI, r428441 landed after this. Although this reverted cleanly, r428441
used
> > > VisibleSecurityStateChanged(), so the options here are:
> > > 
> > > 1) r428441 also needs to be reverted
> > > 2) Fix the VisibleSecurityStateChanged call in the r428441 unit test, if
> > > possible.
> > > 3) Revert the revert. Just fix the silly MSAN error. (Probably a one
liner)
> > 
> > I have a fix in https://codereview.chromium.org/2456383003/. I just putting
it
> through a CQ dry run because I figured there wasn't any rush... but maybe I
> should just skip CQ checks on that and land it?
> 
> Already created and started https://codereview.chromium.org/2454393003/ --
> thanks for using the CQ dry run though :)

Ok, thanks! I will upload a separate CL with the fix for the msan tests

[estark, 2016-10-28 23:06:17]

On 2016/10/28 23:03:46, Lei Zhang wrote:
>
https://codereview.chromium.org/2444383007/diff/60001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/ui_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/2444383007/diff/60001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/ui_manager_unittest.cc:355:
> MakeUnsafeResource(kBadURL, true /* is_subresource */);
> I think the simple cause of the MSAN error is resource.is_subframe not being
> initialized. Given the simple error, I recommended we do (3) - revert the
> revert, and just init is_subframe and call it a day.

Yes, the fix is in https://codereview.chromium.org/2458223002/