Bug 1139913 - Do not delete placeholders when keeping partial downloads. r?paolo draft
authorJohann Hofmann <jhofmann@mozilla.com>
Thu, 28 Jul 2016 08:48:44 +0200
changeset 393628 f1f0b4d503b040e1122e8d046e4e8d37737b61d9
parent 393192 78dd94ba93c77d0bba45f8e4525947629a305a41
child 526631 de838ff7dd6df37b6e5e8f510cbcb1d198a32521
push id24369
push usermail@johann-hofmann.com
push dateThu, 28 Jul 2016 06:54:19 +0000
reviewerspaolo
bugs1139913
milestone50.0a1
Bug 1139913 - Do not delete placeholders when keeping partial downloads. r?paolo MozReview-Commit-ID: 4rFZ5rP8saJ
toolkit/components/jsdownloads/src/DownloadCore.jsm
toolkit/components/jsdownloads/test/unit/common_test_Download.js
--- a/toolkit/components/jsdownloads/src/DownloadCore.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ -486,26 +486,23 @@ this.Download.prototype = {
         // Now that the actual saving finished, read the actual file size on
         // disk, that may be different from the amount of data transferred.
         yield this.target.refresh();
 
         // Check for the last time if the download has been canceled. This must
         // be done right before setting the "stopped" property of the download,
         // without any asynchronous operations in the middle, so that another
         // cancellation request cannot start in the meantime and stay unhandled.
-        if (this._promiseCanceled) {
+        if (this._promiseCanceled && !this.tryToKeepPartialData) {
           try {
-            yield OS.File.remove(this.target.path);
+            yield this.saver.removePartialData();
           } catch (ex) {
             Cu.reportError(ex);
           }
 
-          this.target.exists = false;
-          this.target.size = 0;
-
           // Cancellation exceptions will be changed in the catch block below.
           throw new DownloadError();
         }
 
         // Update the status properties for a successful download.
         this.progress = 100;
         this.succeeded = true;
         this.hasPartialData = false;
@@ -674,17 +671,17 @@ this.Download.prototype = {
 
     if (!this.hasBlockedData) {
       return Promise.reject(new Error(
         "confirmBlock may only be called on Downloads with blocked data."));
     }
 
     this._promiseConfirmBlock = Task.spawn(function* () {
       try {
-        yield OS.File.remove(this.target.partFilePath);
+        yield this.saver.removePartialData();
       } catch (ex) {
         yield this.refresh();
         this._promiseConfirmBlock = null;
         throw ex;
       }
 
       this.hasBlockedData = false;
       this._notifyChange();
@@ -2138,24 +2135,25 @@ this.DownloadCopySaver.prototype = {
         yield deferSaveComplete.promise;
 
         yield this._checkReputationAndMove(aSetPropertiesFn);
       } 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);
+          if (!this.download.tryToKeepPartialData || this.download.currentBytes <= 0) {
+            yield this.removePartialData();
+          }
         } 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))) {
+          if (!(e2 instanceof OS.File.Error && e2.becauseAccessDenied)) {
             Cu.reportError(e2);
           }
         }
         throw ex;
       }
     }.bind(this));
   },
 
@@ -2183,17 +2181,17 @@ this.DownloadCopySaver.prototype = {
       let newProperties = { progress: 100, hasPartialData: false };
 
       // We will remove the potentially dangerous file if instructed by
       // DownloadIntegration. We will always remove the file when the
       // download did not use a partial file path, meaning it
       // currently has its final filename.
       if (!DownloadIntegration.shouldKeepBlockedData() || !partFilePath) {
         try {
-          yield OS.File.remove(partFilePath || targetPath);
+          yield this.removePartialData();
         } catch (ex) {
           Cu.reportError(ex);
         }
       } else {
         newProperties.hasBlockedData = true;
       }
 
       aSetPropertiesFn(newProperties);
@@ -2222,25 +2220,30 @@ this.DownloadCopySaver.prototype = {
   },
 
   /**
    * Implements "DownloadSaver.removePartialData".
    */
   removePartialData: function ()
   {
     return Task.spawn(function* task_DCS_removePartialData() {
-      if (this.download.target.partFilePath) {
         try {
-          yield OS.File.remove(this.download.target.partFilePath);
+          if (this.download.target.path) {
+            yield OS.File.remove(this.download.target.path);
+            this.download.target.exists = false;
+            this.download.target.size = 0;
+          }
+          if (this.download.target.partFilePath) {
+            yield OS.File.remove(this.download.target.partFilePath);
+          }
         } catch (ex) {
           if (!(ex instanceof OS.File.Error) || !ex.becauseNoSuchFile) {
             throw ex;
           }
         }
-      }
     }.bind(this));
   },
 
   /**
    * Implements "DownloadSaver.toSerializable".
    */
   toSerializable: function ()
   {
@@ -2539,24 +2542,25 @@ this.DownloadLegacySaver.prototype = {
 
         yield this._checkReputationAndMove(aSetPropertiesFn);
 
       } 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);
+          if (!this.download.tryToKeepPartialData || this.download.currentBytes <= 0) {
+            yield this.removePartialData();
+          }
         } 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))) {
+          if (!(e2 instanceof OS.File.Error && e2.becauseAccessDenied)) {
             Cu.reportError(e2);
           }
         }
         // In case the operation failed, ensure we stop downloading data.  Since
         // we never re-enter this function, deferCanceled is always available.
         this.deferCanceled.resolve();
         throw ex;
       } finally {
--- a/toolkit/components/jsdownloads/test/unit/common_test_Download.js
+++ b/toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ -659,16 +659,17 @@ add_task(function* test_cancel_midway()
 
   do_check_true(download.stopped);
   do_check_true(download.canceled);
   do_check_true(download.error === null);
   do_check_false(download.target.exists);
   do_check_eq(download.target.size, 0);
 
   do_check_false(yield OS.File.exists(download.target.path));
+  do_check_false(yield OS.File.exists(download.target.partFilePath));
 
   // Progress properties are not reset by canceling.
   do_check_eq(download.progress, 50);
   do_check_eq(download.totalBytes, TEST_DATA_SHORT.length * 2);
   do_check_eq(download.currentBytes, TEST_DATA_SHORT.length);
 
   if (!gUseLegacySaver) {
     // The promise returned by "start" should have been rejected meanwhile.
@@ -794,20 +795,19 @@ add_task(function* test_cancel_midway_re
 add_task(function* test_cancel_midway_restart_tryToKeepPartialData()
 {
   let download = yield promiseStartDownload_tryToKeepPartialData();
   yield download.cancel();
 
   do_check_true(download.stopped);
   do_check_true(download.hasPartialData);
 
-  // The target file should not exist, but we should have kept the partial data.
-  do_check_false(yield OS.File.exists(download.target.path));
+  // The target file should exist and we should have kept the partial data.
+  do_check_true(yield OS.File.exists(download.target.path));
   yield promiseVerifyContents(download.target.partFilePath, TEST_DATA_SHORT);
-  do_check_false(download.target.exists);
   do_check_eq(download.target.size, 0);
 
   // Verify that the server sent the response from the start.
   do_check_eq(gMostRecentFirstBytePos, 0);
 
   // The second time, we'll request and obtain the second part of the response,
   // but we still stop when half of the remaining progress is reached.
   let deferMidway = Promise.defer();
@@ -1246,16 +1246,17 @@ 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));
+    do_check_false(yield OS.File.exists(download.target.partFilePath));
     do_check_false(download.target.exists);
     do_check_eq(download.target.size, 0);
   } finally {
     serverSocket.close();
   }
 });
 
 /**
@@ -1676,16 +1677,19 @@ add_task(function* test_blocked_parental
       throw ex;
     }
     do_check_true(ex.becauseBlockedByParentalControls);
     do_check_true(download.error.becauseBlockedByParentalControls);
     do_check_true(download.stopped);
   }
 
   do_check_false(yield OS.File.exists(download.target.path));
+  do_check_false(download.target.exists);
+  do_check_eq(download.target.size, 0);
+  do_check_false(yield OS.File.exists(download.target.partFilePath));
 });
 
 /**
  * Download with runtime permissions
  */
 add_task(function* test_blocked_runtime_permissions()
 {
   let blockFn = base => ({
@@ -1795,17 +1799,17 @@ var promiseBlockedDownload = Task.async(
                 Downloads.Error.BLOCK_VERDICT_UNCOMMON);
     do_check_true(download.error.becauseBlockedByReputationCheck);
     do_check_eq(download.error.reputationCheckVerdict,
                 Downloads.Error.BLOCK_VERDICT_UNCOMMON);
   }
 
   do_check_true(download.stopped);
   do_check_false(download.succeeded);
-  do_check_false(yield OS.File.exists(download.target.path));
+  do_check_eq(download.target.size, 0);
 
   cleanup();
   return download;
 });
 
 /**
  * Checks that application reputation blocks the download and the target file
  * does not exist.
@@ -1906,16 +1910,17 @@ add_task(function* test_blocked_applicat
 add_task(function* test_blocked_applicationReputation_confirmBlock()
 {
   let download = yield promiseBlockedDownload({
     keepPartialData: true,
     keepBlockedData: true,
   });
 
   do_check_true(download.hasBlockedData);
+  do_check_true(yield OS.File.exists(download.target.path));
   do_check_true(yield OS.File.exists(download.target.partFilePath));
 
   yield download.confirmBlock();
 
   // After confirming the block the download should be in a failed state and
   // have no downloaded data left on disk.
   do_check_true(download.stopped);
   do_check_false(download.succeeded);
@@ -1933,26 +1938,28 @@ add_task(function* test_blocked_applicat
 add_task(function* test_blocked_applicationReputation_unblock()
 {
   let download = yield promiseBlockedDownload({
     keepPartialData: true,
     keepBlockedData: true,
   });
 
   do_check_true(download.hasBlockedData);
+  do_check_true(yield OS.File.exists(download.target.path));
   do_check_true(yield OS.File.exists(download.target.partFilePath));
 
   yield download.unblock();
 
   // After unblocking the download should have succeeded and be
   // present at the final path.
   do_check_true(download.stopped);
   do_check_true(download.succeeded);
   do_check_false(download.hasBlockedData);
   do_check_false(yield OS.File.exists(download.target.partFilePath));
+  do_check_true(yield OS.File.exists(download.target.path));
   yield promiseVerifyTarget(download.target, TEST_DATA_SHORT + TEST_DATA_SHORT);
 
   // The only indication the download was previously blocked is the
   // existence of the error, so we make sure it's still set.
   do_check_true(download.error instanceof Downloads.Error);
   do_check_true(download.error.becauseBlocked);
   do_check_true(download.error.becauseBlockedByReputationCheck);
 });