Bug Tracker

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#9585 closed bug (wontfix)

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

Reported by: Niall Smart <niall.smart@…> 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.

Change History (9)

comment:1 Changed 8 years ago by Niall Smart <niall.smart@…>

Patch sent as pull request:

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

comment:2 Changed 8 years ago by addyosmani

Cc: jaubourg added
Component: unfiledajax

comment:3 Changed 8 years ago by timmywil

Priority: undecidedlow
Status: newopen

comment:4 Changed 8 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 8 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 8 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 8 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 8 years ago by Niall Smart <niall.smart@…>

@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 8 years ago by anonymous

I believe we typically fall into this category here. 

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

Note: See TracTickets for help on using tickets.