Bug 1546287 - Fix leak in BITS update tests r=rstrong

Differential Revision: https://phabricator.services.mozilla.com/D29751

--HG--
extra : moz-landing-system : lando

toolkit/components/bitsdownload/Bits.jsm

@@ -188,6 +188,14 @@ async function requestPromise(errorAction, actionFn) {
 /**
  * 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) {
@@ -195,6 +203,39 @@ class BitsRequest {
     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.
@@ -204,33 +245,78 @@ class BitsRequest {
    * 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;
   }
 
@@ -238,6 +324,9 @@ class BitsRequest {
    * This function wraps nsIBitsRequest::bitsId.
    */
   get bitsId() {
+    if (!this._request) {
+      return this._bitsId;
+    }
     return this._request.bitsId;
   }
 
@@ -248,7 +337,12 @@ class BitsRequest {
    * 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;
     }
@@ -265,6 +359,12 @@ class BitsRequest {
    * 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);
@@ -280,13 +380,19 @@ class BitsRequest {
    * 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());
   }
 
   /**
@@ -296,6 +402,12 @@ class BitsRequest {
    * 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);
@@ -309,6 +421,12 @@ class BitsRequest {
    * 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);
@@ -322,10 +440,16 @@ class BitsRequest {
    * 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());
   }
 
   /**
@@ -335,6 +459,12 @@ class BitsRequest {
    * 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);
@@ -348,6 +478,12 @@ class BitsRequest {
    * 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);

toolkit/components/bitsdownload/nsIBits.idl

@@ -89,6 +89,7 @@ interface nsIBits : nsISupports
   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

toolkit/mozapps/update/UpdateService.jsm

@@ -1956,6 +1956,7 @@ UpdateService.prototype = {
           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.
@@ -4645,6 +4646,20 @@ Downloader.prototype = {
     }
   },
 
+  /**
+   * 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
    */

toolkit/mozapps/update/tests/browser/browser.bits.ini

@@ -1,6 +1,6 @@
 [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