[Resumption 8/11] Remove intermediate files when no longer necessary.

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 37ddf76e915e905d508f8cd2551007f534cc5ea7..2a2986e88bce8288a3932261c5629389d1f5fc4e 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -369,6 +369,13 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
     request_handle_->CancelRequest();
   }
 
+  if ((state_ == INTERRUPTED_INTERNAL || state_ == RESUMING_INTERNAL) &&
+      !current_path_.empty() && current_path_ != target_path_) {
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                            base::Bind(&DeleteDownloadedFile, current_path_));
+    current_path_.clear();
+  }
+
   TransitionTo(CANCELLED_INTERNAL);
 }
 
@@ -409,7 +416,13 @@ void DownloadItemImpl::Remove() {
   // Remove the intermediate file if we are removing an interrupted download.
   // Continuable interruptions leave the intermediate file around. However, the
   // intermediate file will be unusable if the download item is removed.
-  if (!current_path_.empty() && IsInterrupted() && !download_file_.get()) {
+  //
+  // Be extra cautious here and only delete an intermediate file if we sure it's
+  // really an intermediate file. Downloads imported from history may have a
+  // non-empty current_path_ that's the same as target_path_.

[Randy Smith (Not in Mondays), 2013/05/15 15:34:56]

I'm slightly confused.  If the download was important from the history and is in
the interrupted state, shouldn't we remove the current_path_?  What's the
process by which we get into a state s.t. this extra code is needed?

Also, below in this file we're going to cancel the download; why doesn't the
code in Cancel for cancelling interrupted downloads take care of this case?

[asanka, 2013/05/21 21:09:02]

https://codereview.chromium.org/11363222 introduced persisting the target and
current paths. Since previously we only persisted full_path, the migration
script set both target and current path to full_path.

A download that was imported from history might look like the following:
  { current_path: 'foo.crdownload',
    target_path: 'foo.crdownload',
    state: INTERRUPTED }

The problem is that that prior to downloads resumption, the intermediate file
would be deleted on interrupt with the exception of interruptions due to browser
crashes. So if 'foo.crdownload' exists, then its likely that it's associated
with another download. Removing it is probably the wrong thing to do.

Then again, a more recent download might look like:
  { current_path: 'foo.crdownload',
    target_path: 'foo',
    state: INTERRUPTED }

This download could suffer from the same problem (i.e. 'foo.crdownload' if it
exists, may be associated with a different download).

I've removed the check for now and we aggressively delete the intermediate path
on Cancel()/Remove().

Also, you're right that we can rely on the deletion logic in Cancel() here.

[Randy Smith (Not in Mondays), 2013/05/21 21:40:37]

I hear you about the entries from before we started persisting current path. 
I'm ok with specifically checking for that, though I wince to have code that's
put in to handle old database entries--we should figure out a way to update the
database entry once so that the problem doesn't exist and remove the code. 
OTOH, I'm also good with just deleting the file and assuming that no user will
have that problem very often :-}.

With regard to a more recent download, I'm afraid that we sorta have to assume
that if there's an interrupted download with a non-empty() current_path in the
DB, that that path belongs to that download.  It might not, yes, but I can't
come up with a consistent way of thinking about it that allows for that and
allows for resuming downloads across history persistence.  Can you?

As noted above, I'm good with just getting rid of the check.

[asanka, 2013/05/22 22:55:52]

Yeah. I got rid of the check.

+  if (IsInterrupted() && !current_path_.empty() && !target_path_.empty() &&
+      current_path_ != target_path_) {
+    DCHECK(!download_file_.get());
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                             base::Bind(&DeleteDownloadedFile, current_path_));
     current_path_.clear();