Bug 908240 - Legacy downloads not executed by nsIHelperAppLauncher should be added to history. r=enn
☠☠ backed out by 94d10f159d3a ☠ ☠
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Mon, 09 Sep 2013 15:45:29 +0200
changeset 146209 493dd25e60c2a89ba9630089487453c9a74eb9ec
parent 146208 7d73a40c6ee92f5bbc9975eee13b046554583135
child 146210 cca2a6def878aa90a9b69160fda4422ef45da250
push id25245
push userryanvm@gmail.com
push dateMon, 09 Sep 2013 20:57:55 +0000
treeherdermozilla-central@a468b2e34b04 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersenn
bugs908240
milestone26.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 908240 - Legacy downloads not executed by nsIHelperAppLauncher should be added to history. r=enn
toolkit/components/jsdownloads/src/DownloadCore.jsm
toolkit/components/jsdownloads/src/DownloadLegacy.js
toolkit/components/jsdownloads/test/unit/common_test_Download.js
toolkit/components/jsdownloads/test/unit/head.js
toolkit/components/jsdownloads/test/unit/test_DownloadList.js
--- a/toolkit/components/jsdownloads/src/DownloadCore.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ -62,16 +62,19 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "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");
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
 
+XPCOMUtils.defineLazyServiceGetter(this, "gDownloadHistory",
+           "@mozilla.org/browser/download-history;1",
+           Ci.nsIDownloadHistory);
 XPCOMUtils.defineLazyServiceGetter(this, "gExternalHelperAppService",
            "@mozilla.org/uriloader/external-helper-app-service;1",
            Ci.nsIExternalHelperAppService);
 
 const BackgroundFileSaverStreamListener = Components.Constructor(
       "@mozilla.org/network/background-file-saver;1?mode=streamlistener",
       "nsIBackgroundFileSaver");
 
@@ -1201,16 +1204,40 @@ DownloadSaver.prototype = {
    * @rejects JavaScript exception.
    */
   removePartialData: function DS_removePartialData()
   {
     return Promise.resolve();
   },
 
   /**
+   * 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: function ()
+  {
+    if (this.download.source.isPrivate) {
+      return;
+    }
+
+    let sourceUri = NetUtil.newURI(this.download.source.url);
+    let referrer = this.download.source.referrer;
+    let referrerUri = referrer ? NetUtil.newURI(referrer) : null;
+    let targetUri = NetUtil.newURI(new FileUtils.File(
+                                       this.download.target.path));
+
+    // The start time is always available when we reach this point.
+    let startPRTime = this.download.startTime.getTime() * 1000;
+
+    gDownloadHistory.addDownload(sourceUri, referrerUri, startPRTime,
+                                 targetUri);
+  },
+
+  /**
    * 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.");
   },
@@ -1262,16 +1289,21 @@ DownloadCopySaver.prototype = {
   /**
    * Indicates whether the "cancel" method has been called.  This is used to
    * prevent the request from starting in case the operation is canceled before
    * the BackgroundFileSaver instance has been created.
    */
   _canceled: false,
 
   /**
+   * True if the associated download has already been added to browsing history.
+   */
+  alreadyAddedToHistory: false,
+
+  /**
    * String corresponding to the entityID property of the nsIResumableChannel
    * 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".
@@ -1283,16 +1315,26 @@ DownloadCopySaver.prototype = {
     this._canceled = false;
 
     let download = this.download;
     let targetPath = download.target.path;
     let partFilePath = download.target.partFilePath;
     let keepPartialData = download.tryToKeepPartialData;
 
     return Task.spawn(function task_DCS_execute() {
+      // 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 = yield OS.File.open(targetPath, { write: true });
@@ -1628,28 +1670,43 @@ DownloadLegacySaver.prototype = {
    */
   progressWasNotified: false,
 
   /**
    * Called by the nsITransfer implementation when the request has started.
    *
    * @param aRequest
    *        nsIRequest associated to the status update.
+   * @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: function (aRequest)
+  onTransferStarted: function (aRequest, aAlreadyAddedToHistory)
   {
     // 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 &&
                      ex.result == Cr.NS_ERROR_NOT_RESUMABLE) { }
     }
+
+    // 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();
+    }
   },
 
   /**
    * Called by the nsITransfer implementation when the request has finished.
    *
    * @param aRequest
    *        nsIRequest associated to the status update.
    * @param aStatus
@@ -1697,16 +1754,17 @@ DownloadLegacySaver.prototype = {
   {
     // 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;
       }
       return this.copySaver.execute.apply(this.copySaver, arguments);
     }
 
     this.setProgressBytesFn = aSetProgressBytesFn;
 
     return Task.spawn(function task_DLS_execute() {
       try {
--- a/toolkit/components/jsdownloads/src/DownloadLegacy.js
+++ b/toolkit/components/jsdownloads/src/DownloadLegacy.js
@@ -86,26 +86,31 @@ DownloadLegacyTransfer.prototype = {
     if (!Components.isSuccessCode(aStatus)) {
       this._componentFailed = true;
     }
 
     if ((aStateFlags & Ci.nsIWebProgressListener.STATE_START) &&
         (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK)) {
       // The main request has just started.  Wait for the associated Download
       // object to be available before notifying.
-      this._deferDownload.promise.then(function (aDownload) {
-        aDownload.saver.onTransferStarted(aRequest);
+      this._deferDownload.promise.then(download => {
+        download.saver.onTransferStarted(
+                         aRequest,
+                         this._cancelable instanceof Ci.nsIHelperAppLauncher);
       }).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
       // associated Download object to be available before notifying.
-      this._deferDownload.promise.then(function DLT_OSC_onDownload(aDownload) {
-        aDownload.saver.onTransferFinished(aRequest, aStatus);
+      this._deferDownload.promise.then(download => {
+        download.saver.onTransferFinished(aRequest, aStatus);
       }).then(null, Cu.reportError);
+
+      // Release the reference to the component executing the download.
+      this._cancelable = null;
     }
   },
 
   onProgressChange: function DLT_onProgressChange(aWebProgress, aRequest,
                                                   aCurSelfProgress,
                                                   aMaxSelfProgress,
                                                   aCurTotalProgress,
                                                   aMaxTotalProgress)
@@ -159,16 +164,18 @@ DownloadLegacyTransfer.prototype = {
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// nsITransfer
 
   init: function DLT_init(aSource, aTarget, aDisplayName, aMIMEInfo, aStartTime,
                           aTempFile, aCancelable, aIsPrivate)
   {
+    this._cancelable = aCancelable;
+
     let launchWhenSucceeded = false, contentType = null, launcherPath = null;
 
     if (aMIMEInfo instanceof Ci.nsIMIMEInfo) {
       launchWhenSucceeded =
                 aMIMEInfo.preferredAction != Ci.nsIMIMEInfo.saveToDisk;
       contentType = aMIMEInfo.type;
 
       let appHandler = aMIMEInfo.preferredApplicationHandler;
@@ -229,16 +236,22 @@ DownloadLegacyTransfer.prototype = {
 
   /**
    * This deferred object contains a promise that is resolved with the Download
    * object associated with this nsITransfer instance, when it is available.
    */
   _deferDownload: null,
 
   /**
+   * Reference to the component that is executing the download.  This component
+   * allows cancellation through its nsICancelable interface.
+   */
+  _cancelable: null,
+
+  /**
    * Indicates that the component that executes the download has notified a
    * failure condition.  In this case, we should never use the component methods
    * that cancel the download.
    */
   _componentFailed: false,
 };
 
 ////////////////////////////////////////////////////////////////////////////////
--- a/toolkit/components/jsdownloads/test/unit/common_test_Download.js
+++ b/toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ -1671,8 +1671,65 @@ add_task(function test_platform_integrat
     });
 
     // Then, wait for the promise returned by "start" to be resolved.
     yield promiseDownloadStopped(download);
 
     yield promiseVerifyContents(download.target.path, TEST_DATA_SHORT);
   }
 });
+
+/**
+ * Checks that downloads are added to browsing history when they start.
+ */
+add_task(function test_history()
+{
+  mustInterruptResponses();
+
+  // We will wait for the visit to be notified during the download.
+  yield promiseClearHistory();
+  let promiseVisit = promiseWaitForVisit(httpUrl("interruptible.txt"));
+
+  // Start a download that is not allowed to finish yet.
+  let download = yield promiseStartDownload(httpUrl("interruptible.txt"));
+
+  // The history notifications should be received before the download completes.
+  let [time, transitionType] = yield promiseVisit;
+  do_check_eq(time, download.startTime.getTime() * 1000);
+  do_check_eq(transitionType, Ci.nsINavHistoryService.TRANSITION_DOWNLOAD);
+
+  // Restart and complete the download after clearing history.
+  yield promiseClearHistory();
+  download.cancel();
+  continueResponses();
+  yield download.start();
+
+  // The restart should not have added a new history visit.
+  do_check_false(yield promiseIsURIVisited(httpUrl("interruptible.txt")));
+});
+
+/**
+ * Checks that downloads started by nsIHelperAppService are added to the
+ * browsing history when they start.
+ */
+add_task(function test_history_tryToKeepPartialData()
+{
+  // We will wait for the visit to be notified during the download.
+  yield promiseClearHistory();
+  let promiseVisit =
+      promiseWaitForVisit(httpUrl("interruptible_resumable.txt"));
+
+  // Start a download that is not allowed to finish yet.
+  let beforeStartTimeMs = Date.now();
+  let download = yield promiseStartDownload_tryToKeepPartialData();
+
+  // The history notifications should be received before the download completes.
+  let [time, transitionType] = yield promiseVisit;
+  do_check_eq(transitionType, Ci.nsINavHistoryService.TRANSITION_DOWNLOAD);
+
+  // The time set by nsIHelperAppService may be different than the start time in
+  // the download object, thus we only check that it is a meaningful time.
+  do_check_true(time >= beforeStartTimeMs * 1000);
+
+  // Complete the download before finishing the test.
+  continueResponses();
+  yield promiseDownloadStopped(download);
+});
--- a/toolkit/components/jsdownloads/test/unit/head.js
+++ b/toolkit/components/jsdownloads/test/unit/head.js
@@ -165,16 +165,111 @@ function promiseExecuteSoon()
 function promiseTimeout(aTime)
 {
   let deferred = Promise.defer();
   do_timeout(aTime, deferred.resolve);
   return deferred.promise;
 }
 
 /**
+ * Allows waiting for an observer notification once.
+ *
+ * @param aTopic
+ *        Notification topic to observe.
+ *
+ * @return {Promise}
+ * @resolves The array [aSubject, aData] from the observed notification.
+ * @rejects Never.
+ */
+function promiseTopicObserved(aTopic)
+{
+  let deferred = Promise.defer();
+
+  Services.obs.addObserver(
+    function PTO_observe(aSubject, aTopic, aData) {
+      Services.obs.removeObserver(PTO_observe, aTopic);
+      deferred.resolve([aSubject, aData]);
+    }, aTopic, false);
+
+  return deferred.promise;
+}
+
+/**
+ * Clears history asynchronously.
+ *
+ * @return {Promise}
+ * @resolves When history has been cleared.
+ * @rejects Never.
+ */
+function promiseClearHistory()
+{
+  let promise = promiseTopicObserved(PlacesUtils.TOPIC_EXPIRATION_FINISHED);
+  do_execute_soon(function() PlacesUtils.bhistory.removeAllPages());
+  return promise;
+}
+
+/**
+ * Waits for a new history visit to be notified for the specified URI.
+ *
+ * @param aUrl
+ *        String containing the URI that will be visited.
+ *
+ * @return {Promise}
+ * @resolves Array [aTime, aTransitionType] from nsINavHistoryObserver.onVisit.
+ * @rejects Never.
+ */
+function promiseWaitForVisit(aUrl)
+{
+  let deferred = Promise.defer();
+
+  let uri = NetUtil.newURI(aUrl);
+
+  PlacesUtils.history.addObserver({
+    QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver]),
+    onBeginUpdateBatch: function () {},
+    onEndUpdateBatch: function () {},
+    onVisit: function (aURI, aVisitID, aTime, aSessionID, aReferringID,
+                       aTransitionType, aGUID, aHidden) {
+      if (aURI.equals(uri)) {
+        PlacesUtils.history.removeObserver(this);
+        deferred.resolve([aTime, aTransitionType]);
+      }
+    },
+    onTitleChanged: function () {},
+    onDeleteURI: function () {},
+    onClearHistory: function () {},
+    onPageChanged: function () {},
+    onDeleteVisits: function () {},
+  }, false);
+
+  return deferred.promise;
+}
+
+/**
+ * Check browsing history to see whether the given URI has been visited.
+ *
+ * @param aUrl
+ *        String containing the URI that will be visited.
+ *
+ * @return {Promise}
+ * @resolves Boolean indicating whether the URI has been visited.
+ * @rejects JavaScript exception.
+ */
+function promiseIsURIVisited(aUrl) {
+  let deferred = Promise.defer();
+
+  PlacesUtils.asyncHistory.isURIVisited(NetUtil.newURI(aUrl),
+    function (aURI, aIsVisited) {
+      deferred.resolve(aIsVisited);
+    });
+
+  return deferred.promise;
+}
+
+/**
  * Creates a new Download object, setting a temporary file as the target.
  *
  * @param aSourceUrl
  *        String containing the URI for the download source, or null to use
  *        httpUrl("source.txt").
  *
  * @return {Promise}
  * @resolves The newly created Download object.
@@ -437,50 +532,16 @@ function promiseVerifyContents(aPath, aE
       }
       deferred.resolve();
     });
     yield deferred.promise;
   });
 }
 
 /**
- * Adds entry for download.
- *
- * @param aSourceUrl
- *        String containing the URI for the download source, or null to use
- *        httpUrl("source.txt").
- *
- * @return {Promise}
- * @rejects JavaScript exception.
- */
-function promiseAddDownloadToHistory(aSourceUrl) {
-  let deferred = Promise.defer();
-  PlacesUtils.asyncHistory.updatePlaces(
-    {
-      uri: NetUtil.newURI(aSourceUrl || httpUrl("source.txt")),
-      visits: [{
-        transitionType: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD,
-        visitDate:  Date.now()
-      }]
-    },
-    {
-      handleError: function handleError(aResultCode, aPlaceInfo) {
-        let ex = new Components.Exception("Unexpected error in adding visits.",
-                                          aResultCode);
-        deferred.reject(ex);
-      },
-      handleResult: function () {},
-      handleCompletion: function handleCompletion() {
-        deferred.resolve();
-      }
-    });
-  return deferred.promise;
-}
-
-/**
  * Starts a socket listener that closes each incoming connection.
  *
  * @returns nsIServerSocket that listens for connections.  Call its "close"
  *          method to stop listening and free the server port.
  */
 function startFakeServer()
 {
   let serverSocket = new ServerSocket(-1, true, -1);
--- a/toolkit/components/jsdownloads/test/unit/test_DownloadList.js
+++ b/toolkit/components/jsdownloads/test/unit/test_DownloadList.js
@@ -5,16 +5,72 @@
 
 /**
  * Tests the DownloadList object.
  */
 
 "use strict";
 
 ////////////////////////////////////////////////////////////////////////////////
+//// Globals
+
+/**
+ * Returns a PRTime in the past usable to add expirable visits.
+ *
+ * @note Expiration ignores any visit added in the last 7 days, but it's
+ *       better be safe against DST issues, by going back one day more.
+ */
+function getExpirablePRTime()
+{
+  let dateObj = new Date();
+  // Normalize to midnight
+  dateObj.setHours(0);
+  dateObj.setMinutes(0);
+  dateObj.setSeconds(0);
+  dateObj.setMilliseconds(0);
+  dateObj = new Date(dateObj.getTime() - 8 * 86400000);
+  return dateObj.getTime() * 1000;
+}
+
+/**
+ * Adds an expirable history visit for a download.
+ *
+ * @param aSourceUrl
+ *        String containing the URI for the download source, or null to use
+ *        httpUrl("source.txt").
+ *
+ * @return {Promise}
+ * @rejects JavaScript exception.
+ */
+function promiseExpirableDownloadVisit(aSourceUrl)
+{
+  let deferred = Promise.defer();
+  PlacesUtils.asyncHistory.updatePlaces(
+    {
+      uri: NetUtil.newURI(aSourceUrl || httpUrl("source.txt")),
+      visits: [{
+        transitionType: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD,
+        visitDate: getExpirablePRTime(),
+      }]
+    },
+    {
+      handleError: function handleError(aResultCode, aPlaceInfo) {
+        let ex = new Components.Exception("Unexpected error in adding visits.",
+                                          aResultCode);
+        deferred.reject(ex);
+      },
+      handleResult: function () {},
+      handleCompletion: function handleCompletion() {
+        deferred.resolve();
+      }
+    });
+  return deferred.promise;
+}
+
+////////////////////////////////////////////////////////////////////////////////
 //// Tests
 
 /**
  * Checks the testing mechanism used to build different download lists.
  */
 add_task(function test_construction()
 {
   let downloadListOne = yield promiseNewDownloadList();
@@ -200,71 +256,72 @@ add_task(function test_notifications_thi
   do_check_true(receivedOnDownloadRemoved);
 });
 
 /**
  * Checks that download is removed on history expiration.
  */
 add_task(function test_history_expiration()
 {
+  mustInterruptResponses();
+
   function cleanup() {
     Services.prefs.clearUserPref("places.history.expiration.max_pages");
   }
   do_register_cleanup(cleanup);
 
   // Set max pages to 0 to make the download expire.
   Services.prefs.setIntPref("places.history.expiration.max_pages", 0);
 
-  // Add expirable visit for downloads.
-  yield promiseAddDownloadToHistory();
-  yield promiseAddDownloadToHistory(httpUrl("interruptible.txt"));
-
   let list = yield promiseNewDownloadList();
   let downloadOne = yield promiseNewDownload();
   let downloadTwo = yield promiseNewDownload(httpUrl("interruptible.txt"));
-  list.add(downloadOne);
-  list.add(downloadTwo);
 
   let deferred = Promise.defer();
   let removeNotifications = 0;
   let downloadView = {
     onDownloadRemoved: function (aDownload) {
       if (++removeNotifications == 2) {
         deferred.resolve();
       }
     },
   };
   list.addView(downloadView);
 
-  // Start download one.
+  // Work with one finished download and one canceled download.
   yield downloadOne.start();
-
-  // Start download two and then cancel it.
   downloadTwo.start();
   yield downloadTwo.cancel();
 
+  // We must replace the visits added while executing the downloads with visits
+  // that are older than 7 days, otherwise they will not be expired.
+  yield promiseClearHistory();
+  yield promiseExpirableDownloadVisit();
+  yield promiseExpirableDownloadVisit(httpUrl("interruptible.txt"));
+
+  // After clearing history, we can add the downloads to be removed to the list.
+  list.add(downloadOne);
+  list.add(downloadTwo);
+
   // Force a history expiration.
   let expire = Cc["@mozilla.org/places/expiration;1"]
                  .getService(Ci.nsIObserver);
   expire.observe(null, "places-debug-start-expiration", -1);
 
+  // Wait for both downloads to be removed.
   yield deferred.promise;
 
   cleanup();
 });
 
 /**
  * Checks all downloads are removed after clearing history.
  */
 add_task(function test_history_clear()
 {
-  // Add expirable visit for downloads.
-  yield promiseAddDownloadToHistory();
-  yield promiseAddDownloadToHistory();
-
   let list = yield promiseNewDownloadList();
   let downloadOne = yield promiseNewDownload();
   let downloadTwo = yield promiseNewDownload();
   list.add(downloadOne);
   list.add(downloadTwo);
 
   let deferred = Promise.defer();
   let removeNotifications = 0;
@@ -275,18 +332,19 @@ add_task(function test_history_clear()
       }
     },
   };
   list.addView(downloadView);
 
   yield downloadOne.start();
   yield downloadTwo.start();
 
-  PlacesUtils.history.removeAllPages();
+  yield promiseClearHistory();
 
+  // Wait for the removal notifications that may still be pending.
   yield deferred.promise;
 });
 
 /**
  * Tests the removeFinished method to ensure that it only removes
  * finished downloads.
  */
 add_task(function test_removeFinished()