Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Description • 16 years ago

Have to look at the XHR spec how this should work.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 1 • 16 years ago

3.8
"readystatechange  	Event  	The readyState attribute changes and at some seemingly arbitrary times for historical reasons. "

	Hallvord R. M. Steen [:hallvors]
	Comment 2 • 11 years ago

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)

	Thomas Wisniewski Assignee
	Comment 3 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 4 • 8 years ago

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

	Anne (:annevk)
	Comment 5 • 8 years ago

send() does not change the state though until some network activity happened (it does set a flag). So the tests look correct to me.

	Thomas Wisniewski Assignee
	Comment 6 • 8 years ago

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?

	Anne (:annevk)
	Comment 7 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 8 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 9 • 8 years ago

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

	Thomas Wisniewski Assignee
	Comment 11 • 8 years ago

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

	Pulsebot
	Comment 12 • 8 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 13 • 8 years ago

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

	Thomas Wisniewski Assignee
	Comment 14 • 8 years ago

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?!");

	Thomas Wisniewski Assignee
	Comment 15 • 8 years ago

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

	Pulsebot
	Comment 16 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 17 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/eb8925e99832