When an error thrown from ready handler all subsequent handlers are not being invoked #1823

crmouli · 2014-10-30T10:49:04Z

Jquery version: 2.1.1
Browser: on any browser:
Testcase:
Please take a look at this jsfiddle
Example with jquery2.1.1
http://jsfiddle.net/q7gbzrbu/2/

Example with jquery1.5(where it works fine)
http://jsfiddle.net/q7gbzrbu/1/

[Edit: OLD and improper test case] http://jsfiddle.net/8b3a92yo/1/

Problem:
When an error is thrown from one of the handlers registered through $(docuement).ready() then all subsequent handler are not being invoked.

Addl info:
This is what happening when an error is thrown from a handler function which is registered with $.ready(). jQuery.Callbacks.fire()(refer line 3073 in jquery-2.1.1.js) will invoke the user's ready Handler function. Below is related piece of code. When an error was thrown, "firing" flag will not be reset to false. Due to this any handlers registered after error will not be triggered.(refer line 3113 in jquery-2.1.1.js)

firing = true;
for ( ; list && firingIndex < firingLength; firingIndex++ ) {
if ( list[ firingIndex ].apply( data[ 0 ], data[ 1 ] ) === false && options.stopOnFalse ) { //this statement invokes our ready handler
memory = false; // To prevent further calls using add
break;
}
}
firing = false;

It looks like "firing" flag need to be reset in a finally block.

Workaround:
Obvious workaround is handler code need to be wrapped with try/catch to avoid this issue.

dmethvin · 2014-10-30T13:10:39Z

What would you want us to do with the error once it's caught?

jaubourg · 2014-10-30T13:54:40Z

We could force the ready handlers to be called asynchronously, ie replace https://github.com/jquery/jquery/blob/master/src/core/ready.js#L12 with:

jQuery.ready.promise().done( function() {
    setTimeout( function() {
        fn.call( document, jQuery );
    } );
} );

But it won't solve the problem if the functions are added on the promise directly.

Note that using Standard Promises would be even worse, a handler throwing an exception would fail silently and nothing else would break making it quite tricky to know where the bug is.

Also, the previous implementation (<1.5) which would go through an array of function and call them sequentially did behave exactly the same way except when adding a function after the document is ready.

dmethvin · 2014-10-30T14:59:56Z

Agreed, this is far from an obvious fix. If we catch the error we have to report it somehow, otherwise we're masking the problem.

crmouli · 2014-10-31T10:35:05Z

Apologies for improper test case earlier..let me give you exact use case here

Example with jquery2.1.1
http://jsfiddle.net/q7gbzrbu/2/

Example with jquery1.5(where it works fine)
http://jsfiddle.net/q7gbzrbu/1/

yes, jquery should not suppress the error. It may be correct to not execute the pending handlers(in the queue at that time) when the first handler throws error. But that should not cause a problem for newly registered handlers. In the above example ready handler is added on click of a button.

It works fine on jquery 1.5 but fails with jquery 2.1.1. it looks like 1.5 uses finally block to reset "firing" flag. Is it possible to have similar fix in 2.1.1?.

Thank you for looking in to this.

dmethvin · 2014-10-31T14:04:51Z

Yes, I think it should be possible to restore the finally in version 3.0, which we had to remove because IE7 considers it to be a syntax error. @jaubourg what do you think?

jaubourg · 2014-10-31T14:23:53Z

Well, you know how it pained me we had to remove this finally block in the first place... so no argument from me.

mgol · 2014-10-31T14:25:35Z

How so? According to
http://msdn.microsoft.com/en-us/library/ie/4yahc5d8(v=vs.94).aspx, finally
is supported in IE7.

dmethvin · 2014-10-31T14:34:45Z

@mzgol the problem was that IE7 required catch so you couldn't have try {} finally {}. In other words, "There has to be a catch!" 😸

jaubourg · 2014-10-31T14:43:36Z

Yeah and rethrowing in the catch ended up in losing the actual context of the original exception.

mgol · 2014-10-31T14:49:11Z

Ah, right. It's good we're dropping IE<8 then!

ParsnipCommander · 2014-11-06T04:17:39Z

Nice! Have a beer on me with @changetip

Fixes jquerygh-1823

timmywil · 2016-03-28T15:27:44Z

For posterity, the behavior change here is that ready handlers are now executed async. In other words, if any code after adding a ready handler expected the handler to be executed synchronously
(i.e. jQuery( fn ); expectsSync();), that code will break. Hopefully, that isn't very common.

Note: This commit breaks assumption that ready callbacks called syncronously and immediatelly when DOM already parsed at the moment of adding the callback. I hope this assumption isn't very common, considering that jQuery now doesn't provide such guarantee (see [3]). HTML [spec][1] stated that one script can interrupt another one: > Otherwise > Immediately execute the script block, even if other scripts are already executing. I'm not sure which set of attributes should have scripts tags to make interruption possible. I guess if one script has `src` and `async` attributes and another one has no `src` attribute (inline script), than it is. As I understand Zepto functions (as wel as most of JS code at all) assume that interpreter is single-thread and doesn't interrupt scripts execution. That means that calling any Zepto functions is not safe in general. Since most of Zepto-related code likely placed inside `ready` callbacks, it's worth to protect the callbacks from interruption. I guess it can be achived by piping callback execution over task/events loop, i. e. wrapping callback call into `setTimeout` when it's not piped already via event queue. That's how jQuery [behaves][2]. By the way, they have one more [argument][3] to call callbacks asynchronously. [1]: https://html.spec.whatwg.org/multipage/scripting.html#script-processing-model [2]: https://github.com/jquery/jquery/blob/7bb62bb3ae771fc57cc62ee14bd10d94680efb4f/src/core/ready-no-deferred.js#L94 [3]: jquery/jquery#1823 Build size ---------- Calculated for a build with the default modules set: ``` zepto.js 58726 -> 58820 (+94 bytes) zepto.min.js 26427 -> 26451 (+24 bytes) zepto.min.gz 9804 -> 9802 ( -2 bytes) ```

Note: This commit breaks assumption that ready callbacks called syncronously and immediatelly when DOM already parsed at the moment of adding the callback. I hope this assumption isn't very common, considering that jQuery now doesn't provide such guarantee (see [3]). HTML [spec][1] stated that one script can interrupt another one: > Otherwise > Immediately execute the script block, even if other scripts are already executing. I'm not sure which set of attributes should have scripts tags to make interruption possible. I guess if one script has `src` and `async` attributes and another one has no `src` attribute (inline script), than it is. As I understand Zepto functions (as well as most of JS code at all) assume that interpreter is single-thread and doesn't interrupt scripts execution. That means that calling any Zepto functions is not safe in general. Since most of Zepto-related code likely placed inside `ready` callbacks, it's worth to protect the callbacks from interruption. I guess it can be achived by piping callback execution over task/events loop, i. e. wrapping callback call into `setTimeout` when it's not piped already via event queue. That's how jQuery [behaves][2]. By the way, they have one more [argument][3] to call callbacks asynchronously. [1]: https://html.spec.whatwg.org/multipage/scripting.html#script-processing-model [2]: https://github.com/jquery/jquery/blob/7bb62bb3ae771fc57cc62ee14bd10d94680efb4f/src/core/ready-no-deferred.js#L94 [3]: jquery/jquery#1823 Build size ---------- Calculated for a build with the default modules set: ``` zepto.js 58726 -> 58755 (+28 bytes) zepto.min.js 26427 -> 26453 (+26 bytes) zepto.min.gz 9804 -> 9803 ( -1 bytes) ```

dmethvin mentioned this issue Oct 30, 2014

Remove core/ready hard dependency on $.Deferred #1778

Closed

markelog added the Core label Nov 2, 2014

gibson042 mentioned this issue May 5, 2015

One ReferenceError or TypeError breaks $(document).ready and prevents it from ever firing again #2262

Closed

svenackermann mentioned this issue May 27, 2015

If a callback throws an error all subsequent calls to fire are ignored #2353

Closed

timmywil modified the milestones: 3.0.1, 3.0.0 Jan 15, 2016

timmywil added a commit to timmywil/jquery that referenced this issue Jan 21, 2016

Core: use try/finally to ensure future callbacks are executed

5ee4583

Fixes jquerygh-1823

timmywil mentioned this issue Jan 21, 2016

Core: implement ready without Deferred #2850

Closed

timmywil added the Has Pull Request label Jan 27, 2016

timmywil mentioned this issue Feb 1, 2016

Custom builds: use different ready code when excluding Deferred #2891

Closed

timmywil self-assigned this Mar 21, 2016

timmywil added a commit to timmywil/jquery that referenced this issue Mar 28, 2016

Core: use try/finally to ensure future callbacks are executed

6e5d33a

Fixes jquerygh-1823

timmywil added a commit to timmywil/jquery that referenced this issue Mar 28, 2016

fixup! use .then and give up sync guarantee

e4221ed

Fixes jquerygh-1823

timmywil added the Behavior Change label Mar 28, 2016

timmywil closed this as completed in 5cbb234 Apr 4, 2016

timmywil mentioned this issue Jun 24, 2016

Ready is much too late. #3197

Closed

Totktonada mentioned this issue Oct 31, 2016

More reliable way to examine DOM readiness on IE madrobby/zepto#1235

Merged

gibson042 mentioned this issue Dec 12, 2016

"Ready" Callbacks Should Not be Deferred #3447

Closed

lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When an error thrown from ready handler all subsequent handlers are not being invoked #1823

When an error thrown from ready handler all subsequent handlers are not being invoked #1823

crmouli commented Oct 30, 2014

dmethvin commented Oct 30, 2014

jaubourg commented Oct 30, 2014

dmethvin commented Oct 30, 2014

crmouli commented Oct 31, 2014

dmethvin commented Oct 31, 2014

jaubourg commented Oct 31, 2014

mgol commented Oct 31, 2014

dmethvin commented Oct 31, 2014

jaubourg commented Oct 31, 2014

mgol commented Oct 31, 2014

ParsnipCommander commented Nov 6, 2014

timmywil commented Mar 28, 2016

When an error thrown from ready handler all subsequent handlers are not being invoked #1823

When an error thrown from ready handler all subsequent handlers are not being invoked #1823

Comments

crmouli commented Oct 30, 2014

dmethvin commented Oct 30, 2014

jaubourg commented Oct 30, 2014

dmethvin commented Oct 30, 2014

crmouli commented Oct 31, 2014

dmethvin commented Oct 31, 2014

jaubourg commented Oct 31, 2014

mgol commented Oct 31, 2014

dmethvin commented Oct 31, 2014

jaubourg commented Oct 31, 2014

mgol commented Oct 31, 2014

ParsnipCommander commented Nov 6, 2014

timmywil commented Mar 28, 2016