Have XHR.getResponseHeader() return null and XHR.getAllResponseHeaders() return empty string in initial ready states

• Component: XML
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Also see https://chromium.googlesource.com/chromium/blink/+/7387fe5c5e40100e9219cd89c71970829eb5108d

[Alexey Proskuryakov]

Is this a duplicate of bug 45994?

[Ryosuke Niwa]

(In reply to comment #2)
> Is this a duplicate of bug 45994?

Slightly different one.

[Ryosuke Niwa]

Also see https://chromium.googlesource.com/chromium/blink/+/39ac9a416be6499ff63cb9e90138a66e7f7a8557

[Alexey Proskuryakov]

(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?

[youenn fablet]

Created attachment 219631 [details]
Patch

[youenn fablet]

Patch also aligns with W3C/WPT tests XMLHttpRequest/getresponseheader-error-state.htm and getresponseheader-unsent-opened-state.htm

[Build Bot]

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

[Build Bot]

Comment on attachment 219631 [details]
Patch

Attachment 219631 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/47398007

[Build Bot]

Comment on attachment 219631 [details]
Patch

Attachment 219631 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/48388164

[youenn fablet]

Created attachment 219634 [details]
fixing IDL

[youenn fablet]

(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.

[Alexey Proskuryakov]

> if that helps the review process

It would certainly help me to have clarity about how these bugs are related.

[youenn fablet]

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.

[Alexey Proskuryakov]

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.

[youenn fablet]

(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?

[youenn fablet]

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.

[youenn fablet]

> 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

[Dominik Röttsches (drott)]

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?

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.