Skip to main content

Bug Tracker

Side navigation

#9585 closed bug (wontfix)

Opened June 14, 2011 06:53PM UTC

Closed July 01, 2011 01:47PM UTC

Last modified November 21, 2011 08:43PM UTC

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

Reported by: Niall Smart <niall.smart@gmail.com> Owned by:
Priority: low Milestone: 1.next
Component: ajax Version: 1.6.1
Keywords: Cc: jaubourg
Blocked by: Blocking:
Description

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.

https://github.com/jquery/jquery/blob/1.6.1/src/ajax.js#L730

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.

Attachments (0)
Change History (9)

Changed June 14, 2011 06:59PM UTC by Niall Smart <niall.smart@gmail.com> comment:1

Patch sent as pull request:

https://github.com/jquery/jquery/pull/411

Changed June 14, 2011 07:27PM UTC by addyosmani comment:2

cc: → jaubourg
component: unfiledajax

Changed June 29, 2011 07:29PM UTC by timmywil comment:3

priority: undecidedlow
status: newopen

Changed June 29, 2011 10:18PM UTC by jaubourg comment:4

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.

Changed June 29, 2011 10:22PM UTC by anonymous comment:5

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

Changed June 29, 2011 10:52PM UTC by jaubourg comment:6

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

Changed July 01, 2011 01:47PM UTC by jaubourg comment:7

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.

Changed July 01, 2011 05:15PM UTC by Niall Smart <niall.smart@gmail.com> comment:8

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

Changed November 21, 2011 08:43PM UTC by anonymous comment:9

I believe we typically fall into this category here. 

http://bugs.jquery.com/ticket/7363