696586 - Allow access to XHR2 blob response in progress events

Status: RESOLVED FIXED

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

Description

[Masatoshi Kimura [:emk]]

While the blob response tends to be large (it may be even larger than the system RAM!), we are forced to wait until the download completes because the current spec doesn't allow access to the response blob in the LOADING state.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Add "moz-blob" response type and ability to access to the blob in progress events

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 568880 [details] [diff] [review]
Part 2: Add "moz-blob" response type and ability to access to the blob in progress events

Apply this patch on top of bug 689008.
I added the response type to clarify that this ability is a Mozilla extension at the moment.
It will be merged into regular "blob" response type when the spec has been changed.

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Tests

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 568879 [details] [diff] [review]
Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp

>--- a/content/base/src/nsDOMBlobBuilder.cpp
>+++ b/content/base/src/nsDOMBlobBuilder.h
>+#ifndef nsDOMBlobBuilder_h__
>+#define nsDOMBlobBuilder_h__

No trailing underscores, please. All identifiers with two consecutive underscores are reserved for the compiler.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp

Removed trailing underscores.

Comment 7

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

Since blobs never can change size, and since we won't know the size until the download is finished, what is the behavior his patch is implementing?

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to Jonas Sicking (:sicking) from comment #7)
> Since blobs never can change size, and since we won't know the size until
> the download is finished, what is the behavior his patch is implementing?
With this patch, .response will return a different blob object for each progress event. All of those objects are sliced from the same internal nsDOMFileBase object.
.response keeps are reflexivity in the same progress event (I added a testcase to make sure |.response === .response|).

Comment 9

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

What is the use case for this? How is moz-chunked-arraybuffer not enough?

Comment 10

[Masatoshi Kimura [:emk]]

After bug 689008, Blob will be much faster than ArrayBuffer if the response is cached. However, we have to wait until the whole response is downloaded if the response is not cached. If we can access to the blob response in progress events, We don't have to change the access method depending on whether the response is cached or not.

Comment 11

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

I'm still not sure when this would be needed.

I agree that we'd "load" the blob much faster, but if the page wants to access the contents of the resource, it'd have to use a FileReader which would be just as slow as using XHR.

If the page doesn't care about the contents of the resource (for example if it's just going to store it in IndexedDB, or send it to a Worker), then it still has to wait for the whole blob to load.


Could you describe a example application where this would be useful?

Comment 12

[Masatoshi Kimura [:emk]]

For example, access to the file inside ISO image or huge ZIP archive. It requires random access for which chunked-(arraybuffer|text) would not be suitable.
While we can build "Range:" header and receive partial response to achieve random access, it does not sound be intuitive and reliable to me.

Comment 13

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

Comment on attachment 568880 [details] [diff] [review]
Part 2: Add "moz-blob" response type and ability to access to the blob in progress events

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

Don't you need to ensure that everywhere where we clear mResponseBody today, you should now set mBuilder to nsnull?

r=me with those changes.

Your patches are awesome!

::: content/base/src/nsXMLHttpRequest.cpp
@@ +582,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_CLASS(nsXMLHttpRequest)
>  
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsXMLHttpRequest,
>                                                    nsXHREventTarget)

What's the reason for adding all of these to the cycle collector? Some of them seems to make a lot of sense, but others doesn't seem like they could participate in a cycle.

@@ +957,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +nsXMLHttpRequest::CreatePartialBlob(void)

Remove the 'void'

@@ +960,5 @@
> +nsresult
> +nsXMLHttpRequest::CreatePartialBlob(void)
> +{
> +  nsCAutoString contentType;
> +  mChannel->GetContentType(contentType);

I think we should only set the type when we're returning the "final" Blob.

Say for example that you're downloading an image/jpg file. After you've downloaded the first 10 bytes, the 10-byte Blob that we'd return doesn't actually contain a image/jpg file. It just contains the prefix of one. If code looked at the type and tried to treat it as a image/jpg file it would most likely fail.

@@ +968,5 @@
> +      mResponseBlob = mDOMFile;
> +    } else {
> +      mResponseBlob =
> +        mDOMFile->CreateSlice(0, mLoadTransferred,
> +                              NS_ConvertASCIItoUTF16(contentType));

Is there a reason we can't (or even shouldn't) always create a slice here. It seems more predictable that only once the readystate is "done" do we return the final Blob.

@@ +1116,5 @@
> +    *aResult = JSVAL_NULL;
> +    if (mState & XML_HTTP_REQUEST_DONE) {
> +      // do nothing here
> +    } else if (mResponseType == XML_HTTP_RESPONSE_TYPE_MOZ_BLOB &&
> +               mInLoadProgressEvent) {

Is there a reason you have the "mInLoadProgressEvent" test? Elsewhere we only use that for chunked stuff since chunked data is thrown away after each progress event.

I think it'd be fine to get a partial blob at any time for "moz-blob".

@@ +3129,5 @@
>        mResponseBody.Truncate();
>        mResponseText.Truncate();
>        mResultArrayBuffer = nsnull;
> +    } else if (mResponseType == XML_HTTP_RESPONSE_TYPE_MOZ_BLOB) {
> +      mResponseBlob = nsnull;

I think you should do this every time more data is received, so that we allow the page to get an updated blob as often as it wants (similar to how we can update .responseText more often than we send progress events)

::: content/base/src/nsXMLHttpRequest.h
@@ +315,5 @@
>    } mResponseType;
>  
>    nsCOMPtr<nsIDOMBlob> mResponseBlob;
> +  nsRefPtr<nsDOMFileBase> mDOMFile;
> +  nsRefPtr<nsDOMBlobBuilder> mBuilder;

It would help a lot to document all these three members and how they interact. For example that we stream data to mBuilder when the type is "blob" or "moz-blob".

And that mDOMFile is only non-null when we are able to get a os-file representation of the response, i.e. when loading from a file, or when the http-stream caches into a file or is reading from a cached file.

And that mResponseBlob is either a cached blob-response from the last call to GetResponse, but is also explicitly set in OnStopRequest.

Comment 14

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

Comment on attachment 568880 [details] [diff] [review]
Part 2: Add "moz-blob" response type and ability to access to the blob in progress events

Actually, I think I'd like to see updated cycle collector changes before r+'ing.

Comment 15

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

Comment on attachment 568881 [details] [diff] [review]
Part 3: Tests

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

r=me with that fixed.

::: content/base/test/test_xhr_progressevents.html
@@ +51,4 @@
>  
>    if (!data.nodata && !data.encoded) {
> +    if (data.blob) {
> +      is(e.loaded, response.size, "event.loaded matches response size" + test);

This won't always be true if update the blob continuously, right? Can you move this check to after the updateProgress-loop?

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp; r=jonas

unbitrotted

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Add moz-blob response type and ability to access to the blob in progress events, v2

> Don't you need to ensure that everywhere where we clear mResponseBody today,
> you should now set mBuilder to nsnull?
In GetResponseText and MaybeDispatchProgressEvents, it will not be required because we will never reach there if the responseType is "moz-blob"/"blob" and mBuilder will not be used unless the responseType is "moz-blob"/"blob".

> What's the reason for adding all of these to the cycle collector? Some of
> them seems to make a lot of sense, but others doesn't seem like they could
> participate in a cycle.
Probably I'm wrong because I'm not really familiar with the cycle collector
I've reverted all cycle collector changes to separate them to another bug.

> I think we should only set the type when we're returning the "final" Blob.
>
> Say for example that you're downloading an image/jpg file. After you've
> downloaded the first 10 bytes, the 10-byte Blob that we'd return doesn't
> actually contain a image/jpg file. It just contains the prefix of one. If
> code looked at the type and tried to treat it as a image/jpg file it would
> most likely fail.
Changed, but we will return the mime-type for 206 HTTP responses anyway.
Does it also need to change (in another bug)?

> Is there a reason we can't (or even shouldn't) always create a slice here.
> It seems more predictable that only once the readystate is "done" do we
> return the final Blob.
I thought the case of onload/onreadystatechange event being not set. It's possible because we always fire the final progress event.
Also if we always create a slice, final progress event will return the different object from the one returned in load/readtstatechange events despite that the size does not change. That is:
 var b;
 xhr.onprogresss = function() {
   b = xhr.responseType;
 }
 xhr.onload = function() {
   xhr.responseType == b // false
   xhr.responseType.size == b.size // true
 }
Is this really intuitive?

Other points are resolved.

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Tests; r=jonas

> This won't always be true if update the blob continuously, right?

I think it's guaranteed to be true because the partial blob is sliced from mDOMFile using mLoadTransferred. Am I missing something?

Comment 19

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

(In reply to Masatoshi Kimura [:emk] from comment #17)
> Created attachment 591479 [details] [diff] [review]
> Part 2: Add moz-blob response type and ability to access to the blob in
> progress events, v2
> 
> > Don't you need to ensure that everywhere where we clear mResponseBody today,
> > you should now set mBuilder to nsnull?
> In GetResponseText and MaybeDispatchProgressEvents, it will not be required
> because we will never reach there if the responseType is "moz-blob"/"blob"
> and mBuilder will not be used unless the responseType is "moz-blob"/"blob".

Cool.

I wonder if it'd be slightly cleaner to make MaybeDispatchProgressEvents call ResetResponse but make it restore mLoadTransferred after the call. Feel free to make that change here or not, totally up to you.

> > What's the reason for adding all of these to the cycle collector? Some of
> > them seems to make a lot of sense, but others doesn't seem like they could
> > participate in a cycle.
> Probably I'm wrong because I'm not really familiar with the cycle collector
> I've reverted all cycle collector changes to separate them to another bug.

Cool, I look at the changes and it doesn't look like you need to add cycle collector anywhere so the new patch is fine.

For future reference, you should only add members which could point to a cycle collected object to the cycle collector. If a member sometimes points to a cycle collected object and sometimes not you should add it.

If a member never points to a cycle collected object it just wastes cycles to add it to the cycle collector.

> > I think we should only set the type when we're returning the "final" Blob.
> >
> > Say for example that you're downloading an image/jpg file. After you've
> > downloaded the first 10 bytes, the 10-byte Blob that we'd return doesn't
> > actually contain a image/jpg file. It just contains the prefix of one. If
> > code looked at the type and tried to treat it as a image/jpg file it would
> > most likely fail.
> Changed, but we will return the mime-type for 206 HTTP responses anyway.
> Does it also need to change (in another bug)?

For a 206 response we'll always return the full blob, right? So we'll never end up returning a partial file and claim that it has a mimetype.

> > Is there a reason we can't (or even shouldn't) always create a slice here.
> > It seems more predictable that only once the readystate is "done" do we
> > return the final Blob.
> I thought the case of onload/onreadystatechange event being not set. It's
> possible because we always fire the final progress event.
> Also if we always create a slice, final progress event will return the
> different object from the one returned in load/readtstatechange events
> despite that the size does not change. That is:
>  var b;
>  xhr.onprogresss = function() {
>    b = xhr.responseType;
>  }
>  xhr.onload = function() {
>    xhr.responseType == b // false
>    xhr.responseType.size == b.size // true
>  }
> Is this really intuitive?
> 
> Other points are resolved.

Hmm.. I think this is ok but I agree it could be confusing. So I'd say that we can return something with a mimetype before going into the "done" state, but only if we *know* that we've received all contents.

So only if mLoadTotal == mLoadTransferred (note that we set that in MaybeDispatchProgressEvents) we can return the full blob with a mimetype rather than a slice. This might require moving some code around though.

If you want to make those changes here, please attach an interdiff to v2.

Either way, r=me on the patch as is.

Comment 20

[Masatoshi Kimura [:emk]]

Thank you. I would like to land the patch without further change.

> For a 206 response we'll always return the full blob, right? So we'll never end
> up returning a partial file and claim that it has a mimetype.
If XHR added its own "Range:" request header, the partial response was observable from XHR.
Is this a bug of our implementation?

Comment 21

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

Hmm... I'm not actually sure what to do for a Range request. Something to bring up with the webapps WG.

Mind following a followup regarding this and the last issue in comment 19?

Comment 22

[Masatoshi Kimura [:emk]]

Filed bug 721946 and 721949.

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/50cc1a5530da
http://hg.mozilla.org/integration/mozilla-inbound/rev/027e317d7b5c
http://hg.mozilla.org/integration/mozilla-inbound/rev/50d035da14cc

Comment 24

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/50cc1a5530da
https://hg.mozilla.org/mozilla-central/rev/027e317d7b5c
https://hg.mozilla.org/mozilla-central/rev/50d035da14cc

Comment 25

[Eric Shepherd [:sheppy]]

"moz-blob" is now listed among the supported response types:

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

And its use in progress events is mentioned here:

https://developer.mozilla.org/en/DOM/XMLHttpRequest/Using_XMLHttpRequest#In_Firefox_3.5_and_later

I was unable to find any code anywhere to ensure that this documentation is correct, or to build an example to show how it works, so if anyone can help with that, it would be appreciated.

Mentioned on Firefox 12 for developers.

Comment 26

[justin]

(In reply to Masatoshi Kimura [:emk] from comment #0)
> While the blob response tends to be large (it may be even larger than the
> system RAM!), we are forced to wait until the download completes because the
> current spec doesn't allow access to the response blob in the LOADING state.

Hi, I'm a bit confused as to how mob-blob should be used in order to avoid excessive memory use as hinted by this comment. Is there a way to free memory used by the data that has been read so far?
or is this something that a streams API should do (but the streams API is still under development).