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 #68

Closed
sszabolcs opened this issue Apr 4, 2016 · 3 comments
Closed

Memory leak #68

sszabolcs opened this issue Apr 4, 2016 · 3 comments

Comments

@sszabolcs
Copy link

It seems to me, that although there is a destroy() method available in the API to cleanup everything before removing the input the datepicker is applied to, to avoid memory leaks, there remain references to event handlers in JQuery objects, specifically for 'resize' and 'touchstart' event handlers, and this keeps the GC from throwing the datepicker instance out of memory.

Steps to produce the leak (in Chrome):

  1. Open this fiddle and run it: https://jsfiddle.net/fz9gxn4q/5/
  2. Open up Chrome Dev Tools, click on Profiles and create a new heap snapshot.
  3. Click on Toggle button, the input - to which the datepicker is applied to - appears.
  4. Click on Toggle, now the input disappears and we expect that everything is cleaned up.
  5. Take a second heap snapshot.
  6. Click on Toggle again, the input appears.
  7. Click on Toggle to destroy the input.
  8. Take the last heap snapshot.
  9. Now if you list the objects which were created between the 1. and 2. snapshots (these are the objects which should not be there, because the GC should have cleaned them up) you can see that there is a $.Zebra_Datepicker instance, because the 'resize' event handler is referenced from JQuery:
    image

I have found the cause of the problem. JQuery does not match the handler for the 'resize' event when you call $(window).unbind() for 'resize' event:
image
handleObj.namespace does not match the regular exp. in tmp
It does not match because of the comma at the end of the namespace, I think.

The same problem arises when you register the event handler for 'mousedown' and 'touchstart' events and the event names are separated with comma.

When I split up the event handler registration in like manner for both 'resize' and 'orientationchange' and for 'mousedown' and 'touchstart' then the memory leak goes away (zebra_datepicker.src.js ~1417. row):

`
//whenever anything is clicked on the page
var mouseDownAndTouchStartHandler = function (e) {

                // if the date picker is visible
                if (datepicker.hasClass('dp_visible')) {

                    // if the calendar icon is visible and we clicked it, let the onClick event of the icon to handle the event
                    // (we want it to toggle the date picker)
                    if (plugin.settings.show_icon && $(e.target).get(0) === icon.get(0)) return true;

                    // if what's clicked is not inside the date picker
                    // hide the date picker
                    if ($(e.target).parents().filter('.Zebra_DatePicker').length === 0) plugin.hide();

                }

            };
            $(document).bind('mousedown.Zebra_DatePicker_' + uniqueid, mouseDownAndTouchStartHandler);
            $(document).bind('touchstart.Zebra_DatePicker_' + uniqueid, mouseDownAndTouchStartHandler);

`

I've found another problem which connects to this closely. In the destroy() method you do not unbind the event handler for 'touchstart' event, this causes another memory leak.

@PhiLhoSoft
Copy link

Is this fixed by 127ebd2 ?

@sszabolcs
Copy link
Author

sszabolcs commented Apr 5, 2017

It is only partly fixed with this commit. I made a pull request with a fix (#81) for this.

@stefangabos
Copy link
Owner

This is now fixed by c1e8f93 Thank you!

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

3 participants