Closed Bug 447689 Opened 12 years ago Closed 4 years ago

[XHR2] XHR fires readystatechange event even though readystate hasn't changed - too many readyState=1 events

Categories

(Core :: DOM: Core & HTML, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: smaug, Assigned: wisniewskit)

References

()

Details

Attachments

(1 file, 3 obsolete files)

Have to look at the XHR spec how this should work.
3.8
"readystatechange  	Event  	The readyState attribute changes and at some seemingly arbitrary times for historical reasons. "
The spec is modernised and somewhat less arbitrary - we've largely concluded that the web did not depend on emulating those old IE bugs.

In these tests, Gecko fires too many readyState = 1 events
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/open-open-send.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/open-open-sync-send.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/open-send-open.htm

(We still have cases of the opposite, where readystate changes without any event)
Blocks: xhr2pass
Summary: XHR fires readystatechange event even though readystate hasn't changed → [XHR2] XHR fires readystatechange event even though readystate hasn't changed - too many readyState=1 events
Here's a patch which passes these web platform tests. Two things were necessary here:

1. Not firing another readyState=1/opened event if one had already been fired before on the XHR, unless an abort() was done in between the calls. I added a new internal mState to keep track of this.

2. Not firing readyState=4/done events during the call to Abort() in Open(). The spec says to indeed terminate the request, but not fire any events while doing so. As such I factored out the CloseRequest() part of CloseRequestWithError(), and called that instead along with ResetResponse() instead.

This patch also fixes bug 918736, which is testing http://w3c-test.org/XMLHttpRequest/open-sync-open-send.htm in a similar fashion.
Attachment #8760077 - Flags: review?(jst)
Actually, Anne, upon closer inspection some of these tests seem erroneous. Based on the spec, step 12, anytime open() is called and we are not currently in the OPENED state already, another OPENED event should be dispatched. (https://xhr.spec.whatwg.org/#the-open()-method)

As such, these two tests, which send() between successive opens, should expect that event, but don't:
http://w3c-test.org/XMLHttpRequest/open-send-open.htm
http://w3c-test.org/XMLHttpRequest/open-sync-open-send.htm

I'll have to adjust my patch to take that into consideration, and I'll include a patch to change those two tests (unless you disagree).
Flags: needinfo?(annevk)
send() does not change the state though until some network activity happened (it does set a flag). So the tests look correct to me.
Flags: needinfo?(annevk)
Are you sure? That doesn't seem to match with what the spec says:

>To handle response end-of-file for response, run these steps: 
>5. Set state to done.
>7. Fire an event named readystatechange.

And the request is being made for folder.txt, which exists and should be read to end-of-file without issue.

So at least in the sync case, it makes sense that there would be a DONE event in between the OPENEDs. I'm not sure how event-loop timing would make the async case order the events, but it too should hit DONE eventually, and Firefox ends up triggering DONE before the second open() call happens. As such it should send a second OPENED event, no?
Flags: needinfo?(annevk)
Apart from the second call to open() in the second test, which doesn't really matter since it's not followed by send(), it doesn't seem like these tests are testing the synchronous path.
Flags: needinfo?(annevk)
Comment on attachment 8760077 [details] [diff] [review]
447689-send-xhr-readyState-1-events-according-to-spec.diff

Review of attachment 8760077 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm. Alright then, if you're sure then I won't push the point, since Chrome passes these tests anyhow. Thanks for the feedback!

I'll try to find out the remaining worker-related mochitest failure with my patch before re-requesting a review.
Attachment #8760077 - Flags: review?(jst)
Here's a version of the patch that fixes the XHR open() code to match the spec where it counts for these four web platform tests. I had to modify the worker code a little to compensate, but otherwise it seems straightforward.

It gets a successful try run (one unrelated intermittent failure): https://treeherder.mozilla.org/#/jobs?repo=try&revision=140878950ee4
Attachment #8760077 - Attachment is obsolete: true
Attachment #8767249 - Flags: review?(amarchesini)
Duplicate of this bug: 1285580
Attachment #8767249 - Flags: review?(amarchesini) → review+
Simple rebase. Carrying over r+. The "dropping from sent to opened" chunk was no longer necessary thanks to patches landed in the XHR refactoring in bug 1285036.

A fresh try run only shows unrelated intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d866a846ecb&selectedJob=23926998
Assignee: nobody → wisniewskit
Attachment #8767249 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5028ff81cb
clean up XMLHttpRequest::Open so XHR readyState=1 events are fired according to spec. r=baku
Keywords: checkin-needed
Turns out that specific assertion shouldn't actually be necessary, so I just removed it from this new version of the patch. That's the only change.

I was hoping to do a try-run just in case, but for some reason any jobs I trigger never appear in the treeherder UI today.

As such I'll needinfo baku instead to see if he thinks this change is ok. If he does, and I still can't do a try run myself, I'll re-request checkin.

baku, the change to the patch is the chunk removing this line from XMLHttpRequestWorker.cpp:

>  NS_ASSERTION(!mMainThreadSeenLoadStart, "Huh?!");
Flags: needinfo?(wisniewskit) → needinfo?(amarchesini)
Attachment #8771640 - Attachment is obsolete: true
A fresh try-run seems fine after all, so I'm clearing the ni? and re-requesting checkin: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b63b2c80983
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8925e99832
clean up XMLHttpRequest::Open so XHR readyState=1 events are fired according to spec. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb8925e99832
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.