Skip to content
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

code has no cleanup function to remove event handlers for preventing memory leaks #21

Closed
trs79 opened this issue Sep 3, 2013 · 3 comments

Comments

@trs79
Copy link

trs79 commented Sep 3, 2013

Thanks for the great plugin, but I'm using this in a single page application and each time I setup the file uploaders, the previous divs with file inputs are still in the DOM. Any chance a cleanup function could be added that removes events and the nodes?

@LPology
Copy link
Owner

LPology commented Sep 3, 2013

The divs with file inputs are being removed after every upload (line 689):

ss.remove(this._input.parentNode);

What you're seeing is the new file input that is created after every upload is finished.

@trs79
Copy link
Author

trs79 commented Sep 3, 2013

Ah that makes sense. The newly created divs with inputs are what I must be seeing. Currently I'm manually looping through each file input and removing it's parent div with jquery in my cleanup code (otherwise the divs keep accumulating), but since the events were attached without jquery, I'm not sure if the memory is freed.

I've seen some plugins have a "destroy" type method that removes all elements and event handlers

@LPology
Copy link
Owner

LPology commented Sep 4, 2013

It's odd that you're seeing more than one div containing a file input. Here is the pattern the plugin uses:

  1. A file input is created upon initialization, usually when the page is loaded.
  2. After an upload begins, the file input is no longer needed and is removed, after which a new div/file input is created and appended to the DOM.
  3. The next upload uses the newly created file input, and the cycle continues.

Point is, you shouldn't have to manually remove any elements, and actually, doing so requires the plugin to create twice as many elements -- the file input that is created automatically, and then another file input to replace that one after you've removed it and an upload is attempted:

if (!self._input) {
  self._createInput();
}

When you don't manually remove the file inputs, are you still seeing more than one div/input per plugin instance?

In re: removing event handlers, the reason the events aren't just removed with a line of code is because they're being attached with anonymous functions, which can't be later removed. (see: http://stackoverflow.com/questions/3106605/removing-an-anonymous-event-listener)

It is true that a memory leak can result from removing an element without first removing event listeners, but the issue is confined to older versions of Internet Explorer, namely IE6 and IE7. (see: http://stackoverflow.com/questions/6033821/do-i-need-to-remove-event-listeners) All other browsers remove event listeners when an element is removed.

However, since IE6 and 7 still somehow have 7.5% of browser market share, it's an issue worth considering. Fortunately, there is an easy fix: http://javascript.crockford.com/memory/leak.html

I updated the demo (http://www.lpology.com/code/ajaxuploader/) with the new code (starting line 310) and will push the new version after a little more testing.

Thanks a lot for bringing this up, I definitely appreciate it.

@LPology LPology closed this as completed Sep 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants