This CL integrates the new SafeBrowsing download service class

This CL integrates the new SafeBrowsing download service class
with the download manager.  This change should have no effect on
when the user sees a warning or not.

BUG=102540
TEST=No visible change yet.


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

[Randy Smith (Not in Mondays), 2011-11-14 19:42:59]

Asanka, could you also take a look at this?  I'm figuring since the UI will
eventually need to be integrated, you're a good first pass.  I'll take a quick
look through when you're done.

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:89: #if
defined(ENABLE_SAFE_BROWSING)
Remind me what the context around this define is?  Under what circumstances do
we want to compile out safe browsing?  (Just asking for context).

[noelutz, 2011-11-14 20:30:42]

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:89: #if
defined(ENABLE_SAFE_BROWSING)
On 2011/11/14 19:43:00, rdsmith wrote:
> Remind me what the context around this define is?  Under what circumstances do
> we want to compile out safe browsing?  (Just asking for context).

SafeBrowsing is currently disabled (and compiled out) for ChromeOs.  There might
also be other targets that do not compile SafeBrowsing but ChromeOs is the main
one.

[asanka, 2011-11-14 21:51:08]

Thanks Randy.  The chrome/browser/download/* changes LG.

I don't have an objection to the assumption that the download will complete
without calling  DownloadManagerDelegate::ShouldCompleteDownload() again if that
function returns true.  This is a stronger claim that what the comment for DMD
currently implies.

I'm assuming that the plan is to expose the danger state to the DownloadItem. 
The UI can then observe the danger state change, interact with the user and
eventually unblock the download by validating the dangerous state.

Shouldn't the call to CheckClientDownload() be conditioned on whether or not the
download item has already been marked as dangerous (due to a prior
CheckDownloadUrl())?

[noelutz, 2011-11-14 22:01:08]

Thanks Asanka,

On Mon, Nov 14, 2011 at 1:51 PM,  <asanka@chromium.org> wrote:
> Thanks Randy.  The chrome/browser/download/* changes LG.
>
> I don't have an objection to the assumption that the download will complete
> without calling  DownloadManagerDelegate::ShouldCompleteDownload() again if
> that
> function returns true.  This is a stronger claim that what the comment for
> DMD
> currently implies.
>
> I'm assuming that the plan is to expose the danger state to the
> DownloadItem.
> The UI can then observe the danger state change, interact with the user and
> eventually unblock the download by validating the dangerous state.

Exactly.  I was going to add a third danger type to track the verdict
in the download item.  E.g.,  DANGEROUS_CONTENT.

> Shouldn't the call to CheckClientDownload() be conditioned on whether or not
> the
> download item has already been marked as dangerous (due to a prior
> CheckDownloadUrl())?

Yes. I think there is a TODO for that in the code.  Right now it
doesn't matter because we're not showing any warning based on the
second check.  I will send out a separate change either today or
tomorrow for this.  Note: we also have to disable the dangerous file
type check & warning for file types that are handled by this new
download protection.

Thanks,
noe.

>
>
> http://codereview.chromium.org/8536041/
>



-- 
-|||--

[Brian Ryner, 2011-11-14 22:10:39]

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

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:117: std::string
chain = "";
Not necessary to initialize strings to empty.

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:125: "DownloadInfo
{download_url_chain:[%s], local_file:%s, "
It might be useful to include the address of the object (this) as well.

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:141:
download_info.local_file = item.GetTargetFilePath();
Can you explain what this change is for?

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:294: // a match with
the digest list.  However, for UMA users, we want to
I'm not sure I follow the comment -- what part of the code ensures that we are
reporting the malware URL for UMA users?

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:355: if
(final_url.SchemeIs("https") || !IsBinaryFile(info_.local_file)) {
So this might cause us to start checking non-HTTP urls that are not HTTPS, like
FTP.  I assume that's ok though?

[Randy Smith (Not in Mondays), 2011-11-14 22:24:22]

LGTM (presuming Asanka's ok with your answer to his question).

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:325: if (!item)
There's a (minor) leak here if the DownloadItem's gone away; you haven't cleaned
up the state in the map.  That'll fix itself when we switch to the new storage
model, but you might want to fix it here as well.

[noelutz, 2011-11-14 22:26:35]

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:325: if (!item)
On 2011/11/14 22:24:22, rdsmith wrote:
> There's a (minor) leak here if the DownloadItem's gone away; you haven't
cleaned
> up the state in the map.  That'll fix itself when we switch to the new storage
> model, but you might want to fix it here as well.

I think we're OK. There aren't any pointers in the map.  Everything should be
deleted by the destructor as far as I know.

[Randy Smith (Not in Mondays), 2011-11-14 22:29:01]

On 2011/11/14 22:26:35, noelutz wrote:
>
http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
> chrome/browser/download/chrome_download_manager_delegate.cc:325: if (!item)
> On 2011/11/14 22:24:22, rdsmith wrote:
> > There's a (minor) leak here if the DownloadItem's gone away; you haven't
> cleaned
> > up the state in the map.  That'll fix itself when we switch to the new
storage
> > model, but you might want to fix it here as well.
> 
> I think we're OK. There aren't any pointers in the map.  Everything should be
> deleted by the destructor as far as I know.

So you have my LGTM, but I think the fact that the map'll be deleted by the
destructor is a red herring.  The leaks we really care about are the ones that
take up memory while chrome is running; they effect the user's experience. 
Shutdown leaks are annoying just because they make it hard to find the other
type.  

(The fact that this really isn't much memory is less of a red herring :-}).

[asanka, 2011-11-14 22:44:20]

Sounds good.  LGTM.

On 2011/11/14 22:01:08, noelutz wrote:
> Thanks Asanka,
> 
> On Mon, Nov 14, 2011 at 1:51 PM,  <mailto:asanka@chromium.org> wrote:
> > Thanks Randy. &nbsp;The chrome/browser/download/* changes LG.
> >
> > I don't have an objection to the assumption that the download will complete
> > without calling &nbsp;DownloadManagerDelegate::ShouldCompleteDownload()
again if
> > that
> > function returns true. &nbsp;This is a stronger claim that what the comment
for
> > DMD
> > currently implies.
> >
> > I'm assuming that the plan is to expose the danger state to the
> > DownloadItem.
> > The UI can then observe the danger state change, interact with the user and
> > eventually unblock the download by validating the dangerous state.
> 
> Exactly.  I was going to add a third danger type to track the verdict
> in the download item.  E.g.,  DANGEROUS_CONTENT.
> 
> > Shouldn't the call to CheckClientDownload() be conditioned on whether or not
> > the
> > download item has already been marked as dangerous (due to a prior
> > CheckDownloadUrl())?
> 
> Yes. I think there is a TODO for that in the code.  Right now it
> doesn't matter because we're not showing any warning based on the
> second check.  I will send out a separate change either today or
> tomorrow for this.  Note: we also have to disable the dangerous file
> type check & warning for file types that are handled by this new
> download protection.
> 
> Thanks,
> noe.
> 
> >
> >
> > http://codereview.chromium.org/8536041/
> >
> 
> 
> 
> -- 
> -|||--

[noelutz, 2011-11-15 00:14:10]

Thanks for your comments.  Please take another look.

Thanks,
noe.

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:325: if (!item)
On 2011/11/14 22:26:35, noelutz wrote:
> On 2011/11/14 22:24:22, rdsmith wrote:
> > There's a (minor) leak here if the DownloadItem's gone away; you haven't
> cleaned
> > up the state in the map.  That'll fix itself when we switch to the new
storage
> > model, but you might want to fix it here as well.
> 
> I think we're OK. There aren't any pointers in the map.  Everything should be
> deleted by the destructor as far as I know.

Fixed.

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

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:117: std::string
chain = "";
On 2011/11/14 22:10:39, Brian Ryner wrote:
> Not necessary to initialize strings to empty.

Done.

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:125: "DownloadInfo
{download_url_chain:[%s], local_file:%s, "
On 2011/11/14 22:10:39, Brian Ryner wrote:
> It might be useful to include the address of the object (this) as well.

Done.

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:141:
download_info.local_file = item.GetTargetFilePath();
On 2011/11/14 22:10:39, Brian Ryner wrote:
> Can you explain what this change is for?

Woops.  I don't think this will work.  GetTargetFilePath() returns the file path
where the file will eventually written. full_path() is where there file is
written right now.  For exe those are different paths.

Changed since we need both.

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:294: // a match with
the digest list.  However, for UMA users, we want to
On 2011/11/14 22:10:39, Brian Ryner wrote:
> I'm not sure I follow the comment -- what part of the code ensures that we are
> reporting the malware URL for UMA users?

Updated the comment.  See CheckDone() above too.

http://codereview.chromium.org/8536041/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:355: if
(final_url.SchemeIs("https") || !IsBinaryFile(info_.local_file)) {
On 2011/11/14 22:10:39, Brian Ryner wrote:
> So this might cause us to start checking non-HTTP urls that are not HTTPS,
like
> FTP.  I assume that's ok though?

I think that's what we want.

[Brian Ryner, 2011-11-15 00:23:38]

lgtm

[commit-bot: I haz the power, 2011-11-15 01:45:46]

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

[commit-bot: I haz the power, 2011-11-15 02:15:12]

Try job failure for 8536041-8004 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-15 03:42:33]

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

[commit-bot: I haz the power, 2011-11-15 04:59:39]

Change committed as 110044

[jam, 2012-01-27 23:06:33]

lgtm

http://codereview.chromium.org/8536041/diff/11003/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8536041/diff/11003/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:222: //
TODO(noelutz): remove this method from the delegate API.
ping: can you send me a change to remove this? the TODO has been in the code for
2 months

[jam, 2012-01-27 23:06:35]

lgtm

[jam, 2012-01-27 23:06:51]

(pressed lgtm button by mistake, I just meant to publish)

[jam, 2012-01-27 23:41:33]

On 2012/01/27 23:06:51, John Abd-El-Malek wrote:
> (pressed lgtm button by mistake, I just meant to publish)

ok i'm in this code now so i'll remove it in the meantime. in the future, please
do small cleanups like that in the change, otherwise they never get done.