Bug 76760

Summary: Chrome 18 fails html5test.com XHR Blob response test
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Tools / TestsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bugmail, dglazkov, ericu, haraken, info, michaeln, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 40829    
Attachments:
Description Flags
speculative fix
none
Patch
none
Patch
none
Patch abarth: review+, webkit.review.bot: commit-queue-

Eric Seidel (no email)
Reported 2012-01-20 17:56:25 PST
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.
Attachments
speculative fix (1.91 KB, patch)
2012-01-23 03:10 PST, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Patch (6.83 KB, patch)
2012-01-23 16:47 PST, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Patch (8.91 KB, patch)
2012-02-01 15:39 PST, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Patch (11.20 KB, patch)
2012-02-05 23:39 PST, Eric Seidel (no email) 		abarth: review+
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2012-01-20 17:57:49 PST
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)
Comment 2 2012-01-20 20:08:20 PST
See also http://code.google.com/p/chromium/issues/detail?id=52486
Eric U.
Comment 3 2012-01-20 20:16:23 PST
As far as I know, responseBlob just isn't implemented yet.
Eric Seidel (no email)
Comment 4 2012-01-20 20:45:08 PST
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)
Comment 5 2012-01-23 03:10:42 PST
Created attachment 123535 [details] speculative fix
Eric Seidel (no email)
Comment 6 2012-01-23 03:11:37 PST
The AppleMac port disables thsi feature... and the Chromium Mac port is failign to build for me, making this difficult to test.
Adam Barth
Comment 7 2012-01-23 11:37:29 PST
I can help you build the chromium-mac port.
Eric Seidel (no email)
Comment 8 2012-01-23 16:47:43 PST
Created attachment 123659 [details] Patch
Michael Nordman
Comment 9 2012-01-23 17:39:15 PST
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)
Comment 10 2012-01-23 18:19:09 PST
(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 11 2012-01-24 00:09:25 PST
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
Comment 12 2012-01-24 09:20:12 PST
> 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)
Comment 13 2012-02-01 15:35:04 PST
Sorry for the delay. Distracted by other patches/obligations. Have updated the patch and will post shortly.
Eric Seidel (no email)
Comment 14 2012-02-01 15:39:41 PST
Created attachment 125034 [details] Patch
Eric Seidel (no email)
Comment 15 2012-02-01 15:40:14 PST
Updated per Michael's recommendations. I'm happy to make further changes/further testing as desired.
Michael Nordman
Comment 16 2012-02-01 16:14:08 PST
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)
Comment 17 2012-02-01 16:18:31 PST
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 18 2012-02-01 16:24:26 PST
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)
Comment 19 2012-02-01 16:36:55 PST
I will rebaseline chromium after landing. Dealing with the results cascade is neigh impossible.
Michael Nordman
Comment 20 2012-02-01 16:59:41 PST
(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)
Comment 21 2012-02-01 17:05:21 PST
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)
Comment 22 2012-02-05 18:24:43 PST
(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)
Comment 23 2012-02-05 18:26:47 PST
I see. The type should probably be the empty string: http://dev.w3.org/2006/webapi/FileAPI/#attributes-blob
Eric Seidel (no email)
Comment 24 2012-02-05 23:39:16 PST
Created attachment 125582 [details] Patch
WebKit Review Bot
Comment 25 2012-02-06 01:48:57 PST
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)
Comment 26 2012-02-06 09:35:08 PST
I will rebaseline cr-linux when this lands. cr-mac passes for me locally.
Eric Seidel (no email)
Comment 27 2012-02-06 09:40:08 PST
(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 28 2012-02-27 15:41:11 PST
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 29 2012-02-27 15:43:50 PST
Comment on attachment 125582 [details] Patch Looks like you've addressed all of Michael's comments.
Eric Seidel (no email)
Comment 30 2012-03-02 16:57:32 PST
Committed r109635: <http://trac.webkit.org/changeset/109635>
Eric Seidel (no email)
Comment 31 2012-03-02 17:49:17 PST
Committed r109643: <http://trac.webkit.org/changeset/109643>
Kentaro Hara
Comment 32 2012-03-04 16:21:03 PST
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)
Comment 33 2012-03-04 17:54:16 PST
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
Comment 34 2012-03-04 17:55:41 PST
OK, I'll rebaseline it. Thanks.
Note You need to log in before you can comment on or make changes to this bug.