Bug 941317 - Switch Downloads.jsm to use Promise.jsm. r=enn
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Mon, 17 Mar 2014 15:58:44 +0100
changeset 173953 00e095a5bf18528b1dd8a836e8fd760e33c94a9c
parent 173952 ef4ee2255eebe629dc932e62ee20f43c0714e86b
child 173954 8b8de1d6a3f059e0ce13d8fc1ba9e57d485088a6
push id41140
push userryanvm@gmail.com
push dateMon, 17 Mar 2014 21:33:33 +0000
treeherdermozilla-inbound@dc2225034769 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersenn
bugs941317
milestone30.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 941317 - Switch Downloads.jsm to use Promise.jsm. r=enn
toolkit/components/jsdownloads/src/DownloadCore.jsm
toolkit/components/jsdownloads/src/DownloadIntegration.jsm
toolkit/components/jsdownloads/src/DownloadLegacy.js
toolkit/components/jsdownloads/src/DownloadList.jsm
toolkit/components/jsdownloads/src/DownloadUIHelper.jsm
toolkit/components/jsdownloads/src/Downloads.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
@@ -58,17 +58,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/DownloadIntegration.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "FileUtils",
                                   "resource://gre/modules/FileUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
                                   "resource://gre/modules/NetUtil.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "OS",
                                   "resource://gre/modules/osfile.jsm")
 XPCOMUtils.defineLazyModuleGetter(this, "Promise",
-                                  "resource://gre/modules/commonjs/sdk/core/promise.js");
+                                  "resource://gre/modules/Promise.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                   "resource://gre/modules/Services.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
 
 XPCOMUtils.defineLazyServiceGetter(this, "gDownloadHistory",
            "@mozilla.org/browser/download-history;1",
            Ci.nsIDownloadHistory);
@@ -406,37 +406,58 @@ this.Download.prototype = {
         try {
           yield this._promiseRemovePartialData;
         } catch (ex) {
           // Ignore any errors, which are already reported by the original
           // caller of the removePartialData method.
         }
       }
 
+      // In case the download was restarted while cancellation was in progress,
+      // but the previous attempt actually succeeded before cancellation could
+      // be processed, it is possible that the download has already finished.
+      if (this.succeeded) {
+        return;
+      }
+
       try {
         // Disallow download if parental controls service restricts it.
         if (yield DownloadIntegration.shouldBlockForParentalControls(this)) {
           throw new DownloadError({ becauseBlockedByParentalControls: true });
         }
 
+        // We should check if we have been canceled in the meantime, after all
+        // the previous asynchronous operations have been executed and just
+        // 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;
         yield this.saver.execute(DS_setProgressBytes.bind(this),
                                  DS_setProperties.bind(this));
+
         // Check for application reputation, which requires the entire file to
-        // be downloaded.
-        if (yield DownloadIntegration.shouldBlockForReputationCheck(this)) {
-          // Delete the target file that BackgroundFileSaver already moved
-          // into place.
+        // be downloaded.  After that, check for the last time if the download
+        // has been canceled.  Both cases require the target file to be deleted,
+        // thus we process both in the same block of code.
+        if ((yield DownloadIntegration.shouldBlockForReputationCheck(this)) ||
+            this._promiseCanceled) {
           try {
             yield OS.File.remove(this.target.path);
           } catch (ex) {
             Cu.reportError(ex);
           }
+          // If this is actually a cancellation, this exception will be changed
+          // in the catch block below.
           throw new DownloadError({ becauseBlockedByReputationCheck: true });
         }
+
         // Update the status properties for a successful download.
         this.progress = 100;
         this.succeeded = true;
         this.hasPartialData = false;
       } catch (ex) {
         // Fail with a generic status code on cancellation, so that the caller
         // is forced to actually check the status properties to see if the
         // download was canceled or failed because of other reasons.
@@ -456,16 +477,17 @@ this.Download.prototype = {
         // Update the download error, unless a new attempt already started. The
         // change in the status property is notified in the finally block.
         if (this._currentAttempt == currentAttempt || !this._currentAttempt) {
           this.error = ex;
         }
         throw ex;
       } finally {
         // Any cancellation request has now been processed.
+        this._saverExecuting = false;
         this._promiseCanceled = null;
 
         // Update the status properties, unless a new attempt already started.
         if (this._currentAttempt == currentAttempt || !this._currentAttempt) {
           this._currentAttempt = null;
           this.stopped = true;
           this.speed = 0;
           this._notifyChange();
@@ -542,16 +564,22 @@ this.Download.prototype = {
   /**
    * When a request to cancel the download is received, contains a promise that
    * will be resolved when the cancellation request is processed.  When the
    * request is processed, this property becomes null again.
    */
   _promiseCanceled: null,
 
   /**
+   * True between the call to the "execute" method of the saver and the
+   * completion of the current download attempt.
+   */
+  _saverExecuting: false,
+
+  /**
    * Cancels the download.
    *
    * The cancellation request is asynchronous.  Until the cancellation process
    * finishes, temporary files or part files may still exist even if they are
    * expected to be deleted.
    *
    * In case the download completes successfully before the cancellation request
    * could be processed, this method has no effect, and it returns a resolved
@@ -584,18 +612,22 @@ this.Download.prototype = {
 
       // The download can already be restarted.
       this._currentAttempt = null;
 
       // Notify that the cancellation request was received.
       this.canceled = true;
       this._notifyChange();
 
-      // Execute the actual cancellation through the saver object.
-      this.saver.cancel();
+      // Execute the actual cancellation through the saver object, in case it
+      // has already started.  Otherwise, the cancellation will be handled just
+      // before the saver is started.
+      if (this._saverExecuting) {
+        this.saver.cancel();
+      }
     }
 
     return this._promiseCanceled;
   },
 
   /**
    * Indicates whether any partially downloaded data should be retained, to use
    * when restarting a failed or canceled download.  The default is false.
@@ -1359,41 +1391,16 @@ this.DownloadSaver.prototype = {
     // The start time is always available when we reach this point.
     let startPRTime = this.download.startTime.getTime() * 1000;
 
     gDownloadHistory.addDownload(sourceUri, referrerUri, startPRTime,
                                  targetUri);
   },
 
   /**
-   * Return true if the request's response has been blocked by Windows parental
-   * controls with an HTTP 450 error code.
-   *
-   * @param aRequest
-   *        nsIRequest object
-   * @return True if the response is blocked.
-   */
-  isResponseParentalBlocked: function(aRequest)
-  {
-    // If the HTTP status is 450, then Windows Parental Controls have
-    // blocked this download.
-    if (aRequest instanceof Ci.nsIHttpChannel &&
-        aRequest.responseStatus == 450) {
-      // Cancel the request, but set a flag on the download that can be
-      // retrieved later when handling the cancellation so that the proper
-      // blocked by parental controls error can be thrown.
-      this.download._blockedByParentalControls = true;
-      aRequest.cancel(Cr.NS_BINDING_ABORTED);
-      return true;
-    }
-
-    return false;
-  },
-
-  /**
    * Returns a static representation of the current object state.
    *
    * @return A JavaScript object that can be serialized to JSON.
    */
   toSerializable: function ()
   {
     throw new Error("Not implemented.");
   },
@@ -1607,17 +1614,24 @@ this.DownloadCopySaver.prototype = {
           };
 
           // Open the channel, directing output to the background file saver.
           backgroundFileSaver.QueryInterface(Ci.nsIStreamListener);
           channel.asyncOpen({
             onStartRequest: function (aRequest, aContext) {
               backgroundFileSaver.onStartRequest(aRequest, aContext);
 
-              if (this.isResponseParentalBlocked(aRequest)) {
+              // 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.
@@ -1702,16 +1716,23 @@ this.DownloadCopySaver.prototype = {
             onDataAvailable: function (aRequest, aContext, aInputStream,
                                        aOffset, aCount) {
               backgroundFileSaver.onDataAvailable(aRequest, aContext,
                                                   aInputStream, aOffset,
                                                   aCount);
             }.bind(copySaver),
           }, null);
 
+          // 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." });
+          }
+
           // 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);
           throw ex;
         }
@@ -1925,20 +1946,16 @@ this.DownloadLegacySaver.prototype = {
         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) { }
     }
 
-    if (this.isResponseParentalBlocked(aRequest)) {
-      return;
-    }
-
     // For legacy downloads, we must update the referrer at this time.
     if (aRequest instanceof Ci.nsIHttpChannel && aRequest.referrer) {
       this.download.source.referrer = aRequest.referrer.spec;
     }
 
     if (!aAlreadyAddedToHistory) {
       this.addToHistory();
     }
--- a/toolkit/components/jsdownloads/src/DownloadIntegration.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ -41,17 +41,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/NetUtil.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "OS",
                                   "resource://gre/modules/osfile.jsm");
 #ifdef MOZ_PLACES
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 #endif
 XPCOMUtils.defineLazyModuleGetter(this, "Promise",
-                                  "resource://gre/modules/commonjs/sdk/core/promise.js");
+                                  "resource://gre/modules/Promise.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                   "resource://gre/modules/Services.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
                                   "resource://gre/modules/NetUtil.jsm");
 
 XPCOMUtils.defineLazyServiceGetter(this, "gDownloadPlatform",
--- a/toolkit/components/jsdownloads/src/DownloadLegacy.js
+++ b/toolkit/components/jsdownloads/src/DownloadLegacy.js
@@ -24,17 +24,17 @@ const Ci = Components.interfaces;
 const Cu = Components.utils;
 const Cr = Components.results;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "Downloads",
                                   "resource://gre/modules/Downloads.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Promise",
-                                  "resource://gre/modules/commonjs/sdk/core/promise.js");
+                                  "resource://gre/modules/Promise.jsm");
 
 ////////////////////////////////////////////////////////////////////////////////
 //// DownloadLegacyTransfer
 
 /**
  * nsITransfer implementation that provides a bridge to a Download object.
  *
  * Legacy downloads work differently than the JavaScript implementation.  In the
@@ -84,19 +84,35 @@ DownloadLegacyTransfer.prototype = {
                                             aStatus)
   {
     if (!Components.isSuccessCode(aStatus)) {
       this._componentFailed = true;
     }
 
     if ((aStateFlags & Ci.nsIWebProgressListener.STATE_START) &&
         (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK)) {
+
+      // If the request's response has been blocked by Windows Parental Controls
+      // with an HTTP 450 error code, we must cancel the request synchronously.
+      let blockedByParentalControls = aRequest instanceof Ci.nsIHttpChannel &&
+                                      aRequest.responseStatus == 450;
+      if (blockedByParentalControls) {
+        aRequest.cancel(Cr.NS_BINDING_ABORTED);
+      }
+
       // The main request has just started.  Wait for the associated Download
       // object to be available before notifying.
       this._deferDownload.promise.then(download => {
+        // If the request was blocked, now that we have the download object we
+        // should set a flag that can be retrieved later when handling the
+        // cancellation so that the proper error can be thrown.
+        if (blockedByParentalControls) {
+          download._blockedByParentalControls = true;
+        }
+
         download.saver.onTransferStarted(
                          aRequest,
                          this._cancelable instanceof Ci.nsIHelperAppLauncher);
 
         // To handle asynchronous cancellation properly, we should hook up the
         // handler only after we have been notified that the main request
         // started.  We will wait until the main request stopped before
         // notifying that the download has been canceled.  Since the request has
--- a/toolkit/components/jsdownloads/src/DownloadList.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadList.jsm
@@ -32,17 +32,17 @@ this.EXPORTED_SYMBOLS = [
 const Cc = Components.classes;
 const Ci = Components.interfaces;
 const Cu = Components.utils;
 const Cr = Components.results;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "Promise",
-                                  "resource://gre/modules/commonjs/sdk/core/promise.js");
+                                  "resource://gre/modules/Promise.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
 
 ////////////////////////////////////////////////////////////////////////////////
 //// DownloadList
 
 /**
  * Represents a collection of Download objects that can be viewed and managed by
--- a/toolkit/components/jsdownloads/src/DownloadUIHelper.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadUIHelper.jsm
@@ -22,17 +22,17 @@ const Ci = Components.interfaces;
 const Cu = Components.utils;
 const Cr = Components.results;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "OS",
                                   "resource://gre/modules/osfile.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Promise",
-                                  "resource://gre/modules/commonjs/sdk/core/promise.js");
+                                  "resource://gre/modules/Promise.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                   "resource://gre/modules/Services.jsm");
 
 const kStringBundleUrl =
   "chrome://mozapps/locale/downloads/downloads.properties";
 
 const kStringsRequiringFormatting = {
   fileExecutableSecurityWarning: true,
--- a/toolkit/components/jsdownloads/src/Downloads.jsm
+++ b/toolkit/components/jsdownloads/src/Downloads.jsm
@@ -31,17 +31,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/DownloadIntegration.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "DownloadList",
                                   "resource://gre/modules/DownloadList.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "DownloadSummary",
                                   "resource://gre/modules/DownloadList.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "DownloadUIHelper",
                                   "resource://gre/modules/DownloadUIHelper.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Promise",
-                                  "resource://gre/modules/commonjs/sdk/core/promise.js");
+                                  "resource://gre/modules/Promise.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
 
 ////////////////////////////////////////////////////////////////////////////////
 //// Downloads
 
 /**
  * This object is exposed directly to the consumers of this JavaScript module,
--- a/toolkit/components/jsdownloads/test/unit/common_test_Download.js
+++ b/toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ -801,17 +801,20 @@ add_task(function test_cancel_immediatel
   do_check_eq(download.totalBytes, 0);
   do_check_eq(download.currentBytes, 0);
 
   // Ensure the next request is now allowed to complete, regardless of whether
   // the canceled request was received by the server or not.
   continueResponses();
   try {
     yield promiseAttempt;
-    do_throw("The download should have been canceled.");
+    // If we get here, it means that the first attempt actually succeeded.  In
+    // fact, this could be a valid outcome, because the cancellation request may
+    // not have been processed in time before the download finished.
+    do_print("The download should have been canceled.");
   } catch (ex if ex instanceof Downloads.Error) {
     do_check_false(ex.becauseSourceFailed);
     do_check_false(ex.becauseTargetFailed);
   }
 
   yield promiseRestarted;
 
   do_check_true(download.stopped);
--- a/toolkit/components/jsdownloads/test/unit/head.js
+++ b/toolkit/components/jsdownloads/test/unit/head.js
@@ -29,17 +29,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/FileUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "HttpServer",
                                   "resource://testing-common/httpd.js");
 XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
                                   "resource://gre/modules/NetUtil.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Promise",
-                                  "resource://gre/modules/commonjs/sdk/core/promise.js");
+                                  "resource://gre/modules/Promise.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                   "resource://gre/modules/Services.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "OS",
                                   "resource://gre/modules/osfile.jsm");
 
 XPCOMUtils.defineLazyServiceGetter(this, "gExternalHelperAppService",
@@ -726,16 +726,25 @@ let gMostRecentFirstBytePos;
 
 add_task(function test_common_initialize()
 {
   // Start the HTTP server.
   gHttpServer = new HttpServer();
   gHttpServer.registerDirectory("/", do_get_file("../data"));
   gHttpServer.start(-1);
 
+  // Cache locks might prevent concurrent requests to the same resource, and
+  // this may block tests that use the interruptible handlers.
+  Services.prefs.setBoolPref("browser.cache.disk.enable", false);
+  Services.prefs.setBoolPref("browser.cache.memory.enable", false);
+  do_register_cleanup(function () {
+    Services.prefs.clearUserPref("browser.cache.disk.enable");
+    Services.prefs.clearUserPref("browser.cache.memory.enable");
+  });
+
   registerInterruptibleHandler("/interruptible.txt",
     function firstPart(aRequest, aResponse) {
       aResponse.setHeader("Content-Type", "text/plain", false);
       aResponse.setHeader("Content-Length", "" + (TEST_DATA_SHORT.length * 2),
                           false);
       aResponse.write(TEST_DATA_SHORT);
     }, function secondPart(aRequest, aResponse) {
       aResponse.write(TEST_DATA_SHORT);