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 174117 f785bf2e2c5177c2465dcefa211281aa1c32839b
parent 174116 bc9014b4439a89f70ce975bfb8b72802bdb18bda
child 174118 e6b278c315bcb08d8b9c8884d501f9fae7df0889
push id445
push userffxbld
push dateMon, 10 Mar 2014 22:05:19 +0000
treeherdermozilla-release@dc38b741b04e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersenn
bugs941726
milestone28.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 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);
   },