Last Comment Bug 696586 - Allow access to XHR2 blob response in progress events
: Allow access to XHR2 blob response in progress events
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
Depends on: 721949 689008 722962 750023
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-22 08:36 PDT by Masatoshi Kimura [:emk]
Modified: 2015-04-06 20:43 PDT (History)
6 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp (12.92 KB, patch)
2011-10-22 08:39 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Part 2: Add "moz-blob" response type and ability to access to the blob in progress events (22.54 KB, patch)
2011-10-22 08:40 PDT, Masatoshi Kimura [:emk]
jonas: review-
Details | Diff | Review
Part 3: Tests (9.22 KB, patch)
2011-10-22 08:42 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Review
Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp (12.91 KB, patch)
2011-10-22 08:51 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Review
Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp; r=jonas (12.75 KB, patch)
2012-01-25 08:30 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 2: Add moz-blob response type and ability to access to the blob in progress events, v2 (18.99 KB, patch)
2012-01-25 08:33 PST, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Review
Part 3: Tests; r=jonas (9.27 KB, patch)
2012-01-25 08:35 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review

Description Masatoshi Kimura [:emk] 2011-10-22 08:36:08 PDT
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] 2011-10-22 08:39:50 PDT
Created attachment 568879 [details] [diff] [review]
Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp
Comment 2 Masatoshi Kimura [:emk] 2011-10-22 08:40:28 PDT
Created attachment 568880 [details] [diff] [review]
Part 2: Add "moz-blob" response type and ability to access to the blob in progress events
Comment 3 Masatoshi Kimura [:emk] 2011-10-22 08:40:59 PDT
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] 2011-10-22 08:42:06 PDT
Created attachment 568881 [details] [diff] [review]
Part 3: Tests
Comment 5 :Ms2ger 2011-10-22 08:42:24 PDT
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] 2011-10-22 08:51:10 PDT
Created attachment 568884 [details] [diff] [review]
Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp

Removed trailing underscores.
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2011-10-22 14:28:49 PDT
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] 2011-10-22 20:17:22 PDT
(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] (high review load, please consider other reviewers) 2011-10-23 04:37:20 PDT
What is the use case for this? How is moz-chunked-arraybuffer not enough?
Comment 10 Masatoshi Kimura [:emk] 2011-10-23 04:51:13 PDT
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) PTO Until July 5th 2011-10-26 11:42:48 PDT
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] 2011-10-26 18:06:58 PDT
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) PTO Until July 5th 2012-01-25 01:31:28 PST
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) PTO Until July 5th 2012-01-25 01:32:30 PST
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) PTO Until July 5th 2012-01-25 01:36:35 PST
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] 2012-01-25 08:30:09 PST
Created attachment 591477 [details] [diff] [review]
Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp; r=jonas

unbitrotted
Comment 17 Masatoshi Kimura [:emk] 2012-01-25 08:33:58 PST
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".

> 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] 2012-01-25 08:35:25 PST
Created attachment 591480 [details] [diff] [review]
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) PTO Until July 5th 2012-01-26 15:42:30 PST
(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] 2012-01-27 08:18:22 PST
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) PTO Until July 5th 2012-01-27 12:46:28 PST
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] 2012-01-27 21:02:00 PST
Filed bug 721946 and 721949.
Comment 25 Eric Shepherd [:sheppy] 2012-04-24 08:04:50 PDT
"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 2015-04-06 20:43:20 PDT
(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).

Note You need to log in before you can comment on or make changes to this bug.