Skeleton of the Safe Browsing Subresource Filter.

Skeleton of the Safe Browsing Subresource Filter.

This CL lays down the foundation for future work on the subresource filter
component. See the 'intent to implement' posted to security-dev@, under the
title "Extend Safe Browsing protection to embedded sub-resources".

In response to finding that many users proceed through Safe Browsing warning
interstitials that are displayed when the site ahead contains deceptive
embedded content, we would like provide another layer of protection for these
users. After the interstitial has been shown, clicked through and when the
actual content on the page is loaded, this component will be used to filter
network requests for some embedded sub-resources on these sites.

This CL concerns itself merely with adding the skeleton of the architecture,
plus a mock filter to be used in browsertests that disallows all subresource
loads. The component is disabled and has no effect in production code.

BUG=609747
TBR=tsepez@chromium.org

Committed: https://crrev.com/9ae0424e95a8fff9a97adfd0845759bd87b0e129
Cr-Commit-Position: refs/heads/master@{#398534}

[engedy, 2016-05-30 14:30:20]

Description was changed from

==========
Skeleton of Safe Browsing Subresource Filter.

BUG=609747
==========

to

==========
Skeleton of Safe Browsing Subresource Filter.

BUG=609747
==========

[engedy, 2016-05-30 14:30:20]

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

[engedy, 2016-05-30 14:31:29]

Dominic, could you please take an first look?

[battre, 2016-05-30 15:29:26]

https://codereview.chromium.org/2022783002/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2022783002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:520: new
subresource_filter::SubresourceFilterAgent(render_frame);
nit: drop namespace if you have a using above?

https://codereview.chromium.org/2022783002/diff/1/components/components_tests...
File components/components_tests.gyp (right):

https://codereview.chromium.org/2022783002/diff/1/components/components_tests...
components/components_tests.gyp:1033: '<@(sync_bookmarks_unittest_sources)',
nit: added spaces?

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter.gypi (right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter.gypi:76: 'sources': [
# Note: sources list duplicated in GN build.

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter/README (right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/README:1: When users proceed through a Safe
Browsing interstitial displayed because the site
nit: 80 chars

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/browser/content_subresource_filter_driver.h
(right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/content/browser/content_subresource_filter_driver.h:21:
ContentSubresourceFilterDriver(content::RenderFrameHost* render_frame_host);
explicit

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
(right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:34:
ContentSubresourceFilterDriverFactory(content::WebContents* web_contents);
explicit

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter/content/renderer/subresource_filter_agent.h
(right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent.h:15: //
instance per RenderFrame. Responsible for setting up the subresource filter
Grammar: s/\. /, /

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/core/browser/subresource_filter_features.cc:14:
const base::Feature kSafeBrowsingSubresourceFilter{
nit: space before { ?

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/activation_state.cc (right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/activation_state.cc:26: break;
how about replacing default with LAST? The compiler will complain if a new type
is added to the ActivationState but not to this operator.

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
File components/test_runner/mock_web_document_subresource_filter.cc (right):

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_document_subresource_filter.cc:30: }) ==
disallowed_path_suffixes_.end();
Isn't this easier to read?
for (const string& suffix : disallowed_path_suffixes_) {
  if (base::EndsWith(resource_path, suffix, base::CompareCase::SENSITIVE))
    return false;
}
return true;

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
File components/test_runner/mock_web_document_subresource_filter.h (right):

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_document_subresource_filter.h:21: class
MockWebDocumentSubresourceFilter
add a comment?

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_document_subresource_filter.h:24:
MockWebDocumentSubresourceFilter(
explicit?

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-allowed.html
(right):

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-allowed.html:12:
<img src="resources/abe.png" onload="alert(this.width == 76 ? 'PASS.' : 'FAIL:
Image has incorrect size.')" onerror="alert('FAIL: Image failed to load.')">
you have deleted the file abe.png from media/content/abe.png. Is that
intentional?

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ResourceRequest.h:53:
ResourceRequestBlockedReasonClient,
please check all uses of ResourceRequestBlockedReason, like
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8693: class
TestDocumentSubresourceFilter : public WebDocumentSubresourceFilter {
Should this be called BlockEverysthingSubresourceFilter?

[engedy, 2016-05-30 20:54:01]

Thank you, and sorry for the many nits.

https://codereview.chromium.org/2022783002/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2022783002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:520: new
subresource_filter::SubresourceFilterAgent(render_frame);
On 2016/05/30 15:29:25, battre wrote:
> nit: drop namespace if you have a using above?

Done. Dropped the using because the class is referred to in only one place.

https://codereview.chromium.org/2022783002/diff/1/components/components_tests...
File components/components_tests.gyp (right):

https://codereview.chromium.org/2022783002/diff/1/components/components_tests...
components/components_tests.gyp:1033: '<@(sync_bookmarks_unittest_sources)',
On 2016/05/30 15:29:25, battre wrote:
> nit: added spaces?

Removed, done.

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter.gypi (right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter.gypi:76: 'sources': [
On 2016/05/30 15:29:25, battre wrote:
> # Note: sources list duplicated in GN build.

Done.

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter/README (right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/README:1: When users proceed through a Safe
Browsing interstitial displayed because the site
On 2016/05/30 15:29:25, battre wrote:
> nit: 80 chars

Done.

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/browser/content_subresource_filter_driver.h
(right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/content/browser/content_subresource_filter_driver.h:21:
ContentSubresourceFilterDriver(content::RenderFrameHost* render_frame_host);
On 2016/05/30 15:29:25, battre wrote:
> explicit

Done.

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
(right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:34:
ContentSubresourceFilterDriverFactory(content::WebContents* web_contents);
On 2016/05/30 15:29:25, battre wrote:
> explicit

Done.

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter/content/renderer/subresource_filter_agent.h
(right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent.h:15: //
instance per RenderFrame. Responsible for setting up the subresource filter
On 2016/05/30 15:29:25, battre wrote:
> Grammar: s/\. /, /

Done.

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/core/browser/subresource_filter_features.cc:14:
const base::Feature kSafeBrowsingSubresourceFilter{
On 2016/05/30 15:29:25, battre wrote:
> nit: space before { ?

Agreed that it looks horrible, but clang-format does it like this.

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/activation_state.cc (right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/activation_state.cc:26: break;
On 2016/05/30 15:29:25, battre wrote:
> how about replacing default with LAST? The compiler will complain if a new
type
> is added to the ActivationState but not to this operator.

Could you please elaborate?

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
File components/test_runner/mock_web_document_subresource_filter.cc (right):

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_document_subresource_filter.cc:30: }) ==
disallowed_path_suffixes_.end();
On 2016/05/30 15:29:25, battre wrote:
> Isn't this easier to read?
> for (const string& suffix : disallowed_path_suffixes_) {
>   if (base::EndsWith(resource_path, suffix, base::CompareCase::SENSITIVE))
>     return false;
> }
> return true;

I think that's a matter of taste. I have to agree that the loop is easier to
read, but this is more expressive semantically.

How about this: I make note of your vote, and will get rid of the lambda if one
more reviewer points this out? :-)

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
File components/test_runner/mock_web_document_subresource_filter.h (right):

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_document_subresource_filter.h:21: class
MockWebDocumentSubresourceFilter
On 2016/05/30 15:29:25, battre wrote:
> add a comment?

This is Blink territory, so there are no comments anywhere in this neighborhood.
I would like to believe that it's relatively clear what it does, based on the
variable names. Let me know if you'd still add comments.

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_document_subresource_filter.h:24:
MockWebDocumentSubresourceFilter(
On 2016/05/30 15:29:25, battre wrote:
> explicit?

Done.

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-allowed.html
(right):

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-allowed.html:12:
<img src="resources/abe.png" onload="alert(this.width == 76 ? 'PASS.' : 'FAIL:
Image has incorrect size.')" onerror="alert('FAIL: Image failed to load.')">
On 2016/05/30 15:29:26, battre wrote:
> you have deleted the file abe.png from media/content/abe.png. Is that
> intentional?

Nope, restored. Thanks for spotting this.

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ResourceRequest.h:53:
ResourceRequestBlockedReasonClient,
On 2016/05/30 15:29:26, battre wrote:
> please check all uses of ResourceRequestBlockedReason, like
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Great catch, thank you! Done.

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2022783002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8693: class
TestDocumentSubresourceFilter : public WebDocumentSubresourceFilter {
On 2016/05/30 15:29:26, battre wrote:
> Should this be called BlockEverysthingSubresourceFilter?

Yes, I was considering that too, but naming classes Test* is pretty common in
this file, and another important functionality is that it also collects the
resource URLs.

I think I'll ultimately defer to core/loader owners on this.

[engedy, 2016-05-30 21:08:24]

Description was changed from

==========
Skeleton of Safe Browsing Subresource Filter.

BUG=609747
==========

to

==========
Skeleton of the Safe Browsing Subresource Filter.

BUG=609747
==========

[engedy, 2016-05-30 21:08:24]

engedy@chromium.org changed reviewers:
+ esprehn@chromium.org, mkwst@chromium.org

[engedy, 2016-05-30 21:16:51]

@Elliott, Mike: Could you please take a look?

As you will see, the CL has many placeholders and is not intended to accomplish
much in terms of functionality. It implements the minimum amount of logic needed
for a meaningful browser test. The emphasis really is on the overall
architecture.

The design doc is linked from the bug.

[battre, 2016-05-31 08:28:33]

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/activation_state.cc (right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/activation_state.cc:26: break;
On 2016/05/30 20:54:00, engedy wrote:
> On 2016/05/30 15:29:25, battre wrote:
> > how about replacing default with LAST? The compiler will complain if a new
> type
> > is added to the ActivationState but not to this operator.
> 
> Could you please elaborate?

switch(state) {
  case ActivationState::DISABLED: break;
  case ActivationState::DRYRUN: break;
  case ActivationState::ENABLED: break;
  case ActivationState::LAST: break;
}

will generate a compiler error if you add a new value to ActivationState without
updating this switch statement.

Having the default with NOTREACHED is exactly the problem that you encountered
with the devtools. Had they not used default: you would have noticed. Using
default: turns a compile time error into a runtime error.

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
File components/test_runner/mock_web_document_subresource_filter.cc (right):

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_document_subresource_filter.cc:30: }) ==
disallowed_path_suffixes_.end();
On 2016/05/30 20:54:00, engedy wrote:
> On 2016/05/30 15:29:25, battre wrote:
> > Isn't this easier to read?
> > for (const string& suffix : disallowed_path_suffixes_) {
> >   if (base::EndsWith(resource_path, suffix, base::CompareCase::SENSITIVE))
> >     return false;
> > }
> > return true;
> 
> I think that's a matter of taste. I have to agree that the loop is easier to
> read, but this is more expressive semantically.
> 
> How about this: I make note of your vote, and will get rid of the lambda if
one
> more reviewer points this out? :-)

Works for me. Maybe it also makes sense to add something to stl_util.h that
allows you to write
return MatchAny(disallowed_path_suffixes_,
                [&resource_path](const std::string& suffix) {
                   return base::EndsWith(resource_path, suffix,
                       base::CompareCase::SENSITIVE); });

[engedy, 2016-05-31 08:58:16]

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/activation_state.cc (right):

https://codereview.chromium.org/2022783002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/activation_state.cc:26: break;
On 2016/05/31 08:28:32, battre wrote:
> On 2016/05/30 20:54:00, engedy wrote:
> > On 2016/05/30 15:29:25, battre wrote:
> > > how about replacing default with LAST? The compiler will complain if a new
> > type
> > > is added to the ActivationState but not to this operator.
> > 
> > Could you please elaborate?
> 
> switch(state) {
>   case ActivationState::DISABLED: break;
>   case ActivationState::DRYRUN: break;
>   case ActivationState::ENABLED: break;
>   case ActivationState::LAST: break;
> }
> 
> will generate a compiler error if you add a new value to ActivationState
without
> updating this switch statement.
> 
> Having the default with NOTREACHED is exactly the problem that you encountered
> with the devtools. Had they not used default: you would have noticed. Using
> default: turns a compile time error into a runtime error.

Are you sure we want to depend on non-standardized diagnostic messages offered
by (admittedly most) compilers at various warning levels?

How about instead:

static_assert(ActivationState::ENABLED == ActivationState::LAST, ...);

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
File components/test_runner/mock_web_document_subresource_filter.cc (right):

https://codereview.chromium.org/2022783002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_document_subresource_filter.cc:30: }) ==
disallowed_path_suffixes_.end();
On 2016/05/31 08:28:32, battre wrote:
> On 2016/05/30 20:54:00, engedy wrote:
> > On 2016/05/30 15:29:25, battre wrote:
> > > Isn't this easier to read?
> > > for (const string& suffix : disallowed_path_suffixes_) {
> > >   if (base::EndsWith(resource_path, suffix, base::CompareCase::SENSITIVE))
> > >     return false;
> > > }
> > > return true;
> > 
> > I think that's a matter of taste. I have to agree that the loop is easier to
> > read, but this is more expressive semantically.
> > 
> > How about this: I make note of your vote, and will get rid of the lambda if
> one
> > more reviewer points this out? :-)
> 
> Works for me. Maybe it also makes sense to add something to stl_util.h that
> allows you to write
> return MatchAny(disallowed_path_suffixes_,
>                 [&resource_path](const std::string& suffix) {
>                    return base::EndsWith(resource_path, suffix,
>                        base::CompareCase::SENSITIVE); });

Agreed. I will do that in a follow-up CL.

[engedy, 2016-05-31 21:51:45]

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

[engedy, 2016-05-31 21:51:46]

@Mike: Do you think you can find some time to look at this tomorrow? This is on
a critical path for us, and once this lands, we could start working on different
parts in parallel.

@Alexei, please review:

 c/s_f/core/browser/subresource_filter_features/*

One thing I was unsure about is the preferred way of setting up the feature
state + variation params for browser tests. What I did was to factor out parts
of features_unittest.cc into a test support, and make use of that in the
browsertest as well.

[esprehn, 2016-05-31 22:08:23]

This patch is huge, does it actually need this much plumbing? How can we do this
from inside blink instead?

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8772:
TEST_F(WebFrameSubresourceFilterTest, ImportDocument)
Please put the tests in their own file.

[engedy, 2016-06-01 09:29:16]

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

[engedy, 2016-06-01 09:53:35]

(+Jochen to chime in sooner rather than later)

> This patch is huge [...]

I know, sorry. I could technically carve out third_party/WebKit into a separate
CL. but I'd much prefer not to land such a CL without any context. Also, Jochen
wanted to see some code before signing off on adding a component.

> [...] does it actually need this much plumbing? How can we do this from inside
blink instead?

I think we are doing as much as we can from Blink. Most of the plumbing is
necessitated by the requirement to enable/disable subresource filtering on
page-load by page-load basis. The activation decision is made in the
SafeBrowsingResourceThrottle in chrome/browser. In certain cases, it also needs
to be remembered for subsequent navigations by the
ContentSubresourceFilterDriver, potentially spanning over the lifetime of
multiple renderer processes. (All of this will be implemented in the next CL.)

I can see an argument being made for implementing parts directly in chrome/ or
even in content/. It might decrease SLoC, but realistically, I don't think we
can reduce actual complexity.

[Mike West, 2016-06-01 13:43:18]

I've added comments on the bits I know. I haven't taken a close look at
//components/subresource_filter.

On 2016/06/01 at 09:53:35, engedy wrote:
> (+Jochen to chime in sooner rather than later)
> 
> > This patch is huge [...]
> 
> I know, sorry. I could technically carve out third_party/WebKit into a
separate CL. but I'd much prefer not to land such a CL without any context.
Also, Jochen wanted to see some code before signing off on adding a component.

I think you could carve off //third_party/WebKit + //components/test_runner into
one CL that introduces the concept to Blink with functional layout tests, and
//components/subresource_filter et al. into another that instantiates that
concept.

I'm not sure that would make things any easier to review, and I think I agree
that doing so wouldn't fundamentally reduce the complexity.

https://codereview.chromium.org/2022783002/diff/20001/chrome/browser/subresou...
File chrome/browser/subresource_filter/subresource_filter_browsertest.cc
(right):

https://codereview.chromium.org/2022783002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
Why is this in //chrome/browser/subresource_filter, and not in the component?

https://codereview.chromium.org/2022783002/diff/20001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2022783002/diff/20001/components/test_runner/...
components/test_runner/test_runner.cc:2560: data_source->setSubresourceFilter(
Nit: You don't really need |data_source| here.

https://codereview.chromium.org/2022783002/diff/20001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/2022783002/diff/20001/components/test_runner/...
components/test_runner/test_runner.h:410: // created and injected even if
|suffixes| is empty.
Aside: Does an empty suffix mean that everything is allowed, or that everything
is blocked?

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:1392: crbug.com/609747
http/tests/subresource_filter [ NeedsRebaseline ]
This is not a great way to land tests: if the tests are broken, this will just
mindless write new expectation files. You should generate those expectations
locally, verify that they are correct, and commit them along with the CL.

Or just use testharness-style assertions, and forget about expectation files
entirely.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-allowed.html
(right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-allowed.html:12:
<img src="resources/abe.png" onload="alert(this.width == 76 ? 'PASS.' : 'FAIL:
Image has incorrect size.')" onerror="alert('FAIL: Image failed to load.')">
Nit: Please write each of these tests in terms of `testharness.js`. That is,
this could be something like:

```
async_test(t => {
  var i = document.createElement('img');
  i.onerror = t.unreached_func();
  i.onload = t.step_func_done(_ => {
    assert_equals(76, i.naturalWidth);
  });
  i.src = "resources/abe.png";
}, "Resources that aren't disallowed load correctly.");
```

This has a few advantages, among them the fact that you won't need an
`*-expected.txt` file.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:587: if
(documentLoader && documentLoader->subresourceFilter() && type !=
Resource::MainResource && type != Resource::ImportResource &&
!documentLoader->subresourceFilter()->allowLoad(url,
resourceRequest.requestContext()))
Why exclude `Resource::MainResource` and `Resource::ImportResource`? The former
is used for `<iframe>` and the latter is a totally reasonable (if seldom-used)
way of importing data/templates into a document context. Shouldn't the filter
apply to them as well?

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.h:121: inline
DocumentLoader* effectiveDocumentLoader() const;
Splitting this rename out into a separate CL would greatly simplify the
`FrameFetchContext` diff.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ResourceRequest.h:53:
ResourceRequestBlockedReasonClient,
Does this map to `BLOCKED_BY_CLIENT` in `net_error_list.h`? If so, I think it
probably shouldn't, as that error code more or less maps to "blocked by an
extension", and renders that as an error page. If not, it should probably be
renamed. :)

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebDataSourceImpl.h (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebDataSourceImpl.h:69: void
setSubresourceFilter(WebDocumentSubresourceFilter*) override;
Please add a comment about the lifetime/ownership considerations here.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8765:
TEST_F(WebFrameSubresourceFilterTest, MainDocument)
Either here or in layout tests, I'd suggest verifying the behavior of `<frame>`,
`<iframe>`, `<iframe srcdoc>`, as well as `data:` URLs (which are generally
strange) and `blob:`/`filesystem:`/`about:blank` (which inherit things from
their parent).

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8776:
EXPECT_THAT(requestedSubresourcePaths(), ElementsAre("/included-script.js"));
Isn't `include-script.html` a subresource?

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8777:
expectScriptDisallowed();
All of these tests verify that blocked resources are blocked; it would be
helpful to additionally verify that non-blocked resources are allowed.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/blink_headers.gypi (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/blink_headers.gypi:63: "platform/WebDoublePoint.h",
WebDisplayMode?

[jochen (gone - plz use gerrit), 2016-06-01 15:18:30]

can you ask somebody from the safe browsing team to review this code?

[jochen (gone - plz use gerrit), 2016-06-01 15:23:33]

https://codereview.chromium.org/2022783002/diff/20001/chrome/browser/subresou...
File chrome/browser/subresource_filter/DEPS (right):

https://codereview.chromium.org/2022783002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/DEPS:2: "+components/subresource_filter",
that includes renderer side code, can you please add more specific rules

https://codereview.chromium.org/2022783002/diff/20001/components/subresource_...
File components/subresource_filter/content/common/subresource_filter_messages.h
(right):

https://codereview.chromium.org/2022783002/diff/20001/components/subresource_...
components/subresource_filter/content/common/subresource_filter_messages.h:26:
subresource_filter::ActivationState);
any reason this is not using mojo ipc?

[engedy, 2016-06-01 20:55:39]

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

[engedy, 2016-06-01 21:00:44]

@Nathan, although this CL does not yet contain the parts interfacing with SB
(coming near ContentSubresourceFilterDrivers), still, could you please take a
quick look to make sure nothing seems out of place?

@Elliott: please see follow-up question.

https://codereview.chromium.org/2022783002/diff/20001/chrome/browser/subresou...
File chrome/browser/subresource_filter/subresource_filter_browsertest.cc
(right):

https://codereview.chromium.org/2022783002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_browsertest.cc:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/06/01 13:43:17, Mike West (OOO until 30th) wrote:
> Why is this in //chrome/browser/subresource_filter, and not in the component?

This is awkward indeed, but there seems to be no better way of doing it. We need
to bring up the chrome/ layer on the renderer side to have it inject the
renderer side of the component (i.e. SubresourceFilterAgent) into the
RenderFrame.

https://codereview.chromium.org/2022783002/diff/20001/components/subresource_...
File components/subresource_filter/content/common/subresource_filter_messages.h
(right):

https://codereview.chromium.org/2022783002/diff/20001/components/subresource_...
components/subresource_filter/content/common/subresource_filter_messages.h:26:
subresource_filter::ActivationState);
On 2016/06/01 15:23:33, jochen wrote:
> any reason this is not using mojo ipc?

Yes. Mojo does not currently provide FIFO ordering between Mojo and legacy IPC
messages [1], and here we need a guarantee that this message arrives to the
renderer before ResourceMsg_DataReceived. Otherwise the provisional might be
committed and subresource loads might already be in flight at the time the
filter is activated.

[1]:
https://groups.google.com/a/chromium.org/d/topic/chromium-mojo/h6L7KSxpl8E/di...

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8772:
TEST_F(WebFrameSubresourceFilterTest, ImportDocument)
On 2016/05/31 22:08:23, esprehn wrote:
> Please put the tests in their own file.

On a related note: Should I just convert these to LayoutTests using
testharness.js? I initially considered this to be a unittest that the correct
documentLoader is accessed for rel="import", but I am not really sure about what
is customary.

[engedy, 2016-06-02 22:27:16]

Patchset #3 (id:40001) has been deleted

[engedy, 2016-06-02 23:07:20]

Everyone, please take another look.

https://codereview.chromium.org/2022783002/diff/20001/chrome/browser/subresou...
File chrome/browser/subresource_filter/DEPS (right):

https://codereview.chromium.org/2022783002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/DEPS:2: "+components/subresource_filter",
On 2016/06/01 15:23:33, jochen wrote:
> that includes renderer side code, can you please add more specific rules

Done.

https://codereview.chromium.org/2022783002/diff/20001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2022783002/diff/20001/components/test_runner/...
components/test_runner/test_runner.cc:2560: data_source->setSubresourceFilter(
On 2016/06/01 13:43:17, Mike West (OOO until 30th) wrote:
> Nit: You don't really need |data_source| here.

Done.

https://codereview.chromium.org/2022783002/diff/20001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/2022783002/diff/20001/components/test_runner/...
components/test_runner/test_runner.h:410: // created and injected even if
|suffixes| is empty.
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> Aside: Does an empty suffix mean that everything is allowed, or that
everything
> is blocked?

Clarified in the comment.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:1392: crbug.com/609747
http/tests/subresource_filter [ NeedsRebaseline ]
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> This is not a great way to land tests: if the tests are broken, this will just
> mindless write new expectation files. You should generate those expectations
> locally, verify that they are correct, and commit them along with the CL.
> 
> Or just use testharness-style assertions, and forget about expectation files
> entirely.

Done.  Out of curiosity, what's the preferred way to create platform-dependent
pixel test expectations?

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-allowed.html
(right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-allowed.html:12:
<img src="resources/abe.png" onload="alert(this.width == 76 ? 'PASS.' : 'FAIL:
Image has incorrect size.')" onerror="alert('FAIL: Image failed to load.')">
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> Nit: Please write each of these tests in terms of `testharness.js`. That is,
> this could be something like:
> 
> ```
> async_test(t => {
>   var i = document.createElement('img');
>   i.onerror = t.unreached_func();
>   i.onload = t.step_func_done(_ => {
>     assert_equals(76, i.naturalWidth);
>   });
>   i.src = "resources/abe.png";
> }, "Resources that aren't disallowed load correctly.");
> ```
> 
> This has a few advantages, among them the fact that you won't need an
> `*-expected.txt` file.

Done, thanks.  I also replaced the image resource with a script resource.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:587: if
(documentLoader && documentLoader->subresourceFilter() && type !=
Resource::MainResource && type != Resource::ImportResource &&
!documentLoader->subresourceFilter()->allowLoad(url,
resourceRequest.requestContext()))
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> Why exclude `Resource::MainResource` and `Resource::ImportResource`? The
former
> is used for `<iframe>` and the latter is a totally reasonable (if seldom-used)
> way of importing data/templates into a document context. Shouldn't the filter
> apply to them as well?

We never want to filter the mainframe document load.

We want to filter <iframe>, but it is tricky: they need to be filtered in the
context of the owner document (i.e. by giving the frame source URL to the
SubresourceFilter injected into the DocumentLoader of the main frame), not in
their own context (i.e. the filter injected into the DocumentLoader of the
subframe).

So they will need to be special cased, but I was planning to add logic for this
in a subsequent CL to avoid inflating this one even more. WDYT?

Regarding import documents, the plan is to not treat them as subresources,
instead treat their subresources as if they were loaded from the master
document.  I think this makes sense because most types of resources referenced
in them will not be loaded anyway until they are cloned into the master
document.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.h:121: inline
DocumentLoader* effectiveDocumentLoader() const;
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> Splitting this rename out into a separate CL would greatly simplify the
> `FrameFetchContext` diff.

Done, landed https://codereview.chromium.org/2030883002/.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ResourceRequest.h:53:
ResourceRequestBlockedReasonClient,
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> Does this map to `BLOCKED_BY_CLIENT` in `net_error_list.h`? If so, I think it
> probably shouldn't, as that error code more or less maps to "blocked by an
> extension", and renders that as an error page. If not, it should probably be
> renamed. :)

It hasn't to do with that error code, so I renamed s/Client/SubresourceFilter/.
WDYT?

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebDataSourceImpl.h (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebDataSourceImpl.h:69: void
setSubresourceFilter(WebDocumentSubresourceFilter*) override;
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> Please add a comment about the lifetime/ownership considerations here.

Just to make sure we are on the same page, there is a comment at the declaration
in the WebDataSource interface. Any chance you had written this comment before
you got there?

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8765:
TEST_F(WebFrameSubresourceFilterTest, MainDocument)
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> Either here or in layout tests, I'd suggest verifying the behavior of
`<frame>`,
> `<iframe>`, `<iframe srcdoc>`, as well as `data:` URLs (which are generally
> strange) and `blob:`/`filesystem:`/`about:blank` (which inherit things from
> their parent).

Do I understand correctly that you are suggesting tests that verify these
classes of URLs as subresource URLs?

Somewhat relatedly, please see my long response above.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8772:
TEST_F(WebFrameSubresourceFilterTest, ImportDocument)
On 2016/06/01 21:00:44, engedy wrote:
> On 2016/05/31 22:08:23, esprehn wrote:
> > Please put the tests in their own file.
> 
> On a related note: Should I just convert these to LayoutTests using
> testharness.js? I initially considered this to be a unittest that the correct
> documentLoader is accessed for rel="import", but I am not really sure about
what
> is customary.

I simplified and extended this test and moved it to a separate file.  Not sure
how much of it we want to keep as a "unit test", though.  Please take another
look.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8776:
EXPECT_THAT(requestedSubresourcePaths(), ElementsAre("/included-script.js"));
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> Isn't `include-script.html` a subresource?

Please see my long response above.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8777:
expectScriptDisallowed();
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> All of these tests verify that blocked resources are blocked; it would be
> helpful to additionally verify that non-blocked resources are allowed.

Done.

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/blink_headers.gypi (right):

https://codereview.chromium.org/2022783002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/blink_headers.gypi:63: "platform/WebDoublePoint.h",
On 2016/06/01 13:43:18, Mike West (OOO until 30th) wrote:
> WebDisplayMode?

Uhm. Just a merge error, thanks for spotting it.

[Nathan Parker, 2016-06-02 23:38:02]

I'm not sure I'm much help here.  Is there a design doc that describes the flow
of code you're planning?

[engedy, 2016-06-03 00:30:37]

On 2016/06/02 23:38:02, Nathan Parker wrote:
> I'm not sure I'm much help here.  Is there a design doc that describes the
flow
> of code you're planning?

Yes, the design doc is linked from the bug.  You'll probably want to look at the
`Making and delivering the activation decision` section, although you are right
that the interesting part for you (that talks to SB) will only come in a later
CL.

[battre, 2016-06-03 12:57:56]

lgtm

[Mike West, 2016-06-03 13:48:51]

Thanks for taking another pass. Aside from a few testing nits,
//third_party/WebKit, //components/test_runner, and //ipc/ipc_message_start.h
LGTM

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed-after-redirect.html
(right):

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed-after-redirect.html:10:
// Inject a subresource filter to disallow the script.
Ditto.

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed-after-redirect.html:20:
s.src =
"http://localhost:8000/resources/redirect.php?url=../subresource_filter/resources/included-script.js.png&code=302";
Nit: Drop the `..`. `/` is enough, since the `subresource_filter` directory is
at the HTTP root.

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed.html
(right):

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed.html:10:
// Inject a subresource filter to disallow the script.
Can you add a test that loads a resource that isn't blocked by this suffix?

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed.html:18:
assert_false(!!document.scriptExecuted, "The script should not run.");
Nit: You can avoid the `!!` here and elsewhere by explicitly setting the
variables to `false` at the top of this test. I'd suggest doing that in all of
these tests anyway for clarity.

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-in-import-disallowed.html
(right):

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-in-import-disallowed.html:10:
testRunner.setDisallowedSubresourcePathSuffixes(["included-script.js"]);
Ditto.

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-in-import-disallowed.html:14:
window.onload = t.step_func_done(_ => {
Do imports actually block `onload`? I'm not sure they do. Would you mind
injecting the import asynchronously so you can wait until its `load` event
fires?

[Mike West, 2016-06-03 13:49:23]

mkwst@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Mike West, 2016-06-03 13:49:24]

You'll need someone to stamp third_party/devtools. Perhaps pfeldman@?

[Alexei Svitkine (slow), 2016-06-03 20:18:20]

use of feature API / variations param API LGTM

[jochen (gone - plz use gerrit), 2016-06-06 12:05:59]

lgtm

[engedy, 2016-06-06 13:36:01]

@Mike, I have reworked the test to address your concerns.

@Pavel, please take a look at the 3 lines in:
  /devtools/protocol.json
  /core/inspector/InspectorResourceAgent.cpp

@Elliott, Nathan, please let me know what you think.

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed-after-redirect.html
(right):

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed-after-redirect.html:10:
// Inject a subresource filter to disallow the script.
On 2016/06/03 13:48:51, Mike West (OOO until 30th) wrote:
> Ditto.

Done.

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed-after-redirect.html:20:
s.src =
"http://localhost:8000/resources/redirect.php?url=../subresource_filter/resources/included-script.js.png&code=302";
On 2016/06/03 13:48:51, Mike West (OOO until 30th) wrote:
> Nit: Drop the `..`. `/` is enough, since the `subresource_filter` directory is
> at the HTTP root.

Done.

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed.html
(right):

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed.html:10:
// Inject a subresource filter to disallow the script.
On 2016/06/03 13:48:51, Mike West (OOO until 30th) wrote:
> Can you add a test that loads a resource that isn't blocked by this suffix?

I have added alpha.js and beta.js, and got rid of the `resource-allowed.html`
test.  Instead, each *-disallowed-* test now loads an allowed and a disallowed
script.

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-disallowed.html:18:
assert_false(!!document.scriptExecuted, "The script should not run.");
On 2016/06/03 13:48:51, Mike West (OOO until 30th) wrote:
> Nit: You can avoid the `!!` here and elsewhere by explicitly setting the
> variables to `false` at the top of this test. I'd suggest doing that in all of
> these tests anyway for clarity.

Done. Also, changed the scripts to call a global function instead of setting a
global variable, so that the test will fail if there is a naming mismatch.

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-in-import-disallowed.html
(right):

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-in-import-disallowed.html:10:
testRunner.setDisallowedSubresourcePathSuffixes(["included-script.js"]);
On 2016/06/03 13:48:51, Mike West (OOO until 30th) wrote:
> Ditto.

Done.

https://codereview.chromium.org/2022783002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/resource-in-import-disallowed.html:14:
window.onload = t.step_func_done(_ => {
On 2016/06/03 13:48:51, Mike West (OOO until 30th) wrote:
> Do imports actually block `onload`? I'm not sure they do. Would you mind
> injecting the import asynchronously so you can wait until its `load` event
> fires?

If I understand the spec correctly in 7.3, then yes, "Every import that is not
marked as async delays the load event in the Document.".  Nevertheless, I
changed it for readability.

[engedy, 2016-06-06 13:56:50]

Description was changed from

==========
Skeleton of the Safe Browsing Subresource Filter.

BUG=609747
==========

to

==========
Skeleton of the Safe Browsing Subresource Filter.

This CL lays down the foundation for future work on the subresource filter
component, with the intent to implement posted earlier this year on
security-dev@, under the title "Extend Safe Browsing protection to embedded
sub-resources".

In response to finding that many users proceed through Safe Browsing warning
interstitials that are displayed when the site ahead contains deceptive embedded
content, we would like provide another layer of protection for these users.
After the interstitial has been shown, clicked through and when the actual
content on the page is loaded, this component will be used to filter network
requests for some embedded sub-resources on these sites.

This CL concerns itself merely with adding the skeleton of the architecture,
plus a mock filter to be used in browsertests that disallows all subresource
loads. The component is disabled and has no effect in production code.

BUG=609747
==========

[engedy, 2016-06-06 21:24:01]

Description was changed from

==========
Skeleton of the Safe Browsing Subresource Filter.

This CL lays down the foundation for future work on the subresource filter
component, with the intent to implement posted earlier this year on
security-dev@, under the title "Extend Safe Browsing protection to embedded
sub-resources".

In response to finding that many users proceed through Safe Browsing warning
interstitials that are displayed when the site ahead contains deceptive embedded
content, we would like provide another layer of protection for these users.
After the interstitial has been shown, clicked through and when the actual
content on the page is loaded, this component will be used to filter network
requests for some embedded sub-resources on these sites.

This CL concerns itself merely with adding the skeleton of the architecture,
plus a mock filter to be used in browsertests that disallows all subresource
loads. The component is disabled and has no effect in production code.

BUG=609747
==========

to

==========
Skeleton of the Safe Browsing Subresource Filter.

This CL lays down the foundation for future work on the subresource filter
component. See the 'intent to implement' posted to security-dev@, under the
title "Extend Safe Browsing protection to embedded sub-resources".

In response to finding that many users proceed through Safe Browsing warning
interstitials that are displayed when the site ahead contains deceptive embedded
content, we would like provide another layer of protection for these users.
After the interstitial has been shown, clicked through and when the actual
content on the page is loaded, this component will be used to filter network
requests for some embedded sub-resources on these sites.

This CL concerns itself merely with adding the skeleton of the architecture,
plus a mock filter to be used in browsertests that disallows all subresource
loads. The component is disabled and has no effect in production code.

BUG=609747
==========

[engedy, 2016-06-07 22:05:58]

Friendly ping.

@Pavel, please take a look at the 3 lines in:
  /devtools/protocol.json
  /core/inspector/InspectorResourceAgent.cpp

@Elliott, please review the Blink parts if you want to (although Mike has
already reviewed), or let me know otherwise.

@Nathan, please review overall architecture if you want to (although as
discussed before, the SB parts are yet to come), or let me know otherwise.

[pfeldman, 2016-06-07 23:04:03]

https://codereview.chromium.org/2022783002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2022783002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1019: "enum":
["csp", "mixed-content", "origin", "inspector", "subresource-filter", "other"],
lgtm

[esprehn, 2016-06-08 01:59:46]

Please wrap your change description at 80 or 100 columns. Long term I'm not sure
this is the right direction, blink platform/network should allow installing a
RequestFilter, and blink should create the safe browsing filter and install it
when needed. components/subresource_filter/renderer needs to move into blink as
part of Onion Soup.

[engedy, 2016-06-08 06:39:06]

Description was changed from

==========
Skeleton of the Safe Browsing Subresource Filter.

This CL lays down the foundation for future work on the subresource filter
component. See the 'intent to implement' posted to security-dev@, under the
title "Extend Safe Browsing protection to embedded sub-resources".

In response to finding that many users proceed through Safe Browsing warning
interstitials that are displayed when the site ahead contains deceptive embedded
content, we would like provide another layer of protection for these users.
After the interstitial has been shown, clicked through and when the actual
content on the page is loaded, this component will be used to filter network
requests for some embedded sub-resources on these sites.

This CL concerns itself merely with adding the skeleton of the architecture,
plus a mock filter to be used in browsertests that disallows all subresource
loads. The component is disabled and has no effect in production code.

BUG=609747
==========

to

==========
Skeleton of the Safe Browsing Subresource Filter.

This CL lays down the foundation for future work on the subresource filter
component. See the 'intent to implement' posted to security-dev@, under the
title "Extend Safe Browsing protection to embedded sub-resources".

In response to finding that many users proceed through Safe Browsing warning
interstitials that are displayed when the site ahead contains deceptive
embedded content, we would like provide another layer of protection for these
users. After the interstitial has been shown, clicked through and when the
actual content on the page is loaded, this component will be used to filter
network requests for some embedded sub-resources on these sites.

This CL concerns itself merely with adding the skeleton of the architecture,
plus a mock filter to be used in browsertests that disallows all subresource
loads. The component is disabled and has no effect in production code.

BUG=609747
==========

[esprehn, 2016-06-08 06:53:39]

engedy@ and I talked offline about this, I think we're fine to land these
patches to get an MVP. He's going to start thinking about how to move this into
blink too. :)

[esprehn, 2016-06-08 06:53:48]

On 2016/06/08 at 06:53:39, esprehn wrote:
> engedy@ and I talked offline about this, I think we're fine to land these
patches to get an MVP. He's going to start thinking about how to move this into
blink too. :)

lgtm

[engedy, 2016-06-08 09:10:52]

engedy@chromium.org changed reviewers:
- nparker@chromium.org

[engedy, 2016-06-08 10:16:25]

@Mike, as discussed, could you please also review
components/subresource_filter/content/common/? Also, presubmit checks complain
that adding //ipc to DEPS needs a review. Is that actually needed for new IPC
messages?

[Mike West, 2016-06-08 11:43:43]

On 2016/06/08 at 10:16:25, engedy wrote:
> @Mike, as discussed, could you please also review
components/subresource_filter/content/common/? Also, presubmit checks complain
that adding //ipc to DEPS needs a review. Is that actually needed for new IPC
messages?

Thanks for adding the OWNERS file that we talked about. Other than that, adding
the IPC and DEPS from the new component LGTM.

[engedy, 2016-06-08 13:06:50]

The CQ bit was checked by engedy@chromium.org

[engedy, 2016-06-08 13:06:51]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
asvitkine@chromium.org, battre@chromium.org, esprehn@chromium.org,
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2022783002/#ps300001
(title: "Implement RenderFrameObserver::OnDestruct introduced by rebase.")

[commit-bot: I haz the power, 2016-06-08 13:07:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2022783002/300001

[commit-bot: I haz the power, 2016-06-08 13:16:09]

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

[commit-bot: I haz the power, 2016-06-08 13:16:11]

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

[engedy, 2016-06-08 13:20:54]

Description was changed from

==========
Skeleton of the Safe Browsing Subresource Filter.

This CL lays down the foundation for future work on the subresource filter
component. See the 'intent to implement' posted to security-dev@, under the
title "Extend Safe Browsing protection to embedded sub-resources".

In response to finding that many users proceed through Safe Browsing warning
interstitials that are displayed when the site ahead contains deceptive
embedded content, we would like provide another layer of protection for these
users. After the interstitial has been shown, clicked through and when the
actual content on the page is loaded, this component will be used to filter
network requests for some embedded sub-resources on these sites.

This CL concerns itself merely with adding the skeleton of the architecture,
plus a mock filter to be used in browsertests that disallows all subresource
loads. The component is disabled and has no effect in production code.

BUG=609747
==========

to

==========
Skeleton of the Safe Browsing Subresource Filter.

This CL lays down the foundation for future work on the subresource filter
component. See the 'intent to implement' posted to security-dev@, under the
title "Extend Safe Browsing protection to embedded sub-resources".

In response to finding that many users proceed through Safe Browsing warning
interstitials that are displayed when the site ahead contains deceptive
embedded content, we would like provide another layer of protection for these
users. After the interstitial has been shown, clicked through and when the
actual content on the page is loaded, this component will be used to filter
network requests for some embedded sub-resources on these sites.

This CL concerns itself merely with adding the skeleton of the architecture,
plus a mock filter to be used in browsertests that disallows all subresource
loads. The component is disabled and has no effect in production code.

BUG=609747
TBR=tsepez@chromium.org
==========

[engedy, 2016-06-08 13:21:18]

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

[engedy, 2016-06-08 13:25:20]

@Tom, I have TBR'd you only for the //ipc dependency in c/s_f/content/DEPS. Mike
has reviewed the IPC already.

In case you are wondering, please see my explanation as to why we are still
using legacy IPC here:
https://codereview.chromium.org/2022783002/diff/20001/components/subresource_...

[engedy, 2016-06-08 13:25:48]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2016-06-08 13:26:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2022783002/300001

[commit-bot: I haz the power, 2016-06-08 13:34:48]

Description was changed from

==========
Skeleton of the Safe Browsing Subresource Filter.

This CL lays down the foundation for future work on the subresource filter
component. See the 'intent to implement' posted to security-dev@, under the
title "Extend Safe Browsing protection to embedded sub-resources".

In response to finding that many users proceed through Safe Browsing warning
interstitials that are displayed when the site ahead contains deceptive
embedded content, we would like provide another layer of protection for these
users. After the interstitial has been shown, clicked through and when the
actual content on the page is loaded, this component will be used to filter
network requests for some embedded sub-resources on these sites.

This CL concerns itself merely with adding the skeleton of the architecture,
plus a mock filter to be used in browsertests that disallows all subresource
loads. The component is disabled and has no effect in production code.

BUG=609747
TBR=tsepez@chromium.org
==========

to

==========
Skeleton of the Safe Browsing Subresource Filter.

This CL lays down the foundation for future work on the subresource filter
component. See the 'intent to implement' posted to security-dev@, under the
title "Extend Safe Browsing protection to embedded sub-resources".

In response to finding that many users proceed through Safe Browsing warning
interstitials that are displayed when the site ahead contains deceptive
embedded content, we would like provide another layer of protection for these
users. After the interstitial has been shown, clicked through and when the
actual content on the page is loaded, this component will be used to filter
network requests for some embedded sub-resources on these sites.

This CL concerns itself merely with adding the skeleton of the architecture,
plus a mock filter to be used in browsertests that disallows all subresource
loads. The component is disabled and has no effect in production code.

BUG=609747
TBR=tsepez@chromium.org

Committed: https://crrev.com/9ae0424e95a8fff9a97adfd0845759bd87b0e129
Cr-Commit-Position: refs/heads/master@{#398534}
==========

[commit-bot: I haz the power, 2016-06-08 13:34:50]

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

[commit-bot: I haz the power, 2016-06-08 13:34:51]

Failed to apply patch for components/subresource_filter/content/renderer/DEPS:
While running git apply --index -3 -p1;
  error: repository lacks the necessary blob to fall back on 3-way merge.
  error: components/visitedlink/renderer/DEPS: patch does not apply

Patch:   NR 
components/visitedlink/renderer/DEPS->components/subresource_filter/content/renderer/DEPS
Index: components/subresource_filter/content/renderer/DEPS
diff --git a/components/visitedlink/renderer/DEPS
b/components/subresource_filter/content/renderer/DEPS
similarity index 100%
copy from components/visitedlink/renderer/DEPS
copy to components/subresource_filter/content/renderer/DEPS

[xidachen, 2016-06-08 14:53:32]

A revert of this CL (patchset #15 id:300001) has been created in
https://codereview.chromium.org/2049733002/ by xidachen@chromium.org.

The reason for reverting is: This CL causes this test:
KioskTest.LaunchAppNetworkDownConfigureNotAllowed

fail on this bot:
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....

[engedy, 2016-06-08 15:04:27]

For documentation's sake: looks like an unrelated flake, CL wasn't reverted in
the end. (At least, not just yet.)