GM_xmlhttpRequest doesn't report connection errors

[arantius]

Originally: DevjaVu ticket 100

GM_xmlhttpRequest doesn't call onerror callback function when some connection error occurs (cannot find address in DNS, server refuses connections, timeout, etc.)

[arantius]

The problem seems to arise from this xmlhttprequester.js chunk, part of setupRequestEvent():

    responseHeaders:(req.readyState == 4 ?
                     req.getAllResponseHeaders() :
                     ""),

setupRequestEvent() is actually building our handlers for onload, onerror and onreadystatechange events from a single "prototype" function. The above code assumes that, when readyState==4 (operation is complete) there will be a usable getAllResponseHeaders(), which does not seem to be the case:

Erreur : Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXMLHttpRequest.getAllResponseHeaders]
Fichier source : chrome://greasemonkey/content/xmlhttprequester.js
Ligne : 99

Fixing it raises a similar issue with statusText:

Erreur : Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXMLHttpRequest.statusText]
Fichier source : chrome://greasemonkey/content/xmlhttprequester.js
Ligne : 102

I'm not sure whether it's a mozilla bug or not. Documentation does not talk about it, afaict. I reckon though that it does not make much sense to read headers or the HTTP status response when the connection has failed...

Beside that (unless there's more hiding and waiting to be exposed) the fix is trivial:

--- a/src/chrome/chromeFiles/content/xmlhttprequester.js
+++ b/src/chrome/chromeFiles/content/xmlhttprequester.j..@@ -95,11 +95,12 @@ function(unsafeContentWin, req, event, details) {
         // let the browser call properties on it
         responseText:req.responseText,
         readyState:req.readyState,
-        responseHeaders:(req.readyState == 4 ?
+        responseHeaders:(req.readyState == 4 && event != "onerror" ?
                          req.getAllResponseHeaders() :
                          ""),
         status:(req.readyState == 4 ? req.status : 0),
-        statusText:(req.readyState == 4 ? req.statusText : ""),
+        statusText:(req.readyState == 4 && event != "onerror" ?
+                    req.statusText : ""),
         finalUrl:(req.readyState == 4 ? req.channel.URI.spec : "")
       }

[johan]

I'd say we apply that; hurts less than not to.

(I do see how you think, though, if you're considering it sort of a layering violation fixing bugs in the underlying mozilla layer, but I'm not convinced it is an example of that -- as you note, there wouldn't be any headers to get in an error case, and we're probably the better place to catch it.)

[arantius]

FYI everything above I copy/pasted from DevjaVu just so it wouldn't get lost. I personally haven't looked at this much yet, but it seems very reasonable.

[ocornu]

Yes, that was my prose. :-p
If you find any problem with this code please warn me (for it's been merged into WM).

[arantius]

Patch available, no obvious backwards compatibility issues. Tagged for v.next