Bug 772191 - Change default stream encoding of RESTRequest to utf-8; r=philikon
authorAnant Narayanan <anant@kix.in>
Fri, 13 Jul 2012 16:52:31 -0700
changeset 99302 f7acb805fe4fcd4d74bd27a15e7c47a0b7b11088
parent 99301 992e01fbc1b8d60d9c23c90e34f8b31afc7f7e14
child 99303 85bc47c2d64689836e4b516f79023db8274175f3
push id23117
push usergszorc@mozilla.com
push dateSat, 14 Jul 2012 20:35:19 +0000
treeherdermozilla-central@8e2f9cc15bd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersphilikon
bugs772191
milestone16.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 772191 - Change default stream encoding of RESTRequest to utf-8; r=philikon
services/common/rest.js
services/common/tests/unit/test_restrequest.js
--- a/services/common/rest.js
+++ b/services/common/rest.js
@@ -143,16 +143,24 @@ RESTRequest.prototype = {
    * Request timeout (in seconds, though decimal values can be used for
    * up to millisecond granularity.)
    *
    * 0 for no timeout.
    */
   timeout: null,
 
   /**
+   * The encoding with which the response to this request must be treated.
+   * If a charset parameter is available in the HTTP Content-Type header for
+   * this response, that will always be used, and this value is ignored. We
+   * default to UTF-8 because that is a reasonable default.
+   */
+  charset: "utf-8",
+
+  /**
    * Called when the request has been completed, including failures and
    * timeouts.
    *
    * @param error
    *        Error that occurred while making the request, null if there
    *        was no error.
    */
   onComplete: function onComplete(error) {
@@ -305,16 +313,20 @@ RESTRequest.prototype = {
       let type = headers["content-type"] || "text/plain";
       channel.QueryInterface(Ci.nsIUploadChannel);
       channel.setUploadStream(stream, type, data.length);
     }
     // We must set this after setting the upload stream, otherwise it
     // will always be 'PUT'. Yeah, I know.
     channel.requestMethod = method;
 
+    // Before opening the channel, set the charset that serves as a hint
+    // as to what the response might be encoded as.
+    channel.contentCharset = this.charset;
+
     // Blast off!
     channel.asyncOpen(this, null);
     this.status = this.SENT;
     this.delayTimeout();
     return this;
   },
 
   /**
@@ -364,28 +376,16 @@ RESTRequest.prototype = {
     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 = "";
 
-    // Define this here so that we don't have make a new one each time
-    // onDataAvailable() gets called. If the Content-Type specified a charset,
-    // make sure we use the correct converter stream, instead of a generic
-    // ScriptableInputStream (onDataAvailable will pick the right one).
-    if (channel.contentCharset) {
-      response.charset = channel.contentCharset;
-      this._converterStream = Cc["@mozilla.org/intl/converter-input-stream;1"]
-                                .createInstance(Ci.nsIConverterInputStream);
-    } else {
-      this._inputStream = Cc["@mozilla.org/scriptableinputstream;1"]
-                          .createInstance(Ci.nsIScriptableInputStream);
-    }
     this.delayTimeout();
   },
 
   onStopRequest: function onStopRequest(channel, context, statusCode) {
     if (this.timeoutTimer) {
       // Clear the abort timer now that the channel is done.
       this.timeoutTimer.clear();
     }
@@ -436,44 +436,73 @@ RESTRequest.prototype = {
     }
 
     delete this._inputStream;
 
     this.onComplete(null);
     this.onComplete = this.onProgress = null;
   },
 
-  onDataAvailable: function onDataAvailable(req, cb, stream, off, count) {
+  onDataAvailable: function onDataAvailable(channel, cb, stream, off, count) {
+    // We get an nsIRequest, which doesn't have contentCharset.
     try {
-      if (this._inputStream) {
-        this._inputStream.init(stream);
-        this.response.body += this._inputStream.read(count);
-      } else {
+      channel.QueryInterface(Ci.nsIHttpChannel);
+    } catch (ex) {
+      this._log.error("Unexpected error: channel not nsIHttpChannel!");
+      this.abort();
+
+      if (this.onComplete) {
+        this.onComplete(ex);
+      }
+
+      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 str = {};
-        this._converterStream.init(
-          stream, this.response.charset, 0,
-          this._converterStream.DEFAULT_REPLACEMENT_CHARACTER
-        );
         let num = this._converterStream.readString(count, str);
         if (num != 0) {
           this.response.body += str.value;
         }
+      } catch (ex) {
+        this._log.warn("Exception thrown reading " + count + " bytes from " +
+                       "the channel.");
+        this._log.warn(CommonUtils.exceptionStr(ex));
+        throw ex;
       }
-    } catch (ex) {
-      this._log.warn("Exception thrown reading " + count +
-                     " bytes from the channel.");
-      this._log.debug(CommonUtils.exceptionStr(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 + " " + req.URI.spec);
+                     this.method + " " + channel.URI.spec);
       this._log.debug("Exception: " + CommonUtils.exceptionStr(ex));
       this.abort();
 
       if (!this.onComplete) {
         this._log.error("Unexpected error: onComplete not defined in " +
                         "onDataAvailable.");
         this.onProgress = null;
         return;
@@ -630,9 +659,9 @@ TokenAuthenticatedRESTRequest.prototype 
     );
 
     this.setHeader("Authorization", sig.getHeader());
 
     return RESTRequest.prototype.dispatch.call(
       this, method, data, onComplete, onProgress
     );
   },
-};
\ No newline at end of file
+};
--- a/services/common/tests/unit/test_restrequest.js
+++ b/services/common/tests/unit/test_restrequest.js
@@ -8,17 +8,17 @@ Cu.import("resource://services-common/ut
 
 const TEST_RESOURCE_URL = TEST_SERVER_URL + "resource";
 
 //DEBUG = true;
 
 function run_test() {
   Log4Moz.repository.getLogger("Services.Common.RESTRequest").level =
     Log4Moz.Level.Trace;
-  initTestLogging();
+  initTestLogging("Trace");
 
   run_next_test();
 }
 
 /**
  * Initializing a RESTRequest with an invalid URI throws
  * NS_ERROR_MALFORMED_URI.
  */
@@ -159,38 +159,105 @@ add_test(function test_get() {
   });
 });
 
 /**
  * Test HTTP GET with UTF-8 content, and custom Content-Type.
  */
 add_test(function test_get_utf8() {
   let response = "Hello World or Καλημέρα κόσμε or こんにちは 世界";
-  let contentType = "text/plain; charset=UTF-8";
+
+  let contentType = "text/plain";
+  let charset = true;
+  let charsetSuffix = "; charset=UTF-8";
 
   let server = httpd_setup({"/resource": function(req, res) {
     res.setStatusLine(req.httpVersion, 200, "OK");
-    res.setHeader("Content-Type", contentType);
+    res.setHeader("Content-Type", contentType + (charset ? charsetSuffix : ""));
 
     let converter = Cc["@mozilla.org/intl/converter-output-stream;1"]
                     .createInstance(Ci.nsIConverterOutputStream);
     converter.init(res.bodyOutputStream, "UTF-8", 0, 0x0000);
     converter.writeString(response);
     converter.close();
   }});
 
-  let request = new RESTRequest(TEST_RESOURCE_URL);
-  request.get(function(error) {
-    do_check_eq(error, null);
+  // Check if charset in Content-Type is propertly interpreted.
+  let request1 = new RESTRequest(TEST_RESOURCE_URL);
+  request1.get(function(error) {
+    do_check_null(error);
+
+    do_check_eq(request1.response.status, 200);
+    do_check_eq(request1.response.body, response);
+    do_check_eq(request1.response.headers["content-type"],
+                contentType + charsetSuffix);
+
+    // Check that we default to UTF-8 if Content-Type doesn't have a charset.
+    charset = false;
+    let request2 = new RESTRequest(TEST_RESOURCE_URL);
+    request2.get(function(error) {
+      do_check_null(error);
+
+      do_check_eq(request2.response.status, 200);
+      do_check_eq(request2.response.body, response);
+      do_check_eq(request2.response.headers["content-type"], contentType);
+      do_check_eq(request2.response.charset, "utf-8");
+
+      server.stop(run_next_test);
+    });
+  });
+});
+
+/**
+ * Test more variations of charset handling.
+ */
+add_test(function test_charsets() {
+  let response = "Hello World, I can't speak Russian";
+
+  let contentType = "text/plain";
+  let charset = true;
+  let charsetSuffix = "; charset=us-ascii";
 
-    do_check_eq(request.response.status, 200);
-    do_check_eq(request.response.body, response);
-    do_check_eq(request.response.headers["content-type"], contentType);
+  let server = httpd_setup({"/resource": function(req, res) {
+    res.setStatusLine(req.httpVersion, 200, "OK");
+    res.setHeader("Content-Type", contentType + (charset ? charsetSuffix : ""));
+
+    let converter = Cc["@mozilla.org/intl/converter-output-stream;1"]
+                    .createInstance(Ci.nsIConverterOutputStream);
+    converter.init(res.bodyOutputStream, "us-ascii", 0, 0x0000);
+    converter.writeString(response);
+    converter.close();
+  }});
+
+  // Check that provided charset overrides hint.
+  let request1 = new RESTRequest(TEST_RESOURCE_URL);
+  request1.charset = "not-a-charset";
+  request1.get(function(error) {
+    do_check_null(error);
 
-    server.stop(run_next_test);
+    do_check_eq(request1.response.status, 200);
+    do_check_eq(request1.response.body, response);
+    do_check_eq(request1.response.headers["content-type"],
+                contentType + charsetSuffix);
+    do_check_eq(request1.response.charset, "us-ascii");
+
+    // Check that hint is used if Content-Type doesn't have a charset.
+    charset = false;
+    let request2 = new RESTRequest(TEST_RESOURCE_URL);
+    request2.charset = "us-ascii";
+    request2.get(function(error) {
+      do_check_null(error);
+
+      do_check_eq(request2.response.status, 200);
+      do_check_eq(request2.response.body, response);
+      do_check_eq(request2.response.headers["content-type"], contentType);
+      do_check_eq(request2.response.charset, "us-ascii");
+
+      server.stop(run_next_test);
+    });
   });
 });
 
 /**
  * Test HTTP PUT with a simple string argument and default Content-Type.
  */
 add_test(function test_put() {
   let handler = httpd_handler(200, "OK", "Got it!");
@@ -646,17 +713,17 @@ add_test(function test_timeout() {
 add_test(function test_exception_in_onProgress() {
   let handler = httpd_handler(200, "OK", "Foobar");
   let server = httpd_setup({"/resource": handler});
 
   let request = new RESTRequest(TEST_RESOURCE_URL);
   request.onProgress = function onProgress() {
     it.does.not.exist();
   };
-  request.get(function (error) {
+  request.get(function onComplete(error) {
     do_check_eq(error, "ReferenceError: it is not defined");
     do_check_eq(this.status, this.ABORTED);
 
     server.stop(run_next_test);
   });
 });
 
 add_test(function test_new_channel() {