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

Memory leak when unbinding document events #428

Merged
merged 5 commits into from
Apr 10, 2018
Merged

Memory leak when unbinding document events #428

merged 5 commits into from
Apr 10, 2018

Conversation

monovertex
Copy link
Collaborator

@monovertex monovertex commented Apr 10, 2018

When unbinding the document click event, the call is incorrect, leaving the document event attached, creating a memory leak that can quickly clog the memory when instantiating / destroying many instances of the datepicker.

This PR fixes that issue.

In order to showcase it, see this JSFiddle. If you open the Performance profiler in Chrome and track memory allocation (make sure to force garbage collection before stopping the profiling), after running the JSFiddle you'll see this:

image

Which is an obvious HTML node memory leak.

You can check this JSFiddle to see this fix in action. After doing the same profiling process, we get this:

image

@holtkamp
Copy link
Collaborator

holtkamp commented Apr 10, 2018

Nice catch!

Which is an obvious HTML node memory leak.

Just to help me understand, what part exactly do you derive that from? The "quick" drop in number of nodes around 4500 ms? I see the same pattern for the "fix" profile, that is why I try to understand this. I have little experience with this 😊

@holtkamp holtkamp merged commit bb727b0 into longbill:master Apr 10, 2018
@monovertex
Copy link
Collaborator Author

No, please ignore the drop around 4500ms, that's because there's leftover memory allocated from the previous JSFiddle run. The important part is at the end, where the number of nodes remains constantly high, even though I forced garbage collection.

Unfortunately the JSFiddle examples seem to be a bit fickle, I'm guessing it's because of the JSFiddle environment itself. I could provide a standalone example, outside of the JSFiddle framework, if desired.

However, the bug is still obvious, even without memory profiles, since the event is attached using an anonymous function, but then cleared with closeDatePicker. This can't work, since the function reference has to be the same for the .unbind call to work. Not clearing bound event handlers is a classic memory leak, as the handler itself retains the reference to the datepicker instance, preventing it from being garbage collected.

@holtkamp
Copy link
Collaborator

@monovertex great, thanks for the detailed response, this really provides the required insight!

Just tagged a new release which includes your patch: https://github.com/longbill/jquery-date-range-picker/releases/tag/v0.16.4

@monovertex monovertex deleted the master-memory-leak-fix branch April 10, 2018 12:27
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

Successfully merging this pull request may close these issues.

2 participants