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

content/browser/download/download_item_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // File method ordering: Methods in this file are in the same order as
 // in download_item_impl.h, with the following exception: The public
 // interface Start is placed in chronological order with the other
 // (private) routines that together define a DownloadItem's state
 // transitions as the download progresses.  See "Download progression
 // cascade" later in this file.
@@ skipping 351 matching lines @@
   // download file has been released.
   if (!is_save_package_download_ && download_file_)
     ReleaseDownloadFile(true);
 
   if (state_ == IN_PROGRESS_INTERNAL) {
     // Cancel the originating URL request unless it's already been cancelled
     // by interrupt.
     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);
 }
 
 void DownloadItemImpl::Delete(DeleteReason reason) {
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   switch (reason) {
     case DELETE_DUE_TO_USER_DISCARD:
       UMA_HISTOGRAM_ENUMERATION(
@@ skipping 20 matching lines @@
   // We have now been deleted.
 }
 
 void DownloadItemImpl::Remove() {
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // 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();
   }
 
   delegate_->AssertStateConsistent(this);
   Cancel(true);
   delegate_->AssertStateConsistent(this);
 
   NotifyRemoved();
@@ skipping 1301 matching lines @@
     case RESUME_MODE_USER_CONTINUE:
       return "USER_CONTINUE";
     case RESUME_MODE_USER_RESTART:
       return "USER_RESTART";
   }
   NOTREACHED() << "Unknown resume mode " << mode;
   return "unknown";
 }
 
 }  // namespace content