Opened 4 years ago
Last modified 4 years ago
#15309 new bug
Memory leak on draggable.disable and enabled
Reported by: | Rouven Bauer | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | none |
Component: | ui.draggable | Version: | 1.12.1 |
Keywords: | Cc: | ||
Blocked by: | Blocking: |
Description (last modified by )
I've noticed that repeatedly calling $(...).draggable("disable")
and then $(...).draggable("enable")
causes a memory leak. Please check out this jsFiddlehttps://jsfiddle.net/1hz9b4yk/23/ to see it in action or have a look at these screenshots.
- https://chat.opendfki.de/files/qkmjkpfys7nndktwf8c89uojuy/public?h=4IjgAYUsPQ5PloraoXuV6GD9n3wDNa0nZ2t53ATFbTg
- https://chat.opendfki.de/files/qtfarjppzbguzy4kpe3uj3hc4a/public?h=gXc9i6wTB0rXjXlJs9kv5lWDmS5ZxtXFHpInMOIvf1U
- https://chat.opendfki.de/files/c3uhwgwfobyiinoi7uaua5mnyo/public?h=v6JjFTH0ltc1Oa1V7WGCQ98zKoxLtCze9mKRNTVsLgY
- https://chat.opendfki.de/files/kce7x1j8z7ro5jommfzc6t7cfc/public?h=KkKPfQOdZ4olM_0CCfucZC4PJmT1w-nq2u-0ceI1i1U
- Note: screenshot taken on a colleague's MacBook running Chrome.
This happens to me on Linux Mint 18.4 (64 bit) Firefox and Chromium (even though I doubt that any of this is relevant). jQuery-UI is up to date (1.12.1) and jQuery is on version 3.3.1.
I've tracked down the problem and found this
... _setOptions (jquery-ui.js:434) (anonymous) (jquery-ui.js:144) _setOption (jquery-ui.js:2092) _super (jquery-ui.js:129) _setOption (jquery-ui.js:448) _setOptionDisabled (jquery-ui.js:486) _toggleClass (jquery-ui.js:569) _classes (jquery-ui.js:529) _on (jquery-ui.js:575)
So basically the _classes helper method calls ._on
over and over again (jquery-ui.js:529):
this._on( options.element, { "remove": "_untrackClassesElement" } );
while ._on
does two things that consume memory at each call:
- jquery-ui.js:591
this.bindings = this.bindings.add( element );
- jquery-ui.js:622
element.on( eventName, handlerProxy );
while handlerProxy is new function each time.
I came up with a quick-fix but since I'm not too familiar with the whole code base I'm unsure if this breaks anything else. I've replaced the ._on
method with this:
_on: function( suppressDisabledCheck, element, handlers ) { var delegateElement; var instance = this; // No suppressDisabledCheck flag, shuffle arguments if ( typeof suppressDisabledCheck !== "boolean" ) { handlers = element; element = suppressDisabledCheck; suppressDisabledCheck = false; } // No element argument, shuffle and use this.element if ( !handlers ) { handlers = element; element = this.element; delegateElement = this.widget(); } else { element = delegateElement = $( element ); if ( this.bindings.length !== this.bindings.not( element ).length ) { this.bindings = this.bindings.add( element ); } } if (instance._registeredUIHanlders == null) { instance._registeredUIHanlders = {}; } $.each( handlers, function( event, handler ) { if ( !instance._registeredUIHanlders.hasOwnProperty(event) ) { instance._registeredUIHanlders[ event ] = []; } if ( $.inArray( handler, instance._registeredUIHanlders[ event ] ) !== -1 ) { return; } instance._registeredUIHanlders[ event ].push( handler ); function handlerProxy() { // Allow widgets to customize the disabled handling // - disabled as an array instead of boolean // - disabled class as method for disabling individual parts if ( !suppressDisabledCheck && ( instance.options.disabled === true || $( this ).hasClass( "ui-state-disabled" ) ) ) { return; } return ( typeof handler === "string" ? instance[ handler ] : handler ) .apply( instance, arguments ); } // Copy the guid so direct unbinding works if ( typeof handler !== "string" ) { handlerProxy.guid = handler.guid = handler.guid || handlerProxy.guid || $.guid++; } var match = event.match( /^([\w:-]*)\s*(.*)$/ ); var eventName = match[ 1 ] + instance.eventNamespace; var selector = match[ 2 ]; if ( selector ) { delegateElement.on( eventName, selector, handlerProxy ); } else { element.on( eventName, handlerProxy ); } } ); },
Change History (3)
comment:1 Changed 4 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 4 years ago by
Component: | ui.core → ui.draggable |
---|
Oh my quick fix is missing something: One also needs to do some extra clean up in
._off
: