| Summary: | [XHR] Ensure response return null when error flag is set for blob and arraybuffer | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> |
| Component: | XML | Assignee: | youenn fablet <youennf> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | ap, buildbot, commit-queue, eflews.bot, gyuyoung.kim, rniwa |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
youenn fablet
2014-01-15 07:45:33 PST
Created attachment 221270 [details]
First patch
Comment on attachment 221270 [details] First patch Attachment 221270 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5314705006526464 New failing tests: fast/files/xhr-response-blob.html Created attachment 221272 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 221270 [details] First patch Attachment 221270 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6184651169202176 New failing tests: fast/files/xhr-response-blob.html Created attachment 221274 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 221277 [details]
Fixing failing test
Comment on attachment 221277 [details] Fixing failing test View in context: https://bugs.webkit.org/attachment.cgi?id=221277&action=review > Source/WebCore/xml/XMLHttpRequest.cpp:241 > + if (m_error) > + return ""; Hmm, how are we supposed to handle interrupted connections? We would have responseText attribute have the partial text until the error, at which time it duddenly becomes empty. That's strange. > Source/WebCore/xml/XMLHttpRequest.cpp:327 > + if (!doneWithoutErrors()) > + return nullptr; Do we now have two places where we essentially null out the response - one here for the ontimeout/onabort case, and another elsewhere for all the cases that already worked correctly? Can they be merged? > LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:8 > + <script src="../../../resources/testharness.js"></script> > + <script src="../../../resources/testharnessreport.js"></script> Why the relative paths? I think that this should be just "/resources/testharness.js". > LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:17 > + var url = "/resources/load-then-wait.cgi?name=../../../http/tests/xmlhttprequest/" + fileName + "&waitFor=0.1&mimeType=" + mimeType Also unsure about this deep relative path. (In reply to comment #7) > (From update of attachment 221277 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221277&action=review > > > Source/WebCore/xml/XMLHttpRequest.cpp:241 > > + if (m_error) > > + return ""; > > Hmm, how are we supposed to handle interrupted connections? We would have responseText attribute have the partial text until the error, at which time it duddenly becomes empty. That's strange. I am not exactly sure to understand which case you are referring to. Can you give details? For usual cases, I do not think the behavior is changed. The response buffer is cleared when m_error is set to true. One potential case where the patch changes the behavior is if a call to xhr.abort() or xhr.open() triggers a window.onload event (as part of cancelling the loader in internalAbort()). Inside the window.onload event callback, the m_error flag is already set to true but the response buffer is not yet cleared. I can see room for improvement in that area (like ensuring that response information remains accessible for the last progress event before the readystatechange DONE event is sent), but that seems like bigger changes than this patch. > > > Source/WebCore/xml/XMLHttpRequest.cpp:327 > > + if (!doneWithoutErrors()) > > + return nullptr; > > Do we now have two places where we essentially null out the response - one here for the ontimeout/onabort case, and another elsewhere for all the cases that already worked correctly? > > Can they be merged? We can factorize the doneWithoutError check in JSXMLHttpRequest::response for all responseType values except ResponseTypeDefault and ResponseTypeText. This would let the behavior unchanged but the code would look more closely aligned with the spec. I will upload a new patch accordingly Note that onabort test works without the patch. That may change when fixing abort() behavior, as proposed in bug 98404, attachment 221163 [details]. ontimeout cases do not work without the patch right now. > > > LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:8 > > + <script src="../../../resources/testharness.js"></script> > > + <script src="../../../resources/testharnessreport.js"></script> > > Why the relative paths? I think that this should be just "/resources/testharness.js". Right, they should be removed. > > > LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:17 > > + var url = "/resources/load-then-wait.cgi?name=../../../http/tests/xmlhttprequest/" + fileName + "&waitFor=0.1&mimeType=" + mimeType > > Also unsure about this deep relative path. This is inspired from xmlhttprequest-timeout.js I will remove it as well. Created attachment 221356 [details]
factorize doneWithoutError check
Comment on attachment 221356 [details] factorize doneWithoutError check Attachment 221356 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/5834558519902208 Comment on attachment 221356 [details] factorize doneWithoutError check Attachment 221356 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/4692406255484928 Comment on attachment 221356 [details] factorize doneWithoutError check Attachment 221356 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5478046505107456 Comment on attachment 221356 [details] factorize doneWithoutError check Attachment 221356 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5962754502426624 Created attachment 221374 [details]
fixed compilation error
Created attachment 221377 [details]
Readded precondition in responseXML
Comment on attachment 221377 [details] Readded precondition in responseXML Attachment 221377 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/4942846872256512 Created attachment 221380 [details]
Fixed typo
Created attachment 222319 [details]
Made tests more robust
Comment on attachment 222319 [details] Made tests more robust View in context: https://bugs.webkit.org/attachment.cgi?id=222319&action=review > Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:-236 > + default: > + return jsUndefined(); > } > > - return jsUndefined(); Why this change? The point of leaving out the default is that we get a warning from compilers like clang if any enum value does not have a case. By adding a default, that warning is turned off. > Source/WebCore/xml/XMLHttpRequest.cpp:291 > + ASSERT(m_responseTypeCode == ResponseTypeBlob && doneWithoutErrors()); This should be two assertions, not one. If the assertion fails we’d like to know which half failed. ASSERT(m_responseTypeCode == ResponseTypeBlob); ASSERT(doneWithoutErrors()); > Source/WebCore/xml/XMLHttpRequest.cpp:321 > + ASSERT(m_responseTypeCode == ResponseTypeArrayBuffer && doneWithoutErrors()); Ditto. > > Hmm, how are we supposed to handle interrupted connections? We would have responseText attribute have the partial text until the error, at which time it duddenly becomes empty. That's strange. > I am not exactly sure to understand which case you are referring to. > Can you give details? Here is the scenario I'm referring to: 1. A load starts. 2. With every onreadystatechange, JS code looks at xhr.responseText, which contains all the data that has been received so far. 3. The server suddenly decides to drop the connection. 4. The error is detected client side. 5. At this point xhr.responseText suddenly becomes empty. This seems weird. Comment on attachment 222319 [details] Made tests more robust View in context: https://bugs.webkit.org/attachment.cgi?id=222319&action=review > LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:72 > + runTest("getting XML response within abort event callback", > + "onabort-response-getters.html","text/html", > + function(test, client) {}, > + function(test, client) {assert_true(client.responseXML == null, "xml response must be empty")} > + ) I don't think that this test actually works. Parsing a malformed XML document results in a null responseXML anyway. We'd probably need a test case that has many space characters after the XML, and only abort once the whole XML part has been received. (In reply to comment #19) > (From update of attachment 222319 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222319&action=review > > > Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:-236 > > + default: > > + return jsUndefined(); > > } > > > > - return jsUndefined(); > > Why this change? The point of leaving out the default is that we get a warning from compilers like clang if any enum value does not have a case. By adding a default, that warning is turned off. Some plaforms use -Werror -Wswitch, leading to failing builds. That is what happened in https://bugs.webkit.org/attachment.cgi?id=221356 > > > Source/WebCore/xml/XMLHttpRequest.cpp:291 > > + ASSERT(m_responseTypeCode == ResponseTypeBlob && doneWithoutErrors()); > > This should be two assertions, not one. If the assertion fails we’d like to know which half failed. > > ASSERT(m_responseTypeCode == ResponseTypeBlob); > ASSERT(doneWithoutErrors()); > > > Source/WebCore/xml/XMLHttpRequest.cpp:321 > > + ASSERT(m_responseTypeCode == ResponseTypeArrayBuffer && doneWithoutErrors()); > > Ditto. OK, I will fix this (In reply to comment #21) > (From update of attachment 222319 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222319&action=review > > > LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:72 > > + runTest("getting XML response within abort event callback", > > + "onabort-response-getters.html","text/html", > > + function(test, client) {}, > > + function(test, client) {assert_true(client.responseXML == null, "xml response must be empty")} > > + ) > > I don't think that this test actually works. Parsing a malformed XML document results in a null responseXML anyway. > > We'd probably need a test case that has many space characters after the XML, and only abort once the whole XML part has been received. I think that the load-then-wait.cgi should already handle that: it sends the whole document and waits some time before ending the response. It is true though that the test may suffer from changes in the sending of progress events. To ensure that the test is actually working, abort() should be called when onprogress event loaded = total. (In reply to comment #20) > > > Hmm, how are we supposed to handle interrupted connections? We would have responseText attribute have the partial text until the error, at which time it duddenly becomes empty. That's strange. > > > I am not exactly sure to understand which case you are referring to. > > Can you give details? > > Here is the scenario I'm referring to: > > 1. A load starts. > 2. With every onreadystatechange, JS code looks at xhr.responseText, which contains all the data that has been received so far. > 3. The server suddenly decides to drop the connection. > 4. The error is detected client side. > 5. At this point xhr.responseText suddenly becomes empty. > > This seems weird. This is already happening today. In case of a network error, XMLHttpRequest::genericError will probably be called and the response will be cleared before m_error is even set. In that case, the proposed change in XMLHttpRequest::responseText will not change the behavior (responseText will return "" anyway). It will just change the code path. The proposed change will only change the behavior in some specific cases when m_error is set to true and the response is not yet cleared. To ease the review, I will split that change in a different bug. Created attachment 222587 [details]
Limiting change to arraybuffer and blob
Comment on attachment 222587 [details] Limiting change to arraybuffer and blob View in context: https://bugs.webkit.org/attachment.cgi?id=222587&action=review > Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:193 > + // FIXME: Use CachedAttribute for other types than JSON as well. I'm not sure why this is something we'd want to do. Other types are pure DOM ones, so caching in XMLHttpRequest is sufficient - and if the returned object has a wrapper already, we'll pick up that wrapper. I see that you only moved this comment, yet it would be good to clarify it. (In reply to comment #26) > (From update of attachment 222587 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222587&action=review > > > Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:193 > > + // FIXME: Use CachedAttribute for other types than JSON as well. > > I'm not sure why this is something we'd want to do. > > Other types are pure DOM ones, so caching in XMLHttpRequest is sufficient - and if the returned object has a wrapper already, we'll pick up that wrapper. > > I see that you only moved this comment, yet it would be good to clarify it. I guess that, When responseType is not "default", all response builders could be cleared once the response object is in cache. Response builders are currently cleared for "json", "blob" and "arrayBuffer", but not for "text" and "document". Since the code for caching is already there, this may also slightly reduce code footprint. Comment on attachment 222587 [details] Limiting change to arraybuffer and blob Clearing flags on attachment: 222587 Committed r163527: <http://trac.webkit.org/changeset/163527> All reviewed patches have been landed. Closing bug. |