[Webkit-unassigned] [Bug 54162] XHR inconsistent in handling access to response attributes after an error

Mon Feb 14 16:41:21 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=54162

--- Comment #8 from Jarred Nicholls <jarred.nicholls at gmail.com>  2011-02-14 16:41:21 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.