Consider merging https://chromium.googlesource.com/chromium/blink/+/d201caf874a0bd6f101f517462b3cf1d8c5fce3d Follow the XMLHttpRequest specification for getResponseHeader() and return null if the object is in ready states {UNSENT, OPENED}. Similarly for getAllResponseHeaders() and returning "" (the empty string) when in these initial two ready states. This replaces the older spec behavior of throwing InvalidStateError, something that no other engines currently do, hence cross-browser compatibility risk is low. When reconciling the relevant tests, reworked these to use js-test-{pre,post}.js instead.
Also see https://chromium.googlesource.com/chromium/blink/+/7387fe5c5e40100e9219cd89c71970829eb5108d
Is this a duplicate of bug 45994?
(In reply to comment #2) > Is this a duplicate of bug 45994? Slightly different one.
Also see https://chromium.googlesource.com/chromium/blink/+/39ac9a416be6499ff63cb9e90138a66e7f7a8557
(In reply to comment #3) > (In reply to comment #2) > > Is this a duplicate of bug 45994? > > Slightly different one. Could you please elaborate on what the difference is, and whether we need to keep both bugs?
Created attachment 219631 [details] Patch
Patch also aligns with W3C/WPT tests XMLHttpRequest/getresponseheader-error-state.htm and getresponseheader-unsent-opened-state.htm
Comment on attachment 219631 [details] Patch Attachment 219631 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/49828187
Comment on attachment 219631 [details] Patch Attachment 219631 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/47398007
Comment on attachment 219631 [details] Patch Attachment 219631 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/48388164
Created attachment 219634 [details] fixing IDL
(In reply to comment #5) > (In reply to comment #3) > > (In reply to comment #2) > > > Is this a duplicate of bug 45994? > > > > Slightly different one. > > Could you please elaborate on what the difference is, and whether we need to keep both bugs? I had not checked the thread messages before... Checking bug 45994, it makes sense to me to migrate that patch (probably with the related blink clearResponse method update) into 45994 patch if that helps the review process.
> if that helps the review process It would certainly help me to have clarity about how these bugs are related.
FWIU, the relationship lies in the fact that the behavior of getResponseHeader(), getAllResponseHeaders(), status and statusText should be consistent when the request is not sent: - In the past (http://www.w3.org/TR/2009/WD-XMLHttpRequest2-20090820/), an InvalidStateError was thrown when state was UNSENT or OPENED. - Since http://www.w3.org/TR/2010/WD-XMLHttpRequest2-20100907, the behavior was changed to return null/O/empty string. I do not think that there is any other "code interaction" between the two bugs. Given that bug 45994 description is now focused on status/statusText, it seems to make sense to process those two in parallel.
Comment on attachment 219634 [details] fixing IDL View in context: https://bugs.webkit.org/attachment.cgi?id=219634&action=review > Source/WebCore/ChangeLog:3 > + Have XHR.getResponseHeader() return null in initial ready states Here and in the bug title, please mention getAllResponseHeaders as well. > Source/WebCore/ChangeLog:8 > + Merging https://chromium.googlesource.com/chromium/blink/+/d201caf874a0bd6f101f517462b3cf1d8c5fce3d What do other browsers do? The spec changed many times over the last few years, and it can't be taken very seriously. > Source/WebCore/xml/XMLHttpRequest.cpp:1013 > + if (m_state < HEADERS_RECEIVED || m_error) Why was "|| m_error" added here? This is not mentioned in ChangeLog. If this is a behavior change, it should be landed in a separate patch to facilitate regression testing when a site breaks. If there is no behavior change, why is the code changed? I'm not sure if there is any test coverage for this.
(In reply to comment #15) > What do other browsers do? The spec changed many times over the last few years, and it can't be taken very seriously. > Firefox is aligned with the patch and latest spec. Blink recently aligned to it as well. > > Source/WebCore/xml/XMLHttpRequest.cpp:1013 > > + if (m_state < HEADERS_RECEIVED || m_error) > > Why was "|| m_error" added here? This is not mentioned in ChangeLog. > It makes it clearer that the code is aligned with the spec. The code is also made more robust. > If this is a behavior change, it should be landed in a separate patch to facilitate regression testing when a site breaks. If there is no behavior change, why is the code changed? I'm not sure if there is any test coverage for this. I can add a test to cover the "m_error" check (e.g. xhr.send();xhr.abort();). The test would pass today without the patch: the current code is already aligned with the spec in most cases, m_response is cleared when m_error is set to true. There may be corner cases though but I am not aware of any right now. Filing a separate bug may therefore be safer?
Created attachment 221050 [details] fixed change logs and added test case for new code path patch contains a new test that check getResponseHeader and getAllResponseHeader when state is DONE and error flag is set. The test is passing with and without the patch but it allows exercicing the new code path introduced in the patch.
> There may be corner cases though but I am not aware of any right now. > Filing a separate bug may therefore be safer? Here is potentially such a corner case: var xhr = new XMLHttpRequest() xhr.onprogress = function(e) { // abort will trigger window.onload xhr.abort() } window.onload = function () { // error flag is set to true but response headers are not yet cleared // without the patch, getResponseHeader will NOT return null // with the patch, getResponseHeader will return null ... } xhr.onreadystatechange = function() { if (xhr.readyState == xhr.DONE) { // with and without the patch, getResponseHeader will return null ... } } // give time to abort on progress event xhr.open("GET", "/resources/load-and-stall.cgi?name=response-access-on-error.html&stallFor=0.1&stallAt=100&mimeType=text/html", true) xhr.setRequestHeader("Content-Type","text/plain") xhr.send(null) This may also apply to bug 45994 and bug 127050
I see you set this to cq? - Did you intend to set it to cq+? Or were you waiting for a committer to cq+ this?
Comment on attachment 221050 [details] fixed change logs and added test case for new code path Clearing flags on attachment: 221050 Committed r163022: <http://trac.webkit.org/changeset/163022>
All reviewed patches have been landed. Closing bug.