The default bug view has changed. See this FAQ.

Allow access to XHR2 blob response in progress events

RESOLVED FIXED in mozilla12

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

unspecified
mozilla12
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 568879 [details] [diff] [review]
Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #568879 - Flags: review?(jonas)
(Assignee)

Comment 2

6 years ago
Created attachment 568880 [details] [diff] [review]
Part 2: Add "moz-blob" response type and ability to access to the blob in progress events
Attachment #568880 - Flags: review?(jonas)
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 689008
(Assignee)

Comment 4

6 years ago
Created attachment 568881 [details] [diff] [review]
Part 3: Tests
Attachment #568881 - Flags: review?(jonas)
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.
(Assignee)

Comment 6

6 years ago
Created attachment 568884 [details] [diff] [review]
Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp

Removed trailing underscores.
Attachment #568879 - Attachment is obsolete: true
Attachment #568879 - Flags: review?(jonas)
Attachment #568884 - Flags: review?(jonas)
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?
(Assignee)

Comment 8

6 years ago
(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

6 years ago
What is the use case for this? How is moz-chunked-arraybuffer not enough?
(Assignee)

Comment 10

6 years ago
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.
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?
(Assignee)

Comment 12

6 years ago
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.
Attachment #568884 - Flags: review?(jonas) → review+
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.
Attachment #568880 - Flags: review?(jonas) → review+
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.
Attachment #568880 - Flags: review+ → review-
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?
Attachment #568881 - Flags: review?(jonas) → review+
(Assignee)

Comment 16

5 years ago
Created attachment 591477 [details] [diff] [review]
Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp; r=jonas

unbitrotted
Attachment #568884 - Attachment is obsolete: true
Attachment #591477 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #591477 - Attachment description: Part1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp; r=jonas → Part 1: Split nsDOMBlobBuilder.h from nsDOMBlobBuilder.cpp; r=jonas
(Assignee)

Comment 17

5 years ago
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.
Attachment #568880 - Attachment is obsolete: true
Attachment #591479 - Flags: review?(jonas)
(Assignee)

Comment 18

5 years ago
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?
Attachment #568881 - Attachment is obsolete: true
Attachment #591480 - Flags: review+
(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.
Attachment #591479 - Flags: review?(jonas) → review+
(Assignee)

Comment 20

5 years ago
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?
Keywords: checkin-needed
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?
(Assignee)

Updated

5 years ago
Depends on: 721949
(Assignee)

Comment 22

5 years ago
Filed bug 721946 and 721949.
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
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/50cc1a5530da
https://hg.mozilla.org/mozilla-central/rev/027e317d7b5c
https://hg.mozilla.org/mozilla-central/rev/50d035da14cc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Depends on: 722962
"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.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

5 years ago
Depends on: 750023

Comment 26

2 years ago
(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).
You need to log in before you can comment on or make changes to this bug.