Network service: Safe browsing check for sub-resources from renderer.

This CL:
- introduces mojom interface for safe browsing URL checks. This interface is registered on each render frame.
- introduces a C++ interface URLLoaderThrottle in content/ for renderers, which is similar to ResourceThrottle at the browser side; implements a subclass SafeBrowsingURLLoaderThrottle in chrome/.
- extends ContentRendererClient::WillSendRequest() to also return a list of throttle objects, so that chrome can attach the safe browsing throttle.
- provides ThrottlingURLLoader which wraps URLLoader(Factory) mojom interfaces and handles throttling properly.

The following will be in follow-up CLs:
- make sure it works on Android. The work should be minimal. But I haven't got a chance to test it on Android so I didn't bother adding Android-specific code.
- delete cache entry when safe browsing considers a URL as bad.
- handle sync loading.


BUG=715673
Review-Url: https://codereview.chromium.org/2900563002
Cr-Commit-Position: refs/heads/master@{#476414}
Committed: https://chromium.googlesource.com/chromium/src/+/2d8fb42490681813d05294e44166615c03a6aaff

[yzshen1, 2017-05-24 19:22:20]

Description was changed from

==========
[DRAFT] Safe-browing check for sub-resources from renderer.

BUG=715673
==========

to

==========
This CL:
- introduces mojom interface for safe browsing URL checks. This interface is
registered on each render frame.
- introduces a C++ interface URLLoaderThrottle in content/ for renderers, which
is similar to ResourceThrottle at the browser side; implements a subclass
SafeBrowsingURLLoaderThrottle in chrome/.
- extends ContentRendererClient::WillSendRequest() to also return a list of
throttle objects, so that chrome can attach the safe browsing throttle.
- provides ThrottlingURLLoader which wraps URLLoader(Factory) mojom interfaces
and handles throttling properly.

The following will be in follow-up CLs:
- make sure it works on Android. The work should be minimal. But I haven't got a
chance to test it on Android so I didn't bother adding Android-specific code.
- delete cache entry when safe browsing considers a URL as bad.
- handle sync loading.


BUG=715673
==========

[yzshen1, 2017-05-24 19:22:21]

yzshen@chromium.org changed reviewers:
+ jam@chromium.org, kinuko@chromium.org, nparker@chromium.org,
tsepez@chromium.org

[yzshen1, 2017-05-24 19:25:10]

Hi,

Would you please take a look? Thanks!

I have CC-ed people who are interested in this work. Please feel free to
comment.

[kinuko, 2017-05-25 11:30:58]

Haven't looked very carefully yet but the overall structure looks reasonable to
me.

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:53: 
nit: extra empty line

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1224: //
|frame_data.safe_browsing| goes away.
This is actually not true (or not going to be true at least), but I'm not sure
how much problematic it could be.

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_render_frame_observer.h (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_render_frame_observer.h:38: explicit
ChromeRenderFrameObserver(ChromeContentRendererClient* client,
nit: no explicit

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
File content/child/throttling_url_loader.cc (right):

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
content/child/throttling_url_loader.cc:51: // of URLLoaderThrottles.
Do you expect we might be adding other throttlers here like the ones we have in
the browser process today?

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
content/child/throttling_url_loader.cc:226:
client_binding_.ResumeIncomingMethodCallProcessing();
I see this looks neat.

[Adam Rice, 2017-05-25 11:33:24]

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

[Adam Rice, 2017-05-25 11:33:24]

Hooking this up to WebSockets is going to be harder than I realised, because I
can't mention safe browsing inside WebKit source code. I will have to do
something like create a WebSocketThrottle class in WebKit/public/web
(WebWebSocketThrottle?) and then implement SafeBrowsingWebSocketThrottle in
//chrome.

I wonder if it would be useful to build SafeBrowsingURLLoaderThrottle on top of
the RequestChecker class I have been working on in my refactoring? You could
then get UMA metrics and Android support for free. RequestChecker would have to
be modified to not directly depend on URLRequest similar to what you have done
here. Please take a look at https://codereview.chromium.org/2876473003/
particularly
https://codereview.chromium.org/2876473003/diff/100001/components/safe_browsi...
and let me know what you think.

Are you planning to add Android webview support in the near future, or is that
out-of-scope?

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.h:285: FrameDataMap
frame_data_map_;
Could content::RenderFrame be made to inherit from base::SupportsUserData rather
than doing this?

[kinuko, 2017-05-25 11:36:25]

We have resource fetch code path where frame is not really associated, e.g. for
fetches from workers, if we can minimize direct dependency to frames that might
be a little more convenient...

[jam, 2017-05-25 15:45:51]

looks great, some comments

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.h (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:20: class SafeBrowsingImpl :
public chrome::mojom::SafeBrowsing {
nit: document purpose of this class

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:22:
SafeBrowsingImpl(scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
did you consider having one of these classes per process instead of per frame?
Then CreateCheckerAndCheck could take a render_frame_id as a parameter, and we
would avoid having to create a message pipe/extra process hop for each frame?

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:47: int render_process_id_ =
0;
nit: here and below, use MSG_ROUTING_NONE

https://codereview.chromium.org/2900563002/diff/160001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2900563002/diff/160001/content/child/resource...
content/child/resource_dispatcher.cc:592: // That may require to extend the
bindings API to change the priority of
btw it's not clear to me that we actually care to change the priority for IPCs
for sync XHRs.. we should validate that before doing any changes

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
File content/child/throttling_url_loader.cc (right):

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
content/child/throttling_url_loader.cc:51: // of URLLoaderThrottles.
On 2017/05/25 11:30:57, kinuko wrote:
> Do you expect we might be adding other throttlers here like the ones we have
in
> the browser process today?

Most likely yes, i.e. extensions webRequest

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
File content/child/throttling_url_loader.h (right):

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
content/child/throttling_url_loader.h:1: // Copyright 2017 The Chromium Authors.
All rights reserved.
please add unit tests for this class to handle pausing/cancelling requests in
all the stages

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
content/child/throttling_url_loader.h:21: class ThrottlingURLLoader : public
mojom::URLLoaderClient,
nit: please document

https://codereview.chromium.org/2900563002/diff/160001/content/public/child/u...
File content/public/child/url_loader_throttle.h (right):

https://codereview.chromium.org/2900563002/diff/160001/content/public/child/u...
content/public/child/url_loader_throttle.h:22: class CONTENT_EXPORT
URLLoaderThrottle {
nit: please document

https://codereview.chromium.org/2900563002/diff/160001/content/public/child/u...
content/public/child/url_loader_throttle.h:26: class CONTENT_EXPORT Delegate {
nit: export not needed since it's not copyable

https://codereview.chromium.org/2900563002/diff/160001/content/public/child/u...
content/public/child/url_loader_throttle.h:30: // Resumes the deferred resource
load. It is a no-op if the resource load is
nit: blank line above

[yzshen1, 2017-05-25 19:44:50]

(I am still updating the CL and will reply to other comments once I upload a new
patch.)

On 2017/05/25 11:33:24, Adam Rice wrote:
> Hooking this up to WebSockets is going to be harder than I realised, because I
> can't mention safe browsing inside WebKit source code. I will have to do
> something like create a WebSocketThrottle class in WebKit/public/web
> (WebWebSocketThrottle?) and then implement SafeBrowsingWebSocketThrottle in
> //chrome.
> 
> I wonder if it would be useful to build SafeBrowsingURLLoaderThrottle on top
of
> the RequestChecker class I have been working on in my refactoring?

Are you thinking about sharing RequestChecker with the old code path?

SafeBrowsingURLLoaderThrottle lives at the renderer side, it will need to use
mojo interface to talk with the browser process. Besides, the state machine is
not the same as the current BaseResourceThrottle. IMO it doesn't gain us much to
try to generalize RequestChecker to support that. WDYT?

These meeting notes describe the differences between current behavior and with
network service:
https://docs.google.com/document/d/1AyW5oF3h6QaihAGWYASPn-XPrz2a1P9fv9T8EAuMq...

> You could
> then get UMA metrics and Android support for free. RequestChecker would have
to
> be modified to not directly depend on URLRequest similar to what you have done
> here. Please take a look at https://codereview.chromium.org/2876473003/
> particularly
>
https://codereview.chromium.org/2876473003/diff/100001/components/safe_browsi...
> and let me know what you think.
> 
> Are you planning to add Android webview support in the near future, or is that
> out-of-scope?
Yes. The new code path need to work wherever currently code path works.

[yzshen1, 2017-05-25 19:52:50]

On 2017/05/25 11:36:25, kinuko wrote:
> We have resource fetch code path where frame is not really associated, e.g.
for
> fetches from workers, if we can minimize direct dependency to frames that
might
> be a little more convenient...

Ah, I didn't know about this.
Where do we show warning page for bad resource fetches, when those fetches are
not associated with a frame?
Could you please point me to the code where such fetches happen? I would like to
learn more about it.

Thanks!

[Adam Rice, 2017-05-26 04:50:09]

On 2017/05/25 19:44:50, yzshen1 wrote:
> On 2017/05/25 11:33:24, Adam Rice wrote:
> > I wonder if it would be useful to build SafeBrowsingURLLoaderThrottle on top
> of
> > the RequestChecker class I have been working on in my refactoring?
> 
> Are you thinking about sharing RequestChecker with the old code path?

Yes, RequestChecker is just the logic from the existing
safe_browsing::BaseResourceThrottle factored out to be reusable.

> SafeBrowsingURLLoaderThrottle lives at the renderer side, it will need to use
> mojo interface to talk with the browser process. Besides, the state machine is
> not the same as the current BaseResourceThrottle. IMO it doesn't gain us much
to
> try to generalize RequestChecker to support that. WDYT?

I was thinking about SafeBrowsingUrlCheckerImpl when I wrote 
SafeBrowsingURLLoaderThrottle. Sorry for the confusion.

I've been looking again at the difference between RequestChecker and
SafeBrowsingUrlCheckerImpl. It's interesting that SafeBrowsingUrlCheckerImpl
ends
up having simpler logic despite having to deal with possibly multiple pending
redirect lookups. I think this is at least partly because
SafeBrowsingUrlCheckerImpl unifies the behaviour for Android / non-Android.

So in conclusion I think it makes sense to keep RequestChecker and
SafeBrowsingUrlCheckerImpl separate. RequestChecker can continue to implement
the legacy behaviour until it is finally removed, and SafeBrowsingUrlCheckerImpl
won't have to deal with the mismatch between the old and new behaviour.

> > Are you planning to add Android webview support in the near future, or is
that
> > out-of-scope?
> Yes. The new code path need to work wherever currently code path works.

Great!

[kinuko, 2017-05-26 07:23:18]

On 2017/05/25 19:52:50, yzshen1 wrote:
> On 2017/05/25 11:36:25, kinuko wrote:
> > We have resource fetch code path where frame is not really associated, e.g.
> for
> > fetches from workers, if we can minimize direct dependency to frames that
> might
> > be a little more convenient...
> 
> Ah, I didn't know about this.
> Where do we show warning page for bad resource fetches, when those fetches are
> not associated with a frame?
> Could you please point me to the code where such fetches happen? I would like
to
> learn more about it.
> 
> Thanks!

In some cases (e.g. for fetches from Service Worker) I don't think we really
show UI today (i.e. it's a bit broken already). It goes through regular
ResourceDispatcher code path but it doesn't have corresponding RenderFrameImpl.

Separately we also have some new code path (that's not enabled by default) that
binds and uses URLLoader on non-main-thread, and therefore they don't go through
RenderFrameImpl. For these we set 'frame ID' of the parent frame in requests so
that security / auth errors can be displayed on the right frame.

Their entry points are:

https://cs.chromium.org/chromium/src/content/renderer/service_worker/service_...
https://cs.chromium.org/chromium/src/content/renderer/service_worker/worker_f...

(I'm planning to have generic 'fetch context' base interface for all these
cases, hoping that it could be useful for these various use cases)

[yzshen1, 2017-05-26 20:43:43]

On 2017/05/26 07:23:18, kinuko wrote:
> On 2017/05/25 19:52:50, yzshen1 wrote:
> > On 2017/05/25 11:36:25, kinuko wrote:
> > > We have resource fetch code path where frame is not really associated,
e.g.
> > for
> > > fetches from workers, if we can minimize direct dependency to frames that
> > might
> > > be a little more convenient...
> > 
> > Ah, I didn't know about this.
> > Where do we show warning page for bad resource fetches, when those fetches
are
> > not associated with a frame?
> > Could you please point me to the code where such fetches happen? I would
like
> to
> > learn more about it.
> > 
> > Thanks!
> 
> In some cases (e.g. for fetches from Service Worker) I don't think we really
> show UI today (i.e. it's a bit broken already). It goes through regular
> ResourceDispatcher code path but it doesn't have corresponding
RenderFrameImpl.
> 
> Separately we also have some new code path (that's not enabled by default)
that
> binds and uses URLLoader on non-main-thread, and therefore they don't go
through
> RenderFrameImpl. For these we set 'frame ID' of the parent frame in requests
so
> that security / auth errors can be displayed on the right frame.
> 
> Their entry points are:
> 
>
https://cs.chromium.org/chromium/src/content/renderer/service_worker/service_...
>
https://cs.chromium.org/chromium/src/content/renderer/service_worker/worker_f...
> 
> (I'm planning to have generic 'fetch context' base interface for all these
> cases, hoping that it could be useful for these various use cases)

Thanks for the information, Kinuko!
I have changed SafeBrowsing to be per-process instead of per-frame.
CreateCheckerAndCheckUrl() now takes a render_frame_id, which could be an
invalid ID for those non-frame-associated URL requests.

PTAL.

[yzshen1, 2017-05-26 20:43:53]

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:53: 
On 2017/05/25 11:30:57, kinuko wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.h (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:20: class SafeBrowsingImpl :
public chrome::mojom::SafeBrowsing {
On 2017/05/25 15:45:50, jam wrote:
> nit: document purpose of this class

Done.

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:22:
SafeBrowsingImpl(scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
On 2017/05/25 15:45:50, jam wrote:
> did you consider having one of these classes per process instead of per frame?
> Then CreateCheckerAndCheck could take a render_frame_id as a parameter, and we
> would avoid having to create a message pipe/extra process hop for each frame?

I have changed it as you suggested.
It not only reduces the cost, but also better supports non-frame-associated URL
requests.

https://codereview.chromium.org/2900563002/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:47: int render_process_id_ =
0;
On 2017/05/25 15:45:50, jam wrote:
> nit: here and below, use MSG_ROUTING_NONE

Done.

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1224: //
|frame_data.safe_browsing| goes away.
On 2017/05/25 11:30:57, kinuko wrote:
> This is actually not true (or not going to be true at least), but I'm not sure
> how much problematic it could be.

Since URL loads may not always associated with render frames (thanks for telling
me that!), I changed the SafeBrowsing interface to be per-process instead of
per-frame. It is stored as a member variable of this class. I think that should
solve the lifespan issue.

Thanks!

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.h:285: FrameDataMap
frame_data_map_;
On 2017/05/25 11:33:24, Adam Rice wrote:
> Could content::RenderFrame be made to inherit from base::SupportsUserData
rather
> than doing this?

I have changed the SafeBrowsing interface to be per-process. That way we don't
need this map (or base::SupportUserData).

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_render_frame_observer.h (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_render_frame_observer.h:38: explicit
ChromeRenderFrameObserver(ChromeContentRendererClient* client,
On 2017/05/25 11:30:57, kinuko wrote:
> nit: no explicit

Done.

https://codereview.chromium.org/2900563002/diff/160001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2900563002/diff/160001/content/child/resource...
content/child/resource_dispatcher.cc:592: // That may require to extend the
bindings API to change the priority of
On 2017/05/25 15:45:51, jam wrote:
> btw it's not clear to me that we actually care to change the priority for IPCs
> for sync XHRs.. we should validate that before doing any changes

Makes sense. Thanks for the input!

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
File content/child/throttling_url_loader.cc (right):

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
content/child/throttling_url_loader.cc:51: // of URLLoaderThrottles.
On 2017/05/25 15:45:51, jam wrote:
> On 2017/05/25 11:30:57, kinuko wrote:
> > Do you expect we might be adding other throttlers here like the ones we have
> in
> > the browser process today?
> 
> Most likely yes, i.e. extensions webRequest

Acknowledged.

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
content/child/throttling_url_loader.cc:226:
client_binding_.ResumeIncomingMethodCallProcessing();
On 2017/05/25 11:30:57, kinuko wrote:
> I see this looks neat.

:)

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
File content/child/throttling_url_loader.h (right):

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
content/child/throttling_url_loader.h:1: // Copyright 2017 The Chromium Authors.
All rights reserved.
On 2017/05/25 15:45:51, jam wrote:
> please add unit tests for this class to handle pausing/cancelling requests in
> all the stages

Done.

https://codereview.chromium.org/2900563002/diff/160001/content/child/throttli...
content/child/throttling_url_loader.h:21: class ThrottlingURLLoader : public
mojom::URLLoaderClient,
On 2017/05/25 15:45:51, jam wrote:
> nit: please document

Done.

https://codereview.chromium.org/2900563002/diff/160001/content/public/child/u...
File content/public/child/url_loader_throttle.h (right):

https://codereview.chromium.org/2900563002/diff/160001/content/public/child/u...
content/public/child/url_loader_throttle.h:22: class CONTENT_EXPORT
URLLoaderThrottle {
On 2017/05/25 15:45:51, jam wrote:
> nit: please document

Done.

https://codereview.chromium.org/2900563002/diff/160001/content/public/child/u...
content/public/child/url_loader_throttle.h:26: class CONTENT_EXPORT Delegate {
On 2017/05/25 15:45:51, jam wrote:
> nit: export not needed since it's not copyable

Done.

https://codereview.chromium.org/2900563002/diff/160001/content/public/child/u...
content/public/child/url_loader_throttle.h:30: // Resumes the deferred resource
load. It is a no-op if the resource load is
On 2017/05/25 15:45:51, jam wrote:
> nit: blank line above

Done.

[yzshen1, 2017-05-26 20:47:08]

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

[commit-bot: I haz the power, 2017-05-26 20:48:04]

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, 2017-05-26 20:52:35]

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

[commit-bot: I haz the power, 2017-05-26 20:52:36]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yzshen1, 2017-05-26 21:00:21]

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

[commit-bot: I haz the power, 2017-05-26 21:01:02]

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, 2017-05-26 21:17:30]

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

[commit-bot: I haz the power, 2017-05-26 21:17:31]

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

[yzshen1, 2017-05-26 21:35:41]

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

[commit-bot: I haz the power, 2017-05-26 21:36:26]

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, 2017-05-26 21:55:24]

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

[commit-bot: I haz the power, 2017-05-26 21:55:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jam, 2017-05-26 22:43:55]

lgtm

[yzshen1, 2017-05-27 05:17:08]

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

[commit-bot: I haz the power, 2017-05-27 05:17:18]

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, 2017-05-27 06:43:50]

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

[commit-bot: I haz the power, 2017-05-27 06:43:51]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Adam Rice, 2017-05-29 02:18:41]

https://codereview.chromium.org/2900563002/diff/260001/chrome/common/safe_bro...
File chrome/common/safe_browsing.mojom (right):

https://codereview.chromium.org/2900563002/diff/260001/chrome/common/safe_bro...
chrome/common/safe_browsing.mojom:10: interface SafeBrowsing {
FYI: I need this to be in //components in order to implement WebSocket safe
browsing for Android webview.

It's fine for that to be in a followup CL.

[kinuko, 2017-05-29 13:36:50]

Looking pretty good.

https://codereview.chromium.org/2900563002/diff/260001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:102:
callbacks_[next_index_].Reset();
Can we use OnceCallback here?

https://codereview.chromium.org/2900563002/diff/260001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.cc (right):

https://codereview.chromium.org/2900563002/diff/260001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.cc:97: // If a
service-side disconnect happens, treate all URLs as if they are safe.
nit: treate -> treat

https://codereview.chromium.org/2900563002/diff/260001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.h (right):

https://codereview.chromium.org/2900563002/diff/260001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.h:23: // object,
whichever happens earlier.
Can you also note that render_frame_id is used for displaying UI?

https://codereview.chromium.org/2900563002/diff/260001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.h:24: explicit
SafeBrowsingURLLoaderThrottle(
nit: no need of explicit

https://codereview.chromium.org/2900563002/diff/260001/content/child/request_...
File content/child/request_extra_data.h (right):

https://codereview.chromium.org/2900563002/diff/260001/content/child/request_...
content/child/request_extra_data.h:179:
std::vector<std::unique_ptr<URLLoaderThrottle>>& url_loader_throttles() {
let's name this TakeUrlLoaderThrottles() and return
std::move(url_loader_throttles_)

https://codereview.chromium.org/2900563002/diff/260001/content/child/throttli...
File content/child/throttling_url_loader.cc (right):

https://codereview.chromium.org/2900563002/diff/260001/content/child/throttli...
content/child/throttling_url_loader.cc:18: std::unique_ptr<ThrottlingURLLoader>
result(
naming-nit: maybe 'result' -> 'loader' or 'instance' if you don't have strong
preference here?

https://codereview.chromium.org/2900563002/diff/260001/content/child/throttli...
File content/child/throttling_url_loader.h (right):

https://codereview.chromium.org/2900563002/diff/260001/content/child/throttli...
content/child/throttling_url_loader.h:90: // Set if start if deferred.
nit: if deferred -> is deferred

https://codereview.chromium.org/2900563002/diff/260001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/2900563002/diff/260001/content/public/rendere...
content/public/renderer/content_renderer_client.h:227: // |url|.  If the
function returns true, the url is changed to |new_url|.
Add a comment about throttles?

https://codereview.chromium.org/2900563002/diff/260001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4467:
extra_data->set_url_loader_throttles(std::move(throttles));
Can you add a todo comment to note that we need to set up throttles for some
worker cases that don't go through here? (Could have my ldap there)

[Tom Sepez, 2017-05-30 20:18:23]

LGTM. This looks OK, but keep me in the loop about the cache design.

[yzshen1, 2017-05-31 00:29:26]

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

[yzshen1, 2017-05-31 00:30:08]

Thanks Kinuko and Adam.

PTAL

https://codereview.chromium.org/2900563002/diff/260001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:102:
callbacks_[next_index_].Reset();
On 2017/05/29 13:36:50, kinuko wrote:
> Can we use OnceCallback here?

Done. I split this mojom into a separate GN target and enabled OnceCallback for
it.

https://codereview.chromium.org/2900563002/diff/260001/chrome/common/safe_bro...
File chrome/common/safe_browsing.mojom (right):

https://codereview.chromium.org/2900563002/diff/260001/chrome/common/safe_bro...
chrome/common/safe_browsing.mojom:10: interface SafeBrowsing {
On 2017/05/29 02:18:41, Adam Rice wrote:
> FYI: I need this to be in //components in order to implement WebSocket safe
> browsing for Android webview.
> 
> It's fine for that to be in a followup CL.

I see. I think I also need to move safe_browsing_impl.{h,cc} as well.

I will do it in a follow-up CL since a couple of reviewers have LG-ed and this
CL is pretty big.

Thanks for reminding me of this!

https://codereview.chromium.org/2900563002/diff/260001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.cc (right):

https://codereview.chromium.org/2900563002/diff/260001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.cc:97: // If a
service-side disconnect happens, treate all URLs as if they are safe.
On 2017/05/29 13:36:50, kinuko wrote:
> nit: treate -> treat

Done.

https://codereview.chromium.org/2900563002/diff/260001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.h (right):

https://codereview.chromium.org/2900563002/diff/260001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.h:23: // object,
whichever happens earlier.
On 2017/05/29 13:36:50, kinuko wrote:
> Can you also note that render_frame_id is used for displaying UI?

Done.

https://codereview.chromium.org/2900563002/diff/260001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.h:24: explicit
SafeBrowsingURLLoaderThrottle(
On 2017/05/29 13:36:50, kinuko wrote:
> nit: no need of explicit

Done.

https://codereview.chromium.org/2900563002/diff/260001/content/child/request_...
File content/child/request_extra_data.h (right):

https://codereview.chromium.org/2900563002/diff/260001/content/child/request_...
content/child/request_extra_data.h:179:
std::vector<std::unique_ptr<URLLoaderThrottle>>& url_loader_throttles() {
On 2017/05/29 13:36:50, kinuko wrote:
> let's name this TakeUrlLoaderThrottles() and return
> std::move(url_loader_throttles_)

Done.

https://codereview.chromium.org/2900563002/diff/260001/content/child/throttli...
File content/child/throttling_url_loader.cc (right):

https://codereview.chromium.org/2900563002/diff/260001/content/child/throttli...
content/child/throttling_url_loader.cc:18: std::unique_ptr<ThrottlingURLLoader>
result(
On 2017/05/29 13:36:50, kinuko wrote:
> naming-nit: maybe 'result' -> 'loader' or 'instance' if you don't have strong
> preference here?

Done.

https://codereview.chromium.org/2900563002/diff/260001/content/child/throttli...
File content/child/throttling_url_loader.h (right):

https://codereview.chromium.org/2900563002/diff/260001/content/child/throttli...
content/child/throttling_url_loader.h:90: // Set if start if deferred.
On 2017/05/29 13:36:50, kinuko wrote:
> nit: if deferred -> is deferred

Sorry I need a spell checker for my editor. :) Done

https://codereview.chromium.org/2900563002/diff/260001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/2900563002/diff/260001/content/public/rendere...
content/public/renderer/content_renderer_client.h:227: // |url|.  If the
function returns true, the url is changed to |new_url|.
On 2017/05/29 13:36:50, kinuko wrote:
> Add a comment about throttles?

Done.

https://codereview.chromium.org/2900563002/diff/260001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4467:
extra_data->set_url_loader_throttles(std::move(throttles));
On 2017/05/29 13:36:50, kinuko wrote:
> Can you add a todo comment to note that we need to set up throttles for some
> worker cases that don't go through here? (Could have my ldap there)

Done.

[commit-bot: I haz the power, 2017-05-31 00:30:28]

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, 2017-05-31 00:40:51]

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

[commit-bot: I haz the power, 2017-05-31 00:40:52]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Adam Rice, 2017-05-31 06:08:51]

lgtm for use with WebSocket. I will defer to other reviewers for the other
aspects.

https://codereview.chromium.org/2900563002/diff/300001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.cc (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.cc:34:
render_frame_id_, mojo::MakeRequest(&url_checker_), url, load_flags,
Do you need to include "mojo/public/cpp/bindings/interface_request.h" to use
this?

[kinuko, 2017-05-31 06:35:00]

lgtm

[Nathan Parker, 2017-05-31 17:17:06]

SafeBrowsing*: My comments are mostly on naming, and requests for additional
comments. Otherwise looks good.

(I'm not commenting on the throttling architecture -- I'll leave that to the
experts there)

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:26: // TODO(yzshen): Share
such value with safe_browsing::BaseResourceThrottle.
nit: Can you add a class-level or file-level comment to this and other new
files?  There are now lots of files/classes named
(SafeBrowsing).*(Url).*(Check).* and it's important to make it clear what
they're used for.  Thanks.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:116: resource.redirect_urls =
I may be forgetting a detail of the design plan, but is the plan to eventually
replace the SafeBrowsingResourceThrottle code with this code?  Duplicating that
code here will likely lead to them diverging over time, which would mean
inconsistent results.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:178:
std::move(callbacks_[next_index_]).Run(false);
Add comment (assuming this is correct): "If user decided to not proceed through
a warning, mark all the remaining redirects as "bad."

Actually, how about moving this to BlockAndProcessUrls() (instead of calling
ProcessUrls there) since that's the only place it could be come STATE_BLOCKED?
That'd simplify the apparent possible state transitions.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:242: std::vector<GURL> urls_;
Q: Are the callbacks different for each url?  It seems like the action taken
would be the same regardless of which URL was found to be bad.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:244: size_t next_index_ = 0;
Nit: add comment about what next_index refers to.  AFAIKT, urls_ and callbacks_
before next_index are no longer relevant/valid, and those at next_index_ are yet
to be checked. ya?

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.h (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:21: // This class implements
the Mojo interface for renderers to perform safe
Will this always be just for renderers, or will other things in the browser
process use it eventually as well?

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:23: class SafeBrowsingImpl :
public chrome::mojom::SafeBrowsing {
Nit: This name feels pretty generic (SafeBrowsingImpl). Could it be
SafeBrowsingMojomImpl, or is that not the typical mojo style?

https://codereview.chromium.org/2900563002/diff/300001/chrome/common/safe_bro...
File chrome/common/safe_browsing.mojom (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/common/safe_bro...
chrome/common/safe_browsing.mojom:19: CreateCheckerAndCheck(
Naming and/or comment suggestion:

I think we should state more specifically what this is checking against.

The SafeBrowsingDatabase::CheckBrowseUrl() checks against Safe Browsing's
Malware, Phishing, and UwS blacklists.  We could state that here, or just
mention that it uses the above function to determine if a URL is "safe."

If we wanted to add additional kinds of checks (such as... for
subresource-filtering of malicious ads, or a whitelist that'd prevent certain
other checks), would we add another method here? If so, then maybe we should
start with a more specific name.  The CheckUrl() below could be
"CheckBrowseUrl()" so later we could add "CheckWhitelistUrl()" or
"CheckSubresourceBlockingUrl()" or some such.

https://codereview.chromium.org/2900563002/diff/300001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.h (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.h:39: void
OnCheckUrlResult(bool result);
Naming: How about (here and elsewhere where relevant) 
  bool is_unsafe
or
  bool should_be_blocked

[yzshen1, 2017-05-31 23:32:46]

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

[commit-bot: I haz the power, 2017-05-31 23:33:24]

Dry run: CQ is trying da patch.

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

[yzshen1, 2017-05-31 23:34:44]

Thanks!

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:26: // TODO(yzshen): Share
such value with safe_browsing::BaseResourceThrottle.
On 2017/05/31 17:17:06, Nathan Parker wrote:
> nit: Can you add a class-level or file-level comment to this and other new
> files?  There are now lots of files/classes named
> (SafeBrowsing).*(Url).*(Check).* and it's important to make it clear what
> they're used for.  Thanks.

Done.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:116: resource.redirect_urls =
On 2017/05/31 17:17:06, Nathan Parker wrote:
> I may be forgetting a detail of the design plan, but is the plan to eventually
> replace the SafeBrowsingResourceThrottle code with this code?  Duplicating
that
> code here will likely lead to them diverging over time, which would mean
> inconsistent results.

Yes. Eventually when the network service is enabled by default,
SafeBrowsingResourceThrottle will go away.

The renderer-initiated code path is quite different from
SafeBrowsingResourceThrottle: the throttling logic is now split into two parts:
SafeBrowsingURLLoaderThrottle at the renderer side and this class at the
browsing side. Besides, the state machine is also different. The new code path
doesn't wait for SB confirmation before writing disk cache or following
redirects. (Instead, if the SB check says "no", the response won't be given to
the consumer and the disk cache entry is removed.)

So I think at the moment, it won't gain us much trying to share code between
this and SafeBrowsingResourceThrottle.

In my next CL, I am going to support browser-initiated SB check for network
service. There are two alternatives to share code:
(1) share code between the new-browser-side-code-path and
SafeBrowsingResourceThrottle;
(2) share code between the new-renderer-side-code-path and
new-browser-side-code-path.

I am thinking to take approach (2), so that the browser-side and renderer-side
checks are consistent in the new world.
We could also consider refactoring SafeBrowsingResourceThrottle to use the
new-browser-side-code-path if we take approach (2), as a follow-up work item.
That way, the new code path can be shipped to our end users before network
service is fully ready.

WDYT? Thanks!

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:178:
std::move(callbacks_[next_index_]).Run(false);
On 2017/05/31 17:17:06, Nathan Parker wrote:
> Add comment (assuming this is correct): "If user decided to not proceed
through
> a warning, mark all the remaining redirects as "bad."
> 
> Actually, how about moving this to BlockAndProcessUrls() (instead of calling
> ProcessUrls there) since that's the only place it could be come STATE_BLOCKED?
> That'd simplify the apparent possible state transitions.

Done.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:242: std::vector<GURL> urls_;
On 2017/05/31 17:17:06, Nathan Parker wrote:
> Q: Are the callbacks different for each url?  It seems like the action taken
> would be the same regardless of which URL was found to be bad.

Each mojo call has a separate base::OnceCallback instance. Each must be called
exactly once unless the interface is disconnected. That is the requirement of
Mojo.

I agree that the action taken at the renderer side is actually the same.

If we would like to use the same "callback" for multiple times. We will need to
use another mojo interface, whose implementation is at the renderer side. The
browser side can then make calls on that interface to notify the results.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:244: size_t next_index_ = 0;
On 2017/05/31 17:17:06, Nathan Parker wrote:
> Nit: add comment about what next_index refers to.  AFAIKT, urls_ and
callbacks_
> before next_index are no longer relevant/valid, and those at next_index_ are
yet
> to be checked. ya?

Updated with comment. Thanks!

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.h (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:21: // This class implements
the Mojo interface for renderers to perform safe
On 2017/05/31 17:17:06, Nathan Parker wrote:
> Will this always be just for renderers, or will other things in the browser
> process use it eventually as well?

The browser side won't use mojo to talk with safe browsing. So this class is
only used for renderers.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:23: class SafeBrowsingImpl :
public chrome::mojom::SafeBrowsing {
On 2017/05/31 17:17:06, Nathan Parker wrote:
> Nit: This name feels pretty generic (SafeBrowsingImpl). Could it be
> SafeBrowsingMojomImpl, or is that not the typical mojo style?

Agreed. It is the common practice to use FooImpl for implementation of
mojom::Foo. But I agree that this seems too generic in this case. I changed the
name to MojoSafeBrowsingImpl.

https://codereview.chromium.org/2900563002/diff/300001/chrome/common/safe_bro...
File chrome/common/safe_browsing.mojom (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/common/safe_bro...
chrome/common/safe_browsing.mojom:19: CreateCheckerAndCheck(
On 2017/05/31 17:17:06, Nathan Parker wrote:
> Naming and/or comment suggestion:
> 
> I think we should state more specifically what this is checking against.
> 
> The SafeBrowsingDatabase::CheckBrowseUrl() checks against Safe Browsing's
> Malware, Phishing, and UwS blacklists.  We could state that here, or just
> mention that it uses the above function to determine if a URL is "safe."
> 
> If we wanted to add additional kinds of checks (such as... for
> subresource-filtering of malicious ads, or a whitelist that'd prevent certain
> other checks), would we add another method here? If so, then maybe we should
> start with a more specific name.  The CheckUrl() below could be
> "CheckBrowseUrl()" so later we could add "CheckWhitelistUrl()" or
> "CheckSubresourceBlockingUrl()" or some such.
> 
Updated the comments.

I am thinking maybe we could have a CheckType enum as parameter in that case? We
could add new methods as well, but we will need to add to both SafeBrowsing and
SafeBrowsingUrlChecker. That is slightly more annoying.

Because we don't need to consider backward-compatibility, we could change this
interface when the need emerges. If you don't mind, I would like to leave it as
it is. WDYT?

https://codereview.chromium.org/2900563002/diff/300001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.cc (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.cc:34:
render_frame_id_, mojo::MakeRequest(&url_checker_), url, load_flags,
On 2017/05/31 06:08:51, Adam Rice wrote:
> Do you need to include "mojo/public/cpp/bindings/interface_request.h" to use
> this?

Done.

https://codereview.chromium.org/2900563002/diff/300001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.h (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/safe_browsing_url_loader_throttle.h:39: void
OnCheckUrlResult(bool result);
On 2017/05/31 17:17:06, Nathan Parker wrote:
> Naming: How about (here and elsewhere where relevant) 
>   bool is_unsafe
> or
>   bool should_be_blocked

Done. The meaning of this parameter is actually the opposite (which shows that I
really need to rename it! :))
I renamed it to "safe".

[vakh (use Gerrit instead), 2017-05-31 23:53:07]

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

[vakh (use Gerrit instead), 2017-05-31 23:53:09]

Thanks for doing this work.

I have to admit that I tried hard to follow the logic in its entirety but did
not succeed very much. That being said, it seems good overall.

My comments are mostly just nits.

https://codereview.chromium.org/2900563002/diff/160001/chrome/common/safe_bro...
File chrome/common/safe_browsing.mojom (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/common/safe_bro...
chrome/common/safe_browsing.mojom:11: // Queries safe browsing whether |url| is
safe to load, and creates a
nit: Safe Browsing

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:28: // verify a URL. After
this amount of time the outstanding check will be
SafeBrowsing service also checks things that are not URLs. Might be better
worded as:
Maximum time in milliseconds to wait for the SafeBrowsing service reputation
check.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:29: // aborted, and the URL
will be treated as if it were safe.
s/URL/resource

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.h (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:21: // This class implements
the Mojo interface for renderers to perform safe
nit: SafeBrowsing

https://codereview.chromium.org/2900563002/diff/320001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/320001/chrome/browser/safe_br...
chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc:82: DVLOG(1) <<
"SafeBrowsingUrlCheckerImpl checks URL: " << url;
Just for better clarity, could you change this to DVLOG(INFO)? Thanks.

https://codereview.chromium.org/2900563002/diff/320001/chrome/browser/safe_br...
chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc:112:
security_interstitials::UnsafeResource resource;
The code duplication here makes me slightly nervous but I know you said
elsewhere that you'll take care of that shortly. I look forward to that CL also
:)

https://codereview.chromium.org/2900563002/diff/320001/chrome/browser/safe_br...
chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc:180: // TODO(yzshen):
Consider moving CanCheckResourceType() to the renderer
Optional: Since you're the expert here and already diving into this code, it
would be good to add to this comment what it would take to do what the TODO
says.

https://codereview.chromium.org/2900563002/diff/320001/chrome/common/safe_bro...
File chrome/common/safe_browsing.mojom (right):

https://codereview.chromium.org/2900563002/diff/320001/chrome/common/safe_bro...
chrome/common/safe_browsing.mojom:24: url.mojom.Url url,
There are other SafeBrowsing checks that check non-URL things (such as Chrome
Extension IDs).
How do we handle those?
Are we going to have different behavior if the resource to check is a URL vs
not?

I don't know the answer to those questions but I think it might be good to
document that here.

https://codereview.chromium.org/2900563002/diff/320001/content/public/common/...
File content/public/common/resource_type_enum_traits.cc (right):

https://codereview.chromium.org/2900563002/diff/320001/content/public/common/...
content/public/common/resource_type_enum_traits.cc:15: return
content::mojom::ResourceType::RESOURCE_TYPE_MAIN_FRAME;
This is fairly mechanical. Is it possible to use static_cast to convert the
content::ResourceType to content::mojom::ResourceType ?

https://codereview.chromium.org/2900563002/diff/320001/content/public/common/...
content/public/common/resource_type_enum_traits.cc:63: case
content::mojom::ResourceType::RESOURCE_TYPE_MAIN_FRAME:
Same here

https://codereview.chromium.org/2900563002/diff/320001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/2900563002/diff/320001/content/public/rendere...
content/public/renderer/content_renderer_client.h:228: // |throttles| is
appended with URLLoaderThrottle instances that should be
Nit: These three comments could be in one paragraph.

[vakh (use Gerrit instead), 2017-05-31 23:54:31]

https://codereview.chromium.org/2900563002/diff/160001/chrome/common/safe_bro...
File chrome/common/safe_browsing.mojom (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/common/safe_bro...
chrome/common/safe_browsing.mojom:11: // Queries safe browsing whether |url| is
safe to load, and creates a
On 2017/05/31 23:53:08, vakh (Varun Khaneja) wrote:
> nit: Safe Browsing

nit nit: SafeBrowsing (here and elsewhere)

[commit-bot: I haz the power, 2017-06-01 01:11:20]

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

[commit-bot: I haz the power, 2017-06-01 01:11:22]

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

[yzshen1, 2017-06-01 17:43:22]

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

[commit-bot: I haz the power, 2017-06-01 17:43:42]

Dry run: CQ is trying da patch.

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

[yzshen1, 2017-06-01 17:45:04]

Thanks Varun!

> I have to admit that I tried hard to follow the logic in 
> its entirety but did not succeed very much.

If you think it is necessary, I am happy to chat with you about this work.
Please let me know.

https://codereview.chromium.org/2900563002/diff/160001/chrome/common/safe_bro...
File chrome/common/safe_browsing.mojom (right):

https://codereview.chromium.org/2900563002/diff/160001/chrome/common/safe_bro...
chrome/common/safe_browsing.mojom:11: // Queries safe browsing whether |url| is
safe to load, and creates a
On 2017/05/31 23:54:30, vakh (Varun Khaneja) wrote:
> On 2017/05/31 23:53:08, vakh (Varun Khaneja) wrote:
> > nit: Safe Browsing
> 
> nit nit: SafeBrowsing (here and elsewhere)

Done.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:28: // verify a URL. After
this amount of time the outstanding check will be
On 2017/05/31 23:53:08, vakh (Varun Khaneja) wrote:
> SafeBrowsing service also checks things that are not URLs. Might be better
> worded as:
> Maximum time in milliseconds to wait for the SafeBrowsing service reputation
> check.

Done.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:29: // aborted, and the URL
will be treated as if it were safe.
On 2017/05/31 23:53:08, vakh (Varun Khaneja) wrote:
> s/URL/resource

Done.

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.h (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.h:21: // This class implements
the Mojo interface for renderers to perform safe
On 2017/05/31 23:53:09, vakh (Varun Khaneja) wrote:
> nit: SafeBrowsing

Done.

https://codereview.chromium.org/2900563002/diff/320001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/320001/chrome/browser/safe_br...
chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc:82: DVLOG(1) <<
"SafeBrowsingUrlCheckerImpl checks URL: " << url;
On 2017/05/31 23:53:09, vakh (Varun Khaneja) wrote:
> Just for better clarity, could you change this to DVLOG(INFO)? Thanks.

Do you mean DLOG(INFO)? DVLOG are always logged at the INFO level, the number is
the verbose level.

The nice thing about verbose logging is that it can also be turned on
module-by-module.  For instance:
  --vmodule=profile=2,icon_loader=1,browser_*=3,*/chromeos/*=4 --v=0

https://codereview.chromium.org/2900563002/diff/320001/chrome/browser/safe_br...
chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc:112:
security_interstitials::UnsafeResource resource;
On 2017/05/31 23:53:09, vakh (Varun Khaneja) wrote:
> The code duplication here makes me slightly nervous but I know you said
> elsewhere that you'll take care of that shortly. I look forward to that CL
also
> :)

Yeah, I would like to find ways to tackle the duplication.
I wish I could get the new code path fully working (including the
browser-initiated case), before making any significant changes to the existing
code path.
Then I will work on reducing duplicate code.

https://codereview.chromium.org/2900563002/diff/320001/chrome/browser/safe_br...
chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc:180: // TODO(yzshen):
Consider moving CanCheckResourceType() to the renderer
On 2017/05/31 23:53:09, vakh (Varun Khaneja) wrote:
> Optional: Since you're the expert here and already diving into this code, it
> would be good to add to this comment what it would take to do what the TODO
> says.

Done.

https://codereview.chromium.org/2900563002/diff/320001/chrome/common/safe_bro...
File chrome/common/safe_browsing.mojom (right):

https://codereview.chromium.org/2900563002/diff/320001/chrome/common/safe_bro...
chrome/common/safe_browsing.mojom:24: url.mojom.Url url,
On 2017/05/31 23:53:09, vakh (Varun Khaneja) wrote:
> There are other SafeBrowsing checks that check non-URL things (such as Chrome
> Extension IDs).
> How do we handle those?
> Are we going to have different behavior if the resource to check is a URL vs
> not?
> 
> I don't know the answer to those questions but I think it might be good to
> document that here.

I feel that it should be fine to consider that later.
This interface is not considered "stable", and it certainly doesn't expose all
functionalities of SB.
we can add / change methods when we need to.

This work focuses on interactions between the new network service and
SafeBrowsing, so there is no need to worry about non-URL for now. (The network
service only handles URLs.)

WDYT? Thanks!

https://codereview.chromium.org/2900563002/diff/320001/content/public/common/...
File content/public/common/resource_type_enum_traits.cc (right):

https://codereview.chromium.org/2900563002/diff/320001/content/public/common/...
content/public/common/resource_type_enum_traits.cc:15: return
content::mojom::ResourceType::RESOURCE_TYPE_MAIN_FRAME;
On 2017/05/31 23:53:09, vakh (Varun Khaneja) wrote:
> This is fairly mechanical. Is it possible to use static_cast to convert the
> content::ResourceType to content::mojom::ResourceType ?

Yeah. It is tedious.
But static_cast won't warn us if the two types go out of sync. Therefore it is
the common practice to have such a tedious switch statement. Maybe we could
invent some macros to reduce the typing.

https://codereview.chromium.org/2900563002/diff/320001/content/public/common/...
content/public/common/resource_type_enum_traits.cc:63: case
content::mojom::ResourceType::RESOURCE_TYPE_MAIN_FRAME:
On 2017/05/31 23:53:09, vakh (Varun Khaneja) wrote:
> Same here

Please see my reply above.

https://codereview.chromium.org/2900563002/diff/320001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/2900563002/diff/320001/content/public/rendere...
content/public/renderer/content_renderer_client.h:228: // |throttles| is
appended with URLLoaderThrottle instances that should be
On 2017/05/31 23:53:09, vakh (Varun Khaneja) wrote:
> Nit: These three comments could be in one paragraph.

Done.

[vakh (use Gerrit instead), 2017-06-01 17:57:48]

lgtm

https://codereview.chromium.org/2900563002/diff/320001/chrome/common/safe_bro...
File chrome/common/safe_browsing.mojom (right):

https://codereview.chromium.org/2900563002/diff/320001/chrome/common/safe_bro...
chrome/common/safe_browsing.mojom:24: url.mojom.Url url,
On 2017/06/01 17:45:04, yzshen1 wrote:
> On 2017/05/31 23:53:09, vakh (Varun Khaneja) wrote:
> > There are other SafeBrowsing checks that check non-URL things (such as
Chrome
> > Extension IDs).
> > How do we handle those?
> > Are we going to have different behavior if the resource to check is a URL vs
> > not?
> > 
> > I don't know the answer to those questions but I think it might be good to
> > document that here.
> 
> I feel that it should be fine to consider that later.
Agree

> This interface is not considered "stable", and it certainly doesn't expose all
> functionalities of SB.
> we can add / change methods when we need to.
Agree

> 
> This work focuses on interactions between the new network service and
> SafeBrowsing, so there is no need to worry about non-URL for now. (The network
> service only handles URLs.)
> 
> WDYT? Thanks!
> 

I guess I am missing the big picture here. I don't want to block this CL but
let's sync up in person later.

[Nathan Parker, 2017-06-01 18:22:04]

lgtm

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_impl.cc:116: resource.redirect_urls =
On 2017/05/31 23:34:43, yzshen1 wrote:
> On 2017/05/31 17:17:06, Nathan Parker wrote:
> > I may be forgetting a detail of the design plan, but is the plan to
eventually
> > replace the SafeBrowsingResourceThrottle code with this code?  Duplicating
> that
> > code here will likely lead to them diverging over time, which would mean
> > inconsistent results.
> 
> Yes. Eventually when the network service is enabled by default,
> SafeBrowsingResourceThrottle will go away.
> 
> The renderer-initiated code path is quite different from
> SafeBrowsingResourceThrottle: the throttling logic is now split into two
parts:
> SafeBrowsingURLLoaderThrottle at the renderer side and this class at the
> browsing side. Besides, the state machine is also different. The new code path
> doesn't wait for SB confirmation before writing disk cache or following
> redirects. (Instead, if the SB check says "no", the response won't be given to
> the consumer and the disk cache entry is removed.)
> 
> So I think at the moment, it won't gain us much trying to share code between
> this and SafeBrowsingResourceThrottle.
> 
> In my next CL, I am going to support browser-initiated SB check for network
> service. There are two alternatives to share code:
> (1) share code between the new-browser-side-code-path and
> SafeBrowsingResourceThrottle;
> (2) share code between the new-renderer-side-code-path and
> new-browser-side-code-path.
> 
> I am thinking to take approach (2), so that the browser-side and renderer-side
> checks are consistent in the new world.
> We could also consider refactoring SafeBrowsingResourceThrottle to use the
> new-browser-side-code-path if we take approach (2), as a follow-up work item.
> That way, the new code path can be shipped to our end users before network
> service is fully ready.
> 
> WDYT? Thanks!

Thanks.  I think #2 is good. I'm mostly concerned about long-term code sharing
and understandability.

https://codereview.chromium.org/2900563002/diff/340001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/340001/chrome/browser/safe_br...
chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc:293: int32_t load_flags,
load_flags is int32_t here, and int above.  Want to make it consistent?

[yzshen1, 2017-06-01 18:36:53]

https://codereview.chromium.org/2900563002/diff/340001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/340001/chrome/browser/safe_br...
chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc:293: int32_t load_flags,
On 2017/06/01 18:22:04, Nathan Parker wrote:
> load_flags is int32_t here, and int above.  Want to make it consistent?

Yeah. That is a good question.
I used it as int32_t here because this is a mojom-generated interface. In mojom
we only allow integers with explicit size: int{8, 16, 32, 64}. They are mapped
to int{8, 16, 32, 64}_t in C++.

In all other places, I noticed that load_flags have been defined as "int", so I
followed the majority.

(The same issue also occurs for |render_frame_id|.)

Do you think it is okay to leave it as it is?
I am happy to make changes, but it seems we won't be perfectly consistent
anyway. :)

[Nathan Parker, 2017-06-01 18:38:49]

lgtm

https://codereview.chromium.org/2900563002/diff/340001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/340001/chrome/browser/safe_br...
chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc:293: int32_t load_flags,
On 2017/06/01 18:36:52, yzshen1 wrote:
> On 2017/06/01 18:22:04, Nathan Parker wrote:
> > load_flags is int32_t here, and int above.  Want to make it consistent?
> 
> Yeah. That is a good question.
> I used it as int32_t here because this is a mojom-generated interface. In
mojom
> we only allow integers with explicit size: int{8, 16, 32, 64}. They are mapped
> to int{8, 16, 32, 64}_t in C++.
> 
> In all other places, I noticed that load_flags have been defined as "int", so
I
> followed the majority.
> 
> (The same issue also occurs for |render_frame_id|.)
> 
> Do you think it is okay to leave it as it is?
> I am happy to make changes, but it seems we won't be perfectly consistent
> anyway. :)
> 

I assume all our platforms have 32-bit "ints" so it's not a functional diff.
Maybe just add a comment or cast so it doesn't look like an oversight.

[yzshen1, 2017-06-01 18:44:44]

https://codereview.chromium.org/2900563002/diff/340001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc (right):

https://codereview.chromium.org/2900563002/diff/340001/chrome/browser/safe_br...
chrome/browser/safe_browsing/mojo_safe_browsing_impl.cc:293: int32_t load_flags,
On 2017/06/01 18:38:49, Nathan Parker wrote:
> On 2017/06/01 18:36:52, yzshen1 wrote:
> > On 2017/06/01 18:22:04, Nathan Parker wrote:
> > > load_flags is int32_t here, and int above.  Want to make it consistent?
> > 
> > Yeah. That is a good question.
> > I used it as int32_t here because this is a mojom-generated interface. In
> mojom
> > we only allow integers with explicit size: int{8, 16, 32, 64}. They are
mapped
> > to int{8, 16, 32, 64}_t in C++.
> > 
> > In all other places, I noticed that load_flags have been defined as "int",
so
> I
> > followed the majority.
> > 
> > (The same issue also occurs for |render_frame_id|.)
> > 
> > Do you think it is okay to leave it as it is?
> > I am happy to make changes, but it seems we won't be perfectly consistent
> > anyway. :)
> > 
> 
> I assume all our platforms have 32-bit "ints" so it's not a functional diff.
> Maybe just add a comment or cast so it doesn't look like an oversight.

Done. I added static_cast to be explicit. Thanks!

[yzshen1, 2017-06-01 18:44:55]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2017-06-01 18:44:55]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
tsepez@chromium.org, ricea@chromium.org, kinuko@chromium.org,
nparker@chromium.org, vakh@chromium.org
Link to the patchset: https://codereview.chromium.org/2900563002/#ps360001
(title: ".")

[commit-bot: I haz the power, 2017-06-01 18:45:38]

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, 2017-06-01 20:29:46]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1496342695216000,
"parent_rev": "3538d39da5877e597441971b5424ccc08ec282f9", "commit_rev":
"2d8fb42490681813d05294e44166615c03a6aaff"}

[commit-bot: I haz the power, 2017-06-01 20:29:56]

Description was changed from

==========
This CL:
- introduces mojom interface for safe browsing URL checks. This interface is
registered on each render frame.
- introduces a C++ interface URLLoaderThrottle in content/ for renderers, which
is similar to ResourceThrottle at the browser side; implements a subclass
SafeBrowsingURLLoaderThrottle in chrome/.
- extends ContentRendererClient::WillSendRequest() to also return a list of
throttle objects, so that chrome can attach the safe browsing throttle.
- provides ThrottlingURLLoader which wraps URLLoader(Factory) mojom interfaces
and handles throttling properly.

The following will be in follow-up CLs:
- make sure it works on Android. The work should be minimal. But I haven't got a
chance to test it on Android so I didn't bother adding Android-specific code.
- delete cache entry when safe browsing considers a URL as bad.
- handle sync loading.


BUG=715673
==========

to

==========
This CL:
- introduces mojom interface for safe browsing URL checks. This interface is
registered on each render frame.
- introduces a C++ interface URLLoaderThrottle in content/ for renderers, which
is similar to ResourceThrottle at the browser side; implements a subclass
SafeBrowsingURLLoaderThrottle in chrome/.
- extends ContentRendererClient::WillSendRequest() to also return a list of
throttle objects, so that chrome can attach the safe browsing throttle.
- provides ThrottlingURLLoader which wraps URLLoader(Factory) mojom interfaces
and handles throttling properly.

The following will be in follow-up CLs:
- make sure it works on Android. The work should be minimal. But I haven't got a
chance to test it on Android so I didn't bother adding Android-specific code.
- delete cache entry when safe browsing considers a URL as bad.
- handle sync loading.


BUG=715673

Review-Url: https://codereview.chromium.org/2900563002
Cr-Commit-Position: refs/heads/master@{#476414}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d8fb42490681813d05294e44166...
==========

[commit-bot: I haz the power, 2017-06-01 20:29:59]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/2d8fb42490681813d05294e44166...