Use DownloadItem directly in DownloadProtectionService.

Use DownloadItem directly in DownloadProtectionService.

BUG=169557
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185748

[mattm, 2013-02-27 01:46:19]

There was discussion of doing this a while back on
https://codereview.chromium.org/8345033/.  I am making the change now because I
want to store the download check ping & response data in the DownloadItem (using
SetUserData), so that it will be available later to send in the download
feedback request metadata.

safe_browsing files: bryner
downloads files: asanka (probably also check download_protection_service.cc to
make sure I'm not doing anything weird in the DownloadItem usage.)

[asanka, 2013-02-27 18:06:43]

/download/ LGTM. Thanks for doing this!

https://codereview.chromium.org/12313141/diff/9005/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/12313141/diff/9005/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/download_protection_service.cc:379: Cancel();
Nit: DCHECK(item_ == NULL)

https://codereview.chromium.org/12313141/diff/9005/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/download_protection_service.cc:620:
request.set_length(item_->GetTotalBytes());
GetTotalBytes() might return <= 0 if the Content-Length wasn't known or found to
be a lie. By the time GetClientDownload() is called, we have received all the
bytes (i.e. item_->AllDataSaved() is true) so item_->GetReceivedBytes() returns
the actual size of the download.

[mattm, 2013-02-28 01:33:06]

https://codereview.chromium.org/12313141/diff/9005/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/12313141/diff/9005/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/download_protection_service.cc:379: Cancel();
On 2013/02/27 18:06:43, asanka wrote:
> Nit: DCHECK(item_ == NULL)

Done.

https://codereview.chromium.org/12313141/diff/9005/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/download_protection_service.cc:620:
request.set_length(item_->GetTotalBytes());
On 2013/02/27 18:06:43, asanka wrote:
> GetTotalBytes() might return <= 0 if the Content-Length wasn't known or found
to
> be a lie. By the time GetClientDownload() is called, we have received all the
> bytes (i.e. item_->AllDataSaved() is true) so item_->GetReceivedBytes()
returns
> the actual size of the download.

Thanks, switched to GetReceivedBytes.

[noelutz, 2013-03-01 20:12:28]

LG overall.  Just some threading questions.
noé.

https://codereview.chromium.org/12313141/diff/18001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/12313141/diff/18001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:474:
base::Bind(&CheckClientDownloadRequest::CheckWhitelists, this));
this isn't a weak reference.  Does that mean that we increment the reference
count for this?  I think this means that item_ might be NULL in CheckWhitelists,
no?

https://codereview.chromium.org/12313141/diff/18001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:496: base::TimeTicks
start_time = base::TimeTicks::Now();
Could the request get cancelled beween StartExtractSignatureFeatures() and
ExtractSignatureFeatures()?  If that's the case we should check here whether
item_ is NULL.

https://codereview.chromium.org/12313141/diff/18001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:519:
weakptr_factory_.GetWeakPtr()));
In that case it looks like we're safe.

[mattm, 2013-03-01 22:13:20]

https://codereview.chromium.org/12313141/diff/18001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/12313141/diff/18001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:474:
base::Bind(&CheckClientDownloadRequest::CheckWhitelists, this));
On 2013/03/01 20:12:28, noelutz wrote:
> this isn't a weak reference.  Does that mean that we increment the reference
> count for this?  I think this means that item_ might be NULL in
CheckWhitelists,
> no?

Oh crap.  Good catch, I totally forgot the threading in this class.

Actually DownloadItem is not thread safe, so I made a bunch of changes to avoid
accessing it on other threads at all.

https://codereview.chromium.org/12313141/diff/18001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:496: base::TimeTicks
start_time = base::TimeTicks::Now();
On 2013/03/01 20:12:28, noelutz wrote:
> Could the request get cancelled beween StartExtractSignatureFeatures() and
> ExtractSignatureFeatures()?  If that's the case we should check here whether
> item_ is NULL.

We can't just check item_ unless we also add locking, but I made it not depend
on item_ for the reasons above anyway.

https://codereview.chromium.org/12313141/diff/18001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:519:
weakptr_factory_.GetWeakPtr()));
On 2013/03/01 20:12:28, noelutz wrote:
> In that case it looks like we're safe.

Probably, yeah.  I added a test in OnZipAnalysisFinished just to be safe.

[noelutz, 2013-03-01 22:24:09]

lgtm

Nice cleanup!
noé.

https://codereview.chromium.org/12313141/diff/18001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/12313141/diff/18001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:613:
RecordImprovedProtectionStats(REASON_REQUEST_CANCELED);
nice catch.

[commit-bot: I haz the power, 2013-03-01 22:31:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/12313141/20017

[commit-bot: I haz the power, 2013-03-02 16:06:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/12313141/20017

[commit-bot: I haz the power, 2013-03-02 18:51:14]

Change committed as 185748