| Summary: | Chrome 18 fails html5test.com XHR Blob response test | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||
| Component: | Tools / Tests | Assignee: | 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: |
|
||||||||||||
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 As far as I know, responseBlob just isn't implemented yet. 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 Created attachment 123535 [details]
speculative fix
The AppleMac port disables thsi feature... and the Chromium Mac port is failign to build for me, making this difficult to test. I can help you build the chromium-mac port. Created attachment 123659 [details]
Patch
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. (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())? 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 > 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". Sorry for the delay. Distracted by other patches/obligations. Have updated the patch and will post shortly. Created attachment 125034 [details]
Patch
Updated per Michael's recommendations. I'm happy to make further changes/further testing as desired. 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? 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. 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 I will rebaseline chromium after landing. Dealing with the results cascade is neigh impossible. (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) Thank you for the clarification. I will update the patch to return an empty blob in the after-done, zero-length response case. (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... I see. The type should probably be the empty string: http://dev.w3.org/2006/webapi/FileAPI/#attributes-blob Created attachment 125582 [details]
Patch
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 I will rebaseline cr-linux when this lands. cr-mac passes for me locally. (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. 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. Comment on attachment 125582 [details]
Patch
Looks like you've addressed all of Michael's comments.
Committed r109635: <http://trac.webkit.org/changeset/109635> Committed r109643: <http://trac.webkit.org/changeset/109643> 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? 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. OK, I'll rebaseline it. Thanks. |
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.