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

Exceptions from .trigger("remove") are silently swallowed #3554

Closed
tim-janik opened this Issue Feb 28, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@tim-janik

tim-janik commented Feb 28, 2017

Users can't see exceptions thrown by erroneous remove handlers, example:

$('<i/>').appendTo(document.body).on('remove', function() { console.log('remove:'); throw "some bug"; }).remove()

Observed:

remove:

Expected:

remove:
Uncaught "some bug"

Fix:

diff --git jquery-ui-1.12.1.js jquery-ui.js
index 0213552..32c6865 100644
--- jquery-ui-1.12.1.js
+++ jquery-ui.js
@@ -48,12 +48,12 @@ $.cleanData = ( function( orig ) {
 
                                // Only trigger remove when necessary to save time
                                events = $._data( elem, "events" );
-                               if ( events && events.remove ) {
-                                       $( elem ).triggerHandler( "remove" );
-                               }
-
                        // Http://bugs.jquery.com/ticket/8235
                        } catch ( e ) {}
+                       if ( events && events.remove ) {
+                               $( elem ).triggerHandler( "remove" );
+                       }
+
                }
                orig( elems );
        };
@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Feb 28, 2017

Member

I can't confirm that: https://jsfiddle.net/w975uww4/1/.

Note that .trigger('remove') is not the same as .remove().

Member

mgol commented Feb 28, 2017

I can't confirm that: https://jsfiddle.net/w975uww4/1/.

Note that .trigger('remove') is not the same as .remove().

@mgol mgol closed this Feb 28, 2017

@tim-janik

This comment has been minimized.

Show comment
Hide comment
@tim-janik

tim-janik Feb 28, 2017

@MoGL you are testing the wrong thing.

Note that .trigger('remove') is not the same as .remove(). The bug is not in jquery 'trigger()', but in the jqueryui implementation of cleanData which is called upon remove().
Please take a look at cleanData in:
https://github.com/jquery/jquery-ui/blob/master/ui/widget.js

I.e. your jsfiddle must test .remove(), not trigger('remove'), you'll be able to reproduce it that way.

tim-janik commented Feb 28, 2017

@MoGL you are testing the wrong thing.

Note that .trigger('remove') is not the same as .remove(). The bug is not in jquery 'trigger()', but in the jqueryui implementation of cleanData which is called upon remove().
Please take a look at cleanData in:
https://github.com/jquery/jquery-ui/blob/master/ui/widget.js

I.e. your jsfiddle must test .remove(), not trigger('remove'), you'll be able to reproduce it that way.

scottgonzalez added a commit to jquery/jquery-ui that referenced this issue Feb 28, 2017

Widget: Don't swallow errors in `remove` events
The try/catch was only there to support jQuery <1.6.3, which we no
longer support.

Ref jquery/jquery#3554
@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Feb 28, 2017

Member

The jQuery UI bug tracker is at https://bugs.jqueryui.com. I have already fixed this by removing the try/catch since it was only there for old versions of jQuery which we no longer support. See jquery/jquery-ui@1f2011e

Member

scottgonzalez commented Feb 28, 2017

The jQuery UI bug tracker is at https://bugs.jqueryui.com. I have already fixed this by removing the try/catch since it was only there for old versions of jQuery which we no longer support. See jquery/jquery-ui@1f2011e

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