Effects: exception in animation callback causes endless loop

[jixxer]

This bug was (in some sense) already discovered 1 year ago (Ticket #2846), but no action was taken.

If a function, which is passed as callback function to any animation-effect-function (animate/show/hide/...), throws an exception jQuery generates an endless loop (actually it's a function called via a setInterval which is never canceled and the setInterval keeps running every 13ms(!).

All linenumbers refer to ​effects.js jQuery 1.4a2

The proposed patch is (XXXX to be replaced by actual ticket number)

401c401
401c401,402
< this.options.complete.call( this.elem );
---
> // ignore exceptions in callback check Ticket #XXXX
> try { this.options.complete.call( this.elem ); } catch () { }

---

More in detail the following things can happen

• the callback function is called repeatedly forever (unless at a certain time it stops throwing the exception)

• if multiple elements where being animated the callback function is only called for the first element which finished animating

• if the reason for the callback function to throw an exception depends on the element which finished animating, the callback is never called for all elements which complete after the first element which caused the exception in the callback

• all animations started, after such a faulty callback function was triggered, fail to work completely

• depending on browser + hardware CPU utilization could og to 100% or even whole browser/OS could stall

• other jQuery code not related to animation continues too function normally

As already pointed out in the other bug ticket the cause for this is that the call to clearInterval( timerId ); on line 440 in the jQuery.fx.stop function is never reached.

It is also clear why this happens if you are familiar with the inner workings of jQuery for animations. The call to this.options.complete.call( this.elem ); on line 401 in jQuery.fx.step throws the exception thus (further up the call stack) the code on the lines 430-435 in jQuery.fx.tick() won't ever be reached which means the timers/jQuery.timers array is never emptied and jQuery.fx.stop never called.

I understand that it is debatable if this fix should even be applied as it masks an exception and the root fault which triggers this bug is caused by the "programmer" and not directly by jQuery.

So if the decision is to not apply this fix both tickets this one and #2846 should be marked as nofix. But we should add a note to the animation documentation that a fault callback function can have this nasty endless "recursion" effect.

---

Before reporting I tested in Opera 10.10, FF 3.5.6 and IE6.

Opera 10.10 and FF 3.5.6 show the reported behavior.

IE6 seems to degrade in some strange way also fails to run any animations afterwards

Can't test on IE7, IE8, Safari, Chrome, ...

[jixxer]

small test case

[anonymous]

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'); });

[SlexAxton]

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.

[mail@…]

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)

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

[Jason a.k.a. Driskell <mail@…>]

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

[Jason a.k.a. Driskell <mail@…>]

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

[john]

We should look into this for 1.7.

[john]

Nominating ticket for 1.7 discussion.

[Rick Waldron]

+1, Sounds like a bug, should be fixed

[jaubourg]

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

[ajpiano]

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

[Timmy Willison]

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

[danheberden]

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

[dmethvin]

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

[mail@…]

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.

[Driskell]

+1, Just registered an account and hoping I'll get email notifications this time! :) (I'm the Jason mail@ guy)

[john]

+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).

[scottgonzalez]

+1, repeating errors is obviously wrong

[jzaefferer]

+0

[cowboy]

+0

[ajpiano]

Targeting for 1.7 at bug triage

[gnarf]

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

[paulpro@…]

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

[gnarf]

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

[Corey Frang]

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

[Timmy Willison]

#10425 is a duplicate of this ticket.