655727 - Implement responseType='moz-json'

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Masatoshi Kimura [:emk]]

See bug 649133 comment #17.

Comment 1

[Masatoshi Kimura [:emk]]

I doubt the usefulness of the extension. This is no more than a syntax sugar of JSON.parse(xhr.responseText) unless streaming JSON parser is used.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Well, there's also the fact that we'd use less memory since we can toss out the source text as soon as onload fires and we've parsed into a JSON object.

Additionally, it would let us add streaming parsing in the future if it turns out that this is something that people use a lot for large JSON objects.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Masatoshi, Jonas, have you brought this up in WebApps WG?
I think json makes sense and should be standardized.

Comment 4

[Shawn Gong]

Attachment: Patch 1.

Since the sending type is still plain text, the contentType is defaulted to "text/plain" and we need to set the it manually to "application/json" - let me know if we should change the behavior here.

Comment 5

[Shawn Gong]

Also please note this patch needs to go after Bug 678432 is checked in.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 553984 [details] [diff] [review]
Patch 1.

Review of attachment 553984 [details] [diff] [review]:
-----------------------------------------------------------------

Two higher-level comments:

You should only do the JSON conversion once and remember the result (if successful). The same way that you fixed for arraybuffer returns. Sorry, I should have mentioned this earlier, my bad :(

Second, don't do the testing using test_XHRSendData, that test is better adapted to testing various ways of sending data, whereas what you're testing here is a new way of receiving data. Instead modify test_XHR.html. And make sure to test that we return the same object if accessed multiple times. And that we throw if the response isn't valid JSON, and that we throw twice if you access the property twice.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1000,5 @@
>  
> +  case XML_HTTP_RESPONSE_TYPE_JSON:
> +    if (mState & XML_HTTP_REQUEST_DONE) {
> +      nsString bodyString;
> +      CopyUTF8toUTF16(mResponseBody.BeginReading(), bodyString);

You can't assume that the response is UTF8. Instead convert the same way as we do for text.

@@ +1004,5 @@
> +      CopyUTF8toUTF16(mResponseBody.BeginReading(), bodyString);
> +      if (!JS_ParseJSON(aCx,
> +                        (jschar*)PromiseFlatString(bodyString).get(),
> +                        bodyString.Length(), aResult)) {
> +        NS_ERROR("Wrong JSON");

NS_ERROR is equivalent to an assertion, and we don't want to assert on things that are simply errors on the webpage (i.e. a webpage should never be able to cause us to assert).

If this returns false, simple return NS_ERROR_FAILURE.

Comment 7

[Shawn Gong]

Attachment: Patch 2.

Fix!

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 554572 [details] [diff] [review]
Patch 2.

Review of attachment 554572 [details] [diff] [review]:
-----------------------------------------------------------------

Still need to review the test. But looks good otherwise.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1027,5 @@
> +  case XML_HTTP_RESPONSE_TYPE_JSON:
> +    if (mState & XML_HTTP_REQUEST_DONE) {
> +      if (mResultJSON == JSVAL_VOID) {
> +        rv = CreateResponseParsedJSON(aCx);
> +        NS_ENSURE_SUCCESS(rv, rv);

After parsing you can clear mResponseBuffer and mResponseBufferUnicode since they won't be used any more. However only do this in the case of parsing succeeding so that we throw the correct error if someone gets the property again.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 554572 [details] [diff] [review]
Patch 2.

Review of attachment 554572 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good otherwise!

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1027,5 @@
> +  case XML_HTTP_RESPONSE_TYPE_JSON:
> +    if (mState & XML_HTTP_REQUEST_DONE) {
> +      if (mResultJSON == JSVAL_VOID) {
> +        rv = CreateResponseParsedJSON(aCx);
> +        NS_ENSURE_SUCCESS(rv, rv);

After parsing you can clear mResponseBuffer and mResponseBufferUnicode since they won't be used any more. However only do this in the case of parsing succeeding so that we throw the correct error if someone gets the property again.

::: content/base/test/responseIdentical.sjs
@@ +11,5 @@
> +  var bytes = [], avail = 0;
> +  while ((avail = bodyStream.available()) > 0)
> +    body += String.fromCharCode.apply(String, bodyStream.readByteArray(avail));
> +
> +  response.setHeader("Content-Type", "text/html", false);

Make this "application/octet-stream" instead.

::: content/base/test/test_XHR.html
@@ +18,5 @@
> +// test receiving as JSON
> +function checkSetResponseTypeJSONThrows(xhr) {
> +  var didthrow = false;
> +  try { xhr.responseType = 'moz-json'; } catch (e) { didthrow = true; }
> +  ok(didthrow, "should have thrown when accessing responseType as JSON");

"should have thrown when setting responseType to moz-json"

Also, no need to do this as a separate function

@@ +31,5 @@
> +  xhr.responseType = 'moz-json';
> +  xhr.send(aJsonStr);
> +
> +  var isResponseReceived = false;
> +  xhr.onreadystatechange = function() {

Both the onreadystatechange function and the onload function can be replaced with the following code:

if (!invalid) {
  is(JSON.stringify(xhr.response), aJsonStr);
  is(xhr.response, xhr.response, "returning the same object on each access");
}
else {
  var didThrow = false;
  try { xhr.response } catch(ex) { didThrow = true; }
  ok(didThrow, "accessing response should throw");

  didThrow = false;
  try { xhr.response } catch(ex) { didThrow = true; }
  ok(didThrow, "accessing response should throw");
}

Also, you need to change the test to do the load synchronously, as to ensure that the load happens before the test finishes.

Comment 10

[Shawn Gong]

Attachment: Patch 3 - corrections according to Jonas' comments

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 559279 [details] [diff] [review]
Patch 3 - corrections according to Jonas' comments

Review of attachment 559279 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed!

::: content/base/test/test_XHR.html
@@ +29,5 @@
> +  xhr.open("POST", 'responseIdentical.sjs', false);
> +  xhr.responseType = 'moz-json';
> +  xhr.send(aJsonStr);
> +
> +  var isResponseReceived = false;

This variable isn't used anywhere.

Comment 12

[Shawn Gong]

Attachment: Patch 4. Review+ by sicking

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/27c140d86b1b

Thanks for the patch Shawn!! Very excited to have this support!

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/27c140d86b1b

Comment 15

[[e-mail]]

Comment on attachment 559301 [details] [diff] [review]
Patch 4. Review+ by sicking

>+  nsString bodyString;
>+  ConvertBodyToText(bodyString);
>+  if (!JS_ParseJSON(aCx,
>+                    (jschar*)PromiseFlatString(bodyString).get(),
[bodyString is an nsString so it's already flat]

Comment 16

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/DOM/XMLHttpRequest#Properties

(see the response and responseType properties)

Also mentioned on Firefox 9 for developers.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

For anyone that Google to this bug: the "moz-json" keyword have been replaced by "json" in bug 655727.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

oops, bug 707142