Bug 13141 – XMLHttpRequest should set readyState to 0 after abort()

• Component: XML
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

See bug URL for a test case.

[Anne van Kesteren]

http://dev.w3.org/cvsweb/~checkout~/2006/webapi/XMLHttpRequest/Overview.html?content-type=text/html;%20charset=utf-8#dfn-abort should define accurately how abort() is to be implemented.

[Julien Chaffraix]

Created attachment 16423 [details]
Proposed patch and testcase

The expected results of the test case seem strange and might be related to the platform on which I have tested it.

Opera goes all the way to the 4th state, which should be the case here too.

[Eric Seidel (no email)]

Comment on attachment 16423 [details]
Proposed patch and testcase

The patch in general looks fine.  Two things though.

1. I don't understand why you're removing the state change in open() I can see that affecting behavior (a second open call on a completed xhr?  what about a call which fails due to a domain check?)  Maybe I'm being over cautious, but if we're to remove that call, I think there should be a test to show that desired result.
2.  I'm not sure I fully understand your test case, and why it needs to be recursive as such.  Perhaps it's just to clever for me...

[Julien Chaffraix]

Created attachment 16477 [details]
Non recursive test case

> 1. I don't understand why you're removing the state change in open() I can see
> that affecting behavior (a second open call on a completed xhr?  what about a
> call which fails due to a domain check?)  Maybe I'm being over cautious, but if
> we're to remove that call, I think there should be a test to show that desired
> result.

The call to changeState is not necessary. The changeState is done in abort() and not in open() now.
The instructions between the new call to changeReadyState and the old one are related to the cancellation of the previous XMLHttpRequest (clearing document and creating a new ResourceResponse()). They are thus unaffected by the change and cannot return unexpectedly.
For the check, they were done after the call to changeState and it is also the case with the patch.
 
> 2.  I'm not sure I fully understand your test case, and why it needs to be
> recursive as such.  Perhaps it's just to clever for me...

The aim is to try to abort the XMLHttpRequest for all the readyState. The recursive is just a choice I have made. It implies an order in which we execute the tests (from state 1 to 4).
I am not sure that this order is guaranteed in the test case attached which is not recursive. Tell me which one should be the better (I would the output of the previous one so that it is more understandable ;)) and I will create a new patch.

[Julien Chaffraix]

Created attachment 16515 [details]
Updated patch

After a few test, you have to always do the tests in the same order. As it is not the case in the non recursive test, I have updated the recursive test.

If someone finds a better structure, I am welcome to update the test-case.

I have added an ASSERT to replace the changeState in Open().

[Darin Adler]

Comment on attachment 16515 [details]
Updated patch

I think the change looks great.

+        * ChangeLog:

The change log shouldn't be mentioned in the ChangeLog.

+        * fast/dom/xmlhttprequest-html-response-encoding.html:
+
+        Corrected a mal-formed test page

This doesn't quite match our change log format. Please look at other ChangeLog entries. The overall concept of the change should be at the top, before the list of files. What each file change contributes should be after the file and/or function, without a blank space.

r=me for feature branch

[Julien Chaffraix]

Created attachment 16553 [details]
Patch updated according to Darin comments

Before inclusion, the test results should be verified as they seems strange and could be related to my platform (ie linux/Qt).
Thanks !

[Adam Roben (:aroben)]

Comment on attachment 16553 [details]
Patch updated according to Darin comments

r=me+darin for feature-branch.

Whomever commits this should of course verify the test results first.

[Alexey Proskuryakov]

Comment on attachment 16553 [details]
Patch updated according to Darin comments

Actually, this doesn't seem to do what the specification says, namely, step 4 says that readystatechange event shouldn't be fired when zeroing out the state. Perhaps it would be appropriate to implement the rest of the algorithm in this patch, but that can be done later, too.

1. Abort the send() algorithm, set the response entity body to "null", the error flag to "true" and remove any registered request headers.

2. The user agent should cancel any network activity for which the object is responsible.

3. If the state is UNSENT, OPENED and the send() flag is "false", or DONE go to the next step.

Otherwise, switch the state to DONE, set the send() flag to "false" and synchronously dispatch a readystatechange event on the object.

4. Switch the state to UNSENT. (Do not dispatch the readystatechange event.)


Sorry for not catching this earlier.

[Darin Adler]

Comment on attachment 16515 [details]
Updated patch

Clearing the review flag here so it doesn't show up in the commit queue.

[Julien Chaffraix]

Forgot to CC'ed me on that bug. Sorry for the lack of update.

> Actually, this doesn't seem to do what the specification says, namely, step 4
> says that readystatechange event shouldn't be fired when zeroing out the state.
> Perhaps it would be appropriate to implement the rest of the algorithm in this
> patch, but that can be done later, too.

You are totally true, the patch was way too simple. I will implement the rest of the algorithm as you suggested (following strictly the specification this time) and add the required test cases.

[Julien Chaffraix]

Created attachment 16901 [details]
Updated patch

The patch set readyState to 0 when abort() is called without dispatching an event. It is the behaviour of IE and Opera (FF dispatch an event).

This patch will correct the behaviour until the new algorithm is implemented.

[Alexey Proskuryakov]

Comment on attachment 16901 [details]
Updated patch

r=me

When landing this, I'm going to update the test to mention success criteria for people running it manually.

-        Fix incorrect test results landed with r26986.
+        Fix inCorrect test results landed with r26986.

I think this is unintentional.

[Julien Chaffraix]

> I think this is unintentional.

Yes, I should have messed up with the ChangeLog while reading it.

[Alexey Proskuryakov]

Committed revision 27151.