Bug 918466 - Residual file left after canceling a download. r=enn

toolkit/components/jsdownloads/src/DownloadCore.jsm

@@ -1671,7 +1671,16 @@ DownloadCopySaver.prototype = {
         // background file saver may have already removed the file.
         try {
           yield OS.File.remove(targetPath);
-        } catch (e2 if e2 instanceof OS.File.Error && e2.becauseNoSuchFile) { }
+        } catch (e2) {
+          // If we failed during the operation, we report the error but use the
+          // original one as the failure reason of the download.  Note that on
+          // Windows we may get an access denied error instead of a no such file
+          // error if the file existed before, and was recently deleted.
+          if (!(e2 instanceof OS.File.Error &&
+                (e2.becauseNoSuchFile || e2.becauseAccessDenied))) {
+            Cu.reportError(e2);
+          }
+        }
         throw ex;
       }
     }.bind(this));
@@ -1947,6 +1956,23 @@ DownloadLegacySaver.prototype = {
             yield file.close();
           } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) { }
         }
+      } catch (ex) {
+        // Ensure we always remove the final target file on failure,
+        // independently of which code path failed.  In some cases, the
+        // component executing the download may have already removed the file.
+        try {
+          yield OS.File.remove(this.download.target.path);
+        } catch (e2) {
+          // If we failed during the operation, we report the error but use the
+          // original one as the failure reason of the download.  Note that on
+          // Windows we may get an access denied error instead of a no such file
+          // error if the file existed before, and was recently deleted.
+          if (!(e2 instanceof OS.File.Error &&
+                (e2.becauseNoSuchFile || e2.becauseAccessDenied))) {
+            Cu.reportError(e2);
+          }
+        }
+        throw ex;
       } finally {
         // We don't need the reference to the request anymore.
         this.request = null;

toolkit/components/jsdownloads/test/unit/common_test_Download.js

@@ -539,6 +539,28 @@ add_task(function test_cancel_midway()
   }
 });
 
+/**
+ * Cancels a download while keeping partially downloaded data, and verifies that
+ * both the target file and the ".part" file are deleted.
+ */
+add_task(function test_cancel_midway_tryToKeepPartialData()
+{
+  let download = yield promiseStartDownload_tryToKeepPartialData();
+
+  do_check_true(yield OS.File.exists(download.target.path));
+  do_check_true(yield OS.File.exists(download.target.partFilePath));
+
+  yield download.cancel();
+  yield download.removePartialData();
+
+  do_check_true(download.stopped);
+  do_check_true(download.canceled);
+  do_check_true(download.error === null);
+
+  do_check_false(yield OS.File.exists(download.target.path));
+  do_check_false(yield OS.File.exists(download.target.partFilePath));
+});
+
 /**
  * Cancels a download right after starting it.
  */
@@ -1032,6 +1054,8 @@ add_task(function test_error_source()
     do_check_true(download.error !== null);
     do_check_true(download.error.becauseSourceFailed);
     do_check_false(download.error.becauseTargetFailed);
+
+    do_check_false(yield OS.File.exists(download.target.path));
   } finally {
     serverSocket.close();
   }

toolkit/components/jsdownloads/test/unit/head.js

@@ -815,7 +815,9 @@ add_task(function test_common_initialize()
                                             aSuggestedFileExtension,
                                             aForcePrompt)
         {
+          // The dialog should create the empty placeholder file.
           let file = getTempFile(TEST_TARGET_FILE_NAME);
+          file.create(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
           aLauncher.saveDestinationAvailable(file);
         },
       }.QueryInterface(aIid);