Bug 1348062 - Test that channels are properly marked for downloads. r=mak draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Sun, 16 Apr 2017 15:59:27 +0100
changeset 563366 24e89ea8bcbfd171797ff0a5c008bb593a869970
parent 563365 744c15cf2fcb20a98eff7e9532cc595413189cc9
child 624443 b53fb7eb7880ea5b7d5e86182d696d7a12f628fc
push id54266
push userpaolo.mozmail@amadzone.org
push dateSun, 16 Apr 2017 15:00:06 +0000
reviewersmak
bugs1348062
milestone55.0a1
Bug 1348062 - Test that channels are properly marked for downloads. r=mak Downloads handled by nsIExternalHelperAppService pass a null request to the final onStateChange notification, thus we need to hold a reference to the request earlier. This also allows unit tests to access the request while the download is running. MozReview-Commit-ID: 21rapDmMIZw
toolkit/components/jsdownloads/src/DownloadCore.jsm
toolkit/components/jsdownloads/src/DownloadLegacy.js
toolkit/components/jsdownloads/test/unit/common_test_Download.js
--- a/toolkit/components/jsdownloads/src/DownloadCore.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ -2406,16 +2406,20 @@ this.DownloadLegacySaver.prototype = {
    * @param aAlreadyAddedToHistory
    *        Indicates that the nsIExternalHelperAppService component already
    *        added the download to the browsing history, unless it was started
    *        from a private browsing window.  When this parameter is false, the
    *        download is added to the browsing history here.  Private downloads
    *        are never added to history even if this parameter is false.
    */
   onTransferStarted(aRequest, aAlreadyAddedToHistory) {
+    // Store a reference to the request, used in some cases when handling
+    // completion, and also checked during the download by unit tests.
+    this.request = aRequest;
+
     // Store the entity ID to use for resuming if required.
     if (this.download.tryToKeepPartialData &&
         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) ||
@@ -2433,25 +2437,20 @@ this.DownloadLegacySaver.prototype = {
     if (!aAlreadyAddedToHistory) {
       this.addToHistory();
     }
   },
 
   /**
    * Called by the nsITransfer implementation when the request has finished.
    *
-   * @param aRequest
-   *        nsIRequest associated to the status update.
    * @param aStatus
    *        Status code received by the nsITransfer implementation.
    */
-  onTransferFinished: function DLS_onTransferFinished(aRequest, aStatus) {
-    // Store a reference to the request, used when handling completion.
-    this.request = aRequest;
-
+  onTransferFinished: function DLS_onTransferFinished(aStatus) {
     if (Components.isSuccessCode(aStatus)) {
       this.deferExecuted.resolve();
     } else {
       // Infer the origin of the error from the failure code, because more
       // specific data is not available through the nsITransfer implementation.
       let properties = { result: aStatus, inferCause: true };
       this.deferExecuted.reject(new DownloadError(properties));
     }
--- a/toolkit/components/jsdownloads/src/DownloadLegacy.js
+++ b/toolkit/components/jsdownloads/src/DownloadLegacy.js
@@ -122,17 +122,17 @@ DownloadLegacyTransfer.prototype = {
         // notifying that the download has been canceled.  Since the request has
         // not completed yet, deferCanceled is guaranteed to be set.
         return download.saver.deferCanceled.promise.then(() => {
           // Only cancel if the object executing the download is still running.
           if (this._cancelable && !this._componentFailed) {
             this._cancelable.cancel(Cr.NS_ERROR_ABORT);
             if (this._cancelable instanceof Ci.nsIWebBrowserPersist) {
               // This component will not send the STATE_STOP notification.
-              download.saver.onTransferFinished(aRequest, Cr.NS_ERROR_ABORT);
+              download.saver.onTransferFinished(Cr.NS_ERROR_ABORT);
               this._cancelable = null;
             }
           }
         });
       }).then(null, Cu.reportError);
     } else if ((aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) &&
         (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK)) {
       // The last file has been received, or the download failed.  Wait for the
@@ -140,17 +140,17 @@ DownloadLegacyTransfer.prototype = {
       this._deferDownload.promise.then(download => {
         // At this point, the hash has been set and we need to copy it to the
         // DownloadSaver.
         if (Components.isSuccessCode(aStatus)) {
           download.saver.setSha256Hash(this._sha256Hash);
           download.saver.setSignatureInfo(this._signatureInfo);
           download.saver.setRedirects(this._redirects);
         }
-        download.saver.onTransferFinished(aRequest, aStatus);
+        download.saver.onTransferFinished(aStatus);
       }).then(null, Cu.reportError);
 
       // Release the reference to the component executing the download.
       this._cancelable = null;
     }
   },
 
   onProgressChange: function DLT_onProgressChange(aWebProgress, aRequest,
@@ -170,17 +170,17 @@ DownloadLegacyTransfer.prototype = {
     // The status change may optionally be received in addition to the state
     // change, but if no network request actually started, it is possible that
     // we only receive a status change with an error status code.
     if (!Components.isSuccessCode(aStatus)) {
       this._componentFailed = true;
 
       // Wait for the associated Download object to be available.
       this._deferDownload.promise.then(function DLT_OSC_onDownload(aDownload) {
-        aDownload.saver.onTransferFinished(aRequest, aStatus);
+        aDownload.saver.onTransferFinished(aStatus);
       }).then(null, Cu.reportError);
     }
   },
 
   onSecurityChange() { },
 
   // nsIWebProgressListener2
 
--- a/toolkit/components/jsdownloads/test/unit/common_test_Download.js
+++ b/toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ -225,16 +225,48 @@ add_task(function* test_basic_tryToKeepP
 
   // The target file should now have been created, and the ".part" file deleted.
   yield promiseVerifyTarget(download.target, TEST_DATA_SHORT + TEST_DATA_SHORT);
   do_check_false(yield OS.File.exists(download.target.partFilePath));
   do_check_eq(32, download.saver.getSha256Hash().length);
 });
 
 /**
+ * Tests that the channelIsForDownload property is set for the network request.
+ */
+add_task(function* test_channelIsForDownload() {
+  let downloadChannel = null;
+
+  // We use a different method based on whether we are testing legacy downloads.
+  if (!gUseLegacySaver) {
+    let download = yield Downloads.createDownload({
+      source: {
+        url: httpUrl("interruptible_resumable.txt"),
+        async adjustChannel(channel) {
+          downloadChannel = channel;
+        },
+      },
+      target: getTempFile(TEST_TARGET_FILE_NAME).path,
+    });
+    yield download.start();
+  } else {
+    // Start a download using nsIExternalHelperAppService, but ensure it cannot
+    // finish before we retrieve the "request" property.
+    mustInterruptResponses();
+    let download = yield promiseStartExternalHelperAppServiceDownload();
+    downloadChannel = download.saver.request;
+    continueResponses();
+    yield promiseDownloadStopped(download);
+  }
+
+  do_check_true(downloadChannel.QueryInterface(Ci.nsIHttpChannelInternal)
+                               .channelIsForDownload);
+});
+
+/**
  * Tests the permissions of the final target file once the download finished.
  */
 add_task(function* test_unix_permissions() {
   // This test is only executed on some Desktop systems.
   if (Services.appinfo.OS != "Darwin" && Services.appinfo.OS != "Linux" &&
       Services.appinfo.OS != "WINNT") {
     do_print("Skipping test.");
     return;