New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
crmouli opened this Issue Oct 30, 2014 · 12 comments

Comments

Projects
None yet
7 participants
@crmouli

crmouli commented Oct 30, 2014

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Oct 30, 2014

Member

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

Member

dmethvin commented Oct 30, 2014

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

@jaubourg

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Oct 30, 2014

Member

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.

Member

jaubourg commented Oct 30, 2014

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Oct 30, 2014

Member

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.

Member

dmethvin commented Oct 30, 2014

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

This comment has been minimized.

Show comment
Hide comment
@crmouli

crmouli Oct 31, 2014

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.

crmouli commented Oct 31, 2014

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Oct 31, 2014

Member

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?

Member

dmethvin commented Oct 31, 2014

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

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Oct 31, 2014

Member

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

Member

jaubourg commented Oct 31, 2014

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

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 31, 2014

Member

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

Member

mgol commented Oct 31, 2014

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

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Oct 31, 2014

Member

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

Member

dmethvin commented Oct 31, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Oct 31, 2014

Member

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

Member

jaubourg commented Oct 31, 2014

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

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 31, 2014

Member

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

Member

mgol commented Oct 31, 2014

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

@markelog markelog added the Core label Nov 2, 2014

@ParsnipCommander

This comment has been minimized.

Show comment
Hide comment
@ParsnipCommander

ParsnipCommander Nov 6, 2014

Nice! Have a beer on me with @changetip

ParsnipCommander commented Nov 6, 2014

Nice! Have a beer on me with @changetip

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Mar 28, 2016

Member

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.

Member

timmywil commented Mar 28, 2016

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.

@timmywil timmywil closed this in 5cbb234 Apr 4, 2016

Totktonada added a commit to Totktonada/zepto that referenced this issue Oct 31, 2016

Prevent interruption of ready callbacks
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)
```

Totktonada added a commit to Totktonada/zepto that referenced this issue Oct 31, 2016

Prevent interruption of ready callbacks
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)
```

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.