User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624 (checked it in Mozilla 1.5b as well) nsXMLHttpRequest.cpp does have a method called Error to trigger the error event listeners but it is never called. The problem: if you try to load data from a server that is currently unavailable no event is triggered. You get the readystatechanged event when readyState is changed to 2 and nothing after this one - no way to know that the request has terminated. This code in nsXMLHttpRequest.cpp is responsible for error handling (method OnStopRequest): if (NS_FAILED(status)) { // This can happen if the user leaves the page while the request was still // active. This might also happen if request was active during the regular // page load and the user was able to hit STOP button. If XMLHttpRequest is // the only active network connection on the page the throbber nor the STOP // button are active. Abort(); // By nulling out channel here we make it so that Send() can test for that // and throw. Also calling the various status methods/members will not throw. // This matches what IE does. mChannel = nsnull; ChangeState(XML_HTTP_REQUEST_COMPLETED, PR_FALSE); // IE also seems to set this I think there should be a call to the Error method before Abort() to indicate that the request stopped executing. And it would do no harm moving ChangeState() before Abort() as well (this means - before the event handlers are cleared) and call it with the broadcast parameter set to PR_TRUE. IE in fact *does* call onreadystatechanged when readyState is changed to 4. Reproducible: Always Steps to Reproduce: 1. Create a file test.html on your disk: <script> netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect"); var request = new XMLHttpRequest(); request.onreadystatechange = function(){alert(request.readyState)}; request.onerror=function(){alert('Error')}; request.onload=function(){alert('OK')}; request.open('GET','http://asdfasdf/'); request.send(null); </script> 2. Open it in Mozilla as file:///.../test.html 3. Grant the script the necessary priviledges Actual Results: The script shows the messages: 1, 1, 2 (readyState changes) Expected Results: It should be more like: 1, 1, 2, 4, Error

I had to use the 1.4 sources as I don't have the latest version so the line numbers might be incorrect, sorry (maybe I should install CVS). I can't test this patch but it should work properly.

That's the correct patch, created with CVS now.

The problem is: I'm using an undefined event constant NS_PAGE_ERROR, there seems to be really no constant for the error event. I'm not sure if inserting a new one in /mozilla/widget/public/nsGUIEvent.h is correct.

Sorry for the bug spam and all the other "patches" - it isn't easy to write code without a compiler :) I've managed to compile Mozilla and test the patch, it works fine in both synchronous and asynchronous mode. The message order for an error changed to: 1, 1, 2, 0, Error (getting rid of the 0 is easy but I'm not sure it's really necessary). The error event triggers as well when the loading is aborted (e.g. if the user hits the ESC key) or when the user changes the page. This can be avoided but I don't think it is necessary.

Does IE have an error event? Does it trigger in this case? In any case, you should request reviews through Bugzilla: click the edit attachment link, then change the review flag to '?' and type in the email address of the person you want to review the patch. When you feel you patch might be final, also request a super-review (you need both before the fix can be checked in).

IE doesn't have onerror but it doesn't have onload either. IE just sets readyState to 4 on errors (which means that I probably should change one more line and do the same instead of setting it to 0). But there is an onerror in http://unstable.elemental.com/mozilla/build/latest/mozilla/extensions/dox/interfacensIJSXMLHttpRequest.html which really makes sense - at the end XMLHttpRequest should always trigger either onload or onerror (like the image tags).

If you change the readyState stuff, looks good to me.

I have fixed readyState changes for the cases that an error occurs or abort() is called. I have also added a check to OnStartRequest() and OnStopRequest() to return if the request has been aborted (it seems that the channel sometimes doesn't abort properly, OnStartRequest() is still called then). I have compared the behavior of the patched version with IE 6.0. Here are the sequences of events in different situations: Mozilla events IE 6.0 events ============================================================ 200 OK: 1 1 2 3 3 4 load 1 1 2 3 4 404 Not Found: 1 1 2 3 4 load 1 1 2 3 4 Connection error: 1 1 2 4 error 1 1 4 abort() called: 1 1 4 (0) 1 1 4 (0) or: 1 1 2 4 (0) 1 1 2 (0) (bug?) ESC pressed: 1 1 2 4 error 1 1 2 3 4 (not aborted) Page changed: 1 1 2 4 error 1 1 The numbers represent readyState changes and the correspoding readystatechange events (except for 0 - it is set without calling the event handlers). "Connection error" means DNS error, unable to connect, no data from server - all the same in my tests. For abort() I've tried calling it immediately after send() and after readyState changes to 2, in the second case IE seems to have a bug (ommiting readyState 4).

Comment on attachment 142742 [details] [diff] [review] Patch v2 - In nsXMLHttpRequest::OnStopRequest(): + // This can happen if the server is unreachable. Other possible reasons + // are that the user leaves the page or hits the ESC key. + Error(); + + // We should null out document even if Error() failed to do it. + mDocument = nsnull; How about making Error() always clear mDocument in stead? Looks easy enough to do... - In nsXMLHttpRequest::Error(): The event creation code here looks identical to the event creation code in nsXMLHttpRequest::RequestCompleted() (with the exception of the event type). Break that out into a function you can call from both places. r=jst wiht those changes.

(In reply to comment #11) > I have compared the behavior of the patched version with IE 6.0. Here are the > sequences of events in different situations: Great stuff! FYI, we knowingly differ from IE in readyStateChange status 3: we sent them out on every buffer we read, on purpose, to make progress meters easier to code. Other differences are bugs, and should be fixed (but file new bugs for the differencies, let's not fix them here).

Forget what I wrote about a bug in the abort() handling in IE - IE just doesn't like to be aborted from a readystatechange handler. If you consider this you will get exactly the same event sequence as in Mozilla. jst: This code is copy&paste from RequestCompleted(). I thought about moving it into a function but I didn't want to change too much with my first patch. Moved this and the code to call the listeners into separate functions now. You will have to review again - I'm not used to XPCOM yet, my parameter handling might be wrong.

Comment on attachment 142914 [details] [diff] [review] Patch v3 r=jst

Comment on attachment 142914 [details] [diff] [review] Patch v3 For future reference, diffs with more context and using the -p option are a lot easier to review... (use "cvs diff -pu8" or so). >Index: nsXMLHttpRequest.cpp >+ nsCOMPtr<nsIPrivateDOMEvent> privevent(do_QueryInterface(*aDOMEvent)); >+ if (!privevent) { >+ return NS_ERROR_FAILURE; >+ } Please NS_RELEASE(*aDOMEvent) in that if statement so we don't return the event when we didn't fully initialize it (especially since the callers don't all check rv). >+nsXMLHttpRequest::NotifyEventListeners(nsIDOMEventListener* aHandler, nsISupportsArray* aListeners, nsIDOMEvent* aEvent) >+ nsCOMPtr<nsIDOMEventListener> listener; >+ >+ aListeners->QueryElementAt(index, >+ NS_GET_IID(nsIDOMEventListener), >+ getter_AddRefs(listener)); How about nsCOMPtr<nsIDOMEventListener> listener = do_QueryElementAt(aListeners, index); ? Avoiding NS_GET_IID is a worthy goal. ;) sr=bzbarsky with those two changes. If you need someone to check this in, please let me know.

I removed the "*aDOMEvent = nsnull" after the NS_IF_RELEASE (since it's not needed) and checked this in.

Testing the cases from the table above with the exception of the last two that require some interactivity. Requesting UniversalXPConnect privileges because otherwise XMLHttpRequest won't allow me to query localhost on a different port (need a connection error). UniversalBrowserRead would be sufficient but it pops up a dialog.

Perhaps mochitest should allow more than just UniversalXPConnect?

Comment on attachment 246926 [details] mochitest compatible testcase > // Ignore duplicated calls for the same ready state Why are those happening? File a bug as needed, and add a todo() test here? Other than that, looks good.

(In reply to comment #21) > Perhaps mochitest should allow more than just UniversalXPConnect? > Oh, sure. We just need to add that to the prefs.

Boris, the duplicated calls happen because there are more internal states than the four communicated outside. The event is triggered every time the internal state is changed. Of course we could check readyState before and after the change of internal state and only trigger the readystatechange event if there really was a change but I'm not really sure this is worth doing.

Why wouldn't it be? Does _any_ other browser fire onreadystatechange repeatedly with the same readyState? If not, we should fix it, imho.

As the table above shows - IE does...

Ah, ok. Nevermind then.

(In reply to comment #22) > (From update of attachment 246926 [details] [edit]) > > > // Ignore duplicated calls for the same ready state > > Why are those happening? File a bug as needed, and add a todo() test here? I implemented the status code 3 (INTERACTIVE) so that it would be reported every time we loaded a chunk of data because it allowed netscape.com web developers to implement progress meter without polling. I think IE reports that status only once. To test this you'd need to load big enough data file - I think our chunk size is 4k but to be safe I'd check with some >64k file.

Checking in test_bug218236.html; /cvsroot/mozilla/testing/mochitest/tests/test_bug218236.html,v <-- test_bug218236.html initial revision: 1.1 done