Bug 76760 – Chrome 18 fails html5test.com XHR Blob response test

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Chrome 18 fails html5test.com XHR Blob response test

This is the test:

testResponseTypeBlob: function(item) {
					if (!window.XMLHttpRequest || !window.Blob) return;
					
					var xhr = new window.XMLHttpRequest();

					if (typeof xhr.responseType == 'undefined') return;
										
					var done = false;

					xhr.onreadystatechange = function() {
						if (this.readyState == 4 && !done) {
							done = true;
							passed = false;
							
							try {
								passed = !!(this.response && this.response instanceof Blob);
							} catch(e) {
							}							

							item.stopBackground();
							item.update({
								'passed': passed
							});
						}
					}
					
					try {
						item.startBackground();
						xhr.open("GET", "detect.html");
						xhr.responseType = "blob";
						xhr.send();
					} catch (e) {
						item.stopBackground();
					}
				},

i've run the test manually in the inspector (while inspecting the main page on html5test.com)

And found that we get xhr.response == null when fetching detect.html.

Unclear why.  It's also possible the test is wrong.

[Eric Seidel (no email)]

By "run the test manually", I mean I executed the following in the inspector, while inspecting the main page on that site (so my XHR's would not be x-domain):


> var xhr = new window.XMLHttpRequest();
undefined
> xhr.open("GET", "detect.html");
						xhr.responseType = "blob";
						xhr.send();
undefined
> xhr.responseType
"blob"
> xhr.readyState
4
> xhr.response
null

[Eric Seidel (no email)]

See also http://code.google.com/p/chromium/issues/detail?id=52486

[Eric U.]

As far as I know, responseBlob just isn't implemented yet.

[Eric Seidel (no email)]

I was mistaken in relating thsi to the Chromium bug.  This bug is about .response being a Blob, not about the responseBlob accessor.

However, I think you're right.

It's enabled:
http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/features.gypi#L101

But not implemented??
http://trac.webkit.org/browser/trunk/Source/WebCore/xml/XMLHttpRequest.cpp#L1033
http://trac.webkit.org/browser/trunk/Source/WebCore/xml/XMLHttpRequest.cpp#L261

[Eric Seidel (no email)]

Created attachment 123535 [details]
speculative fix

[Eric Seidel (no email)]

The AppleMac port disables thsi feature... and the Chromium Mac port is failign to build for me, making this difficult to test.

[Adam Barth]

I can help you build the chromium-mac port.

[Eric Seidel (no email)]

Created attachment 123659 [details]
Patch

[Michael Nordman]

Comment on attachment 123659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123659&action=review

> Source/WebCore/xml/XMLHttpRequest.cpp:1035
> +    if (m_responseTypeCode == ResponseTypeBlob) {

This is handled a little differently for the ResponseTypeArrayBuffer case
* creation of the array buffer is deferred until the accessor is called
* and after it's created (which similarly involves copying the data), m_binaryResponseBuilder's copy of the data is cleared.

I'd vote to make responseBlob handling more like responseArrayBuffer handling, so do this Blob::creation stuff in XMLHttpRequest::responseBlob() instead of in this method. Wdty?

> Source/WebCore/xml/XMLHttpRequest.cpp:1042
> +        // a SharedBuffer, even if they don't get the Blob from the network layer directly.

I've got real mixed feelings about implementing it in this way. Developers trying to use this feature for 'large' amounts of data will probably not find this so satisfying. The responseArrayBuffer thing is functionally equivalent to this impl. This impl of responseBlob doesn't actually add value over that.

> Source/WebCore/xml/XMLHttpRequest.cpp:1046
> +        OwnPtr<BlobData> blobData = BlobData::create();

We should probably also set the content type here.

[Eric Seidel (no email)]

(In reply to comment #9)
> (From update of attachment 123659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123659&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:1035
> > +    if (m_responseTypeCode == ResponseTypeBlob) {
> 
> This is handled a little differently for the ResponseTypeArrayBuffer case
> * creation of the array buffer is deferred until the accessor is called
> * and after it's created (which similarly involves copying the data), m_binaryResponseBuilder's copy of the data is cleared.
> 
> I'd vote to make responseBlob handling more like responseArrayBuffer handling, so do this Blob::creation stuff in XMLHttpRequest::responseBlob() instead of in this method. Wdty?

That's fine.  I can change it to match responseArrayBuffer if you're OK with this sort of "hacky" solution in general.

> > Source/WebCore/xml/XMLHttpRequest.cpp:1042
> > +        // a SharedBuffer, even if they don't get the Blob from the network layer directly.
> 
> I've got real mixed feelings about implementing it in this way. Developers trying to use this feature for 'large' amounts of data will probably not find this so satisfying. The responseArrayBuffer thing is functionally equivalent to this impl. This impl of responseBlob doesn't actually add value over that.

I agree.  It's definitely a hack.  I'm not sure how much it matters in the short-term.

There seems to be some interest from developers for this feature, but I'm not sure how many complaints we would have about an implementation with potentially poor performance like this one:
http://stackoverflow.com/questions/8022425/getting-blob-data-from-xhr-request
http://code.google.com/p/chromium/issues/detail?id=52486

> > Source/WebCore/xml/XMLHttpRequest.cpp:1046
> > +        OwnPtr<BlobData> blobData = BlobData::create();
> 
> We should probably also set the content type here.

OK.  I assume that should be: blobData->setContentType(m_response->mimeType())?

[WebKit Review Bot]

Comment on attachment 123659 [details]
Patch

Attachment 123659 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11335291

New failing tests:
fast/files/xhr-response-blob.html

[Michael Nordman]

> That's fine.  I can change it to match responseArrayBuffer if you're OK with this sort of "hacky" solution in general.

sgtm

> OK.  I assume that should be: blobData->setContentType(m_response->mimeType())?

How XMLHttpRequest::responseMIMEType() computes its return value looks relevant, except for how it defaults to "text/xml".

[Eric Seidel (no email)]

Sorry for the delay.  Distracted by other patches/obligations.  Have updated the patch and will post shortly.

[Eric Seidel (no email)]

Created attachment 125034 [details]
Patch

[Eric Seidel (no email)]

Updated per Michael's recommendations.  I'm happy to make further changes/further testing as desired.

[Michael Nordman]

Comment on attachment 125034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125034&action=review

> Source/WebCore/xml/XMLHttpRequest.cpp:275
> +    if (!m_responseBlob.get() && m_binaryResponseBuilder.get() && m_binaryResponseBuilder->size() > 0) {

if size() == 0, should this method return a zero-length blob rather than NULL?

[Eric Seidel (no email)]

I don't believe that's possible.  Unless didRecieveData is called in the zero-length response case.  I can construct a test case for that cornercase if necessary.  I suspect that case should be regarded the same as the FIXME about always returning a blob like the spec says.  But I'm unclear if the spec is right there, since then once you set responseType='blob' .response is never null.

[WebKit Review Bot]

Comment on attachment 125034 [details]
Patch

Attachment 125034 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11389646

New failing tests:
fast/files/xhr-response-blob.html

[Eric Seidel (no email)]

I will rebaseline chromium after landing.  Dealing with the results cascade is neigh impossible.

[Michael Nordman]

(In reply to comment #17)
> I don't believe that's possible.  Unless didRecieveData is called in the zero-length response case.  I can construct a test case for that cornercase if necessary.  I suspect that case should be regarded the same as the FIXME about always returning a blob like the spec says.  But I'm unclear if the spec is right there, since then once you set responseType='blob' .response is never null.


http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-response-attribute
About the .response attribute, looks like the spec says that incremental reading only works when the .responseType attibute is 'text', otherwise, if accessed prior to being DONE you get NULL. We still have type specific .responseXXXX getters, but modulo that difference...

... for the 'blob' type

* if accessed prior to DONE --> NULL
* if accessed after DONE --> NON-NULL (an empty blob in zero-length cases or error cases)

[Eric Seidel (no email)]

Thank you for the clarification.  I will update the patch to return an empty blob in the after-done, zero-length response case.

[Eric Seidel (no email)]

(In reply to comment #20)
> (In reply to comment #17)
> > I don't believe that's possible.  Unless didRecieveData is called in the zero-length response case.  I can construct a test case for that cornercase if necessary.  I suspect that case should be regarded the same as the FIXME about always returning a blob like the spec says.  But I'm unclear if the spec is right there, since then once you set responseType='blob' .response is never null.
> 
> 
> http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-response-attribute
> About the .response attribute, looks like the spec says that incremental reading only works when the .responseType attibute is 'text', otherwise, if accessed prior to being DONE you get NULL. We still have type specific .responseXXXX getters, but modulo that difference...
> 
> ... for the 'blob' type
> 
> * if accessed prior to DONE --> NULL
> * if accessed after DONE --> NON-NULL (an empty blob in zero-length cases or error cases)

I'm confused as to what the blob.response.type should be in the error or empty cases...

[Eric Seidel (no email)]

I see.  The type should probably be the empty string:
http://dev.w3.org/2006/webapi/FileAPI/#attributes-blob

[Eric Seidel (no email)]

Created attachment 125582 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125582 [details]
Patch

Attachment 125582 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11434218

New failing tests:
fast/files/xhr-response-blob.html

[Eric Seidel (no email)]

I will rebaseline cr-linux when this lands.  cr-mac passes for me locally.

[Eric Seidel (no email)]

(In reply to comment #20)
> http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-response-attribute
> About the .response attribute, looks like the spec says that incremental reading only works when the .responseType attibute is 'text', otherwise, if accessed prior to being DONE you get NULL. We still have type specific .responseXXXX getters, but modulo that difference...
> 
> ... for the 'blob' type
> 
> * if accessed prior to DONE --> NULL
> * if accessed after DONE --> NON-NULL (an empty blob in zero-length cases or error cases)

I've tested all 3 of these cases in my latest test case.  I'm happy to test additional cases if desired.  Just let me know.

[Adam Barth]

Comment on attachment 125582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125582&action=review

> Source/WebCore/xml/XMLHttpRequest.cpp:283
> +        if (m_binaryResponseBuilder.get()) {

No need for "get()" here.

[Adam Barth]

Comment on attachment 125582 [details]
Patch

Looks like you've addressed all of Michael's comments.

[Eric Seidel (no email)]

Committed r109635: <http://trac.webkit.org/changeset/109635>

[Eric Seidel (no email)]

Committed r109643: <http://trac.webkit.org/changeset/109643>

[Kentaro Hara]

fast/files/xhr-response-blob.html is failing in Chromium/Win and Chromium/Mac:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Ffiles%2Fxhr-response-blob.html

Is it OK to rebaseline?

[Eric Seidel (no email)]

Yes, the mime-type difference was not an intended part of the test.  Feel free to rebaseline, I'll go about fixing the test to not include this "what mime type does the OS thing '.js' means" part of the test.  It's unclear to me if the default mime type for .js matters, or if it's just an expected OS difference.  In any case, it's not what this test is supposed to be testing.

[Kentaro Hara]

OK, I'll rebaseline it. Thanks.