Closed Bug 447689 Opened 12 years ago Closed 4 years ago
[XHR2] XHR fires readystatechange event even though readystate hasn't changed - too many ready
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)
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.
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).
send() does not change the state though until some network activity happened (it does set a flag). So the tests look correct to me.
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?
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.
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.
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 #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
Pushed by firstname.lastname@example.org: 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
Backed out for assertion in test_worker_xhr_implicit_cancel.html in debug builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc97f8b36a22 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=39cac65eb07c0b386ada98b143ce84bbc671164f Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=31890785&repo=mozilla-inbound
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)
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
Pushed by email@example.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
You need to log in before you can comment on or make changes to this bug.