Bug 1363581 - (part 1) Buffer RESTRequest response in memory before decoding to avoid corruption r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 13 Mar 2018 15:13:56 -0700
changeset 769463 5ae2d805cc2783e08f918b9c4122661ce4706288
parent 766824 8f1b2f872f0ea358a0412eb8b8687f08d47f6621
child 769464 30cf9c848eabaeba537009f4b25886263c1593eb
push id103133
push userbmo:tchiovoloni@mozilla.com
push dateMon, 19 Mar 2018 16:51:38 +0000
reviewersmarkh
bugs1363581
milestone61.0a1
Bug 1363581 - (part 1) Buffer RESTRequest response in memory before decoding to avoid corruption r?markh MozReview-Commit-ID: BS0h4iIr91V
services/common/rest.js
services/common/tests/unit/test_restrequest.js
--- a/services/common/rest.js
+++ b/services/common/rest.js
@@ -13,17 +13,47 @@ ChromeUtils.import("resource://gre/modul
 ChromeUtils.import("resource://gre/modules/NetUtil.jsm");
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 ChromeUtils.import("resource://gre/modules/Log.jsm");
 ChromeUtils.import("resource://services-common/utils.js");
 
 ChromeUtils.defineModuleGetter(this, "CryptoUtils",
                                "resource://services-crypto/utils.js");
 
-const Prefs = new Preferences("services.common.");
+function decodeString(data, charset) {
+  if (!data || !charset) {
+    return data;
+  }
+
+  // This could be simpler if we assumed the charset is only ever UTF-8.
+  // It's unclear to me how willing we are to assume this, though...
+  let stringStream = Cc["@mozilla.org/io/string-input-stream;1"]
+                     .createInstance(Ci.nsIStringInputStream);
+  stringStream.setData(data, data.length);
+
+  let converterStream = Cc["@mozilla.org/intl/converter-input-stream;1"]
+                        .createInstance(Ci.nsIConverterInputStream);
+
+  converterStream.init(stringStream, charset, 0,
+                       converterStream.DEFAULT_REPLACEMENT_CHARACTER);
+
+  let remaining = data.length;
+  let body = "";
+  while (remaining > 0) {
+    let str = {};
+    let num = converterStream.readString(remaining, str);
+    if (!num) {
+      break;
+    }
+    remaining -= num;
+    body += str.value;
+  }
+  return body;
+}
+
 
 /**
  * Single use HTTP requests to RESTish resources.
  *
  * @param uri
  *        URI for the request. This can be an nsIURI object or a string
  *        that can be used to create one. An exception will be thrown if
  *        the string is not a valid URI.
@@ -82,18 +112,17 @@ function RESTRequest(uri) {
   // 'uri' isn't a valid URI string.
   if (!(uri instanceof Ci.nsIURI)) {
     uri = Services.io.newURI(uri);
   }
   this.uri = uri;
 
   this._headers = {};
   this._log = Log.repository.getLogger(this._logName);
-  this._log.level =
-    Log.Level[Prefs.get("log.logger.rest.request")];
+  this._log.manageLevelFromPref("services.common.log.logger.rest.request");
 }
 RESTRequest.prototype = {
 
   _logName: "Services.Common.RESTRequest",
 
   QueryInterface: XPCOMUtils.generateQI([
     Ci.nsIBadCertListener2,
     Ci.nsIInterfaceRequestor,
@@ -427,20 +456,18 @@ RESTRequest.prototype = {
       return;
     }
 
     this.status = this.IN_PROGRESS;
 
     this._log.trace("onStartRequest: " + channel.requestMethod + " " +
                     channel.URI.spec);
 
-    // Create a response object and fill it with some data.
-    let response = this.response = new RESTResponse();
-    response.request = this;
-    response.body = "";
+    // Create a new response object.
+    this.response = new RESTResponse(this);
 
     this.delayTimeout();
   },
 
   onStopRequest: function onStopRequest(channel, context, statusCode) {
     if (this.timeoutTimer) {
       // Clear the abort timer now that the channel is done.
       this.timeoutTimer.clear();
@@ -468,16 +495,30 @@ RESTRequest.prototype = {
 
     if (!this.onComplete) {
       this._log.error("Unexpected error: onComplete not defined in " +
                       "abortRequest.");
       this.onProgress = null;
       return;
     }
 
+    try {
+      // Decode this.response._rawBody,
+      this.response.body = decodeString(this.response._rawBody, this.response.charset);
+      this.response._rawBody = null;
+      // Call the 'progress' callback a single time with all the data.
+      this.onProgress();
+    } catch (ex) {
+      this._log.warn(`Exception handling response - ${this.method} ${channel.URI.spec}`, ex);
+      this.status = this.ABORTED;
+      this.onComplete(ex);
+      this.onComplete = this.onProgress = null;
+      return;
+    }
+
     // Throw the failure code and stop execution.  Use Components.Exception()
     // instead of Error() so the exception is QI-able and can be passed across
     // XPCOM borders while preserving the status code.
     if (!statusSuccess) {
       let message = Components.Exception("", statusCode).name;
       let error = Components.Exception(message, statusCode);
       this._log.debug(this.method + " " + uri + " failed: " + statusCode + " - " + message);
       this.onComplete(error);
@@ -511,71 +552,27 @@ RESTRequest.prototype = {
       }
 
       this.onComplete = this.onProgress = null;
       return;
     }
 
     if (channel.contentCharset) {
       this.response.charset = channel.contentCharset;
-
-      if (!this._converterStream) {
-        this._converterStream = Cc["@mozilla.org/intl/converter-input-stream;1"]
-                                   .createInstance(Ci.nsIConverterInputStream);
-      }
-      this._converterStream.init(stream, channel.contentCharset, 0,
-                                 this._converterStream.DEFAULT_REPLACEMENT_CHARACTER);
-
-      try {
-        let remaining = count;
-        while (remaining > 0) {
-          let str = {};
-          let num = this._converterStream.readString(remaining, str);
-          if (!num) {
-            break;
-          }
-          remaining -= num;
-          this.response.body += str.value;
-        }
-      } catch (ex) {
-        this._log.warn("Exception thrown reading " + count + " bytes from " +
-                       "the channel", ex);
-        throw ex;
-      }
     } else {
       this.response.charset = null;
-
-      if (!this._inputStream) {
-        this._inputStream = Cc["@mozilla.org/scriptableinputstream;1"]
-                              .createInstance(Ci.nsIScriptableInputStream);
-      }
-
-      this._inputStream.init(stream);
-
-      this.response.body += this._inputStream.read(count);
     }
 
-    try {
-      this.onProgress();
-    } catch (ex) {
-      this._log.warn("Got exception calling onProgress handler, aborting " +
-                     this.method + " " + channel.URI.spec, ex);
-      this.abort();
+    if (!this._inputStream) {
+      this._inputStream = Cc["@mozilla.org/scriptableinputstream;1"]
+                            .createInstance(Ci.nsIScriptableInputStream);
+    }
+    this._inputStream.init(stream);
 
-      if (!this.onComplete) {
-        this._log.error("Unexpected error: onComplete not defined in " +
-                        "onDataAvailable.");
-        this.onProgress = null;
-        return;
-      }
-
-      this.onComplete(ex);
-      this.onComplete = this.onProgress = null;
-      return;
-    }
+    this.response._rawBody += this._inputStream.read(count);
 
     this.delayTimeout();
   },
 
   /** nsIInterfaceRequestor **/
 
   getInterface(aIID) {
     return this.QueryInterface(aIID);
@@ -637,20 +634,22 @@ RESTRequest.prototype = {
     callback.onRedirectVerifyCallback(Cr.NS_OK);
   }
 };
 
 /**
  * Response object for a RESTRequest. This will be created automatically by
  * the RESTRequest.
  */
-function RESTResponse() {
+function RESTResponse(request = null) {
+  this.body = "";
+  this._rawBody = "";
+  this.request = request;
   this._log = Log.repository.getLogger(this._logName);
-  this._log.level =
-    Log.Level[Prefs.get("log.logger.rest.response")];
+  this._log.manageLevelFromPref("services.common.log.logger.rest.response");
 }
 RESTResponse.prototype = {
 
   _logName: "Services.Common.RESTResponse",
 
   /**
    * Corresponding REST request
    */
--- a/services/common/tests/unit/test_restrequest.js
+++ b/services/common/tests/unit/test_restrequest.js
@@ -136,17 +136,16 @@ add_test(function test_get() {
 
   request.onProgress = request.onComplete = function() {
     do_throw("This function should have been overwritten!");
   };
 
   let onProgress_called = false;
   function onProgress() {
     onProgress_called = true;
-    Assert.equal(this.status, request.IN_PROGRESS);
     Assert.ok(this.response.body.length > 0);
 
     Assert.ok(!!(this.channel.loadFlags & Ci.nsIRequest.LOAD_BYPASS_CACHE));
     Assert.ok(!!(this.channel.loadFlags & Ci.nsIRequest.INHIBIT_CACHING));
   }
 
   function onComplete(error) {
     Assert.equal(error, null);
@@ -317,17 +316,16 @@ function check_posting_data(method) {
 
   request.onProgress = request.onComplete = function() {
     do_throw("This function should have been overwritten!");
   };
 
   let onProgress_called = false;
   function onProgress() {
     onProgress_called = true;
-    Assert.equal(this.status, request.IN_PROGRESS);
     Assert.ok(this.response.body.length > 0);
   }
 
   function onComplete(error) {
     Assert.equal(error, null);
 
     Assert.equal(this.status, this.COMPLETED);
     Assert.ok(this.response.success);
@@ -387,17 +385,16 @@ add_test(function test_delete() {
 
   request.onProgress = request.onComplete = function() {
     do_throw("This function should have been overwritten!");
   };
 
   let onProgress_called = false;
   function onProgress() {
     onProgress_called = true;
-    Assert.equal(this.status, request.IN_PROGRESS);
     Assert.ok(this.response.body.length > 0);
   }
 
   function onComplete(error) {
     Assert.equal(error, null);
 
     Assert.equal(this.status, this.COMPLETED);
     Assert.ok(this.response.success);