Async Safe Browsing check, on mobile

Run the Safe Browsing check in parallel with the resource load, on mobile. This decreases the mobile page-load time by about 12% so that it's basically on-par with non-safe-browsing mode. This does not change desktop.

Meta: I have tests partially written that require some build refactoring which I'd prefer to put in a separate CL. Then I will convert all the existing CHECKs to DCHECKs.

BUG=496754
Committed: https://crrev.com/9534829e7b24491f690032554327508d4f675de6
Cr-Commit-Position: refs/heads/master@{#341249}

[Nathan Parker, 2015-07-23 00:32:46]

nparker@chromium.org changed reviewers:
+ mattm@google.com

[Nathan Parker, 2015-07-23 00:35:10]

nparker@chromium.org changed reviewers:
+ mattm@chromium.org
- mattm@google.com

[Nathan Parker, 2015-07-23 17:40:08]

nparker@chromium.org changed reviewers:
+ thestig@chromium.org
- mattm@chromium.org

[Nathan Parker, 2015-07-23 17:40:09]

Changing reviewers to match OWNERS.

Hi Lei --
  For your review.  Thank you.
 -- Nathan

[Lei Zhang, 2015-07-23 20:31:11]

I defer to the safe_browsing OWNERS, all CC'd now.

[Nathan Parker, 2015-07-23 21:03:13]

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

[mattm, 2015-07-24 23:24:11]

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
File
chrome/browser/renderer_host/data_reduction_proxy_resource_throttle_android.cc
(right):

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_host/data_reduction_proxy_resource_throttle_android.cc:50:
return SafeBrowsingResourceThrottleFactory::CreateWithoutFactory(
This is kinda confusing at first glance.. maybe have a comment like
"DataReductionProxyResourceThrottleFactory is registered as the
SafeBrowsingResourceThrottleFactory, so need to bypass that and use the base
SafeBrowsingResourceThrottleFactory".

Also the naming SafeBrowsingResourceThrottleFactory::CreateWithoutFactory is not
really obvious what CreateWithoutFactory means when the class has Factory in the
name :) Maybe CreateWithoutRegisteredFactory or something?

As an alternative approach, maybe if the registered factory returns NULL, then
the base factory could just fall back to the base behavior? Guess that could be
confusing since NULL could in theory be a valid result, though there isn't
currently a need for that..

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

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:246: if
(defer_state_ == DEFERRED_UNCHECKED_REDIRECT &&
should do redirect_urls_.push_back(unchecked_redirect_url_)

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_host/safe_browsing_resource_throttle.h (right):

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.h:33: // completes
synchronously (no match in the bloom filter), so the request's
since we don't actually use a bloom filter these days, might as well s/bloom
filter/prefix set/

[Scott Hess - ex-Googler, 2015-07-24 23:55:18]

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

[Scott Hess - ex-Googler, 2015-07-24 23:55:18]

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_host/safe_browsing_resource_throttle.h (right):

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.h:33: // completes
synchronously (no match in the bloom filter), so the request's
On 2015/07/24 23:24:11, mattm wrote:
> since we don't actually use a bloom filter these days, might as well s/bloom
> filter/prefix set/

Or "in-memory filter", since the implementation isn't relevant here.

[Nathan Parker, 2015-07-28 17:42:12]

PTAL.  Thanks.

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
File
chrome/browser/renderer_host/data_reduction_proxy_resource_throttle_android.cc
(right):

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_host/data_reduction_proxy_resource_throttle_android.cc:50:
return SafeBrowsingResourceThrottleFactory::CreateWithoutFactory(
On 2015/07/24 23:24:11, mattm wrote:
> This is kinda confusing at first glance.. maybe have a comment like
> "DataReductionProxyResourceThrottleFactory is registered as the
> SafeBrowsingResourceThrottleFactory, so need to bypass that and use the base
> SafeBrowsingResourceThrottleFactory".
> 
> Also the naming SafeBrowsingResourceThrottleFactory::CreateWithoutFactory is
not
> really obvious what CreateWithoutFactory means when the class has Factory in
the
> name :) Maybe CreateWithoutRegisteredFactory or something?
> 
> As an alternative approach, maybe if the registered factory returns NULL, then
> the base factory could just fall back to the base behavior? Guess that could
be
> confusing since NULL could in theory be a valid result, though there isn't
> currently a need for that..

Done.

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

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:246: if
(defer_state_ == DEFERRED_UNCHECKED_REDIRECT &&
On 2015/07/24 23:24:11, mattm wrote:
> should do redirect_urls_.push_back(unchecked_redirect_url_)

Good catch.  I changed WillRedirectRequest() to always save it.

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_host/safe_browsing_resource_throttle.h (right):

https://codereview.chromium.org/1241583009/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.h:33: // completes
synchronously (no match in the bloom filter), so the request's
On 2015/07/24 23:55:18, Scott Hess wrote:
> On 2015/07/24 23:24:11, mattm wrote:
> > since we don't actually use a bloom filter these days, might as well s/bloom
> > filter/prefix set/
> 
> Or "in-memory filter", since the implementation isn't relevant here.

Done.

[mattm, 2015-07-29 19:39:58]

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

https://codereview.chromium.org/1241583009/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:83:
redirect_urls_.push_back(redirect_info.new_url);
Doing it here means that if there is an in-progress check, when
OnCheckBrowseUrlResult is called for the previous result it'll get the new_url
in the UnsafeResource, even though that redirect wasn't present for that request

[Nathan Parker, 2015-07-30 18:42:52]

PTAL.  Thanks.

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

https://codereview.chromium.org/1241583009/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:83:
redirect_urls_.push_back(redirect_info.new_url);
On 2015/07/29 19:39:58, mattm wrote:
> Doing it here means that if there is an in-progress check, when
> OnCheckBrowseUrlResult is called for the previous result it'll get the new_url
> in the UnsafeResource, even though that redirect wasn't present for that
request

I switched to do it only when the check is done.

This will report less data overall.  I had considered a scheme of letting all
the redirects go through and waiting for all checks to complete before
processing -- that way we could report the whole redirect chain even if the
first one was bad.  Would be useful for SB in some cases.  But this is simpler.

[mattm, 2015-07-30 19:50:15]

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

https://codereview.chromium.org/1241583009/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:83:
redirect_urls_.push_back(redirect_info.new_url);
On 2015/07/30 18:42:52, nparker wrote:
> On 2015/07/29 19:39:58, mattm wrote:
> > Doing it here means that if there is an in-progress check, when
> > OnCheckBrowseUrlResult is called for the previous result it'll get the
new_url
> > in the UnsafeResource, even though that redirect wasn't present for that
> request
> 
> I switched to do it only when the check is done.
> 
> This will report less data overall.  I had considered a scheme of letting all
> the redirects go through and waiting for all checks to complete before
> processing -- that way we could report the whole redirect chain even if the
> first one was bad.  Would be useful for SB in some cases.  But this is
simpler.

It should be fine to do it before the check, if it's inside the "if (state_ ==
STATE_NONE)" block

[Nathan Parker, 2015-07-30 19:58:20]

Fixed.  PTAL.

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

https://codereview.chromium.org/1241583009/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:83:
redirect_urls_.push_back(redirect_info.new_url);
On 2015/07/30 19:50:15, mattm wrote:
> On 2015/07/30 18:42:52, nparker wrote:
> > On 2015/07/29 19:39:58, mattm wrote:
> > > Doing it here means that if there is an in-progress check, when
> > > OnCheckBrowseUrlResult is called for the previous result it'll get the
> new_url
> > > in the UnsafeResource, even though that redirect wasn't present for that
> > request
> > 
> > I switched to do it only when the check is done.
> > 
> > This will report less data overall.  I had considered a scheme of letting
all
> > the redirects go through and waiting for all checks to complete before
> > processing -- that way we could report the whole redirect chain even if the
> > first one was bad.  Would be useful for SB in some cases.  But this is
> simpler.
> 
> It should be fine to do it before the check, if it's inside the "if (state_ ==
> STATE_NONE)" block

Done.

[mattm, 2015-07-30 23:37:24]

lgtm

[Lei Zhang, 2015-07-30 23:47:08]

lgtm stamp

[Nathan Parker, 2015-07-30 23:48:10]

The CQ bit was checked by nparker@chromium.org

[Nathan Parker, 2015-07-30 23:48:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/1241583009/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-30 23:48:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241583009/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1241583009/120001

[commit-bot: I haz the power, 2015-07-31 00:53:18]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-31 00:54:27]

Patchset 7 (id:??) landed as
https://crrev.com/9534829e7b24491f690032554327508d4f675de6
Cr-Commit-Position: refs/heads/master@{#341249}