Bug 1546287 - Fix leak in BITS update tests r=rstrong
authorKirk Steuber <ksteuber@mozilla.com>
Fri, 03 May 2019 17:37:55 +0000
changeset 531347 adb1c1a79a40102f634a419b6c5d1245de474d67
parent 531346 0bb9da8fe6af8020f0acd8b7fc3a768397f31562
child 531348 9491a79c5266732978352b11340b6150c4021f49
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrstrong
bugs1546287
milestone68.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 1546287 - Fix leak in BITS update tests r=rstrong Differential Revision: https://phabricator.services.mozilla.com/D29751
toolkit/components/bitsdownload/Bits.jsm
toolkit/components/bitsdownload/nsIBits.idl
toolkit/mozapps/update/UpdateService.jsm
toolkit/mozapps/update/tests/browser/browser.bits.ini
--- a/toolkit/components/bitsdownload/Bits.jsm
+++ b/toolkit/components/bitsdownload/Bits.jsm
@@ -183,176 +183,312 @@ async function requestPromise(errorActio
       reject(error);
     }
   });
 }
 
 /**
  * This class is a wrapper around nsIBitsRequest that converts functions taking
  * callbacks to asynchronous functions. This class implements nsIRequest.
+ *
+ * Note that once the request has been shutdown, calling methods on it will
+ * cause an exception to be thrown. The request will be shutdown automatically
+ * when the BITS job is successfully completed or cancelled. If the request is
+ * no longer needed, but the transfer is still in progress, the shutdown method
+ * should be called manually to prevent memory leaks.
+ * Getter methods (except loadGroup and loadFlags) should continue to be
+ * accessible, even after shutdown.
  */
 class BitsRequest {
   constructor(request) {
     this._request = request;
     this._request.QueryInterface(Ci.nsIBitsRequest);
   }
 
   /**
+   * This function releases the Rust request that backs this wrapper. Calling
+   * any method on this request after calling release will result in a BitsError
+   * being thrown.
+   *
+   * This step is important, because otherwise a cycle exists that the cycle
+   * collector doesn't know about. To break this cycle, either the Rust request
+   * needs to let go of the observer, or the JS request wrapper needs to let go
+   * of the Rust request (which is what we do here).
+   *
+   * Once there is support for cycle collection of cycles that extend through
+   * Rust, this function may no longer be necessary.
+   */
+  shutdown() {
+    if (this.hasShutdown) {
+      return;
+    }
+    // Cache some values before we shut down so they are still available
+    this._name = this._request.name;
+    this._status = this._request.status;
+    this._bitsId = this._request.bitsId;
+    this._transferError = this._request.transferError;
+
+    this._request = null;
+  }
+
+  /**
+   * Allows consumers to determine if this request has been shutdown.
+   */
+  get hasShutdown() {
+    return !this._request;
+  }
+
+  /**
    * This is the nsIRequest implementation. Since this._request is an
    * nsIRequest, these functions just call the corresponding method on it.
    *
    * Note that nsIBitsRequest does not yet properly implement load groups or
    * load flags. This class will still forward those calls, but they will have
    * not succeed.
    */
   get name() {
+    if (!this._request) {
+      return this._name;
+    }
     return this._request.name;
   }
   isPending() {
+    if (!this._request) {
+      return false;
+    }
     return this._request.isPending();
   }
   get status() {
+    if (!this._request) {
+      return this._status;
+    }
     return this._request.status;
   }
   cancel(status) {
-    return this._request.cancel(status);
+    return this.cancelAsync(status);
   }
   suspend() {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_SUSPEND,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     return this._request.suspend();
   }
   resume() {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_RESUME,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     return this._request.resume();
   }
   get loadGroup() {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_NONE,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     return this._request.loadGroup;
   }
   set loadGroup(group) {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_NONE,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     this._request.loadGroup = group;
   }
   get loadFlags() {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_NONE,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     return this._request.loadFlags;
   }
   set loadFlags(flags) {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_NONE,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     this._request.loadFlags = flags;
   }
 
   /**
    * This function wraps nsIBitsRequest::bitsId.
    */
   get bitsId() {
+    if (!this._request) {
+      return this._bitsId;
+    }
     return this._request.bitsId;
   }
 
   /**
    * This function wraps nsIBitsRequest::transferError.
    *
    * Instead of simply returning the nsBitsErrorType value, however, it returns
    * a BitsError object, or null.
    */
   get transferError() {
-    let result = this._request.transferError;
+    let result;
+    if (this._request) {
+      result = this._request.transferError;
+    } else {
+      result = this._transferError;
+    }
     if (result == Ci.nsIBits.ERROR_TYPE_SUCCESS) {
       return null;
     }
     return new BitsError(result,
                          Ci.nsIBits.ERROR_ACTION_NONE,
                          Ci.nsIBits.ERROR_STAGE_MONITOR,
                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
   }
 
   /**
    * This function wraps nsIBitsRequest::changeMonitorInterval.
    *
    * Instead of taking a callback, the function is asynchronous.
    * This method either resolves with no data, or rejects with a BitsError.
    */
   async changeMonitorInterval(monitorIntervalMs) {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_CHANGE_MONITOR_INTERVAL,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     let action = gBits.ERROR_ACTION_CHANGE_MONITOR_INTERVAL;
     return requestPromise(action, callback => {
       this._request.changeMonitorInterval(monitorIntervalMs, callback);
     });
   }
 
   /**
    * This function wraps nsIBitsRequest::cancelAsync.
    *
    * Instead of taking a callback, the function is asynchronous.
    * This method either resolves with no data, or rejects with a BitsError.
    *
    * Adds a default status of NS_ERROR_ABORT if one is not provided.
    */
   async cancelAsync(status) {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_CANCEL,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     if (status === undefined) {
       status = Cr.NS_ERROR_ABORT;
     }
     let action = gBits.ERROR_ACTION_CANCEL;
     return requestPromise(action, callback => {
       this._request.cancelAsync(status, callback);
-    });
+    }).then(() => this.shutdown());
   }
 
   /**
    * This function wraps nsIBitsRequest::setPriorityHigh.
    *
    * Instead of taking a callback, the function is asynchronous.
    * This method either resolves with no data, or rejects with a BitsError.
    */
   async setPriorityHigh() {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_SET_PRIORITY,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     let action = gBits.ERROR_ACTION_SET_PRIORITY;
     return requestPromise(action, callback => {
       this._request.setPriorityHigh(callback);
     });
   }
 
   /**
    * This function wraps nsIBitsRequest::setPriorityLow.
    *
    * Instead of taking a callback, the function is asynchronous.
    * This method either resolves with no data, or rejects with a BitsError.
    */
   async setPriorityLow() {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_SET_PRIORITY,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     let action = gBits.ERROR_ACTION_SET_PRIORITY;
     return requestPromise(action, callback => {
       this._request.setPriorityLow(callback);
     });
   }
 
   /**
    * This function wraps nsIBitsRequest::complete.
    *
    * Instead of taking a callback, the function is asynchronous.
    * This method either resolves with no data, or rejects with a BitsError.
    */
   async complete() {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_COMPLETE,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     let action = gBits.ERROR_ACTION_COMPLETE;
     return requestPromise(action, callback => {
       this._request.complete(callback);
-    });
+    }).then(() => this.shutdown());
   }
 
   /**
    * This function wraps nsIBitsRequest::suspendAsync.
    *
    * Instead of taking a callback, the function is asynchronous.
    * This method either resolves with no data, or rejects with a BitsError.
    */
   async suspendAsync() {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_SUSPEND,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     let action = gBits.ERROR_ACTION_SUSPEND;
     return requestPromise(action, callback => {
       this._request.suspendAsync(callback);
     });
   }
 
   /**
    * This function wraps nsIBitsRequest::resumeAsync.
    *
    * Instead of taking a callback, the function is asynchronous.
    * This method either resolves with no data, or rejects with a BitsError.
    */
   async resumeAsync() {
+    if (!this._request) {
+      throw new BitsError(Ci.nsIBits.ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN,
+                          Ci.nsIBits.ERROR_ACTION_RESUME,
+                          Ci.nsIBits.ERROR_STAGE_PRETASK,
+                          Ci.nsIBits.ERROR_CODE_TYPE_NONE);
+    }
     let action = gBits.ERROR_ACTION_RESUME;
     return requestPromise(action, callback => {
       this._request.resumeAsync(callback);
     });
   }
 }
 BitsRequest.prototype.QueryInterface = ChromeUtils.generateQI([Ci.nsIRequest]);
 
--- a/toolkit/components/bitsdownload/nsIBits.idl
+++ b/toolkit/components/bitsdownload/nsIBits.idl
@@ -84,16 +84,17 @@ interface nsIBits : nsISupports
   const long ERROR_TYPE_FAILED_TO_GET_JOB_STATUS                          = 42;
   const long ERROR_TYPE_BITS_STATE_ERROR                                  = 43;
   const long ERROR_TYPE_BITS_STATE_TRANSIENT_ERROR                        = 44;
   const long ERROR_TYPE_BITS_STATE_CANCELLED                              = 45;
   const long ERROR_TYPE_BITS_STATE_UNEXPECTED                             = 46;
   const long ERROR_TYPE_VERIFICATION_FAILURE                              = 47;
   const long ERROR_TYPE_ACCESS_DENIED_EXPECTED                            = 48;
   const long ERROR_TYPE_FAILED_TO_CONNECT_TO_BCM                          = 49;
+  const long ERROR_TYPE_USE_AFTER_REQUEST_SHUTDOWN                        = 50;
 
   /**
    * nsBitsErrorAction values
    * These values indicate where the error occurred.
    */
   const long ERROR_ACTION_UNKNOWN                                         = 1;
   const long ERROR_ACTION_NONE                                            = 2;
   const long ERROR_ACTION_START_DOWNLOAD                                  = 3;
--- a/toolkit/mozapps/update/UpdateService.jsm
+++ b/toolkit/mozapps/update/UpdateService.jsm
@@ -1951,16 +1951,17 @@ UpdateService.prototype = {
         // received and networking hasn't started to shutdown. The download will
         // be resumed the next time the application starts. Downloads using
         // Windows BITS are not stopped since they don't require Firefox to be
         // running to perform the download.
         if (this._downloader) {
           if (!this._downloader.usingBits) {
             this.stopDownload();
           } else {
+            this._downloader.cleanup();
             // The BITS downloader isn't stopped on exit so the
             // active-update.xml needs to be saved for the values sent to
             // telemetry to be saved to disk.
             Cc["@mozilla.org/updates/update-manager;1"].
               getService(Ci.nsIUpdateManager).saveUpdates();
           }
         }
         // Prevent leaking the downloader (bug 454964)
@@ -4641,16 +4642,30 @@ Downloader.prototype = {
       }.bind(this.updateService), retryTimeout, Ci.nsITimer.TYPE_ONE_SHOT);
     } else {
       // Prevent leaking the update object (bug 454964)
       this._update = null;
     }
   },
 
   /**
+   * This function should be called when shutting down so that resources get
+   * freed properly.
+   */
+  cleanup: function Downloader_cleanup() {
+    if (this.usingBits) {
+      if (this._pendingRequest) {
+        this._pendingRequest.then(() => this._request.shutdown());
+      } else {
+        this._request.shutdown();
+      }
+    }
+  },
+
+  /**
    * See nsIInterfaceRequestor.idl
    */
   getInterface: function Downloader_getInterface(iid) {
     // The network request may require proxy authentication, so provide the
     // default nsIAuthPrompt if requested.
     if (iid.equals(Ci.nsIAuthPrompt)) {
       var prompt = Cc["@mozilla.org/network/default-auth-prompt;1"].
                    createInstance();
--- a/toolkit/mozapps/update/tests/browser/browser.bits.ini
+++ b/toolkit/mozapps/update/tests/browser/browser.bits.ini
@@ -1,11 +1,11 @@
 [DEFAULT]
-skip-if = debug || os != 'win'
-reason = Debug builds leak (Bug 1546287) and BITS is only available on Windows.
+skip-if = os != 'win'
+reason = BITS is only available on Windows.
 dupe-manifest =
 tags = appupdate bits
 head = head.js
 support-files =
   ../data/shared.js
   ../data/sharedUpdateXML.js
   app_update.sjs
   downloadPage.html