Bug Tracker

Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#9585 closed bug (wontfix)

[patch] jQuery silently eats ajax exception due to typo (regression)

Reported by: Niall Smart <[email protected]…> Owned by:
Priority: low Milestone: 1.next
Component: ajax Version: 1.6.1
Keywords: Cc: jaubourg
Blocked by: Blocking:


Line 730 of ajax.js contains a typo - it refers to a variable status which resolves in global scope (i.e., to window.status - the text shown in the browser status bar. This seems a legacy of some refactoring work as a local status variable is used elsewhere in a nearby function. The correct variable name is state.


This error causes exceptions thrown by callbacks in synchronous ajax functions to be silently discarded. The top level try-catch block executes, but the test in the catch block (status < 2) almost always evaluates to true (given type conversion of an empty string) so done() is called and immediately returns (because state === 2 at this point). As a result the exception is never logged in the browser console.

As a side-effect of implementing a test case for this bug, I noticed jQuery's maintenance of it's internal state (e.g., jQuery.active) is corrupted by exceptions thrown by callbacks in synchronous ajax requests. I have extended to patch to defer such exceptions until jQuery completes it's post-ajax teardown (this could also be done using a finally block).

I am attaching a patch with test case and have tested successfully on Chrome 12.0.742.91, FireFox 4.0.2 and IE 8.0.7600.16385

Please note: regardless of the intended semantics of handling exceptions thrown in user callbacks (there has been debate on other bugs) the current behavior of silently discarding the error is definitely a bug.

Change History (9)

comment:1 Changed 12 years ago by Niall Smart <[email protected]…>

Patch sent as pull request:


comment:2 Changed 12 years ago by addyosmani

Cc: jaubourg added
Component: unfiledajax

comment:3 Changed 12 years ago by timmywil

Priority: undecidedlow
Status: newopen

comment:4 Changed 12 years ago by jaubourg

The pull request is a bit heavy-weight to my tastes and will make debugging a nightmare. I'd rather use a setTimeout( ..., 0 ) to make sure the events are fired and the active counter properly handled. Waiting for 1.6.2 to be released to tackle this.

comment:5 Changed 12 years ago by anonymous

@jaubourg - I'm happy to supply a simpler patch if you have any suggestions. I don't understand how the suggested patch affects debugging?

At a bare minimum, the smallest potential fix would just change the typo "status" to "state" so that at least the exception doesn't silently disappear.

comment:6 Changed 12 years ago by jaubourg

An exception is caught and rethrown which means the call stack is lost.

And yes, I would merge a pull request with just the "status" to "state" fix immediately :)

comment:7 Changed 12 years ago by jaubourg

Resolution: wontfix
Status: openclosed

So I tried a setTimeout approach and it causes all kind of problems due to the fact global event trigerring is synchronous and changing that will break a lot of existing code (which is sad but a reality).

This got me thinking a little more about what we're trying to fix here: something goes wrong in a callback (an unhandled exception which will, 99% of the time, result from some programming error), yet we want to acknowledge the request has completed. Well, it has and, yet, it hasn't: the request is somehow still active and it wouldn't really make sense to fire ajaxStop or ajaxStart, would it?

So far, in jQuery, we took the route of being as vocal as possible when an unexpected exception occurs. I believe we typically fall into this category here.

The "status" typo has been corrected in trunk already. However, it seems to me that jumping through hoops to cater for programming errors is not in the best interest of the developpers themselves.

That's why I'll close this as wontfix and will close the pull request. However, I'd like to thank you for catching the typo, Niall, since this definitly needed to be fixed.

comment:8 Changed 12 years ago by Niall Smart <[email protected]…>

@jaubourg great, thank you! I agree that fixing the typo is the main thing here, it's not even clear that there is a "right" behavior wrt to the ajax events.

comment:9 Changed 11 years ago by anonymous

I believe we typically fall into this category here. 


Note: See TracTickets for help on using tickets.