Closed Bug 447689 Opened 15 years ago Closed 7 years ago

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


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

Not set



Tracking Status
firefox50 --- fixed


(Reporter: smaug, Assigned: wisniewskit)





(1 file, 3 obsolete files)

Have to look at the XHR spec how this should work.
"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

(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 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. (

As such, these two tests, which send() between successive opens, should expect that event, but don't:

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]

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):
Attachment #8760077 - Attachment is obsolete: true
Attachment #8767249 - Flags: review?(amarchesini)
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:
Assignee: nobody → wisniewskit
Attachment #8767249 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by
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:
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Pushed by
clean up XMLHttpRequest::Open so XHR readyState=1 events are fired according to spec. r=baku
Keywords: checkin-needed
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM → DOM: Core & HTML

readyState 3 fires multiple times per one chunk.

What are you guys doing... IE7 had a readyState 3 bug that stopped comet-stream for almost a decade and now trying to fix their stuff you make firefox become the next IE7!

Lucky for the world that only 3% uses firefox now.

Because of this I'm not NEVER working on web again.

I'm 100% Java server and C client from now on.

Pretty sure all browsers exhibit the same behavior as far as this goes…

Hm, Chrome, Safari still work fine but Firefox calls onreadystatechange many times for one chunk, it used to work in firefox... I suspect you might have some tricky to reproduce bug with your new XHR2 implementation because it doesn't happen for all chunks! did XHR change recently?

What's recently? I'm not aware of a change in this area. Could you file a new bug? It should work the same as in other browsers.

It worked 6 months ago I'm pretty sure, but I try not to update... because things just become worse... so... Don't know.

You need to log in before you can comment on or make changes to this bug.