Last Comment Bug 313646 - synchronous XMLHttpRequest does not fire "readystatechange" event
: synchronous XMLHttpRequest does not fire "readystatechange" event
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: x86 All
: P1 normal with 2 votes (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
http://www.baka.ca/test/sjax.htm
: 337810 353808 383304 441988 446210 493565 503061 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-24 12:20 PDT by Alex Lein
Modified: 2011-02-19 03:04 PST (History)
32 users (show)
jst: blocking1.9.1-
dbaron: blocking1.9-
reed: wanted1.9+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Workaround for Synch/Asynch AJAX Calls. (3.50 KB, text/javascript)
2006-08-22 15:34 PDT, mike brenner
no flags Details
simple_sjax (173 bytes, text/html)
2008-07-25 08:52 PDT, bugzilla33
no flags Details
Patch v1 (3.07 KB, patch)
2009-09-02 13:55 PDT, Justin Lebar (not reading bugmail)
bugs: review-
Details | Diff | Review
patch (5.93 KB, patch)
2009-09-03 13:27 PDT, Olli Pettay [:smaug]
justin.lebar+bug: review+
jst: superreview+
Details | Diff | Review
dispatch readystatechange when readyState == 1 or 4 (6.12 KB, patch)
2009-09-10 08:37 PDT, Olli Pettay [:smaug]
jonas: review+
Details | Diff | Review
up-to-date (6.27 KB, patch)
2010-09-16 10:35 PDT, Olli Pettay [:smaug]
jonas: approval2.0+
Details | Diff | Review

Description Alex Lein 2005-10-24 12:20:56 PDT
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) 2005-10-24 19:53:27 PDT
Seems extremely unlikely that this is a Firefox:General bug, as opposed to an XML(HttpRequest) bug.
Comment 2 Mingyi Liu 2005-10-28 10:54:47 PDT
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 [:bz] 2005-11-07 22:22:59 PST
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 2005-11-11 05:31:00 PST
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 2006-02-12 04:06:51 PST
(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) 2006-05-13 10:37:23 PDT
*** Bug 337810 has been marked as a duplicate of this bug. ***
Comment 7 Fernando Wendt 2006-08-15 07:21:55 PDT
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. 2006-08-18 06:03:32 PDT
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 2006-08-22 15:34:42 PDT
Created attachment 234988 [details]
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 2006-09-01 07:59:36 PDT
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) 2006-09-22 21:06:22 PDT
*** Bug 353808 has been marked as a duplicate of this bug. ***
Comment 12 Anne (:annevk) 2007-04-02 04:47:07 PDT
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 13 Aiko 2007-08-16 14:37:36 PDT
*** Bug 383304 has been marked as a duplicate of this bug. ***
Comment 14 Chris Bloom 2007-08-17 07:56:13 PDT
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 2008-01-09 21:26:12 PST
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 2008-07-25 08:52:04 PDT
Created attachment 331311 [details]
simple_sjax
Comment 17 bugzilla33 2008-07-25 08:54:13 PDT
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 18 bugzilla33 2008-07-25 08:59:11 PDT
*** Bug 446210 has been marked as a duplicate of this bug. ***
Comment 19 Boris Zbarsky [:bz] 2008-07-25 12:17:26 PDT
*** Bug 441988 has been marked as a duplicate of this bug. ***
Comment 20 Mark Omohundro 2008-08-19 13:39:47 PDT
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 2008-09-12 05:31:25 PDT
better:

if (!async && navigator.userAgent.indexOf("Gecko") > -1) {
  // call the processing method here
}
Comment 22 Mark Omohundro 2008-09-12 08:29:07 PDT
That would add Chrome, Safari, etc.  Have you seen this synchronous
issue there also??  Thank you.
Comment 23 bugzilla33 2008-09-18 09:24:55 PDT
Chrome, Safari, Opera, IE fire "readystatechange" event.
Comment 24 Jonas Sicking (:sicking) 2008-09-18 14:37:01 PDT
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, jst@mozilla.com) 2008-09-24 16:19:48 PDT
On second thought this is not a blocker, but a wanted P1.
Comment 26 Kevin Brosnan 2009-05-18 09:29:04 PDT
*** Bug 493565 has been marked as a duplicate of this bug. ***
Comment 27 Justin Lebar (not reading bugmail) 2009-09-02 13:55:47 PDT
Created attachment 398209 [details] [diff] [review]
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 28 Justin Lebar (not reading bugmail) 2009-09-02 13:59:36 PDT
*** Bug 503061 has been marked as a duplicate of this bug. ***
Comment 29 Olli Pettay [:smaug] 2009-09-03 05:13:38 PDT
(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) 2009-09-03 10:50:35 PDT
(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] 2009-09-03 12:52:41 PDT
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] 2009-09-03 13:27:22 PDT
Created attachment 398450 [details] [diff] [review]
patch

Justin, want to try this one?
Comment 33 Justin Lebar (not reading bugmail) 2009-09-03 15:11:16 PDT
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] 2009-09-03 15:25:51 PDT
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) 2009-09-04 03:05:22 PDT
FWIW, the specification requires that you do not dispatch events during synchronous requests.
Comment 36 Olli Pettay [:smaug] 2009-09-04 05:36:08 PDT
oh, even readystatechange events?
I thought only progress events.
Things have changed again...
Comment 37 Olli Pettay [:smaug] 2009-09-04 05:39:36 PDT
Anyway, if that is the case, then this bug is invalid.
(And I agree, events shouldn't be dispatched.)
Comment 38 Anne (:annevk) 2009-09-04 09:50:18 PDT
I told you when I changed them and you told me you were happy ;-)
Comment 39 Justin Lebar (not reading bugmail) 2009-09-04 10:02:12 PDT
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] 2009-09-04 10:30:46 PDT
(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) 2009-09-04 14:00:41 PDT
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) 2009-09-04 14:06:02 PDT
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) 2009-09-05 03:56:04 PDT
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] 2009-09-07 01:33:26 PDT
(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) 2009-09-07 17:19:59 PDT
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] 2009-09-10 08:37:21 PDT
Created attachment 399734 [details] [diff] [review]
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) 2009-09-14 11:17:14 PDT
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] 2009-09-14 12:50:36 PDT
(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) 2009-09-14 15:06:13 PDT
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] 2009-09-15 02:33:40 PDT
http://hg.mozilla.org/mozilla-central/rev/36a582d5c645
Comment 51 Olli Pettay [:smaug] 2009-09-15 08:55:39 PDT
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) 2009-10-09 09:58:02 PDT
Olli, any update on this bug? Given by bug 503061 this is a regression?
Comment 53 Olli Pettay [:smaug] 2009-10-09 09:59:52 PDT
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) 2009-10-09 10:07:33 PDT
The memory leak was only on linux or also other platforms?
Comment 55 Olli Pettay [:smaug] 2009-10-09 10:19:06 PDT
Linux only, and it wasn't a leak, but memory usage going up.
Comment 56 Olli Pettay [:smaug] 2009-10-09 10:19:54 PDT
IIRC, I did retry this on tryserver with the same results.
I could try again.
Comment 57 Henrik Skupin (:whimboo) 2009-10-10 09:15:04 PDT
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] 2009-10-10 09:17:32 PDT
(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] 2009-10-10 09:25:08 PDT
I posted this to tryserver yesterday and there linux tp4 shows again peak in
tp4_pbytes and tp4_rss
Comment 60 Henrik Skupin (:whimboo) 2009-10-10 09:53:05 PDT
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] 2010-09-16 09:12:45 PDT
I posted this again to tryserver.
Comment 62 Olli Pettay [:smaug] 2010-09-16 10:35:05 PDT
Created attachment 475896 [details] [diff] [review]
up-to-date
Comment 63 Olli Pettay [:smaug] 2010-09-17 01:28:46 PDT
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] 2010-09-17 03:19:52 PDT
http://hg.mozilla.org/mozilla-central/rev/268ef4ccb5ff

Waiting for the test results before marking this fixed.
Comment 65 Olli Pettay [:smaug] 2010-09-17 03:37:45 PDT
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 2011-02-19 02:51:31 PST
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] 2011-02-19 03:04:07 PST
(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.

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