Bug 941726 - Prevent DownloadLegacy from keeping a reference to the network request after the download is stopped. r=enn
☠☠ backed out by 59228cb34257 ☠ ☠
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Tue, 03 Dec 2013 10:15:22 +0100
changeset 158477 f785bf2e2c5177c2465dcefa211281aa1c32839b
parent 158476 bc9014b4439a89f70ce975bfb8b72802bdb18bda
child 158478 e6b278c315bcb08d8b9c8884d501f9fae7df0889
push idunknown
push userunknown
push dateunknown
reviewersenn
bugs941726
milestone28.0a1
Bug 941726 - Prevent DownloadLegacy from keeping a reference to the network request after the download is stopped. r=enn
toolkit/components/jsdownloads/src/DownloadCore.jsm
toolkit/components/jsdownloads/src/DownloadLegacy.js
--- a/toolkit/components/jsdownloads/src/DownloadCore.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ -2038,38 +2038,48 @@ this.DownloadLegacySaver.prototype = {
           // 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 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;
       }
     }.bind(this));
   },
 
   /**
    * Implements "DownloadSaver.cancel".
    */
   cancel: function DLS_cancel()
   {
     // We may be using a DownloadCopySaver to handle resuming.
     if (this.copySaver) {
       return this.copySaver.cancel.apply(this.copySaver, arguments);
     }
 
-    // Cancel the operation as soon as the object is connected.
-    this.deferCanceled.resolve();
+    // If the download hasn't stopped already, resolve deferCanceled so that the
+    // 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".
    */
   removePartialData: function ()
   {
     // DownloadCopySaver and DownloadLeagcySaver use the same logic for removing
--- a/toolkit/components/jsdownloads/src/DownloadLegacy.js
+++ b/toolkit/components/jsdownloads/src/DownloadLegacy.js
@@ -110,17 +110,18 @@ DownloadLegacyTransfer.prototype = {
 
         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.
+        // 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);
               this._cancelable = null;
@@ -234,21 +235,18 @@ DownloadLegacyTransfer.prototype = {
       contentType: contentType,
       launcherPath: launcherPath
     }).then(function DLT_I_onDownload(aDownload) {
       // Legacy components keep partial data when they use a ".part" file.
       if (aTempFile) {
         aDownload.tryToKeepPartialData = true;
       }
 
-      // Start the download before allowing it to be controlled.
-      aDownload.start().then(null, function () {
-        // In case the operation failed, ensure we stop downloading data.
-        aDownload.saver.deferCanceled.resolve();
-      });
+      // Start the download before allowing it to be controlled.  Ignore errors.
+      aDownload.start().then(null, () => {});
 
       // Start processing all the other events received through nsITransfer.
       this._deferDownload.resolve(aDownload);
 
       // Add the download to the list, allowing it to be seen and canceled.
       return Downloads.getList(Downloads.ALL).then(list => list.add(aDownload));
     }.bind(this)).then(null, Cu.reportError);
   },