313646 - synchronous XMLHttpRequest does not fire "readystatechange" event

Status: RESOLVED FIXED

(Core :: XML, defect, P1)

Description

[Alex Lein]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7

When I create an XMLHttpRequest object and use the open() method with syncronious mode flagged, FF 1.0.x will not fire the "readystatechange" event...  FF 1.5b2 simply freezes.

Reproducible: Always

Steps to Reproduce:
1. Visit my testcase http://www.baka.ca/test/sjax.htm
2. Count the number of alert()s that popup.
Actual Results:  
The alert() is shown 5 times (for async mode only).

Expected Results:  
The alret() should be shown 10 times. 5 times for sync mode, and more 5 times for async mode.

Comment 1

[Phil Ringnalda (:philor)]

Seems extremely unlikely that this is a Firefox:General bug, as opposed to an XML(HttpRequest) bug.

Comment 2

[Mingyi Liu]

I've found this bug too, and almost filed it as a new bug. 

Anyway I confirmed that it's a bug for Firefox 1.0.6, 1.0.7, Mozilla and Firefox latest nightly builds (as of today), so it must be the core code that caused this bug.  IE, on a separate note, behaves correctly.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The XMLHttpRequest code does:

1849   if ((mState & XML_HTTP_REQUEST_ASYNC) &&
1850       (aState & XML_HTTP_REQUEST_LOADSTATES) && // Broadcast load states only
1851       aBroadcast &&
1852       onReadyStateChangeListener) {

hence this bug.  Does IE really fire onreadystatechanged for sync XMLHttpRequests in all cases? If so, we probably should too...

Comment 4

[Mingyi Liu]

Yeah, I'm pretty sure IE fires the event on sync-ed requests, and Mozilla family should do that too.  Otherwise, a simple task that I was facing cannot be solved in Mozilla browsers.  The task is onSubmit, first send a sync-ed request to a script checking username/password for DB permission, and if and only if that passes, submit the form data; otherwise give user a warning.  My sync-ed script only worked in IE, not Mozilla browsers.  I tried a few possible workarounds and none worked.  Seems to me that some tasks that need coordination just can't be accomplished by async calls in a reliable way.

Comment 5

[Jungshik Shin]

(In reply to comment #3)

> hence this bug.  Does IE really fire onreadystatechanged for sync
> XMLHttpRequests in all cases? If so, we probably should too...

It's reported in the Korean mozilla user forum that Opera does that, too. I'm not sure of 'in all cases' part, though.

Comment 6

[Tuukka Tolvanen (sp3000)]

*** Bug 337810 has been marked as a duplicate of this bug. ***

Comment 7

[Fernando Wendt]

I have taken good time trying to make a synchronous request work, but it really doesn´t. On IE, these control works fine, but at Mozilla it´s broken. The XMLHTTPRequest responseText propertie is returning allways 'undefined', via window.alert simple test case. Plus: it seems to be discarding the 'false' flag at the open method. Aditional info: i´m trying to do this, by posting form data (i.e.  XMLHTTPRequest.open("POST",url,false), XMLHTTPRequest.setRequestHeader('Content-Type','application/x-www-form-urlencoded') and XMLHTTPRequest.send(data)).

Comment 8

[Cees T.]

I did the same as comment 7 (except using ajax.open("post", "ebridge.php");), but lately ran into this bug. Odd that it used to work...

Comment 9

[mike brenner]

Attachment: Workaround for Synch/Asynch AJAX Calls.

Seen this behavior as well. Here is a workaround. Since Firefox always calls the Firefox-only ONLOAD event handler, you can always bind to it. (Note, IE always calls ONREADYSTATECHANGED correctly).

Comment 10

[Fernando Wendt]

I make a web page that points a curve fit for that issue. If someone is looking for a solution for that this implementation. The url is http://www.fernandowendt.eti.br/bugzilla/. Maybe help.

Comment 11

[Phil Ringnalda (:philor)]

*** Bug 353808 has been marked as a duplicate of this bug. ***

Comment 12

[Anne (:annevk)]

http://dev.w3.org/cvsweb/~checkout~/2006/webapi/XMLHttpRequest/Overview.html?content-type=text/html;%20charset=utf-8 should be clear on when the events and state changes are to be done. I also wrote a testcase for synchronous requests which passes in Internet Explorer 7:

  http://tc.labs.opera.com/apis/XMLHttpRequest/readyState/002.htm

Comment 14

[Chris Bloom]

As stated in my Bug 383304 report, having the Firebug extension installed fixes this, so it is a solvable problem. My report also includes a link to an example of the bug and an inline workaround for web pages that need this to work as-is. The workaround is future compatible.

Comment 15

[rick ratta]

I don't know if others have noticed this, but in my case in FF 2.0.0.11
Synchronous works if I place an alert after the send call.

The onreadystatechanged handler is called, but if I don't place an alert
after the send call, the send method returns before the handler 
completes.

But there is something "special" about alert. If I use a spin loop
after the send, that still does not have the same effect as calling
alert. Its almost as if the alert call "flushes" or causes "listeners"
to be invoked. Anyway those are the symptoms I see.

-rick

Comment 16

[bugzilla33]

Attachment: simple_sjax

Comment 17

[bugzilla33]

confirm on FF 3.0.1


<script>

  r=new XMLHttpRequest()
  r.onreadystatechange=function(){if(r.readyState==4)alert(r.responseText)}
  r.open('GET','1.txt',false)
  r.send(null)

</script>

Comment 20

[Mark Omohundro]

I ran into this also and came up with a simple workaround:

After my onreadystatechange and send methods, I fire my callback
method manually -- when in Firefox and synchronous mode -- inside
this test:

if (!async && navigator.userAgent.indexOf("Firefox") != -1) {
  // call the processing method here
}

A picture is worth 1k words, so here is my ajax lib source listing
where you can find this code near the end.  Hope this helps.

http://ajamyajax.com/ajaMyCore.html

Comment 21

[bugzilla33]

better:

if (!async && navigator.userAgent.indexOf("Gecko") > -1) {
  // call the processing method here
}

Comment 22

[Mark Omohundro]

That would add Chrome, Safari, etc.  Have you seen this synchronous
issue there also??  Thank you.

Comment 23

[bugzilla33]

Chrome, Safari, Opera, IE fire "readystatechange" event.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, all parties agree that we're doing the wrong thing here I think. So lets get it fixed for next release.

Comment 25

[Johnny Stenback (:jst)]

On second thought this is not a blocker, but a wanted P1.

Comment 27

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

This patch removes the restriction against firing onreadystatechange for synchronous XHRs.

It does not pass the OP's testcase, however, because firing an alert within the sync onreadystatechange handler somehow prevents send() from completing.  We think that this behavior might be related to bug 501501.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #27)
> because firing an alert within the
> sync onreadystatechange handler somehow prevents send() from completing.
I can't reproduce this problem. I tried on Linux and OSX.
Which testcase were you using?

Comment 30

[Justin Lebar (not reading bugmail)]

(In reply to comment #29)

Just to be clear, I meant in comment #27 that if I load http://www.baka.ca/test/sjax.htm in Firefox with my patch, it never begins the asynchronous XHR, because the first XHR doesn't complete its send().  If I don't fire any alerts in the sync onreadystatechange handler, then the sync's send() does complete.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 398209 [details] [diff] [review]
Patch v1

Unfortunately this isn't enough. The problem is that alert() starts a new syncloop already before XHR's syncloop and when it exists, XHR has already been loaded, but XHR syncloop is just started.

I'll post a bit different patch.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Justin, want to try this one?

Comment 33

[Justin Lebar (not reading bugmail)]

Comment on attachment 398450 [details] [diff] [review]
patch

Looks good to me.

I noticed that the spec calls for sync XHRs to dispatch events differently than async XHRs.  Does this bring us more in line with the spec, or should we work on getting it changed to match our (and everyone else's) behavior?

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Yeah, our readystatechange handling isn't quite what the spec says.
Both async and sync behave a bit differently.
I think a followup bug would make sense. When changing the readystatechange
handling, have to make sure that the XHR2 draft specifies things properly.

Comment 35

[Anne (:annevk)]

FWIW, the specification requires that you do not dispatch events during synchronous requests.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

oh, even readystatechange events?
I thought only progress events.
Things have changed again...

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Anyway, if that is the case, then this bug is invalid.
(And I agree, events shouldn't be dispatched.)

Comment 38

[Anne (:annevk)]

I told you when I changed them and you told me you were happy ;-)

Comment 39

[Justin Lebar (not reading bugmail)]

As far as I can tell, everyone (Chrome 2, Safari 4, IE 8, Opera 10) dispatches
onreadystatechange events on sync XHR.  Based on the number of duplicates of
this bug, I think it's fair to say that FF's behavior is confusing a lot of web
developers.  Is that sufficient reason to change the spec and/or our behavior?

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #38)
> I told you when I changed them and you told me you were happy ;-)
Apparently I forgot the readystatechange part, and remembered only progress events.
And seems  like readystatechange is still dispatched with sync, at least in some cases, like after open().

But it is great if we don't have to dispatch those events.
readystatechange events are legacy. New code should use progress events.

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

My feelings are this:

On one hand sync XHR is crappy, and so I don't think adding features for people using it is that high on my list. And it's nice to be consistent in not firing *any* events during sync XHR.

On the other hand, if every other UA is firing readystatechange during sync XHR, it seems easier to standardize on that. Getting every other implementation to remove a feature is a lot of work. Especially a feature that people are clearly using.


So: All in all I think I'd be in favor of changing the spec and fixing this bug.

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So to make it clear: The only reason for dispatching any events during sync requests is for compat with other UAs, and existing content. From that point of view it doesn't (IMHO) make sense to dispatch progress events.

Comment 43

[Anne (:annevk)]

This really be discussed on the list, but to clarify, some readystatechange events are still dispatched. After open(), but also after a successful request. See "switch to the DONE state". Just not during a request as the event loop is busy at that time with the request.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #43)
> After open(), but also after a successful request.
> See "switch to the DONE state". Just not during a request as the event loop is
> busy at that time with the request.

Right, this makes sense. I think almost all the cases when I've seen readystatechange to be used with sync XHR, it has been used to to detect readyState 4.
I'll modify the patch a bit so that only 1 and 4 are reported when xhr is sync.
Does that sound ok to you Jonas?


Thanks Anne for the clarification!

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That makes sense. I think the most important thing here is to align all UAs to a particular behavior. What that behavior is feels less important to me.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Attachment: dispatch readystatechange when readyState == 1 or 4

This is what the XHR2 spec says for normal loads.

Error and abort handling need some work, but those are separate cases.
When we take the patch (in some form), I'll file followups to fix error and
abort.
We also need add support for loadend (Bug 461066), which the draft does specify
nowadays.

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 399734 [details] [diff] [review]
dispatch readystatechange when readyState == 1 or 4

Would be good to have tests that do the evil things that would have broken Justin's initial patch. I take it it's stuff like calling .abort/.open from within the readystatechange handler.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #47)
> Would be good to have tests that do the evil things that would have broken
> Justin's initial patch.
The testcase in the patch does do that; starting a new "event loop"
in a readystatechange listener.

Comment 49

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ooh, I thought the concern was when the call to the eventhandler would change the existing XHR objects state in unexpected ways.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/36a582d5c645

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Backed out to see if this caused memory usage regression on linux.
http://hg.mozilla.org/mozilla-central/rev/834a55bc458b

Comment 52

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Olli, any update on this bug? Given by bug 503061 this is a regression?

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

I can't see when this would have regressed.

Anyway, I haven't figured out why memory usage goes up on linux.

Comment 54

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The memory leak was only on linux or also other platforms?

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Linux only, and it wasn't a leak, but memory usage going up.

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

IIRC, I did retry this on tryserver with the same results.
I could try again.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

When you do this which tool we could use to track the memory consumption under Linux? Valgrind? Which steps someone would have to use to simulate this excessive memory consumption? Just running the testcase from the patch?

Would be great if you can push it again.

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #57)
> Which steps someone would have to use to simulate this
> excessive memory consumption? 
Run tp4 and measure the memory usage the same way as talos does.
I haven't done that locally. I have no idea how to run talos locally.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

I posted this to tryserver yesterday and there linux tp4 shows again peak in
tp4_pbytes and tp4_rss

Comment 60

[Henrik Skupin [:whimboo][⌚️UTC+1]]

What is tp4? Would be great if you could point me to a documentation. Does it run all the automated tests?

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

I posted this again to tryserver.

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date

Comment 63

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 475896 [details] [diff] [review]
up-to-date

So this time tryserver didn't show tp4 tp4_pbytes/tp4_rss regressions.
Have we fixed something else? Don't know why there was regressions earlier, 
but we could try to get this in.

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/268ef4ccb5ff

Waiting for the test results before marking this fixed.

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

This time tp4_pbytes and tp4_rss look ok.

Marking this fixed, but I'll reopen/back-out if some other
test shows regression.

Comment 66

[ophers]

Hi,
Will this be backported to FF 3.6 branch?
As of now I don't see it in _tip_: http://hg.mozilla.org/releases/mozilla-1.9.2/log/tip/content/base/src/nsXMLHttpRequest.cpp

Thanks.

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #66)
> Hi,
> Will this be backported to FF 3.6 branch?
No.

3.6 is in stability-and-security-fixes only mode.