Bug 13141 - XMLHttpRequest should set readyState to 0 after abort()
Summary: XMLHttpRequest should set readyState to 0 after abort()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://tc.labs.opera.com/apis/XMLHttp...
Keywords:
Depends on:
Blocks:
 
Reported: 2007-03-21 06:54 PDT by Alexey Proskuryakov
Modified: 2007-10-27 12:03 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch and testcase (4.34 KB, patch)
2007-09-27 15:28 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Non recursive test case (1.60 KB, text/html)
2007-09-30 14:34 PDT, Julien Chaffraix
no flags Details
Updated patch (4.84 KB, patch)
2007-10-03 02:52 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch updated according to Darin comments (4.85 KB, patch)
2007-10-05 14:15 PDT, Julien Chaffraix
ap: review-
Details | Formatted Diff | Diff
Updated patch (5.08 KB, patch)
2007-10-27 05:18 PDT, Julien Chaffraix
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2007-03-21 06:54:45 PDT
See bug URL for a test case.
Comment 1 Anne van Kesteren 2007-04-02 15:38:24 PDT
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.
Comment 2 Julien Chaffraix 2007-09-27 15:28:39 PDT
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.
Comment 3 Eric Seidel (no email) 2007-09-30 10:52:06 PDT
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...
Comment 4 Julien Chaffraix 2007-09-30 14:34:25 PDT
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.
Comment 5 Julien Chaffraix 2007-10-03 02:52:52 PDT
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().
Comment 6 Darin Adler 2007-10-04 09:21:26 PDT
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
Comment 7 Julien Chaffraix 2007-10-05 14:15:33 PDT
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 !
Comment 8 Adam Roben (:aroben) 2007-10-07 16:05:06 PDT
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.
Comment 9 Alexey Proskuryakov 2007-10-07 21:43:51 PDT
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.
Comment 10 Darin Adler 2007-10-14 17:47:32 PDT
Comment on attachment 16515 [details]
Updated patch

Clearing the review flag here so it doesn't show up in the commit queue.
Comment 11 Julien Chaffraix 2007-10-18 08:41:02 PDT
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.
Comment 12 Julien Chaffraix 2007-10-27 05:18:37 PDT
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.
Comment 13 Alexey Proskuryakov 2007-10-27 11:04:56 PDT
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.
Comment 14 Julien Chaffraix 2007-10-27 12:01:19 PDT
> I think this is unintentional.

Yes, I should have messed up with the ChangeLog while reading it.
Comment 15 Alexey Proskuryakov 2007-10-27 12:03:04 PDT
Committed revision 27151.