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

Memory Leak: `material_select` never removes global `click` handlers #4266

Closed
jvilk opened this Issue Feb 19, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@jvilk

jvilk commented Feb 19, 2017

Description

$.material_select installs multiple global click handlers on window, but never cleans them up:

$(window).on({

This results in a memory leak in single-page applications that call this function when rendering a view. Every time the function is called, new global click handlers are added. Since they are never removed, the browser cannot garbage collect the handler or any data within its closure (which includes DOM NodeLists within jQuery objects).

Is there a way to either avoid a global click handler, to only install a single global click handler, or to clean up global click handlers at some point?

Repro Steps

N/A

Screenshots / Codepen

N/A

@jvilk

This comment has been minimized.

jvilk commented Feb 19, 2017

Note: I discovered this issue while debugging memory leaks in Piwik, which uses this library.

@r3wt

This comment has been minimized.

r3wt commented Feb 20, 2017

@jvilk

the lifecycle of SPA's are beyond the scope of jQuery plugins. one approach you might take is to override jQuery's init() method and use it to maintain a list of selectors with bound events, and use the well known hack to remove all events.

override $.fn.init:

var watchedSelectors = [];
$.fn.init = function(selector,context)
{
    //you could maybe add some logic to see if you want to watch it or not.
    var jQinstance = new jQselector(selector,context||window.document,$);
    
    watchedSelectors.push(selector);

    return jQinstance;
};

remove events when needed:

for(var i=0;i<watchedSelectors.length;i++){
        $(watchedSelectors[i]).find("*").addBack().off();
}
watchedSelectors.length = 0;

you could probably take it a bit further and remove from specific selectors, but i'll leave that exercise up to you.

@jvilk

This comment has been minimized.

jvilk commented Feb 20, 2017

Thanks for the info. I'm not immediately familiar with expected jQuery plugin use cases, but if you anticipate web applications calling material_select a variable amount of times in any scenario, then this is still a valid issue.

A hackfix for a SPA using materialize would be to call $(window).off('click') when the "page" redraws, assuming no other part of the page registers a click handler on window.

A possible solution on your end is to expose a cleanup method in some manner, letting folks who are affected by this issue call the method to remove the handlers (material_select_cleanup?). I do not know if there is a convenient / expected way for jQuery plugins to expose this functionality.

Anyway, I am only here to inform you of the issue; I discovered it while hunting down memory leaks in Piwik. As an outsider to jQuery plugin development, I defer to your expertise regarding what, if anything, should be done about it.

@r3wt

This comment has been minimized.

r3wt commented Feb 20, 2017

@jvilk I'm not associated with the materialize team at all. I built a SPA framework based on jQuery starting in 2015 and ending in may of last year, so i encountered the same type of issues with Foundation and Bootstrap.

As to your question:

A possible solution on your end is to expose a cleanup method in some manner, letting folks who are affected by this issue call the method to remove the handlers (material_select_cleanup?). I do not know if there is a convenient / expected way for jQuery plugins to expose this functionality.

There really isn't a defined standard for destroying plugins. There were several suggested boilerplate plugin frameworks that provide it like $.fn.pluginName('destroy') but alot of plugins don't have this functionality because the boilerplate wasn't widely adopted. Some plugins provide a way to destroy and cleanup, most don't. You're going to run in to this problem time and time again with jQuery plugins and SPA's, in my experience. The only solution i could come up with was the above one i posted.

@fega

This comment has been minimized.

fega commented Apr 15, 2017

@r3wt thanks! 😄

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