search

longbill / jquery-date-range-picker

A jQuery plugin that allows user to select a date range
MIT License
1.12k stars 579 forks source link

Memory leak when unbinding document events #428

Closed monovertex closed 6 years ago

monovertex commented 6 years ago

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 commented 6 years ago

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 😊

monovertex commented 6 years ago

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 commented 6 years ago

@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