Bug 1501277 - Don't remove non-placeholder if placeholder is expected. r=mak

Differential Revision: https://phabricator.services.mozilla.com/D33162

--HG--
extra : moz-landing-system : lando

toolkit/components/downloads/DownloadCore.jsm

@@ -89,6 +89,24 @@ function deserializeUnknownProperties(aObject, aSerializable, aFilterFn) {
   }
 }
 
+/**
+ * Check if the file is a placeholder.
+ *
+ * @return {Promise}
+ * @resolves {boolean}
+ * @rejects Never.
+ */
+async function isPlaceholder(path) {
+  try {
+    if ((await OS.File.stat(path)).size == 0) {
+      return true;
+    }
+  } catch (ex) {
+    Cu.reportError(ex);
+  }
+  return false;
+}
+
 /**
  * This determines the minimum time interval between updates to the number of
  * bytes transferred, and is a limiting factor to the sequence of readings used
@@ -440,7 +458,7 @@ this.Download.prototype = {
           // needed, independently of which code path failed. In some cases, the
           // component executing the download may have already removed the file.
           if (!this.hasPartialData && !this.hasBlockedData) {
-            await this.saver.removeData();
+            await this.saver.removeData(true);
           }
           throw ex;
         }
@@ -458,7 +476,7 @@ this.Download.prototype = {
           // just delete the target and effectively cancel the download. Since
           // the DownloadSaver succeeded, we already renamed the ".part" file to
           // the final name, and this results in all the data being deleted.
-          await this.saver.removeData();
+          await this.saver.removeData(true);
 
           // Cancellation exceptions will be changed in the catch block below.
           throw new DownloadError();
@@ -1700,11 +1718,13 @@ this.DownloadSaver.prototype = {
    * either resolved or rejected, and the "execute" method is not called again
    * until the promise returned by this method is resolved or rejected.
    *
+   * @param canRemoveFinalTarget
+   *        True if can remove target file regardless of it being a placeholder.
    * @return {Promise}
    * @resolves When the operation has finished successfully.
    * @rejects Never.
    */
-  async removeData() {},
+  async removeData(canRemoveFinalTarget) {},
 
   /**
    * This can be called by the saver implementation when the download is already
@@ -2125,7 +2145,7 @@ this.DownloadCopySaver.prototype = {
       // download did not use a partial file path, meaning it
       // currently has its final filename.
       if (!DownloadIntegration.shouldKeepBlockedData() || !partFilePath) {
-        await this.removeData();
+        await this.removeData(!partFilePath);
       } else {
         newProperties.hasBlockedData = true;
       }
@@ -2157,7 +2177,7 @@ this.DownloadCopySaver.prototype = {
   /**
    * Implements "DownloadSaver.removeData".
    */
-  async removeData() {
+  async removeData(canRemoveFinalTarget = false) {
     // Defined inline so removeData can be shared with DownloadLegacySaver.
     async function _tryToRemoveFile(path) {
       try {
@@ -2179,7 +2199,9 @@ this.DownloadCopySaver.prototype = {
     }
 
     if (this.download.target.path) {
-      await _tryToRemoveFile(this.download.target.path);
+      if (canRemoveFinalTarget || await isPlaceholder(this.download.target.path)) {
+        await _tryToRemoveFile(this.download.target.path);
+      }
       this.download.target.exists = false;
       this.download.target.size = 0;
     }
@@ -2507,11 +2529,11 @@ this.DownloadLegacySaver.prototype = {
   /**
    * Implements "DownloadSaver.removeData".
    */
-  removeData() {
+  removeData(canRemoveFinalTarget) {
     // DownloadCopySaver and DownloadLeagcySaver use the same logic for removing
     // partially downloaded data, though this implementation isn't shared by
     // other saver types, thus it isn't found on their shared prototype.
-    return DownloadCopySaver.prototype.removeData.call(this);
+    return DownloadCopySaver.prototype.removeData.call(this, canRemoveFinalTarget);
   },
 
   /**

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

@@ -2537,6 +2537,89 @@ add_task(async function test_history_tryToKeepPartialData() {
   await promiseDownloadStopped(download);
 });
 
+/**
+ * Checks that finished downloads are not removed.
+ */
+add_task(async function test_download_cancel_retry_finalize() {
+  // Start a download that is not allowed to finish yet.
+  let sourceUrl = httpUrl("interruptible.txt");
+  let targetFilePath = getTempFile(TEST_TARGET_FILE_NAME).path;
+  mustInterruptResponses();
+  let download1 = await Downloads.createDownload({
+    source: sourceUrl,
+    target: { path: targetFilePath,
+              partFilePath: targetFilePath + ".part" },
+  });
+  download1.start().catch(() => {});
+  await promiseDownloadMidway(download1);
+  await promisePartFileReady(download1);
+
+  // Cancel the download and make sure that the partial data do not exist.
+  await download1.cancel();
+  Assert.equal(targetFilePath, download1.target.path);
+  Assert.equal(false, await OS.File.exists(download1.target.path));
+  Assert.equal(false, await OS.File.exists(download1.target.partFilePath));
+  continueResponses();
+
+  // Download the same file again with a different download session.
+  let download2 = await Downloads.createDownload({
+    source: sourceUrl,
+    target: { path: targetFilePath,
+              partFilePath: targetFilePath + ".part" },
+  });
+  download2.start().catch(() => {});
+
+  // Wait for download to be completed.
+  await promiseDownloadStopped(download2);
+  Assert.equal(targetFilePath, download2.target.path);
+  Assert.ok(await OS.File.exists(download2.target.path));
+  Assert.equal(false, await OS.File.exists(download2.target.partFilePath));
+
+  // Finalize the first download session.
+  await download1.finalize(true);
+
+  // The complete download should not have been removed.
+  Assert.ok(await OS.File.exists(download2.target.path));
+  Assert.equal(false, await OS.File.exists(download2.target.partFilePath));
+});
+
+/**
+ * Checks that confirmBlock does not clobber unrelated safe files.
+ */
+add_task(async function test_blocked_removeByHand_confirmBlock() {
+  let download1 = await promiseBlockedDownload({
+    keepPartialData: true,
+    keepBlockedData: true,
+  });
+
+  Assert.ok(download1.hasBlockedData);
+  Assert.equal((await OS.File.stat(download1.target.path)).size, 0);
+  Assert.ok(await OS.File.exists(download1.target.partFilePath));
+
+  // Remove the placeholder without telling the download.
+  await OS.File.remove(download1.target.path);
+  Assert.equal(false, await OS.File.exists(download1.target.path));
+
+  // Download a file with the same name as the blocked download.
+  let download2 = await Downloads.createDownload({
+    source: httpUrl("interruptible_resumable.txt"),
+    target: { path: download1.target.path,
+              partFilePath: download1.target.path + ".part" },
+  });
+  download2.start().catch(() => {});
+
+  // Wait for download to be completed.
+  await promiseDownloadStopped(download2);
+  Assert.equal(download1.target.path, download2.target.path);
+  Assert.ok(await OS.File.exists(download2.target.path));
+
+  // Remove the blocked download.
+  await download1.confirmBlock();
+
+  // After confirming the complete download should not have been removed.
+  Assert.ok(await OS.File.exists(download2.target.path));
+});
+
 /**
  * Tests that the temp download files are removed on exit and exiting private
  * mode after they have been launched.