- Flip the flag for improved SafeBrowsing downoad protection.

- Flip the flag for improved SafeBrowsing downoad protection.
- Send the binary sha256 hash.
- Cancel requests that timed out.

BUG=102540
TEST=Run DownloadProtectionServiceTest. Also, if you have SafeBrowsing enabled you should see a server request for every binary download that doesn't match our whitelist.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110649

[Brian Ryner, 2011-11-17 02:07:03]

http://codereview.chromium.org/8586011/diff/4001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8586011/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:583: return;
I don't think this will remove the request from download_requests_ in the
timeout case, since RequestFinished() will never be called on the service.

[noelutz, 2011-11-17 02:33:26]

Please take another look.
noe.

[Brian Ryner, 2011-11-17 04:34:27]

lgtm

[mattm, 2011-11-17 04:56:33]

Noticed some leaked mock messages at the end of the unit_tests logs:
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/3292/ste...

Are those new?

otherwise lgtm

http://codereview.chromium.org/8586011/diff/4003/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

http://codereview.chromium.org/8586011/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:659:
download_service_->download_request_timeout_ms_ = 100;
Does this time matter?  could it be shorter even?

[noelutz, 2011-11-17 06:14:41]

On 2011/11/17 04:56:33, mattm wrote:
> Noticed some leaked mock messages at the end of the unit_tests logs:
>
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/3292/ste...
> 
> Are those new?

I think those are new. Those are request objects that are kept alive in the
message loop by pending cancel callbacks.  I changed the cancel callback to hold
a weak pointer instead.

Please take another look.
thanks,
noé.

[noelutz, 2011-11-17 06:15:03]

http://codereview.chromium.org/8586011/diff/4003/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

http://codereview.chromium.org/8586011/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:659:
download_service_->download_request_timeout_ms_ = 100;
On 2011/11/17 04:56:33, mattm wrote:
> Does this time matter?  could it be shorter even?

Done.

[noelutz, 2011-11-17 19:00:02]

All the ownership issues should be fixed.  Please take another look.
Thanks,
noé.

[Brian Ryner, 2011-11-17 20:25:30]

lgtm

Just a couple of things you can fix before you submit.

http://codereview.chromium.org/8586011/diff/12/chrome/browser/safe_browsing/d...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8586011/diff/12/chrome/browser/safe_browsing/d...
chrome/browser/safe_browsing/download_protection_service.cc:342:
ALLOW_THIS_IN_INITIALIZER_LIST(timeout_weakptr_factory_(this)) {
#include "base/compiler_specific.h" for this macro.

http://codereview.chromium.org/8586011/diff/12/chrome/browser/safe_browsing/d...
chrome/browser/safe_browsing/download_protection_service.cc:409: // the UI
thread that will call FinishRequest() and run the callback.
nit: this comment is now a little out of date.  Since we've just called
FinishRequest, the callback has already been run.  If FinishRequest is called
again later, it will be a no-op.

[noelutz, 2011-11-17 21:28:40]

http://codereview.chromium.org/8586011/diff/12/chrome/browser/safe_browsing/d...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8586011/diff/12/chrome/browser/safe_browsing/d...
chrome/browser/safe_browsing/download_protection_service.cc:409: // the UI
thread that will call FinishRequest() and run the callback.
On 2011/11/17 20:25:30, Brian Ryner wrote:
> nit: this comment is now a little out of date.  Since we've just called
> FinishRequest, the callback has already been run.  If FinishRequest is called
> again later, it will be a no-op.

Done.

[mattm, 2011-11-17 21:43:34]

lgtm

http://codereview.chromium.org/8586011/diff/12/chrome/browser/safe_browsing/d...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8586011/diff/12/chrome/browser/safe_browsing/d...
chrome/browser/safe_browsing/download_protection_service.cc:450:
timeout_weakptr_factory_.InvalidateWeakPtrs();
the WeakPtrFactory destructor should do this already

[noelutz, 2011-11-17 21:46:06]

thanks,
noé.

http://codereview.chromium.org/8586011/diff/12/chrome/browser/safe_browsing/d...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8586011/diff/12/chrome/browser/safe_browsing/d...
chrome/browser/safe_browsing/download_protection_service.cc:450:
timeout_weakptr_factory_.InvalidateWeakPtrs();
On 2011/11/17 21:43:34, mattm wrote:
> the WeakPtrFactory destructor should do this already

Done.

[commit-bot: I haz the power, 2011-11-18 01:37:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelutz@google.com/8586011/6030

[commit-bot: I haz the power, 2011-11-18 04:06:40]

Change committed as 110649