Bug 54162 – XHR inconsistent in handling access to response attributes after an error

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: XML

[Jarred Nicholls]

WebKit handles access to response data inconsistently as well as against w3c spec after a response error (abort, timeout, security, network) occurs (see http://dev.w3.org/2006/webapi/XMLHttpRequest-2/).

The spec says if the state is < HEADERS_RECEIVED, or the error flag is true, to return 0, null, or empty string for the following response attributes/getters:

- status
- statusText
- getResponseHeader()
- getAllResponseHeaders()

WebKit will return the status/statusText even after an error, such as an abort().  This is against the spec.
WebKit will throw a DOM exception (INVALID_STATE) when calling getResponseHeader() or getAllResponseHeaders() after an abort().  It ignores the error flag altogether.

Firefox will allow access to all the above response attributes after an abort().  This is against the spec, but they behavior consistently across all 4.
IE and Opera will throw DOM exceptions when attempting to access any of the above 4, after an abort().  This is against the spec, but again, they behave consistently across the board.

Since IE (and Opera, but mostly IE) doesn't give access to these properties, I propose we simply do the proper checks to-spec, and return the 0/null/"" values in the applicable cases of m_state < 2 or m_error == true.  Let's face it, if IE doesn't allow it, then no one's really trying to do it (sorry Firefox...)

[Jarred Nicholls]

Created attachment 81932 [details]
Proposed patch

Here is a patch that conforms us to W3C, and makes us consistent.  From a usability standpoint (what's available before/after an error; specifically after an abort), we went onto one side of the fence and are aligning ourselves to IE/Opera.

[WebKit Review Bot]

Attachment 81932 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/xml/XMLHttpRequest.cpp:236:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jarred Nicholls]

Created attachment 81933 [details]
Proposed patch

[Alexey Proskuryakov]

Could you please add per-file comments to LayoutTests/ChangeLog? I started reviewing the changes, but it quickly got complicated.

As an example, I've been looking at xmlhttprequest/exceptions.html - the bug description says what results should be according to the spec, but it doesn't say what IE and Firefox do. That's useful information to add to ChangeLog. I suggest omitting Opera results - they are not very relevant because of Opera market share, and because Opera might mask poor compatibility decisions with site-specific hacks.

Standardizing on XHR2 specified behavior is good long term, but I'm having second thoughts about doing this now. The reason is that a lot of sites and JS libraries have two separate code paths - one for IE and another for "standards compliant" browsers, meaning that they test with Firefox. WebKit  usually falls into the latter group, so changing behavior from Firefox compatible to IE compatible would do little good on these sites.

This is less of an issue than several years ago, since WebKit-based browsers now have a much larger market share, even dominating some segments. But it would be good to know if/when Mozilla is going to switch to XHR2 specified behavior in this regard.

[Jarred Nicholls]

Comment on attachment 81933 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81933&action=review

Overall, the idea here is to make XHR (this is XHR1 spec, not XHR2) to-spec.  It "effectively" behaves the same as IE & Opera, not giving any response data on errors or at incorrect times.  The difference is that it always returns a value and doesn't throw DOM exceptions.

While what you say (about split code paths for IE & Firefox) has some truth in general, I assure you it has zero truth for XHR.  I work on one of the largest JS libraries in the world, and I have used all of the other major libraries out there.  When it comes to handling XHR, there isn't a split code path, ever.  No one is saying "well if Firefox, then yeah let's get this response data even though I just threw an error, else don't do it".  It just doesn't happen.  No one tries to get this data after an error, particularly because IE has never allowed it.

The point is, if IE disallows access to this data, and the spec says that the data shouldn't be accessed, then *no one is trying to do it*.

Keep in mind that the only "effective net difference" this patch has to WebKit is access to status & statusText after an error would no longer give a value.  WebKit throws an exception on getAllResponseHeaders() and getResponseHeader() (therein lies the inconsistency).

Hope that helps explain the position.  Thanks!

> LayoutTests/http/tests/xmlhttprequest/exceptions.html:-68
> -    shouldThrow('req.statusText()');

status and statusText will never throw an exception, so they are not tested for an expected exception.

> LayoutTests/http/tests/xmlhttprequest/getAllResponseHeaders.html:39
> +        }

getAllResponseHeaders() will always return a string.  It will either be a string of the concatenated headers (state >= HEADERS_RECEIVED && !error), or an empty string, but a string nonetheless.  So we test for a string on every call to getAllResponseHeaders().  Essentially any other behavior that is encountered is a failed test case.

IE & Opera: throws exceptions after error
Firefox: allows access after error

> LayoutTests/http/tests/xmlhttprequest/getResponseHeader-expected.txt:19
> +PASSED 1 Content-Type: Content-Type is null.

getResponseHeader() will either return the header string (state >= HEADERS_RECEIVED && !error && headerExists) or null.  It no longer throws exceptions when accessed at the incorrect time.

IE & Opera: throws exception after error
Firefox: allows access after error

> LayoutTests/http/tests/xmlhttprequest/status-after-abort-expected.txt:7
> +Sent request. Response status: 0; statusText: ''; readyState: 1

status and statusText no longer throw exceptions when accessed at the wrong time.  They will either return the status code and status text (state >= HEADERS_RECEIVED && !error), or they will return 0 and "", respectively.  This includes the "abort" error.  An error is an error...

IE & Opera: throws exceptions after error
Firefox: allows access after error

> LayoutTests/http/tests/xmlhttprequest/zero-length-response-expected.txt:21
> +- ResponseXML serialized: n/a

Likewise, see above comments on status and statusText.  This test is specifically testing responsetext, but needed to be updated for status/statusText since they don't throw exceptions.

> LayoutTests/http/tests/xmlhttprequest/zero-length-response.html:91
> +       try { log ("- ResponseXML serialized: " + (req.responseXML ? prettyPrintText(window.XMLSerializer ? (new XMLSerializer()).serializeToString(req.responseXML) : req.responseXML.xml) : "n/a")); } catch (ex) { log("  Exception serializing ResponseXML: " + ex.message); }

Someone used stupid spaces, so I made it more legible ascii :)

[Alexey Proskuryakov]

Comment on attachment 81933 [details]
Proposed patch

I'm almost convinced. However, a quick web search shows that people do care about status and statusText after abort() for various reasons, see e.g. <http://groups.google.com/group/jquery-dev/browse_thread/thread/3d8f7ac78c9b0117>. Reuse of XHR object seems to be a common theme in posts I found.

Also, when we changed abort() to reset status a few years ago, someone quickly filed a bug. One of the tests you modified (status-after-abort.html) was there specifically to make sure that we don't break this again! Please use svn log to check history of tests like this.

I guess that I should also mention Dashboard widgets, which are only tested with WebKit. But there isn't much we can do about those now, except for adding quirks if problems are found.

-        return jsUndefined();
+        return jsNull();

These unrelated changes should be in a separate patch.

-    return String();
+    // Return empty string, not a null String()
+    return "";

This, too. Might be best to do smaller changes like that first.

> status and statusText will never throw an exception, so they are not tested for an expected exception.

Please modify XMLHttpRequest.idl to not require exception handling for methods that can never raise an exception.

I'm not sure why you check m_error in functions like returnXML and returnBlob. Aren't these supposed to be empty already? It's good to match algorithms from spec for clarity, but not when that's confusingly redundant with other code that we have. An ASSERT would be the best way to document that we respect the spec here.

r- for the necessary IDL modification. I've e-mailed the person who filed a bug about status after abort(), so I may have more information later.

[Alexey Proskuryakov]

This change has affected SAP in the past. SAP sends Firefox code to Safari, but I don't know what their use case is exactly.

We should probably try changing the XHR2 spec at this point, although it's a bit late in the game. It doesn't really make a lot of sense to reset status on abort().

[Jarred Nicholls]

(In reply to comment #6)
> (From update of attachment 81933 [details])
> I'm almost convinced. However, a quick web search shows that people do care about status and statusText after abort() for various reasons, see e.g. <http://groups.google.com/group/jquery-dev/browse_thread/thread/3d8f7ac78c9b0117>. Reuse of XHR object seems to be a common theme in posts I found.

They are specifically talking about jQuery's event handling and what it returns (normalize from XHR), not the actual status/statusText of XHR.

Reuse of XHR object is fine.  On the next call to open(), error flag and status is reset.  Reuse after an abort() is completely fine.

> 
> Also, when we changed abort() to reset status a few years ago, someone quickly filed a bug. One of the tests you modified (status-after-abort.html) was there specifically to make sure that we don't break this again! Please use svn log to check history of tests like this.

Ok thanks for the tip, I'll be sure to check the logs on test files.  With that said, I have the humble opinion that that bug report should have been denied, as it was operating according to IE and spec beforehand.  I wonder what convinced the reviewer to approve the patch...

> 
> I guess that I should also mention Dashboard widgets, which are only tested with WebKit. But there isn't much we can do about those now, except for adding quirks if problems are found.

True, and mobile web apps on iOS, Android, BlackBerry 6, webOS, etc. But they are no doubt using a library like jQuery or Sencha Touch.  But if someone expects to get a status code after aborting, they have some very backwards logic in their code :)  That's what the readyState 2 is for, if they're interested in the status code and knowingly aborting.

> 
> -        return jsUndefined();
> +        return jsNull();
> 
> These unrelated changes should be in a separate patch.

This is simply to be more precise as far as return values go for "response" (spec says return null, not undefined), and to be consistent with return proper return values of null for blob/arraybuffer.  But in all honesty, that code will never see the light of day at runtime :)

> 
> -    return String();
> +    // Return empty string, not a null String()
> +    return "";
> 
> This, too. Might be best to do smaller changes like that first.

This is to future-proof statusText incase a ConvertStringToNull attribute was added in the IDL (see getAllResponseHeaders).  Nothing more.  Just thinking ahead.  It's not necessary in this case, but it is necessary in getAllResponseHeaders.

On that note, getAllResponseHeaders should probably instead have its ConvertStringToNull declaration in the IDL removed, since it should always return a string no matter what.

> 
> > status and statusText will never throw an exception, so they are not tested for an expected exception.
> 
> Please modify XMLHttpRequest.idl to not require exception handling for methods that can never raise an exception.

Good call, it was in the back of my mind to do this.

> 
> I'm not sure why you check m_error in functions like returnXML and returnBlob. Aren't these supposed to be empty already? It's good to match algorithms from spec for clarity, but not when that's confusingly redundant with other code that we have. An ASSERT would be the best way to document that we respect the spec here.

I can see your point (any error clears the response).

In the case of responseText, checking the boolean is more performant than calling m_responseBuilder.toStringPreserveCapacity(); unnecessarily.  Maybe a bit reaching, but it's consistent with everything else.

In the case of responseXML, m_createdDocument will be reset after an error, so the code in responseXML will unnecessarily run.  If someone called overrideMimeType('text/xml'), responseXML would so much as create a Document unnecessarily, when checking the error flag would avoid that.

Really the same goes for Blob and ArrayBuffer.  In Blob's case, it wasn't even checking the status at all (yikes).

The reason why the status is not enough of a "blocker" for those functions is because on any error (except for abort), genericError() is called, which sets the status to DONE.  In several of those functions, a status of DONE would pass the if() statement and continue running.  Thus the additional m_error check (to not only future proof the code, but to avoid unnecessary code execution).

> 
> r- for the necessary IDL modification. I've e-mailed the person who filed a bug about status after abort(), so I may have more information later.

Ok cool, thanks.

[Jarred Nicholls]

(In reply to comment #7)
> This change has affected SAP in the past. SAP sends Firefox code to Safari, but I don't know what their use case is exactly.
> 
> We should probably try changing the XHR2 spec at this point, although it's a bit late in the game. It doesn't really make a lot of sense to reset status on abort().

Yeah, perhaps.  This route was chosen because 1) it's how IE works, and 2) it's what the spec says.  The important thing is consistency across the board...it's just a pet peeve of mine.  I don't care if we went 180 degrees in the opposite direction and follow Firefox, and allow access to status, statusText, getAllResponseHeaders, and getResponseHeader after an abort :)  But this patch is not just about abort(), it's about all errors and returning the proper response types (i.e. not throwing an exception, or return "" instead of null, etc.).

[Jarred Nicholls]

Found this at the bottom of my stack.  What do you think Alexey, should we pursue this a little further?

[Alexey Proskuryakov]

Seems best to split the changes into as small chunks as possible. That would make both discussion and regression testing more straightforward.

[Dominik Röttsches (drott)]

(In reply to comment #10)
> Found this at the bottom of my stack.  What do you think Alexey, should we pursue this a little further?

Jarred, do you have any plans to update this patch?

[Jarred Nicholls]

(In reply to comment #12)
> (In reply to comment #10)
> > Found this at the bottom of my stack.  What do you think Alexey, should we pursue this a little further?
> 
> Jarred, do you have any plans to update this patch?

Sorry for the delayed response - been filtering (and yes, ignoring) a lot of email to @webkit.org.  No plans for the very short term.  With that said I'd like to see these inconsistencies addressed :)

[Dominik Röttsches (drott)]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > Found this at the bottom of my stack.  What do you think Alexey, should we pursue this a little further?
> > 
> > Jarred, do you have any plans to update this patch?
> 
> Sorry for the delayed response - been filtering (and yes, ignoring) a lot of email to @webkit.org.  No plans for the very short term.  With that said I'd like to see these inconsistencies addressed :)

So you mean someone could take over?

[Ryosuke Niwa]

A related patch has been landed in http://trac.webkit.org/changeset/161051.

[youenn fablet]

(In reply to comment #15)
> A related patch has been landed in http://trac.webkit.org/changeset/161051.

bug 125840 and bug 127050 are probably sub-bugs of this one as well.

[Anne van Kesteren]

I'm pretty sure all issues here have been resolved over the intervening years.