Fix an DCHECK error if download is interrupted and restarted right after resumption

If a download is resumed and the server immediately returns an error
response that requires the download to restart, |download_file_|
is not created.
As a result, resuming the download will not clear up the current
path, and causing the DCHECK to fail.

BUG=982819

Change-Id: Ibe0832e79ca80bd2e76deba454dab2c3d8cb2be1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1706781
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678536}

components/download/internal/common/download_item_impl.cc

@@ -1984,16 +1984,14 @@ void DownloadItemImpl::InterruptWithPartialState(
       FALLTHROUGH;
 
     case IN_PROGRESS_INTERNAL:
-    case TARGET_RESOLVED_INTERNAL:
+    case TARGET_RESOLVED_INTERNAL: {
       // last_reason_ needs to be set for GetResumeMode() to work.
       last_reason_ = reason;
 
-      if (download_file_) {
-        ResumeMode resume_mode = GetResumeMode();
-        ReleaseDownloadFile(resume_mode != ResumeMode::IMMEDIATE_CONTINUE &&
-                            resume_mode != ResumeMode::USER_CONTINUE);
-      }
-      break;
+      ResumeMode resume_mode = GetResumeMode();
+      ReleaseDownloadFile(resume_mode != ResumeMode::IMMEDIATE_CONTINUE &&
+                          resume_mode != ResumeMode::USER_CONTINUE);
+    } break;
 
     case RESUMING_INTERNAL:
     case INTERRUPTED_INTERNAL:
@@ -2004,15 +2002,10 @@ void DownloadItemImpl::InterruptWithPartialState(
         return;
 
       last_reason_ = reason;
-      if (!GetFullPath().empty()) {
-        // There is no download file and this is transitioning from INTERRUPTED
-        // to CANCELLED. The intermediate file is no longer usable, and should
-        // be deleted.
-        GetDownloadTaskRunner()->PostTask(
-            FROM_HERE, base::BindOnce(base::IgnoreResult(&DeleteDownloadedFile),
-                                      GetFullPath()));
-        destination_info_.current_path.clear();
-      }
+      // There is no download file and this is transitioning from INTERRUPTED
+      // to CANCELLED. The intermediate file is no longer usable, and should
+      // be deleted.
+      DeleteDownloadFile();
       break;
   }
 
@@ -2117,15 +2110,19 @@ void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
   DVLOG(20) << __func__ << "() destroy_file:" << destroy_file;
 
   if (destroy_file) {
-    GetDownloadTaskRunner()->PostTask(
-        FROM_HERE,
-        // Will be deleted at end of task execution.
-        base::BindOnce(&DownloadFileCancel, std::move(download_file_)));
+    if (download_file_) {
+      GetDownloadTaskRunner()->PostTask(
+          FROM_HERE,
+          // Will be deleted at end of task execution.
+          base::BindOnce(&DownloadFileCancel, std::move(download_file_)));
+    } else {
+      DeleteDownloadFile();
+    }
     // Avoid attempting to reuse the intermediate file by clearing out
     // current_path_ and received slices.
     destination_info_.current_path.clear();
     received_slices_.clear();
-  } else {
+  } else if (download_file_) {
     GetDownloadTaskRunner()->PostTask(
         FROM_HERE, base::BindOnce(base::IgnoreResult(&DownloadFileDetach),
                                   // Will be deleted at end of task execution.
@@ -2137,6 +2134,15 @@ void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
   weak_ptr_factory_.InvalidateWeakPtrs();
 }
 
+void DownloadItemImpl::DeleteDownloadFile() {
+  if (GetFullPath().empty())
+    return;
+  GetDownloadTaskRunner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(base::IgnoreResult(&DeleteDownloadedFile), GetFullPath()));
+  destination_info_.current_path.clear();
+}
+
 bool DownloadItemImpl::IsDownloadReadyForCompletion(
     const base::Closure& state_change_notification) {
   // If the download hasn't progressed to the IN_PROGRESS state, then it's not

components/download/public/common/download_item_impl.h

@@ -612,6 +612,9 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadItemImpl
   // resets |current_path_|.
   void ReleaseDownloadFile(bool destroy_file);
 
+  // Deletes the download file at |current_path_|.
+  void DeleteDownloadFile();
+
   // Check if a download is ready for completion.  The callback provided
   // may be called at some point in the future if an external entity
   // state has change s.t. this routine should be checked again.

content/browser/download/download_browsertest.cc

@@ -1994,11 +1994,12 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, InvalidRangeHeader) {
 
   ASSERT_EQ(4u, requests.size());
 
-  // None of the request should have transferred the entire resource.
+  // The last request will transfer the entire resource as the interrupt
+  // reason doesn't allow download to continue.
   EXPECT_GT(parameters.size, requests[0]->transferred_byte_count);
   EXPECT_EQ(0, requests[1]->transferred_byte_count);
   EXPECT_EQ(0, requests[2]->transferred_byte_count);
-  EXPECT_GT(parameters.size, requests[3]->transferred_byte_count);
+  EXPECT_EQ(parameters.size, requests[3]->transferred_byte_count);
 }
 
 // If the server response for the resumption request cannot be decoded,