Bug 1546957 - Prevent BITS Requests from being cancelled twice r=rstrong
authorKirk Steuber <ksteuber@mozilla.com>
Wed, 15 May 2019 21:55:36 +0000
changeset 532830 afaf68ba6e54f8309317b3d346396178fde23fb7
parent 532829 14e4705002fef497e1d7d9f92bac75360b81945b
child 532831 bf04bcac450c62314cd6d4deed2b3629651c54e4
push id11272
push userapavel@mozilla.com
push dateThu, 16 May 2019 15:28:22 +0000
treeherdermozilla-beta@2265bfc5920d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrstrong
bugs1546957
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1546957 - Prevent BITS Requests from being cancelled twice r=rstrong Differential Revision: https://phabricator.services.mozilla.com/D31338
toolkit/mozapps/update/UpdateService.jsm
--- a/toolkit/mozapps/update/UpdateService.jsm
+++ b/toolkit/mozapps/update/UpdateService.jsm
@@ -3683,16 +3683,26 @@ Downloader.prototype = {
   /**
    * We get the nsIRequest from nsIBITS asynchronously. When downloadUpdate has
    * been called, but this._request is not yet valid, _pendingRequest will be
    * a promise that will resolve when this._request has been set.
    */
   _pendingRequest: null,
 
   /**
+   * When using BITS, cancel actions happen asynchronously. This variable
+   * keeps track of any cancel action that is in-progress.
+   * If the cancel action fails, this will be set back to null so that the
+   * action can be attempted again. But if the cancel action succeeds, the
+   * resolved promise will remain stored in this variable to prevent cancel
+   * from being called twice (which, for BITS, is an error).
+   */
+  _cancelPromise: null,
+
+  /**
    * BITS receives progress notifications slowly, unless a user is watching.
    * This tracks what frequency notifications are happening at.
    *
    * This is needed because BITS downloads are started asynchronously.
    * Specifically, this is needed to prevent a situation where the download is
    * still starting (Downloader._pendingRequest has not resolved) when the first
    * observer registers itself. Without this variable, there is no way of
    * knowing whether the download was started as Active or Idle and, therefore,
@@ -3720,25 +3730,42 @@ Downloader.prototype = {
    * data around to allow the transfer to be resumed later.
    */
   cancel: async function Downloader_cancel(cancelError) {
     LOG("Downloader: cancel");
     if (cancelError === undefined) {
       cancelError = Cr.NS_BINDING_ABORTED;
     }
     if (this.usingBits) {
+      // If a cancel action is already in progress, just return when that
+      // promise resolved. Trying to cancel the same request twice is an error.
+      if (this._cancelPromise) {
+        await this._cancelPromise;
+        return;
+      }
+
       if (this._pendingRequest) {
         await this._pendingRequest;
       }
       if (this._patch.getProperty("bitsId") != null) {
         // Make sure that we don't try to resume this download after it was
         // cancelled.
         this._patch.deleteProperty("bitsId");
       }
-      await this._request.cancelAsync(cancelError);
+      try {
+        this._cancelPromise = this._request.cancelAsync(cancelError);
+        await this._cancelPromise;
+      } catch (e) {
+        // On success, we will not set the cancel promise to null because
+        // we want to prevent two cancellations of the same request. But
+        // retrying after a failed cancel is not an error, so we will set the
+        // cancel promise to null in the failure case.
+        this._cancelPromise = null;
+        throw e;
+      }
     } else if (this._request && this._request instanceof Ci.nsIRequest) {
       this._request.cancel(cancelError);
     }
   },
 
   /**
    * Whether or not a patch has been downloaded and staged for installation.
    */
@@ -4003,16 +4030,18 @@ Downloader.prototype = {
 
       let updateRootDir = FileUtils.getDir("UpdRootD", [], true);
       let jobName = "MozillaUpdate " + updateRootDir.leafName;
       let updatePath = updateDir.path;
       if (!Bits.initialized) {
         Bits.init(jobName, updatePath, monitorTimeout);
       }
 
+      this._cancelPromise = null;
+
       let bitsId = this._patch.getProperty("bitsId");
       if (bitsId) {
         LOG("Downloader:downloadUpdate - Connecting to in-progress download. " +
             "BITS ID: " + bitsId);
 
         this._pendingRequest = Bits.monitorDownload(bitsId, monitorInterval,
                                                     this, null);
       } else {
@@ -4322,17 +4351,17 @@ Downloader.prototype = {
               e);
           status = Cr.NS_ERROR_FAILURE;
           bitsCompletionError = e;
         }
       } else {
         // BITS jobs that failed to complete should still have cancel called on
         // them to remove the job.
         try {
-          await request.cancelAsync();
+          await this.cancel();
         } catch (e) {
           // This will fail if the job stopped because it was cancelled.
           // Even if this is a "real" error, there isn't really anything to do
           // about it, and it's not really a big problem. It just means that the
           // BITS job will stay around until it is removed automatically
           // (default of 90 days).
         }
       }