Handle the case where IAttachmentExecute::Save() deletes a downloaded file.

Handle the case where IAttachmentExecute::Save() deletes a downloaded file.

As a first step, mark the download as interrupted due to the file being blocked or due to the file being infected by a virus. The interrupt reason will be shown to the user in the downloads shelf and the downloads page.

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

[Randy Smith (Not in Mondays), 2012-10-16 18:19:19]

As long as we're in this code, can we put in some UMA so we can figure more
precisely how often this type of thing happens?

http://codereview.chromium.org/11150027/diff/1/content/browser/download/base_...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/11150027/diff/1/content/browser/download/base_...
content/browser/download/base_file.cc:214: // this happens.
Part of me would like to find *some* different error code and status text to map
this too, just so that we can tell when they occur in the wild without a
net-internals dump.  Maybe "AV Comm Error" / "An error occurred invoking the
anti-virus program"?

http://codereview.chromium.org/11150027/diff/1/content/browser/download/base_...
content/browser/download/base_file.cc:515: net_error = net::ERR_FAILED;
Do we want some better way to call out the case in which the file disappeared
but the error code didn't show a problem?  UMA?

http://codereview.chromium.org/11150027/diff/1/content/browser/download/downl...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/11150027/diff/1/content/browser/download/downl...
content/browser/download/download_item_impl.h:219: // Between commit point
(dispatch of download file release) and
Could you add a note here about how the Download is blocked from being cancelled
in this state, but may still be interrupted if the operations break?  (I'm
pretty sure this is a reasonable choice, but another choice to keep
cancelableness and interruptibleness in sync would be to separate out annotation
(with error return) from detach, and not transition to COMPLETING_INTERNAL until
detach.

[Randy Smith (Not in Mondays), 2012-10-16 18:19:50]

You should probably also get a different reviewer for the windows specific
stuff.

[asanka, 2012-10-25 23:51:27]

sky: content/browser/safe_util_win.{h,cc}
     The changes are used by content/browser/download/base_file_win.cc

The changes were necessary because IAttachmentExecute::Save() can, in some
cases, delete the downloaded file. It's supposed to only happen if the file is
blocked by security policy or if the file is infected with a virus. However as
http://crbug.com/153212#c66 shows, it can happen for other reasons as well. We
would like to log the return code and surface a useful error to the user when
this happens.

The associated UI strings are pending approval.

http://codereview.chromium.org/11150027/diff/1/content/browser/download/base_...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/11150027/diff/1/content/browser/download/base_...
content/browser/download/base_file.cc:214: // this happens.
On 2012/10/16 18:19:19, rdsmith wrote:
> Part of me would like to find *some* different error code and status text to
map
> this too, just so that we can tell when they occur in the wild without a
> net-internals dump.  Maybe "AV Comm Error" / "An error occurred invoking the
> anti-virus program"?

Added a new interrupt reason. I named it 'SECURITY_CHECK_FAILED'. Partly because
things other than anti-virus can be at fault.

http://codereview.chromium.org/11150027/diff/1/content/browser/download/base_...
content/browser/download/base_file.cc:515: net_error = net::ERR_FAILED;
On 2012/10/16 18:19:19, rdsmith wrote:
> Do we want some better way to call out the case in which the file disappeared
> but the error code didn't show a problem?  UMA?

Add a UMA count for this case.

http://codereview.chromium.org/11150027/diff/1/content/browser/download/downl...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/11150027/diff/1/content/browser/download/downl...
content/browser/download/download_item_impl.h:219: // Between commit point
(dispatch of download file release) and
On 2012/10/16 18:19:19, rdsmith wrote:
> Could you add a note here about how the Download is blocked from being
cancelled
> in this state, but may still be interrupted if the operations break?  (I'm
> pretty sure this is a reasonable choice, but another choice to keep
> cancelableness and interruptibleness in sync would be to separate out
annotation
> (with error return) from detach, and not transition to COMPLETING_INTERNAL
until
> detach.

Done.

[asanka, 2012-10-26 19:32:13]

On 2012/10/16 18:19:19, rdsmith wrote:
> As long as we're in this code, can we put in some UMA so we can figure more
> precisely how often this type of thing happens?

The new DownloadInterruptReason values along with the
'Download.InterruptedReason' UMA should tell us how often we hit these cases.

[sky, 2012-10-26 20:38:49]

I only looked at content/browser LGTM

[Randy Smith (Not in Mondays), 2012-10-27 23:54:28]

LGTM with TODO request below, safe_util_win.h comment up to you.

https://codereview.chromium.org/11150027/diff/19008/content/browser/download/...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/11150027/diff/19008/content/browser/download/...
content/browser/download/download_item_impl.h:289: void
OnDownloadFileReleased(content::DownloadInterruptReason reason);
After having banged my head against the state machine wall as part of the
download destination refactor, I'm feeling more uncomfortable about this type of
change (which changes the commit point, and allows state transitions to
INTERRUPTED out of the COMPLETING state).  My current perspective is that the
annotation should conceptually be part of the final rename, and release just do
a detach, and I'll be rewriting it that way as part of the DD refactor, but
could you put a TODO here to eliminate the interrupt reason callback and move
the annotation into the final rename?

https://codereview.chromium.org/11150027/diff/19008/content/browser/safe_util...
File content/browser/safe_util_win.h (right):

https://codereview.chromium.org/11150027/diff/19008/content/browser/safe_util...
content/browser/safe_util_win.h:52: // annotated.
Does this make sense?  If attachment execution services are not available on all
platforms, then wouldn't it make sense for this not to be an error on those
platforms?

(I realize that this is a theoretical point, given that the error is ignored at
the calling site if the file is still around.)

[commit-bot: I haz the power, 2012-10-29 06:34:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/11150027/45001

[commit-bot: I haz the power, 2012-10-29 06:43:20]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-29 12:02:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/11150027/45001

[commit-bot: I haz the power, 2012-10-29 13:24:08]

Change committed as 164632

[asanka, 2012-10-29 15:33:59]

http://codereview.chromium.org/11150027/diff/19008/content/browser/download/d...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/11150027/diff/19008/content/browser/download/d...
content/browser/download/download_item_impl.h:289: void
OnDownloadFileReleased(content::DownloadInterruptReason reason);
On 2012/10/27 23:54:28, rdsmith wrote:
> After having banged my head against the state machine wall as part of the
> download destination refactor, I'm feeling more uncomfortable about this type
of
> change (which changes the commit point, and allows state transitions to
> INTERRUPTED out of the COMPLETING state).  My current perspective is that the
> annotation should conceptually be part of the final rename, and release just
do
> a detach, and I'll be rewriting it that way as part of the DD refactor, but
> could you put a TODO here to eliminate the interrupt reason callback and move
> the annotation into the final rename?

Done.

http://codereview.chromium.org/11150027/diff/19008/content/browser/safe_util_...
File content/browser/safe_util_win.h (right):

http://codereview.chromium.org/11150027/diff/19008/content/browser/safe_util_...
content/browser/safe_util_win.h:52: // annotated.
On 2012/10/27 23:54:28, rdsmith wrote:
> Does this make sense?  If attachment execution services are not available on
all
> platforms, then wouldn't it make sense for this not to be an error on those
> platforms?
> 
> (I realize that this is a theoretical point, given that the error is ignored
at
> the calling site if the file is still around.)

BaseFile will still log the error, and it can be helpful for debugging in cases
where we are unable to instantiate the CLSID_AttachmentServices class.

I don't think it makes sense to add special handling for versions of Windows
where AES isn't available since it's been there since Windows XP SP2.