687087 - Support "chunked" data for XMLHttpRequest

Status: RESOLVED FIXED

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

Description

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

When downloading a large resource with XHR we currently load the whole thing into memory. If someone is able to incrementally consume the data, it would be nice to allow exposing only the data that was downloaded since the last progress event.

For text data this is a bit extra complicated because at the time when we fire the progress event we might have only received half a surrogate.

Anyhow, I've implemented this!

Comment 1

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

Attachment: Part 1: Always fire "progress" event before load

In order to keep things simple for the web author, we should let them only do data processing during the "progress" event when doing chunked download. To enable this we need to fire a "progress" event right before "load" if we've received data since the last "progress" event (which then presumably happened less than 50ms ago).

This patch also cleans up how we track the total number of bytes downloaded. Currently we do this by measuring mResponseBody.Length(), however that doesn't work when responseType is "document" or "blob". I.e. for those types we currently have the wrong event.loaded values.

Finally, this patch makes us fire the "progress" event from OnDataAvailable rather than OnProgress. This means that we can be more sure to have updated the .response* properties before firing the "progress" event. It also means that we don't have to worry about background requests which don't fire OnProgress for the download "progress" event. (The upload "progress" events are still a problem though).

Comment 2

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

Attachment: Part 2: Improve text handling in XHR

We currently do this silly thing where the .responseText getter charset-decode *all* downloaded data if we've received *any* data since the last time .responseText was accessed.

This is *sometimes* needed if we're also parsing into a document and the document-parser has detected a new charset since the last time .responseText was accessed. But this not the common case.

This patch makes us continuously pass data to the charset decoder as we download it in the cases when we're not also parsing a document. When we're parsing a document we lazily pass things to the charset decoder whenever .responseText is accessed. And we only pass *all* data to the decoder if the charset has actually changed since last time .responseText was accessed.

Comment 3

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

Attachment: Part 3: Support chunked data

Also contains tests!! Lots of tests!

Comment 4

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

Attachment: Part 3b: Forbid chunked for sync XHR

We should throw if you try to set .responseType to any of the chunked types for sync XHR. This also improves our tests for moz-json since i had to change that test anyway to test the new throwing behavior.

Comment 5

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

Attachment: Part 2: Improve text handling in XHR

The previous version had a merge error with the JSON support patch. The only code change is to check for responseType=JSON when detecting charset and when receiving data. I also improved the tests to test progress events for responseType=""

Comment 6

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

Comment on attachment 560580 [details] [diff] [review]
Part 1: Always fire "progress" event before load

>   // Reset responseBody
>   mResponseBody.Truncate();
>   mResponseBodyUnicode.SetIsVoid(PR_TRUE);
>   mResponseBlob = nsnull;
>   mResultArrayBuffer = nsnull;
>+  mLoadTransferred = 0;
Could put this also to nsXMLHttpRequest::Send where we reset other things.
Or, actually, would be good to have some ResetVariables() -like method which could be called in different
places like in Abort, Send etc-

Comment 7

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

Comment on attachment 560747 [details] [diff] [review]
Part 3b: Forbid chunked for sync XHR

>diff --git a/content/base/src/nsXMLHttpRequest.cpp b/content/base/src/nsXMLHttpRequest.cpp
>--- a/content/base/src/nsXMLHttpRequest.cpp
>+++ b/content/base/src/nsXMLHttpRequest.cpp
>@@ -960,18 +960,24 @@ NS_IMETHODIMP nsXMLHttpRequest::SetRespo
>     mResponseType = XML_HTTP_RESPONSE_TYPE_BLOB;
>   } else if (aResponseType.EqualsLiteral("document")) {
>     mResponseType = XML_HTTP_RESPONSE_TYPE_DOCUMENT;
>   } else if (aResponseType.EqualsLiteral("text")) {
>     mResponseType = XML_HTTP_RESPONSE_TYPE_TEXT;
>   } else if (aResponseType.EqualsLiteral("moz-json")) {
>     mResponseType = XML_HTTP_RESPONSE_TYPE_JSON;
>   } else if (aResponseType.EqualsLiteral("moz-chunked-text")) {
>+    if (!(mState & XML_HTTP_REQUEST_ASYNC)) {
>+      return NS_ERROR_FAILURE;
>+    }
>     mResponseType = XML_HTTP_RESPONSE_TYPE_CHUNKED_TEXT;
>   } else if (aResponseType.EqualsLiteral("moz-chunked-arraybuffer")) {
>+    if (!(mState & XML_HTTP_REQUEST_ASYNC)) {
>+      return NS_ERROR_FAILURE;
>+    }
I think NS_ERROR_DOM_INVALID_STATE_ERR would be better.

Comment 8

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

Comment on attachment 560748 [details] [diff] [review]
Part 2: Improve text handling in XHR

>+  // Decoder used for decoding into mResponseText
>+  // Only used for DEFAULT and TEXT responseTypes.
>+  nsCOMPtr<nsIUnicodeDecoder> mDecoder;
Also, JSON, right?

Could you add some comments how "we might have only received half a surrogate" case is handled.
Since NS_OK_UDEC_MOREINPUT/NS_PARTIAL_MORE_INPUT wasn't in the source code, it took some time to figure out
that the case is actually handled.

(I hope there are tests for that. I haven't yet looked at them.)

Comment 9

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

Comment on attachment 560588 [details] [diff] [review]
Part 3: Support chunked data

>       nsString str;
>       rv = GetResponseText(str);
>       if (NS_FAILED(rv)) return rv;
>-      nsStringBuffer* buf;
>-      *aResult = XPCStringConvert::ReadableToJSVal(aCx, str, &buf);
>-      if (buf) {
>-        str.ForgetSharedBuffer();
>+      if (str.IsVoid()) {
>+        *aResult = JSVAL_NULL;
>+      }
>+      else {
} else {
Same also elsewhere.

This all will make XHR look more like EventSource, though, the use cases are
quite different.
r=me


>+  yield;
Tests using yield are hard to review. rs=for the tests.

Comment 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/b2fb48fdb1e7
https://hg.mozilla.org/mozilla-central/rev/192254be8f97
https://hg.mozilla.org/mozilla-central/rev/dee5413476bb

Comment 11

[Henri Sivonen (:hsivonen)]

Is there a spec for this feature?

Comment 12

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

I proposed it to the webapps list and got generally good feedback, but no feedback from the editor yet.

I'll write a new email detailing the various mozilla extensions to .responseType

Comment 13

[Anne (:annevk)]

I did give feedback, asking about better interoperability on existing features before implementing new features. Microsoft meanwhile also proposed a Stream API of sorts.

Comment 14

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/DOM/XMLHttpRequest/Using_XMLHttpRequest#Monitoring_progress

And mentioned on Firefox 9 for developers.

Comment 15

[Florian Quèze [:florian]]

If I understand correctly, the feature introduced here is the 2 new "moz-chunked-text" and "moz-chunked-arraybuffer" values for the responseType property.

I don't see how https://developer.mozilla.org/index.php?title=en/DOM/XMLHttpRequest/Using_XMLHttpRequest&action=diff&revision=106&diff=107 (comment 14) documents it.

Setting the dev-doc-needed keyword again.

My understanding is that when setting responseType to any of these values, the responseText and reponseXML properties become unusable and one should instead use the response property only during progress events, and the response property contains only the data that has been received since the last progress event, instead of the whole response text. This is mostly a guess (I hope it is correct!) based on what I understood of the code, I would have liked to find some confirmation in the documentation (+ it would have saved time!).

Comment 16

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

responseXML is indeed disabled. But for "moz-chunked-text" reponseText has the same value as. response.

Note that for moz-chunked-text and moz-chunked-arraybuffer you can only access. response during "progress" events.