218236 - XMLHttpRequest never triggers the error event

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Wladimir Palant]

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

Comment 1

[Wladimir Palant]

Attachment: Testcase, has to be downloaded and run from file://

Comment 2

[Wladimir Palant]

Attachment: Testcase, same thing for Internet Explorer

Comment 3

[Wladimir Palant]

Attachment: Insert an Error() call

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.

Comment 4

[Wladimir Palant]

Attachment: Insert an Error() call (now for the current file version)

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

Comment 5

[Wladimir Palant]

Attachment: Forgot to create the DOM event - fixed 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.

Comment 6

[Wladimir Palant]

Attachment: Couple of other problems fixed - should be correct now

Comment 7

[Wladimir Palant]

Attachment: Proposed patch

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.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 9

[Wladimir Palant]

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

Comment 10

[Doron Rosenberg (IBM)]

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

Comment 11

[Wladimir Palant]

Attachment: Patch v2

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 12

[Johnny Stenback (: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
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.

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 14

[Wladimir Palant]

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 15

[Wladimir Palant]

Attachment: Patch v3

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 142914 [details] [diff] [review]
Patch v3

r=jst

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Wladimir Palant]

Attachment: Patch with bz's corrections

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Wladimir Palant]

Attachment: mochitest compatible testcase

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Perhaps mochitest should allow more than just UniversalXPConnect?

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Robert Sayre]

(In reply to comment #21)
> Perhaps mochitest should allow more than just UniversalXPConnect?
> 

Oh, sure. We just need to add that to the prefs.

Comment 24

[Wladimir Palant]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[Wladimir Palant]

As the table above shows - IE does...

Comment 27

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Nevermind then.

Comment 28

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 29

[Robert Sayre]

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