Transform redirect chains for the Safe Browsing subresource filter.

Transform redirect chains for the Safe Browsing subresource filter.

There has been a discrepancy between how the subresource filter and the rest of
Safe Browsing defines the meaning of `redirect urls`. For the redirect chain A
-> B -> C, the SafeBrowsingResourceThrottle considers A as the |original_url|
and [B, C] as |redirect_urls|.  In contrast, the subresource filter expects C
as the resource URL and [A, B] as redirect URLs.

This CL performs the correct transformation on the |redirect_urls| list before
passing it from the SafeBrowsingResourceThrottle to the subresource filter.
Plus, it also avoid calling out to the ContentSubresourceFilterDriverFactory in
case the unsafe resource is not a main frame document.

BUG=646800, 609747
Committed: https://crrev.com/6cd4e4e728fa5d61e4ae886a8574bc4c893784e1
Cr-Commit-Position: refs/heads/master@{#419373}

[engedy, 2016-09-14 12:35:48]

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

[commit-bot: I haz the power, 2016-09-14 12:35:55]

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

[engedy, 2016-09-14 12:37:42]

Patchset #2 (id:20001) has been deleted

[engedy, 2016-09-14 12:37:47]

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

[commit-bot: I haz the power, 2016-09-14 12:37:56]

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

[engedy, 2016-09-14 12:41:25]

engedy@chromium.org changed reviewers:
+ melandory@chromium.org, nparker@chromium.org

[engedy, 2016-09-14 12:41:26]

@Tanja, Nathan, could you please take a close look?

In a follow-up CL, I am working on some browser tests for this. For now I wanted
to keep the size of the CL as small as possible to facilitate merging if need
be.

[commit-bot: I haz the power, 2016-09-14 13:18:38]

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

[commit-bot: I haz the power, 2016-09-14 13:18:39]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-09-14 13:51:12]

On 2016/09/14 12:41:26, engedy wrote:
> @Tanja, Nathan, could you please take a close look?
> 
> In a follow-up CL, I am working on some browser tests for this. For now I
wanted
> to keep the size of the CL as small as possible to facilitate merging if need
> be.

lgtm

[Nathan Parker, 2016-09-15 21:19:45]

https://codereview.chromium.org/2339733003/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_host/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2339733003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:318:
--resource.redirect_urls.end());
How does decrement work on a const iterator? Is that shortening
resource.reidrect_urls?  Maybe you want .end() - 1. 

Might be good to have a unit test for this.

[Nathan Parker, 2016-09-15 21:20:23]

oops, I now see your comment about adding tests later. nm on that.

[engedy, 2016-09-16 09:04:28]

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

[commit-bot: I haz the power, 2016-09-16 09:04:36]

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

[engedy, 2016-09-16 09:04:43]

https://codereview.chromium.org/2339733003/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_host/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2339733003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:318:
--resource.redirect_urls.end());
On 2016/09/15 21:19:45, Nathan Parker wrote:
> How does decrement work on a const iterator? Is that shortening
> resource.reidrect_urls?  Maybe you want .end() - 1. 

It works as you'd expect it to, i.e. the temporary const_iterator instance that
is created by end() is advanced backwards by one. Nevertheless, I have changed
it to std::prev(...) for better readability.
 
> Might be good to have a unit test for this.

Tests on the way!

[commit-bot: I haz the power, 2016-09-16 09:46:27]

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

[commit-bot: I haz the power, 2016-09-16 09:46:28]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2016-09-16 17:50:20]

lgtm

[engedy, 2016-09-16 18:26:07]

The CQ bit was checked by engedy@chromium.org

[engedy, 2016-09-16 18:26:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
melandory@chromium.org
Link to the patchset: https://codereview.chromium.org/2339733003/#ps60001
(title: "Use std::prev().")

[commit-bot: I haz the power, 2016-09-16 18:26:33]

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-09-16 18:38:10]

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

[commit-bot: I haz the power, 2016-09-16 18:38:11]

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

[engedy, 2016-09-16 19:33:49]

engedy@chromium.org changed reviewers:
+ sky@chromium.org

[engedy, 2016-09-16 19:33:49]

Scott, could you please review for OWNER's approval?

[engedy, 2016-09-16 19:34:14]

Description was changed from

==========
Transform redirect chains for the Safe Browsing subresource filter.

There has been a discrepancy between how the subresource filter and the rest of
Safe Brwosing defines the meaning of `redirect urls`. For the redirect chain A
-> B -> C, the SafeBrowsingResourceThrottle considers A as the |original_url|
and [B, C] as |redirect_urls|.  In contrast, the subresource filter expects C
as the resource URL and [A, B] as redirect URLs.

This CL performs the correct transformation on the |redirect_urls| list before
passing it from the SafeBrowsingResourceThrottle to the subresource filter.
Plus, it also avoid calling out to the ContentSubresourceFilterDriverFactory in
case the unsafe resource is not a main frame document.

BUG=646800,609747
==========

to

==========
Transform redirect chains for the Safe Browsing subresource filter.

There has been a discrepancy between how the subresource filter and the rest of
Safe Browsing defines the meaning of `redirect urls`. For the redirect chain A
-> B -> C, the SafeBrowsingResourceThrottle considers A as the |original_url|
and [B, C] as |redirect_urls|.  In contrast, the subresource filter expects C
as the resource URL and [A, B] as redirect URLs.

This CL performs the correct transformation on the |redirect_urls| list before
passing it from the SafeBrowsingResourceThrottle to the subresource filter.
Plus, it also avoid calling out to the ContentSubresourceFilterDriverFactory in
case the unsafe resource is not a main frame document.

BUG=646800,609747
==========

[sky, 2016-09-16 21:34:52]

sky@chromium.org changed reviewers:
+ mmenke@chromium.org

[sky, 2016-09-16 21:35:00]

+mmenke

[mmenke, 2016-09-16 21:47:10]

On 2016/09/16 21:35:00, sky wrote:
> +mmenke

LGTM

[sky, 2016-09-16 21:55:53]

Rubber stamp until Matt lands OWNERS/move LGTM

[engedy, 2016-09-17 05:59:19]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2016-09-17 05:59:30]

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-09-17 19:37:39]

Description was changed from

==========
Transform redirect chains for the Safe Browsing subresource filter.

There has been a discrepancy between how the subresource filter and the rest of
Safe Browsing defines the meaning of `redirect urls`. For the redirect chain A
-> B -> C, the SafeBrowsingResourceThrottle considers A as the |original_url|
and [B, C] as |redirect_urls|.  In contrast, the subresource filter expects C
as the resource URL and [A, B] as redirect URLs.

This CL performs the correct transformation on the |redirect_urls| list before
passing it from the SafeBrowsingResourceThrottle to the subresource filter.
Plus, it also avoid calling out to the ContentSubresourceFilterDriverFactory in
case the unsafe resource is not a main frame document.

BUG=646800,609747
==========

to

==========
Transform redirect chains for the Safe Browsing subresource filter.

There has been a discrepancy between how the subresource filter and the rest of
Safe Browsing defines the meaning of `redirect urls`. For the redirect chain A
-> B -> C, the SafeBrowsingResourceThrottle considers A as the |original_url|
and [B, C] as |redirect_urls|.  In contrast, the subresource filter expects C
as the resource URL and [A, B] as redirect URLs.

This CL performs the correct transformation on the |redirect_urls| list before
passing it from the SafeBrowsingResourceThrottle to the subresource filter.
Plus, it also avoid calling out to the ContentSubresourceFilterDriverFactory in
case the unsafe resource is not a main frame document.

BUG=646800,609747
==========

[commit-bot: I haz the power, 2016-09-17 19:37:40]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-17 19:40:55]

Description was changed from

==========
Transform redirect chains for the Safe Browsing subresource filter.

There has been a discrepancy between how the subresource filter and the rest of
Safe Browsing defines the meaning of `redirect urls`. For the redirect chain A
-> B -> C, the SafeBrowsingResourceThrottle considers A as the |original_url|
and [B, C] as |redirect_urls|.  In contrast, the subresource filter expects C
as the resource URL and [A, B] as redirect URLs.

This CL performs the correct transformation on the |redirect_urls| list before
passing it from the SafeBrowsingResourceThrottle to the subresource filter.
Plus, it also avoid calling out to the ContentSubresourceFilterDriverFactory in
case the unsafe resource is not a main frame document.

BUG=646800,609747
==========

to

==========
Transform redirect chains for the Safe Browsing subresource filter.

There has been a discrepancy between how the subresource filter and the rest of
Safe Browsing defines the meaning of `redirect urls`. For the redirect chain A
-> B -> C, the SafeBrowsingResourceThrottle considers A as the |original_url|
and [B, C] as |redirect_urls|.  In contrast, the subresource filter expects C
as the resource URL and [A, B] as redirect URLs.

This CL performs the correct transformation on the |redirect_urls| list before
passing it from the SafeBrowsingResourceThrottle to the subresource filter.
Plus, it also avoid calling out to the ContentSubresourceFilterDriverFactory in
case the unsafe resource is not a main frame document.

BUG=646800,609747

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

[commit-bot: I haz the power, 2016-09-17 19:40:56]

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