Bug 1139913 - Downloads with partial data should still keep the placeholder on disk. r=mak draft
authorJohann Hofmann <jhofmann@mozilla.com>
Wed, 23 Aug 2017 15:41:25 +0100
changeset 651317 0d083d3a57d8e92ed07638f85b188b9052e671c2
parent 650941 7c50f0c999c5bf8ee915261997597a5a9b8fb2ae
child 727670 7b87516b9ca727be411a67a071b49d08ee3daf6e
push id75680
push userpaolo.mozmail@amadzone.org
push dateWed, 23 Aug 2017 14:56:10 +0000
reviewersmak
bugs1139913
milestone57.0a1
Bug 1139913 - Downloads with partial data should still keep the placeholder on disk. r=mak 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
@@ -429,36 +429,43 @@ this.Download.prototype = {
         // before we call the "execute" method of the saver.
         if (this._promiseCanceled) {
           // The exception will become a cancellation in the "catch" block.
           throw undefined;
         }
 
         // Execute the actual download through the saver object.
         this._saverExecuting = true;
-        await this.saver.execute(DS_setProgressBytes.bind(this),
-                                 DS_setProperties.bind(this));
+        try {
+          await this.saver.execute(DS_setProgressBytes.bind(this),
+                                   DS_setProperties.bind(this));
+        } catch (ex) {
+          // Remove the target file placeholder and all partial data when
+          // 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();
+          }
+          throw ex;
+        }
 
         // Now that the actual saving finished, read the actual file size on
         // disk, that may be different from the amount of data transferred.
         await 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) {
-          try {
-            await OS.File.remove(this.target.path);
-          } catch (ex) {
-            Cu.reportError(ex);
-          }
-
-          this.target.exists = false;
-          this.target.size = 0;
+          // To keep the internal state of the Download object consistent, we
+          // 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();
 
           // 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;
@@ -632,23 +639,19 @@ this.Download.prototype = {
     }
 
     if (!this.hasBlockedData) {
       return Promise.reject(new Error(
         "confirmBlock may only be called on Downloads with blocked data."));
     }
 
     this._promiseConfirmBlock = (async () => {
-      try {
-        await OS.File.remove(this.target.partFilePath);
-      } catch (ex) {
-        await this.refresh();
-        this._promiseConfirmBlock = null;
-        throw ex;
-      }
+      // This call never throws exceptions. If the removal fails, the blocked
+      // data remains stored on disk in the ".part" file.
+      await this.saver.removeData();
 
       this.hasBlockedData = false;
       this._notifyChange();
     })();
 
     return this._promiseConfirmBlock;
   },
 
@@ -806,17 +809,17 @@ this.Download.prototype = {
     if (!this._promiseRemovePartialData) {
       this._promiseRemovePartialData = (async () => {
         try {
           // Wait upon any pending cancellation request.
           if (this._promiseCanceled) {
             await this._promiseCanceled;
           }
           // Ask the saver object to remove any partial data.
-          await this.saver.removePartialData();
+          await this.saver.removeData();
           // For completeness, clear the number of bytes transferred.
           if (this.currentBytes != 0 || this.hasPartialData) {
             this.currentBytes = 0;
             this.hasPartialData = false;
             this._notifyChange();
           }
         } finally {
           this._promiseRemovePartialData = null;
@@ -1657,41 +1660,40 @@ this.DownloadSaver.prototype = {
    *        download process. It takes an object where the keys represents
    *        the names of the properties to set, and the value represents the
    *        value to set.
    *
    * @return {Promise}
    * @resolves When the download has finished successfully.
    * @rejects JavaScript exception if the download failed.
    */
-  execute: function DS_execute(aSetProgressBytesFn, aSetPropertiesFn) {
+  async execute(aSetProgressBytesFn, aSetPropertiesFn) {
     throw new Error("Not implemented.");
   },
 
   /**
    * Cancels the download.
    */
   cancel: function DS_cancel() {
     throw new Error("Not implemented.");
   },
 
   /**
-   * Removes any partial data kept as part of a canceled or failed download.
+   * Removes any target file placeholder and any partial data kept as part of a
+   * canceled, failed, or temporarily blocked download.
    *
    * This method is never called until the promise returned by "execute" is
    * either resolved or rejected, and the "execute" method is not called again
    * until the promise returned by this method is resolved or rejected.
    *
    * @return {Promise}
    * @resolves When the operation has finished successfully.
-   * @rejects JavaScript exception.
+   * @rejects Never.
    */
-  removePartialData: function DS_removePartialData() {
-    return Promise.resolve();
-  },
+  async removeData() {},
 
   /**
    * This can be called by the saver implementation when the download is already
    * started, to add it to the browsing history.  This method has no effect if
    * the download is private.
    */
   addToHistory() {
     if (this.download.source.isPrivate) {
@@ -1822,300 +1824,279 @@ this.DownloadCopySaver.prototype = {
    * used to execute the download, or null if the channel was not resumable or
    * the saver was instructed not to keep partially downloaded data.
    */
   entityID: null,
 
   /**
    * Implements "DownloadSaver.execute".
    */
-  execute: function DCS_execute(aSetProgressBytesFn, aSetPropertiesFn) {
+  async execute(aSetProgressBytesFn, aSetPropertiesFn) {
     let copySaver = this;
 
     this._canceled = false;
 
     let download = this.download;
     let targetPath = download.target.path;
     let partFilePath = download.target.partFilePath;
     let keepPartialData = download.tryToKeepPartialData;
 
-    return (async () => {
-      // Add the download to history the first time it is started in this
-      // session.  If the download is restarted in a different session, a new
-      // history visit will be added.  We do this just to avoid the complexity
-      // of serializing this state between sessions, since adding a new visit
-      // does not have any noticeable side effect.
-      if (!this.alreadyAddedToHistory) {
-        this.addToHistory();
-        this.alreadyAddedToHistory = true;
+    // Add the download to history the first time it is started in this
+    // session.  If the download is restarted in a different session, a new
+    // history visit will be added.  We do this just to avoid the complexity
+    // of serializing this state between sessions, since adding a new visit
+    // does not have any noticeable side effect.
+    if (!this.alreadyAddedToHistory) {
+      this.addToHistory();
+      this.alreadyAddedToHistory = true;
+    }
+
+    // To reduce the chance that other downloads reuse the same final target
+    // file name, we should create a placeholder as soon as possible, before
+    // starting the network request.  The placeholder is also required in case
+    // we are using a ".part" file instead of the final target while the
+    // download is in progress.
+    try {
+      // If the file already exists, don't delete its contents yet.
+      let file = await OS.File.open(targetPath, { write: true });
+      await file.close();
+    } catch (ex) {
+      if (!(ex instanceof OS.File.Error)) {
+        throw ex;
       }
+      // Throw a DownloadError indicating that the operation failed because of
+      // the target file.  We cannot translate this into a specific result
+      // code, but we preserve the original message using the toString method.
+      let error = new DownloadError({ message: ex.toString() });
+      error.becauseTargetFailed = true;
+      throw error;
+    }
+
+    let deferSaveComplete = PromiseUtils.defer();
+
+    if (this._canceled) {
+      // Don't create the BackgroundFileSaver object if we have been
+      // canceled meanwhile.
+      throw new DownloadError({ message: "Saver canceled." });
+    }
 
-      // To reduce the chance that other downloads reuse the same final target
-      // file name, we should create a placeholder as soon as possible, before
-      // starting the network request.  The placeholder is also required in case
-      // we are using a ".part" file instead of the final target while the
-      // download is in progress.
-      try {
-        // If the file already exists, don't delete its contents yet.
-        let file = await OS.File.open(targetPath, { write: true });
-        await file.close();
-      } catch (ex) {
-        if (!(ex instanceof OS.File.Error)) {
-          throw ex;
-        }
-        // Throw a DownloadError indicating that the operation failed because of
-        // the target file.  We cannot translate this into a specific result
-        // code, but we preserve the original message using the toString method.
-        let error = new DownloadError({ message: ex.toString() });
-        error.becauseTargetFailed = true;
-        throw error;
+    // Create the object that will save the file in a background thread.
+    let backgroundFileSaver = new BackgroundFileSaverStreamListener();
+    try {
+      // When the operation completes, reflect the status in the promise
+      // returned by this download execution function.
+      backgroundFileSaver.observer = {
+        onTargetChange() { },
+        onSaveComplete: (aSaver, aStatus) => {
+          // Send notifications now that we can restart if needed.
+          if (Components.isSuccessCode(aStatus)) {
+            // Save the hash before freeing backgroundFileSaver.
+            this._sha256Hash = aSaver.sha256Hash;
+            this._signatureInfo = aSaver.signatureInfo;
+            this._redirects = aSaver.redirects;
+            deferSaveComplete.resolve();
+          } else {
+            // Infer the origin of the error from the failure code, because
+            // BackgroundFileSaver does not provide more specific data.
+            let properties = { result: aStatus, inferCause: true };
+            deferSaveComplete.reject(new DownloadError(properties));
+          }
+          // Free the reference cycle, to release resources earlier.
+          backgroundFileSaver.observer = null;
+          this._backgroundFileSaver = null;
+        },
+      };
+
+      // Create a channel from the source, and listen to progress
+      // notifications.
+      let channel = NetUtil.newChannel({
+        uri: download.source.url,
+        loadUsingSystemPrincipal: true,
+      });
+      if (channel instanceof Ci.nsIPrivateBrowsingChannel) {
+        channel.setPrivate(download.source.isPrivate);
+      }
+      if (channel instanceof Ci.nsIHttpChannel &&
+          download.source.referrer) {
+        channel.referrer = NetUtil.newURI(download.source.referrer);
       }
 
-      try {
-        let deferSaveComplete = PromiseUtils.defer();
+      // This makes the channel be corretly throttled during page loads
+      // and also prevents its caching.
+      if (channel instanceof Ci.nsIHttpChannelInternal) {
+        channel.channelIsForDownload = true;
+      }
 
-        if (this._canceled) {
-          // Don't create the BackgroundFileSaver object if we have been
-          // canceled meanwhile.
-          throw new DownloadError({ message: "Saver canceled." });
-        }
-
-        // Create the object that will save the file in a background thread.
-        let backgroundFileSaver = new BackgroundFileSaverStreamListener();
+      // If we have data that we can use to resume the download from where
+      // it stopped, try to use it.
+      let resumeAttempted = false;
+      let resumeFromBytes = 0;
+      if (channel instanceof Ci.nsIResumableChannel && this.entityID &&
+          partFilePath && keepPartialData) {
         try {
-          // When the operation completes, reflect the status in the promise
-          // returned by this download execution function.
-          backgroundFileSaver.observer = {
-            onTargetChange() { },
-            onSaveComplete: (aSaver, aStatus) => {
-              // Send notifications now that we can restart if needed.
-              if (Components.isSuccessCode(aStatus)) {
-                // Save the hash before freeing backgroundFileSaver.
-                this._sha256Hash = aSaver.sha256Hash;
-                this._signatureInfo = aSaver.signatureInfo;
-                this._redirects = aSaver.redirects;
-                deferSaveComplete.resolve();
-              } else {
-                // Infer the origin of the error from the failure code, because
-                // BackgroundFileSaver does not provide more specific data.
-                let properties = { result: aStatus, inferCause: true };
-                deferSaveComplete.reject(new DownloadError(properties));
-              }
-              // Free the reference cycle, to release resources earlier.
-              backgroundFileSaver.observer = null;
-              this._backgroundFileSaver = null;
-            },
-          };
+          let stat = await OS.File.stat(partFilePath);
+          channel.resumeAt(stat.size, this.entityID);
+          resumeAttempted = true;
+          resumeFromBytes = stat.size;
+        } catch (ex) {
+          if (!(ex instanceof OS.File.Error) || !ex.becauseNoSuchFile) {
+            throw ex;
+          }
+        }
+      }
 
-          // Create a channel from the source, and listen to progress
-          // notifications.
-          let channel = NetUtil.newChannel({
-            uri: download.source.url,
-            loadUsingSystemPrincipal: true,
-          });
-          if (channel instanceof Ci.nsIPrivateBrowsingChannel) {
-            channel.setPrivate(download.source.isPrivate);
-          }
-          if (channel instanceof Ci.nsIHttpChannel &&
-              download.source.referrer) {
-            channel.referrer = NetUtil.newURI(download.source.referrer);
+      channel.notificationCallbacks = {
+        QueryInterface: XPCOMUtils.generateQI([Ci.nsIInterfaceRequestor]),
+        getInterface: XPCOMUtils.generateQI([Ci.nsIProgressEventSink]),
+        onProgress: function DCSE_onProgress(aRequest, aContext, aProgress,
+                                             aProgressMax) {
+          let currentBytes = resumeFromBytes + aProgress;
+          let totalBytes = aProgressMax == -1 ? -1 : (resumeFromBytes +
+                                                      aProgressMax);
+          aSetProgressBytesFn(currentBytes, totalBytes, aProgress > 0 &&
+                              partFilePath && keepPartialData);
+        },
+        onStatus() { },
+      };
+
+      // If the callback was set, handle it now before opening the channel.
+      if (download.source.adjustChannel) {
+        await download.source.adjustChannel(channel);
+      }
+
+      // Open the channel, directing output to the background file saver.
+      backgroundFileSaver.QueryInterface(Ci.nsIStreamListener);
+      channel.asyncOpen2({
+        onStartRequest: function(aRequest, aContext) {
+          backgroundFileSaver.onStartRequest(aRequest, aContext);
+
+          // Check if the request's response has been blocked by Windows
+          // Parental Controls with an HTTP 450 error code.
+          if (aRequest instanceof Ci.nsIHttpChannel &&
+              aRequest.responseStatus == 450) {
+            // Set a flag that can be retrieved later when handling the
+            // cancellation so that the proper error can be thrown.
+            this.download._blockedByParentalControls = true;
+            aRequest.cancel(Cr.NS_BINDING_ABORTED);
+            return;
           }
 
-          // This makes the channel be corretly throttled during page loads
-          // and also prevents its caching.
-          if (channel instanceof Ci.nsIHttpChannelInternal) {
-            channel.channelIsForDownload = true;
+          aSetPropertiesFn({ contentType: channel.contentType });
+
+          // Ensure we report the value of "Content-Length", if available,
+          // even if the download doesn't generate any progress events
+          // later.
+          if (channel.contentLength >= 0) {
+            aSetProgressBytesFn(0, channel.contentLength);
           }
 
-          // If we have data that we can use to resume the download from where
-          // it stopped, try to use it.
-          let resumeAttempted = false;
-          let resumeFromBytes = 0;
-          if (channel instanceof Ci.nsIResumableChannel && this.entityID &&
-              partFilePath && keepPartialData) {
-            try {
-              let stat = await OS.File.stat(partFilePath);
-              channel.resumeAt(stat.size, this.entityID);
-              resumeAttempted = true;
-              resumeFromBytes = stat.size;
-            } catch (ex) {
-              if (!(ex instanceof OS.File.Error) || !ex.becauseNoSuchFile) {
-                throw ex;
+          // If the URL we are downloading from includes a file extension
+          // that matches the "Content-Encoding" header, for example ".gz"
+          // with a "gzip" encoding, we should save the file in its encoded
+          // form.  In all other cases, we decode the body while saving.
+          if (channel instanceof Ci.nsIEncodedChannel &&
+              channel.contentEncodings) {
+            let uri = channel.URI;
+            if (uri instanceof Ci.nsIURL && uri.fileExtension) {
+              // Only the first, outermost encoding is considered.
+              let encoding = channel.contentEncodings.getNext();
+              if (encoding) {
+                channel.applyConversion =
+                  gExternalHelperAppService.applyDecodingForExtension(
+                                            uri.fileExtension, encoding);
               }
             }
           }
 
-          channel.notificationCallbacks = {
-            QueryInterface: XPCOMUtils.generateQI([Ci.nsIInterfaceRequestor]),
-            getInterface: XPCOMUtils.generateQI([Ci.nsIProgressEventSink]),
-            onProgress: function DCSE_onProgress(aRequest, aContext, aProgress,
-                                                 aProgressMax) {
-              let currentBytes = resumeFromBytes + aProgress;
-              let totalBytes = aProgressMax == -1 ? -1 : (resumeFromBytes +
-                                                          aProgressMax);
-              aSetProgressBytesFn(currentBytes, totalBytes, aProgress > 0 &&
-                                  partFilePath && keepPartialData);
-            },
-            onStatus() { },
-          };
-
-          // If the callback was set, handle it now before opening the channel.
-          if (download.source.adjustChannel) {
-            await download.source.adjustChannel(channel);
+          if (keepPartialData) {
+            // If the source is not resumable, don't keep partial data even
+            // if we were asked to try and do it.
+            if (aRequest instanceof Ci.nsIResumableChannel) {
+              try {
+                // If reading the ID succeeds, the source is resumable.
+                this.entityID = aRequest.entityID;
+              } catch (ex) {
+                if (!(ex instanceof Components.Exception) ||
+                    ex.result != Cr.NS_ERROR_NOT_RESUMABLE) {
+                  throw ex;
+                }
+                keepPartialData = false;
+              }
+            } else {
+              keepPartialData = false;
+            }
           }
 
-          // Open the channel, directing output to the background file saver.
-          backgroundFileSaver.QueryInterface(Ci.nsIStreamListener);
-          channel.asyncOpen2({
-            onStartRequest: function(aRequest, aContext) {
-              backgroundFileSaver.onStartRequest(aRequest, aContext);
-
-              // Check if the request's response has been blocked by Windows
-              // Parental Controls with an HTTP 450 error code.
-              if (aRequest instanceof Ci.nsIHttpChannel &&
-                  aRequest.responseStatus == 450) {
-                // Set a flag that can be retrieved later when handling the
-                // cancellation so that the proper error can be thrown.
-                this.download._blockedByParentalControls = true;
-                aRequest.cancel(Cr.NS_BINDING_ABORTED);
-                return;
-              }
-
-              aSetPropertiesFn({ contentType: channel.contentType });
-
-              // Ensure we report the value of "Content-Length", if available,
-              // even if the download doesn't generate any progress events
-              // later.
-              if (channel.contentLength >= 0) {
-                aSetProgressBytesFn(0, channel.contentLength);
-              }
+          // Enable hashing and signature verification before setting the
+          // target.
+          backgroundFileSaver.enableSha256();
+          backgroundFileSaver.enableSignatureInfo();
+          if (partFilePath) {
+            // If we actually resumed a request, append to the partial data.
+            if (resumeAttempted) {
+              // TODO: Handle Cr.NS_ERROR_ENTITY_CHANGED
+              backgroundFileSaver.enableAppend();
+            }
 
-              // If the URL we are downloading from includes a file extension
-              // that matches the "Content-Encoding" header, for example ".gz"
-              // with a "gzip" encoding, we should save the file in its encoded
-              // form.  In all other cases, we decode the body while saving.
-              if (channel instanceof Ci.nsIEncodedChannel &&
-                  channel.contentEncodings) {
-                let uri = channel.URI;
-                if (uri instanceof Ci.nsIURL && uri.fileExtension) {
-                  // Only the first, outermost encoding is considered.
-                  let encoding = channel.contentEncodings.getNext();
-                  if (encoding) {
-                    channel.applyConversion =
-                      gExternalHelperAppService.applyDecodingForExtension(
-                                                uri.fileExtension, encoding);
-                  }
-                }
-              }
-
-              if (keepPartialData) {
-                // If the source is not resumable, don't keep partial data even
-                // if we were asked to try and do it.
-                if (aRequest instanceof Ci.nsIResumableChannel) {
-                  try {
-                    // If reading the ID succeeds, the source is resumable.
-                    this.entityID = aRequest.entityID;
-                  } catch (ex) {
-                    if (!(ex instanceof Components.Exception) ||
-                        ex.result != Cr.NS_ERROR_NOT_RESUMABLE) {
-                      throw ex;
-                    }
-                    keepPartialData = false;
-                  }
-                } else {
-                  keepPartialData = false;
-                }
-              }
-
-              // Enable hashing and signature verification before setting the
-              // target.
-              backgroundFileSaver.enableSha256();
-              backgroundFileSaver.enableSignatureInfo();
-              if (partFilePath) {
-                // If we actually resumed a request, append to the partial data.
-                if (resumeAttempted) {
-                  // TODO: Handle Cr.NS_ERROR_ENTITY_CHANGED
-                  backgroundFileSaver.enableAppend();
-                }
+            // Use a part file, determining if we should keep it on failure.
+            backgroundFileSaver.setTarget(new FileUtils.File(partFilePath),
+                                          keepPartialData);
+          } else {
+            // Set the final target file, and delete it on failure.
+            backgroundFileSaver.setTarget(new FileUtils.File(targetPath),
+                                          false);
+          }
+        }.bind(copySaver),
 
-                // Use a part file, determining if we should keep it on failure.
-                backgroundFileSaver.setTarget(new FileUtils.File(partFilePath),
-                                              keepPartialData);
-              } else {
-                // Set the final target file, and delete it on failure.
-                backgroundFileSaver.setTarget(new FileUtils.File(targetPath),
-                                              false);
-              }
-            }.bind(copySaver),
-
-            onStopRequest(aRequest, aContext, aStatusCode) {
-              try {
-                backgroundFileSaver.onStopRequest(aRequest, aContext,
-                                                  aStatusCode);
-              } finally {
-                // If the data transfer completed successfully, indicate to the
-                // background file saver that the operation can finish.  If the
-                // data transfer failed, the saver has been already stopped.
-                if (Components.isSuccessCode(aStatusCode)) {
-                  backgroundFileSaver.finish(Cr.NS_OK);
-                }
-              }
-            },
-
-            onDataAvailable(aRequest, aContext, aInputStream,
-                                      aOffset, aCount) {
-              backgroundFileSaver.onDataAvailable(aRequest, aContext,
-                                                  aInputStream, aOffset,
-                                                  aCount);
-            },
-          });
-
-          // We should check if we have been canceled in the meantime, after
-          // all the previous asynchronous operations have been executed and
-          // just before we set the _backgroundFileSaver property.
-          if (this._canceled) {
-            throw new DownloadError({ message: "Saver canceled." });
+        onStopRequest(aRequest, aContext, aStatusCode) {
+          try {
+            backgroundFileSaver.onStopRequest(aRequest, aContext,
+                                              aStatusCode);
+          } finally {
+            // If the data transfer completed successfully, indicate to the
+            // background file saver that the operation can finish.  If the
+            // data transfer failed, the saver has been already stopped.
+            if (Components.isSuccessCode(aStatusCode)) {
+              backgroundFileSaver.finish(Cr.NS_OK);
+            }
           }
+        },
 
-          // If the operation succeeded, store the object to allow cancellation.
-          this._backgroundFileSaver = backgroundFileSaver;
-        } catch (ex) {
-          // In case an error occurs while setting up the chain of objects for
-          // the download, ensure that we release the resources of the saver.
-          backgroundFileSaver.finish(Cr.NS_ERROR_FAILURE);
-          // Since we're not going to handle deferSaveComplete.promise below,
-          // we need to make sure that the rejection is handled.
-          deferSaveComplete.promise.catch(() => {});
-          throw ex;
-        }
+        onDataAvailable(aRequest, aContext, aInputStream,
+                                  aOffset, aCount) {
+          backgroundFileSaver.onDataAvailable(aRequest, aContext,
+                                              aInputStream, aOffset,
+                                              aCount);
+        },
+      });
 
-        // We will wait on this promise in case no error occurred while setting
-        // up the chain of objects for the download.
-        await deferSaveComplete.promise;
+      // We should check if we have been canceled in the meantime, after
+      // all the previous asynchronous operations have been executed and
+      // just before we set the _backgroundFileSaver property.
+      if (this._canceled) {
+        throw new DownloadError({ message: "Saver canceled." });
+      }
 
-        await 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 {
-          await OS.File.remove(targetPath);
-        } 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;
-      }
-    })();
+      // If the operation succeeded, store the object to allow cancellation.
+      this._backgroundFileSaver = backgroundFileSaver;
+    } catch (ex) {
+      // In case an error occurs while setting up the chain of objects for
+      // the download, ensure that we release the resources of the saver.
+      backgroundFileSaver.finish(Cr.NS_ERROR_FAILURE);
+      // Since we're not going to handle deferSaveComplete.promise below,
+      // we need to make sure that the rejection is handled.
+      deferSaveComplete.promise.catch(() => {});
+      throw ex;
+    }
+
+    // We will wait on this promise in case no error occurred while setting
+    // up the chain of objects for the download.
+    await deferSaveComplete.promise;
+
+    await this._checkReputationAndMove(aSetPropertiesFn);
   },
 
   /**
    * Perform the reputation check and cleanup the downloaded data if required.
    * If the download passes the reputation check and is using a part file we
    * will move it to the target path since reputation checking is the final
    * step in the saver.
    *
@@ -2136,21 +2117,17 @@ this.DownloadCopySaver.prototype = {
     if (shouldBlock) {
       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 {
-          await OS.File.remove(partFilePath || targetPath);
-        } catch (ex) {
-          Cu.reportError(ex);
-        }
+        await this.removeData();
       } else {
         newProperties.hasBlockedData = true;
       }
 
       aSetPropertiesFn(newProperties);
 
       throw new DownloadError({
         becauseBlockedByReputationCheck: true,
@@ -2170,30 +2147,44 @@ this.DownloadCopySaver.prototype = {
     this._canceled = true;
     if (this._backgroundFileSaver) {
       this._backgroundFileSaver.finish(Cr.NS_ERROR_FAILURE);
       this._backgroundFileSaver = null;
     }
   },
 
   /**
-   * Implements "DownloadSaver.removePartialData".
+   * Implements "DownloadSaver.removeData".
    */
-  removePartialData() {
-    return (async () => {
-      if (this.download.target.partFilePath) {
-        try {
-          await OS.File.remove(this.download.target.partFilePath);
-        } catch (ex) {
-          if (!(ex instanceof OS.File.Error) || !ex.becauseNoSuchFile) {
-            throw ex;
-          }
+  async removeData() {
+    // Defined inline so removeData can be shared with DownloadLegacySaver.
+    async function _tryToRemoveFile(path) {
+      try {
+        await OS.File.remove(path);
+      } catch (ex) {
+        // 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. This
+        // is likely to happen when the component that executed the download has
+        // just deleted the target file itself.
+        if (!(ex instanceof OS.File.Error &&
+              (ex.becauseNoSuchFile || ex.becauseAccessDenied))) {
+          Cu.reportError(ex);
         }
       }
-    })();
+    }
+
+    if (this.download.target.partFilePath) {
+      await _tryToRemoveFile(this.download.target.partFilePath);
+    }
+
+    if (this.download.target.path) {
+      await _tryToRemoveFile(this.download.target.path);
+      this.download.target.exists = false;
+      this.download.target.size = 0;
+    }
   },
 
   /**
    * Implements "DownloadSaver.toSerializable".
    */
   toSerializable() {
     // Simplify the representation if we don't have other details.
     if (!this.entityID && !this._unknownProperties) {
@@ -2416,17 +2407,17 @@ this.DownloadLegacySaver.prototype = {
    * used to execute the download, or null if the channel was not resumable or
    * the saver was instructed not to keep partially downloaded data.
    */
   entityID: null,
 
   /**
    * Implements "DownloadSaver.execute".
    */
-  execute: function DLS_execute(aSetProgressBytesFn, aSetPropertiesFn) {
+  async execute(aSetProgressBytesFn, aSetPropertiesFn) {
     // Check if this is not the first execution of the download.  The Download
     // object guarantees that this function is not re-entered during execution.
     if (this.firstExecutionFinished) {
       if (!this.copySaver) {
         this.copySaver = new DownloadCopySaver();
         this.copySaver.download = this.download;
         this.copySaver.entityID = this.entityID;
         this.copySaver.alreadyAddedToHistory = true;
@@ -2434,85 +2425,68 @@ this.DownloadLegacySaver.prototype = {
       return this.copySaver.execute.apply(this.copySaver, arguments);
     }
 
     this.setProgressBytesFn = aSetProgressBytesFn;
     if (this.progressWasNotified) {
       this.onProgressBytes(this.currentBytes, this.totalBytes);
     }
 
-    return (async () => {
-      try {
-        // Wait for the component that executes the download to finish.
-        await this.deferExecuted.promise;
+    try {
+      // Wait for the component that executes the download to finish.
+      await this.deferExecuted.promise;
 
-        // At this point, the "request" property has been populated.  Ensure we
-        // report the value of "Content-Length", if available, even if the
-        // download didn't generate any progress events.
-        if (!this.progressWasNotified &&
-            this.request instanceof Ci.nsIChannel &&
-            this.request.contentLength >= 0) {
-          aSetProgressBytesFn(0, this.request.contentLength);
-        }
+      // At this point, the "request" property has been populated.  Ensure we
+      // report the value of "Content-Length", if available, even if the
+      // download didn't generate any progress events.
+      if (!this.progressWasNotified &&
+          this.request instanceof Ci.nsIChannel &&
+          this.request.contentLength >= 0) {
+        aSetProgressBytesFn(0, this.request.contentLength);
+      }
 
-        // If the component executing the download provides the path of a
-        // ".part" file, it means that it expects the listener to move the file
-        // to its final target path when the download succeeds.  In this case,
-        // an empty ".part" file is created even if no data was received from
-        // the source.
-        //
-        // When no ".part" file path is provided the download implementation may
-        // not have created the target file (if no data was received from the
-        // source).  In this case, ensure that an empty file is created as
-        // expected.
-        if (!this.download.target.partFilePath) {
-          try {
-            // This atomic operation is more efficient than an existence check.
-            let file = await OS.File.open(this.download.target.path,
-                                          { create: true });
-            await file.close();
-          } catch (ex) {
-            if (!(ex instanceof OS.File.Error) || !ex.becauseExists) {
-              throw ex;
-            }
+      // If the component executing the download provides the path of a
+      // ".part" file, it means that it expects the listener to move the file
+      // to its final target path when the download succeeds.  In this case,
+      // an empty ".part" file is created even if no data was received from
+      // the source.
+      //
+      // When no ".part" file path is provided the download implementation may
+      // not have created the target file (if no data was received from the
+      // source).  In this case, ensure that an empty file is created as
+      // expected.
+      if (!this.download.target.partFilePath) {
+        try {
+          // This atomic operation is more efficient than an existence check.
+          let file = await OS.File.open(this.download.target.path,
+                                        { create: true });
+          await file.close();
+        } catch (ex) {
+          if (!(ex instanceof OS.File.Error) || !ex.becauseExists) {
+            throw ex;
           }
         }
+      }
 
-        await this._checkReputationAndMove(aSetPropertiesFn);
+      await 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 {
-          await 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);
-          }
-        }
-        // 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 {
-        // We don't need the reference to the request anymore.  We must also set
-        // deferCanceled to null in order to free any indirect references it
-        // may hold to the request.
-        this.request = null;
-        this.deferCanceled = null;
-        // Allow the download to restart through a DownloadCopySaver.
-        this.firstExecutionFinished = true;
-      }
-    })();
+    } catch (ex) {
+      // 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 {
+      // We don't need the reference to the request anymore.  We must also set
+      // deferCanceled to null in order to free any indirect references it
+      // may hold to the request.
+      this.request = null;
+      this.deferCanceled = null;
+      // Allow the download to restart through a DownloadCopySaver.
+      this.firstExecutionFinished = true;
+    }
   },
 
   _checkReputationAndMove() {
     return DownloadCopySaver.prototype._checkReputationAndMove
                                       .apply(this, arguments);
   },
 
   /**
@@ -2528,23 +2502,23 @@ this.DownloadLegacySaver.prototype = {
     // operation is canceled as soon as a cancellation handler is registered.
     // Note that the handler might not have been registered yet.
     if (this.deferCanceled) {
       this.deferCanceled.resolve();
     }
   },
 
   /**
-   * Implements "DownloadSaver.removePartialData".
+   * Implements "DownloadSaver.removeData".
    */
-  removePartialData() {
+  removeData() {
     // 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.removePartialData.call(this);
+    return DownloadCopySaver.prototype.removeData.call(this);
   },
 
   /**
    * Implements "DownloadSaver.toSerializable".
    */
   toSerializable() {
     // This object depends on legacy components that are created externally,
     // thus it cannot be rebuilt during deserialization.  To support resuming
@@ -2637,98 +2611,96 @@ this.DownloadPDFSaver.prototype = {
    * This is null when saving has not started or has completed,
    * or while the operation is being canceled.
    */
   _webBrowserPrint: null,
 
   /**
    * Implements "DownloadSaver.execute".
    */
-  execute(aSetProgressBytesFn, aSetPropertiesFn) {
-    return (async () => {
-      if (!this.download.source.windowRef) {
-        throw new DownloadError({
-          message: "PDF saver must be passed an open window, and cannot be restarted.",
-          becauseSourceFailed: true,
-        });
-      }
+  async execute(aSetProgressBytesFn, aSetPropertiesFn) {
+    if (!this.download.source.windowRef) {
+      throw new DownloadError({
+        message: "PDF saver must be passed an open window, and cannot be restarted.",
+        becauseSourceFailed: true,
+      });
+    }
 
-      let win = this.download.source.windowRef.get();
+    let win = this.download.source.windowRef.get();
 
-      // Set windowRef to null to avoid re-trying.
-      this.download.source.windowRef = null;
+    // Set windowRef to null to avoid re-trying.
+    this.download.source.windowRef = null;
 
-      if (!win) {
-        throw new DownloadError({
-          message: "PDF saver can't save a window that has been closed.",
-          becauseSourceFailed: true,
-        });
-      }
+    if (!win) {
+      throw new DownloadError({
+        message: "PDF saver can't save a window that has been closed.",
+        becauseSourceFailed: true,
+      });
+    }
 
-      this.addToHistory();
+    this.addToHistory();
 
-      let targetPath = this.download.target.path;
+    let targetPath = this.download.target.path;
 
-      // An empty target file must exist for the PDF printer to work correctly.
-      let file = await OS.File.open(targetPath, { truncate: true });
-      await file.close();
+    // An empty target file must exist for the PDF printer to work correctly.
+    let file = await OS.File.open(targetPath, { truncate: true });
+    await file.close();
 
-      let printSettings = gPrintSettingsService.newPrintSettings;
+    let printSettings = gPrintSettingsService.newPrintSettings;
 
-      printSettings.printToFile = true;
-      printSettings.outputFormat = Ci.nsIPrintSettings.kOutputFormatPDF;
-      printSettings.toFileName = targetPath;
+    printSettings.printToFile = true;
+    printSettings.outputFormat = Ci.nsIPrintSettings.kOutputFormatPDF;
+    printSettings.toFileName = targetPath;
 
-      printSettings.printSilent = true;
-      printSettings.showPrintProgress = false;
+    printSettings.printSilent = true;
+    printSettings.showPrintProgress = false;
 
-      printSettings.printBGImages = true;
-      printSettings.printBGColors = true;
-      printSettings.printFrameType = Ci.nsIPrintSettings.kFramesAsIs;
-      printSettings.headerStrCenter = "";
-      printSettings.headerStrLeft = "";
-      printSettings.headerStrRight = "";
-      printSettings.footerStrCenter = "";
-      printSettings.footerStrLeft = "";
-      printSettings.footerStrRight = "";
+    printSettings.printBGImages = true;
+    printSettings.printBGColors = true;
+    printSettings.printFrameType = Ci.nsIPrintSettings.kFramesAsIs;
+    printSettings.headerStrCenter = "";
+    printSettings.headerStrLeft = "";
+    printSettings.headerStrRight = "";
+    printSettings.footerStrCenter = "";
+    printSettings.footerStrLeft = "";
+    printSettings.footerStrRight = "";
 
-      this._webBrowserPrint = win.QueryInterface(Ci.nsIInterfaceRequestor)
-                                 .getInterface(Ci.nsIWebBrowserPrint);
+    this._webBrowserPrint = win.QueryInterface(Ci.nsIInterfaceRequestor)
+                               .getInterface(Ci.nsIWebBrowserPrint);
 
-      try {
-        await new Promise((resolve, reject) => {
-          this._webBrowserPrint.print(printSettings, {
-            onStateChange(webProgress, request, stateFlags, status) {
-              if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
-                if (!Components.isSuccessCode(status)) {
-                  reject(new DownloadError({ result: status,
-                                             inferCause: true }));
-                } else {
-                  resolve();
-                }
+    try {
+      await new Promise((resolve, reject) => {
+        this._webBrowserPrint.print(printSettings, {
+          onStateChange(webProgress, request, stateFlags, status) {
+            if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
+              if (!Components.isSuccessCode(status)) {
+                reject(new DownloadError({ result: status,
+                                           inferCause: true }));
+              } else {
+                resolve();
               }
-            },
-            onProgressChange(webProgress, request, curSelfProgress,
-                                       maxSelfProgress, curTotalProgress,
-                                       maxTotalProgress) {
-              aSetProgressBytesFn(curTotalProgress, maxTotalProgress, false);
-            },
-            onLocationChange() {},
-            onStatusChange() {},
-            onSecurityChange() {},
-          });
+            }
+          },
+          onProgressChange(webProgress, request, curSelfProgress,
+                                     maxSelfProgress, curTotalProgress,
+                                     maxTotalProgress) {
+            aSetProgressBytesFn(curTotalProgress, maxTotalProgress, false);
+          },
+          onLocationChange() {},
+          onStatusChange() {},
+          onSecurityChange() {},
         });
-      } finally {
-        // Remove the print object to avoid leaks
-        this._webBrowserPrint = null;
-      }
+      });
+    } finally {
+      // Remove the print object to avoid leaks
+      this._webBrowserPrint = null;
+    }
 
-      let fileInfo = await OS.File.stat(targetPath);
-      aSetProgressBytesFn(fileInfo.size, fileInfo.size, false);
-    })();
+    let fileInfo = await OS.File.stat(targetPath);
+    aSetProgressBytesFn(fileInfo.size, fileInfo.size, false);
   },
 
   /**
    * Implements "DownloadSaver.cancel".
    */
   cancel: function DCS_cancel() {
     if (this._webBrowserPrint) {
       this._webBrowserPrint.cancel();
--- a/toolkit/components/jsdownloads/test/unit/common_test_Download.js
+++ b/toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ -867,18 +867,18 @@ add_task(async function test_cancel_midw
  */
 add_task(async function test_cancel_midway_restart_tryToKeepPartialData() {
   let download = await promiseStartDownload_tryToKeepPartialData();
   await 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(await OS.File.exists(download.target.path));
+  // We should have kept the partial data and an empty target file placeholder.
+  do_check_true(await OS.File.exists(download.target.path));
   await 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,
@@ -916,25 +916,20 @@ add_task(async function test_cancel_midw
 
 /**
  * Cancels a download while keeping partially downloaded data, then removes the
  * data and restarts the download from the beginning.
  */
 add_task(async function test_cancel_midway_restart_removePartialData() {
   let download = await promiseStartDownload_tryToKeepPartialData();
   await download.cancel();
-
-  do_check_true(download.hasPartialData);
-  await promiseVerifyContents(download.target.partFilePath, TEST_DATA_SHORT);
-  do_check_false(download.target.exists);
-  do_check_eq(download.target.size, 0);
-
   await download.removePartialData();
 
   do_check_false(download.hasPartialData);
+  do_check_false(await OS.File.exists(download.target.path));
   do_check_false(await OS.File.exists(download.target.partFilePath));
   do_check_false(download.target.exists);
   do_check_eq(download.target.size, 0);
 
   // The second time, we'll request and obtain the entire response again.
   continueResponses();
   await download.start();
 
@@ -957,40 +952,43 @@ add_task(async function test_cancel_midw
 
   download.tryToKeepPartialData = false;
 
   // The above property change does not affect existing partial data.
   do_check_true(download.hasPartialData);
   await promiseVerifyContents(download.target.partFilePath, TEST_DATA_SHORT);
 
   await download.removePartialData();
+  do_check_false(await OS.File.exists(download.target.path));
   do_check_false(await OS.File.exists(download.target.partFilePath));
 
   // Restart the download from the beginning.
   mustInterruptResponses();
   download.start().catch(() => {});
 
   await promiseDownloadMidway(download);
   await promisePartFileReady(download);
 
   // While the download is in progress, we should still have a ".part" file.
   do_check_false(download.hasPartialData);
+  do_check_true(await OS.File.exists(download.target.path));
   do_check_true(await OS.File.exists(download.target.partFilePath));
 
   // On Unix, verify that the file with the partially downloaded data is not
   // accessible by other users on the system.
   if (Services.appinfo.OS == "Darwin" || Services.appinfo.OS == "Linux") {
     do_check_eq((await OS.File.stat(download.target.partFilePath)).unixMode,
                 0o600);
   }
 
   await download.cancel();
 
   // The ".part" file should be deleted now that the download is canceled.
   do_check_false(download.hasPartialData);
+  do_check_false(await OS.File.exists(download.target.path));
   do_check_false(await OS.File.exists(download.target.partFilePath));
 
   // The third time, we'll request and obtain the entire response again.
   continueResponses();
   await download.start();
 
   // Verify that the server sent the response from the start.
   do_check_eq(gMostRecentFirstBytePos, 0);
@@ -1213,26 +1211,28 @@ add_task(async function test_finalize() 
  * Checks that the "finalize" method can remove partially downloaded data.
  */
 add_task(async function test_finalize_tryToKeepPartialData() {
   // Check finalization without removing partial data.
   let download = await promiseStartDownload_tryToKeepPartialData();
   await download.finalize();
 
   do_check_true(download.hasPartialData);
+  do_check_true(await OS.File.exists(download.target.path));
   do_check_true(await OS.File.exists(download.target.partFilePath));
 
   // Clean up.
   await download.removePartialData();
 
   // Check finalization while removing partial data.
   download = await promiseStartDownload_tryToKeepPartialData();
   await download.finalize(true);
 
   do_check_false(download.hasPartialData);
+  do_check_false(await OS.File.exists(download.target.path));
   do_check_false(await OS.File.exists(download.target.partFilePath));
 });
 
 /**
  * Checks that whenSucceeded returns a promise that is resolved after a restart.
  */
 add_task(async function test_whenSucceeded_after_restart() {
   mustInterruptResponses();
@@ -1844,17 +1844,16 @@ var promiseBlockedDownload = async funct
                 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(await OS.File.exists(download.target.path));
 
   cleanup();
   return download;
 };
 
 /**
  * Checks that application reputation blocks the download and the target file
  * does not exist.
@@ -1952,16 +1951,17 @@ add_task(async function test_blocked_app
  */
 add_task(async function test_blocked_applicationReputation_confirmBlock() {
   let download = await promiseBlockedDownload({
     keepPartialData: true,
     keepBlockedData: true,
   });
 
   do_check_true(download.hasBlockedData);
+  do_check_eq((await OS.File.stat(download.target.path)).size, 0);
   do_check_true(await OS.File.exists(download.target.partFilePath));
 
   await 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);
@@ -1978,16 +1978,17 @@ add_task(async function test_blocked_app
  */
 add_task(async function test_blocked_applicationReputation_unblock() {
   let download = await promiseBlockedDownload({
     keepPartialData: true,
     keepBlockedData: true,
   });
 
   do_check_true(download.hasBlockedData);
+  do_check_eq((await OS.File.stat(download.target.path)).size, 0);
   do_check_true(await OS.File.exists(download.target.partFilePath));
 
   await 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);