Closed Bug 218236 Opened 21 years ago Closed 20 years ago

XMLHttpRequest never triggers the error event


(Core :: XML, defect)

Not set





(Reporter: jwkbugzilla, Assigned: jwkbugzilla)




(Keywords: testcase)


(5 files, 6 obsolete files)

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

  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.
    // 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:

    var request = new XMLHttpRequest();
    request.onreadystatechange = function(){alert(request.readyState)};

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
OS: Windows XP → All
Hardware: PC → All
Attached patch Insert an Error() call (obsolete) — Splinter Review
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.
Attachment #141882 - Attachment is obsolete: true
Attachment #141884 - Flags: review?(hjtoi-bugzilla)
Keywords: testcase
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.
Attachment #141884 - Attachment is obsolete: true
Attachment #141887 - Attachment is obsolete: true
Attachment #141884 - Flags: review?(hjtoi-bugzilla)
Blocks: 235849
Attached patch Proposed patch (obsolete) — Splinter Review
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.
Attachment #141888 - Attachment is obsolete: true
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
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.
Attached patch Patch v2 (obsolete) — Splinter Review
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).
Attachment #142677 - Attachment is obsolete: true
Attachment #142742 - Flags: review?(jst)
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

- 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.
Attachment #142742 - Flags: review?(jst) → review+
(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).
Assignee: hjtoi-bugzilla → wp.bugzilla
Ever confirmed: true
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
Attached patch Patch v3Splinter Review
Attachment #142742 - Attachment is obsolete: true
Attachment #142914 - Flags: superreview?(bzbarsky)
Attachment #142914 - Flags: review?(jst)
Comment on attachment 142914 [details] [diff] [review]
Patch v3

Attachment #142914 - Flags: review?(jst) → review+
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,

?  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.
Attachment #142914 - Flags: superreview?(bzbarsky) → superreview+
I removed the "*aDOMEvent = nsnull" after the NS_IF_RELEASE (since it's not
needed) and checked this in.
Closed: 20 years ago
Resolution: --- → FIXED
No longer blocks: 235849
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.
Attachment #246926 - Flags: review?(bzbarsky)
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.
Attachment #246926 - Flags: review?(bzbarsky) → review+
(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 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
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.