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 comment:1
Changed June 14, 2011 07:27PM UTC by comment:2
| cc: | → jaubourg |
|---|---|
| component: | unfiled → ajax |
Changed June 29, 2011 07:29PM UTC by comment:3
| priority: | undecided → low |
|---|---|
| status: | new → open |
Changed June 29, 2011 10:18PM UTC by 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 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 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 comment:7
| resolution: | → wontfix |
|---|---|
| status: | open → closed |
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 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 comment:9
I believe we typically fall into this category here.
Patch sent as pull request:
https://github.com/jquery/jquery/pull/411