Fix leak of file upload objects

After an upload completed, we were failing to delete the details of the upload
from the list (because the comparison was bogus), meaning we leaked an object
each time.

While we're in the area:

  - make the request methods take an opts object (instead of a localTimeout
    param), and deprecate the WithPrefix versions.

  - make the non-xhr path for uploadContent use authedRequest instead of
    rolling its own.

  - make cancelUpload use the promise.abort() hack for simplicity

lib/http-api.js

@@ -113,8 +113,6 @@ module.exports.MatrixHttpApi.prototype = {
                 "Expected callback to be a function but got " + typeof callback
             );
         }
-        var defer = q.defer();
-        var url = this.opts.baseUrl + "/_matrix/media/v1/upload";
         // browser-request doesn't support File objects because it deep-copies
         // the options using JSON.parse(JSON.stringify(options)). Instead of
         // loading the whole file into memory as a string and letting
@@ -124,8 +122,9 @@ module.exports.MatrixHttpApi.prototype = {
         // of important here)
 
         var upload = { loaded: 0, total: 0 };
-
+        var promise;
         if (global.XMLHttpRequest) {
+            var defer = q.defer();
             var xhr = new global.XMLHttpRequest();
             upload.xhr = xhr;
             var cb = requestCallback(defer, callback, this.opts.onlyData);
@@ -168,6 +167,7 @@ module.exports.MatrixHttpApi.prototype = {
                 xhr.timeout_timer = callbacks.setTimeout(timeout_fn, 30000);
                 defer.notify(ev);
             });
+            var url = this.opts.baseUrl + "/_matrix/media/v1/upload";
             url += "?access_token=" + encodeURIComponent(this.opts.accessToken);
             url += "&filename=" + encodeURIComponent(file.name);
 
@@ -180,47 +180,48 @@ module.exports.MatrixHttpApi.prototype = {
                 xhr.setRequestHeader("Content-Type", 'application/octet-stream');
             }
             xhr.send(file);
+            promise = defer.promise;
+
+            // dirty hack (as per _request) to allow the upload to be cancelled.
+            promise.abort = xhr.abort.bind(xhr);
         } else {
             var queryParams = {
                 filename: file.name,
-                access_token: this.opts.accessToken
             };
-            upload.request = this.opts.request({
+            promise = this.authedRequest(
-                uri: url,
+                callback, "POST", "/upload", queryParams, file.stream, {
-                qs: queryParams,
+                    prefix: "/_matrix/media/v1",
-                method: "POST",
+                    localTimeoutMs: 30000,
                     headers: {"Content-Type": file.type},
-                body: file.stream
+                }
-            }, requestCallback(defer, callback, this.opts.onlyData));
+            );
         }
 
-        this.uploads.push(upload);
-
         var self = this;
-        upload.promise = defer.promise.finally(function() {
+
-            var uploadsKeys = Object.keys(self.uploads);
+        // remove the upload from the list on completion
-            for (var i = 0; i < uploadsKeys.length; ++i) {
+        var promise0 = promise.finally(function() {
-                if (self.uploads[uploadsKeys[i]].promise === defer.promise) {
+            for (var i = 0; i < self.uploads.length; ++i) {
-                    self.uploads.splice(uploadsKeys[i], 1);
+                if (self.uploads[i] === upload) {
+                    self.uploads.splice(i, 1);
+                    return;
                 }
             }
         });
-        return upload.promise;
+
+        // copy our dirty abort() method to the new promise
+        promise0.abort = promise.abort;
+
+        upload.promise = promise0;
+        this.uploads.push(upload);
+
+        return promise0;
     },
 
     cancelUpload: function(promise) {
-        var uploadsKeys = Object.keys(this.uploads);
+        if (promise.abort) {
-        for (var i = 0; i < uploadsKeys.length; ++i) {
+            promise.abort();
-            var upload = this.uploads[uploadsKeys[i]];
-            if (upload.promise === promise) {
-                if (upload.xhr !== undefined) {
-                    upload.xhr.abort();
             return true;
-                } else if (upload.request !== undefined) {
-                    upload.request.abort();
-                    return true;
-                }
-            }
         }
         return false;
     },
@@ -271,11 +272,22 @@ module.exports.MatrixHttpApi.prototype = {
      * @param {string} method The HTTP method e.g. "GET".
      * @param {string} path The HTTP path <b>after</b> the supplied prefix e.g.
      * "/createRoom".
-     * @param {Object} queryParams A dict of query params (these will NOT be
+     *
-     * urlencoded).
+     * @param {Object=} queryParams A dict of query params (these will NOT be
+     * urlencoded). If unspecified, there will be no query params.
+     *
      * @param {Object} data The HTTP JSON body.
-     * @param {Number=} localTimeoutMs The maximum amount of time to wait before
+     *
+     * @param {Object=} opts additional options
+     *
+     * @param {Number=} opts.localTimeoutMs The maximum amount of time to wait before
      * timing out the request. If not specified, there is no timeout.
+     *
+     * @param {sting=} opts.prefix The full prefix to use e.g.
+     * "/_matrix/client/v2_alpha". If not specified, uses this.opts.prefix.
+     *
+     * @param {Object=} opts.headers map of additional request headers
+     *
      * @return {module:client.Promise} Resolves to <code>{data: {Object},
      * headers: {Object}, code: {Number}}</code>.
      * If <code>onlyData</code> is set, this will resolve to the <code>data</code>
@@ -283,18 +295,25 @@ module.exports.MatrixHttpApi.prototype = {
      * @return {module:http-api.MatrixError} Rejects with an error if a problem
      * occurred. This includes network problems and Matrix-specific error JSON.
      */
-    authedRequest: function(callback, method, path, queryParams, data, localTimeoutMs) {
+    authedRequest: function(callback, method, path, queryParams, data, opts) {
-        if (!queryParams) { queryParams = {}; }
+        if (!queryParams) {
+            queryParams = {};
+        }
+        if (!queryParams.access_token) {
             queryParams.access_token = this.opts.accessToken;
-        var self = this;
+        }
+
         var request_promise = this.request(
-            callback, method, path, queryParams, data, localTimeoutMs
+            callback, method, path, queryParams, data, opts
         );
+
+        var self = this;
         request_promise.catch(function(err) {
             if (err.errcode == 'M_UNKNOWN_TOKEN') {
                 self.event_emitter.emit("Session.logged_out");
             }
         });
+
         // return the original promise, otherwise tests break due to it having to
         // go around the event loop one more time to process the result of the request
         return request_promise;
@@ -307,11 +326,22 @@ module.exports.MatrixHttpApi.prototype = {
      * @param {string} method The HTTP method e.g. "GET".
      * @param {string} path The HTTP path <b>after</b> the supplied prefix e.g.
      * "/createRoom".
-     * @param {Object} queryParams A dict of query params (these will NOT be
+     *
-     * urlencoded).
+     * @param {Object=} queryParams A dict of query params (these will NOT be
+     * urlencoded). If unspecified, there will be no query params.
+     *
      * @param {Object} data The HTTP JSON body.
-     * @param {Number=} localTimeoutMs The maximum amount of time to wait before
+     *
+     * @param {Object=} opts additional options
+     *
+     * @param {Number=} opts.localTimeoutMs The maximum amount of time to wait before
      * timing out the request. If not specified, there is no timeout.
+     *
+     * @param {sting=} opts.prefix The full prefix to use e.g.
+     * "/_matrix/client/v2_alpha". If not specified, uses this.opts.prefix.
+     *
+     * @param {Object=} opts.headers map of additional request headers
+     *
      * @return {module:client.Promise} Resolves to <code>{data: {Object},
      * headers: {Object}, code: {Number}}</code>.
      * If <code>onlyData</code> is set, this will resolve to the <code>data</code>
@@ -319,9 +349,13 @@ module.exports.MatrixHttpApi.prototype = {
      * @return {module:http-api.MatrixError} Rejects with an error if a problem
      * occurred. This includes network problems and Matrix-specific error JSON.
      */
-    request: function(callback, method, path, queryParams, data, localTimeoutMs) {
+    request: function(callback, method, path, queryParams, data, opts) {
-        return this.requestWithPrefix(
+        opts = opts || {};
-            callback, method, path, queryParams, data, this.opts.prefix, localTimeoutMs
+        var prefix = opts.prefix || this.opts.prefix;
+        var fullUri = this.opts.baseUrl + prefix + path;
+
+        return this.requestOtherUrl(
+            callback, method, fullUri, queryParams, data, opts
         );
     },
 
@@ -347,16 +381,16 @@ module.exports.MatrixHttpApi.prototype = {
      * object only.
      * @return {module:http-api.MatrixError} Rejects with an error if a problem
      * occurred. This includes network problems and Matrix-specific error JSON.
+     *
+     * @deprecated prefer authedRequest with opts.prefix
      */
     authedRequestWithPrefix: function(callback, method, path, queryParams, data,
                                       prefix, localTimeoutMs) {
-        var fullUri = this.opts.baseUrl + prefix + path;
+        return this.authedRequest(
-        if (!queryParams) {
+            callback, method, path, queryParams, data, {
-            queryParams = {};
+                localTimeoutMs: localTimeoutMs,
+                prefix: prefix,
             }
-        queryParams.access_token = this.opts.accessToken;
-        return this._request(
-            callback, method, fullUri, queryParams, data, localTimeoutMs
         );
     },
 
@@ -382,15 +416,16 @@ module.exports.MatrixHttpApi.prototype = {
      * object only.
      * @return {module:http-api.MatrixError} Rejects with an error if a problem
      * occurred. This includes network problems and Matrix-specific error JSON.
+     *
+     * @deprecated prefer request with opts.prefix
      */
     requestWithPrefix: function(callback, method, path, queryParams, data, prefix,
                                 localTimeoutMs) {
-        var fullUri = this.opts.baseUrl + prefix + path;
+        return this.request(
-        if (!queryParams) {
+            callback, method, path, queryParams, data, {
-            queryParams = {};
+                localTimeoutMs: localTimeoutMs,
+                prefix: prefix,
             }
-        return this._request(
-            callback, method, fullUri, queryParams, data, localTimeoutMs
         );
     },
 
@@ -400,11 +435,22 @@ module.exports.MatrixHttpApi.prototype = {
      * success/failure. See the promise return values for more information.
      * @param {string} method The HTTP method e.g. "GET".
      * @param {string} uri The HTTP URI
-     * @param {Object} queryParams A dict of query params (these will NOT be
+     *
-     * urlencoded).
+     * @param {Object=} queryParams A dict of query params (these will NOT be
+     * urlencoded). If unspecified, there will be no query params.
+     *
      * @param {Object} data The HTTP JSON body.
-     * @param {Number=} localTimeoutMs The maximum amount of time to wait before
+     *
+     * @param {Object=} opts additional options
+     *
+     * @param {Number=} opts.localTimeoutMs The maximum amount of time to wait before
      * timing out the request. If not specified, there is no timeout.
+     *
+     * @param {sting=} opts.prefix The full prefix to use e.g.
+     * "/_matrix/client/v2_alpha". If not specified, uses this.opts.prefix.
+     *
+     * @param {Object=} opts.headers map of additional request headers
+     *
      * @return {module:client.Promise} Resolves to <code>{data: {Object},
      * headers: {Object}, code: {Number}}</code>.
      * If <code>onlyData</code> is set, this will resolve to the <code>data</code>
@@ -413,12 +459,18 @@ module.exports.MatrixHttpApi.prototype = {
      * occurred. This includes network problems and Matrix-specific error JSON.
      */
     requestOtherUrl: function(callback, method, uri, queryParams, data,
-                                localTimeoutMs) {
+                              opts) {
-        if (!queryParams) {
+        if (opts === undefined || opts === null) {
-            queryParams = {};
+            opts = {};
+        } else if (isFinite(opts)) {
+            // opts used to be localTimeoutMs
+            opts = {
+                localTimeoutMs: opts
+            };
         }
+
         return this._request(
-            callback, method, uri, queryParams, data, localTimeoutMs
+            callback, method, uri, queryParams, data, opts
         );
     },
 
@@ -441,16 +493,15 @@ module.exports.MatrixHttpApi.prototype = {
         return this.opts.baseUrl + prefix + path + queryString;
     },
 
-    _request: function(callback, method, uri, queryParams, data, localTimeoutMs) {
+    _request: function(callback, method, uri, queryParams, data, opts) {
         if (callback !== undefined && !utils.isFunction(callback)) {
             throw Error(
                 "Expected callback to be a function but got " + typeof callback
             );
         }
+        opts = opts || {};
+
         var self = this;
-        if (!queryParams) {
-            queryParams = {};
-        }
         if (this.opts.extraParams) {
             for (var key in this.opts.extraParams) {
                 if (!this.opts.extraParams.hasOwnProperty(key)) { continue; }
@@ -462,6 +513,7 @@ module.exports.MatrixHttpApi.prototype = {
         var timeoutId;
         var timedOut = false;
         var req;
+        var localTimeoutMs = opts.localTimeoutMs;
         if (localTimeoutMs) {
             timeoutId = callbacks.setTimeout(function() {
                 timedOut = true;
@@ -488,6 +540,7 @@ module.exports.MatrixHttpApi.prototype = {
                     body: data,
                     json: true,
                     timeout: localTimeoutMs,
+                    headers: opts.headers || {},
                     _matrix_opts: this.opts
                 },
                 function(err, response, body) {