Bug 918466 - Residual file left after canceling a download. r=enn, a=lsblakk
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Thu, 10 Oct 2013 16:42:03 +0200
changeset 160679 a73d0546a814c2d653f446e43e84467ec02e50b6
parent 160678 8003468490368711f72c82ed5e6a052d4a12ebbd
child 160680 7b483e309add857a6ba3caa6a404a4716ce76046
push id2961
push userlsblakk@mozilla.com
push dateMon, 28 Oct 2013 21:59:28 +0000
treeherdermozilla-beta@73ef4f13486f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersenn, lsblakk
bugs918466
milestone26.0a2
Bug 918466 - Residual file left after canceling a download. r=enn, a=lsblakk
toolkit/components/jsdownloads/src/DownloadCore.jsm
toolkit/components/jsdownloads/test/unit/common_test_Download.js
toolkit/components/jsdownloads/test/unit/head.js
--- a/toolkit/components/jsdownloads/src/DownloadCore.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ -1606,17 +1606,26 @@ DownloadCopySaver.prototype = {
         // up the chain of objects for the download.
         yield deferSaveComplete.promise;
       } catch (ex) {
         // Ensure we always remove the placeholder for the final target file on
         // failure, independently of which code path failed.  In some cases, the
         // 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));
   },
 
   /**
    * Implements "DownloadSaver.cancel".
    */
@@ -1877,16 +1886,33 @@ DownloadLegacySaver.prototype = {
           // empty file is created as expected.
           try {
             // This atomic operation is more efficient than an existence check.
             let file = yield OS.File.open(this.download.target.path,
                                           { create: true });
             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;
         // Allow the download to restart through a DownloadCopySaver.
         this.firstExecutionFinished = true;
       }
     }.bind(this));
   },
--- a/toolkit/components/jsdownloads/test/unit/common_test_Download.js
+++ b/toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ -535,16 +535,38 @@ add_task(function test_cancel_midway()
     } catch (ex if ex instanceof Downloads.Error) {
       do_check_false(ex.becauseSourceFailed);
       do_check_false(ex.becauseTargetFailed);
     }
   }
 });
 
 /**
+ * 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.
  */
 add_task(function test_cancel_immediately()
 {
   mustInterruptResponses();
 
   let download = yield promiseStartDownload(httpUrl("interruptible.txt"));
 
@@ -1027,16 +1049,18 @@ add_task(function test_error_source()
     }
 
     // Check the properties now that the download stopped.
     do_check_true(download.stopped);
     do_check_false(download.canceled);
     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();
   }
 });
 
 /**
  * Ensures download error details are reported on local writing failures.
  */
--- a/toolkit/components/jsdownloads/test/unit/head.js
+++ b/toolkit/components/jsdownloads/test/unit/head.js
@@ -803,17 +803,19 @@ add_task(function test_common_initialize
                              "Synchronous promptForSaveToFile not implemented.",
                              Cr.NS_ERROR_NOT_AVAILABLE);
         },
         promptForSaveToFileAsync: function (aLauncher, aWindowContext,
                                             aDefaultFileName,
                                             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);
     }
   };
 
   let contractID = "@mozilla.org/helperapplauncherdialog;1";
   let cid = registrar.contractIDToCID(contractID);