This bug drove me crazy for weeks. Finally found the problem yesterday and after a while found this ticket. Anyway, here's a simple test case for any skeptics. Just run this in firebug.

$('a:first').hide('blind', function () { console.log('called'); throw new Error('test'); });

I created a live test case for this one: http://jsfiddle.net/shAZT/

The problem occurs because there is an interval that is never cleared because you are throwing an error in every iteration of the callback...

While technically, you should catch your errors, it would probably be good for jQuery to be able to handle situations where the programmer fails.

I'll pass this through as an enhancement to 'protect' the user. Exceptions _usually_ mean _your_ code is broken, so I'd suggest wrapping the code that causes this exception in a try catch block. jQuery could do that, but it'd be too expensive to do for all cases, just so the few people who accidentally cause exceptions don't break (more).

We'll look at doing some code restructuring so it doesn't rely on the successful callback of the function.

Thanks for the ticket and test case! Very much appreciated.

Hi all,

I thought maybe one could try returning the callback function from step(). Then if we return true, it means tick along. If we don't, it means end of the animation. We then drop the timer, and see if we returned a callback. If we returned a callback, we call it.

This adds just 2 extra compares - not sure if this is just as expensive as a try catch block but I would assume not.

An additional benefit is the error is still raised for each call of the function - but it doesn't cause a loop and thousands of entries in the error log, so for each instance the function runs and fails, it only fails once and generates a single exception. Only downside to this is the animations may delay slightly because if an exception raises on the first timer, the function fails and other timers won't tick until the next tick. But I consider this perfectly acceptable when an Exception is raised - getting rid of the loop is the most important thing as far as I'm concerned at least!

Here is the modifications I did to the released 1.4.4 file at http://code.jquery.com/jquery-1.4.4.js. If you would like me to apply the changes to a working copy or something let me know :)

(Sorry for dropping the diff directly in the post - thought it was the best place for it as it is only small)

#!diff
6746a

.
6744,6745c

			// #5684 (Driskell) step() returns true for incomplete animation (push to timers)
			// returns false when an animation in a set is completed and it is not the last
			// returns a callback when all animationsin a set are completed
			if ( (complete = timers[i]()) !== true ) {
				timer = timers.splice(i--, 1);
				if (complete !== false) complete.call(timer.elem);
.
6741c
		var timers = jQuery.timers,
			complete, timer;
.
6715,6716c
				// #5684 (Driskell) return the callback function so we can remove the timer before we call it
				// this will ensure any exception returned by the completion function does not cause a loop because the timer was not removed
				return this.options.complete;
.
6645,6647c
		// #5684 (Driskell) step() returns true for incomplete animation (push to timers)
		// returns false when an animation in a set is completed and it is not the last
		// returns a callback when all animationsin a set are completed
		if ( (complete = t()) === true ) {
			if ( jQuery.timers.push(t) && !timerId ) timerId = setInterval(fx.tick, fx.interval);
		} else if (complete !== false) complete.call(this.elem);
.
6630c
			fx = jQuery.fx,
			complete;
.

Let me know your thoughts.

Jason a.k.a. Driskell

Here's new patch file that follows jQuery Core Style Guidelines:

http://www.jasonwoods.me.uk/jquery/jquery-1.4.4.diff

And a prepatched file for those who need:

http://www.jasonwoods.me.uk/jquery/jquery-1.4.4.js

Let me know your thoughts.

Jason a.k.a. Driskell

Hi,

Found an issue with my previous patch on my development server - it didn't work when you called stop() to stop the animations. Also when taking the elem from the splice I missed the subscript so it returned undefined - which made things worse than before :) sorry!

Resolved it all in this new one, which we're now using in production. Let me know if anyone spots any other issues:

Patch for 1.4.4: http://www.jasonwoods.me.uk/jquery/jquery-1.4.4.diff.fixed

Prepatched file: http://www.jasonwoods.me.uk/jquery/jquery-1.4.4.js.fixed

If you need the 1.4.4 I used to create the patch it is here: http://www.jasonwoods.me.uk/jquery/jquery-1.4.4.js.orig

Also if you need to know I ran diff -E to make the patch. If we need different format let me know.

Kind Regards,

Jason a.k.a. Driskell

We should look into this for 1.7.

Nominating ticket for 1.7 discussion.

+1, Sounds like a bug, should be fixed

-1, No, no, no, no. Never EVER hide an exception. We could use a try/finally block... but we all know how stupidly unsupported this construct is in old IEs.

-1, Agreed with jaubourg, not really into masking the exceptions here.

+1, maybe keep the try/catch, throw a custom error, and stop the animation?

+0, any perf tests on the speed impact of running through try/catch?

+1, If we could use timmywil's suggestion maybe? I don't like a blind try/catch suggested by the OP.

Hi all,

One doesn't need a try/catch... simply calling the completion callback AFTER dropping the animation from the timers queue means the exception occurs ONCE. I agree that a blind try/catch would hide the exception and this is NOT correct behaviour. The issue here is an exception looping. The correct behaviour would be for the exception to occur once - and try/catch would result in no exception and therefore not be correct.

I attached a patch for this for 1.4.4 which moves the callback so it is the LAST thing jQuery calls as part of the animation step and it works perfectly - it merely adds 2 extra compares to the animation and returns a callback instead of "true" from step(). This is then handled in the functions that called step() and the callback called as soon as all jQuery animation code is finished. As a result, even if the callback fails - jQuery had nothing else to do anyway so we no longer get a loop.

Just let me know if you want an updated patch for 1.6.1 or something. I'm glad to help.

Jason.

+1, Just registered an account and hoping I'll get email notifications this time! :)

(I'm the Jason mail@ guy)

+1, This seems like something that we could work around with just some minor logic tweaking (we certainly don't want to hide the execeptions, though).

+1, repeating errors is obviously wrong

+0

+0

Targeting for 1.7 at bug triage

I'm gonna pick this one up and see if I can do anything with it.

Rather than absorb the error, we could execute the callback asynchronously, so that it will still throw the error, but won't affect the current running thread. It's a very simple edit to make. Can anyone think of any reason not to do it like this:

setTimeout(function(){
	options.complete.call(elem);
}, 1);

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

Landing pull request 520. Unset the complete function just before calling it to avoid an exception creating a loop. Fixes #5684.

More Details:

• https://github.com/jquery/jquery/pull/520
• http://bugs.jquery.com/ticket/5684

Changeset: 8dda57f82f1c366f10c591da099f8e9495eaa01b

#10425 is a duplicate of this ticket.