ghusse / jQRangeSlider

A jquery UI range selection slider that supports dates
http://ghusse.github.com/jQRangeSlider/
GNU General Public License v3.0
671 stars 147 forks source link

jQRangeSlider leaks memory even if 'destroy' is called #108

Closed rosenfeld closed 11 years ago

rosenfeld commented 11 years ago

http://jsbin.com/apaduf/3

Also, notice there are some timing issues if you try to use a for-loop to create and destroy the slider several times. That's why I had to use setTimeout and a recursive function.

I'll try to investigate the reasons for the leak once I'm back from lunch.

ghusse commented 11 years ago

Do you have any clue?

rosenfeld commented 11 years ago

I've spent some hours this afternoon trying to figure out what was happening but I could only fix part of the leaks, not all of them. So, I gave up and I'm trying now to adapt my code to use jQuery UI slider component.

Here are the parts I could identify the leaks on Chrome so far.

First, all those detached elements should actually be removed:

https://github.com/ghusse/jQRangeSlider/blob/master/jQRangeSlider.js#L673

Trying to remove the bar element may cause trouble so I had to do it through a setTimeout.

Then, there seems to be a circular dependency here:

https://github.com/ghusse/jQRangeSlider/blob/master/jQRangeSlider.js#L71

Changing it to the code below seemed to fix the leak in $.cache:

var that = this; $(window).resize(function(e){that.resize(e);});
// or:
// $(window).resize($.proxy(function(e){this.resize(e);}, this))

Creating this._resizeProxy seems to be creating a leak.

After those changes it seems no event listener will be leaking anymore, but there are still some detached elements leaking, regarding the handlers. I'm not that good in understanding the Heap Profiler tool of Chrome, so I gave up on trying to remove the leaks and decided to change to jQuery UI sliders for now... :(

ghusse commented 11 years ago

Thanks a lot.

I came to the same conclusion about detached elements and some other events handlers. Thanks a lot, It'll be useful.

I'll fix that as soon as possible.

rosenfeld commented 11 years ago

great, thanks

rosenfeld commented 11 years ago

You can have an overall idea by looking at the Memory pane under the Timeline main pane. Start recording then do this in the console:

doLeak()
$('.ui-rangeSlider').rangeSlider('destroy')

Give it some time between the actions so that it makes it easier for you to see the differences in the Counters pane.

Stop recording the timeline and adjust to show all time between before calling doLeak() and after destroying it. Let me know if this seems confusing to you and I'll try to explain it better.

ghusse commented 11 years ago

Can you test now? It seems ok to me (with latest version on master branch).

rosenfeld commented 11 years ago

The Event Listener leak is fixed now on master, but it is still leaking some DOM nodes: 1LMF88xB

rosenfeld commented 11 years ago

Also it seems there is a leak on some LabelPositioner instance

rosenfeld commented 11 years ago

detached nodes include divs with class "ui-rangeSlider-label", "ui-rangeSlider-handle", "ui-rangeSlider-arrow", "ui-rangeSlider-arrow-inner" and "ui-rangeSlider-innerBar"

ghusse commented 11 years ago

Ok, thanks. I'll take a look.

ghusse commented 11 years ago

Strange, but it seems I cannot have the node graph working on any version of Chrome I have (Mac OS X and Windows). I always have a flat line !

ghusse commented 11 years ago

Ok, got it, I have to select a range in timeline, it does not select the whole range by default.

ghusse commented 11 years ago

I had to recreate your script locally, run it in chrome with the option --js-flags="--expose-gc", call gc() at the end.

After that, I managed to have consistent results in the timeline tool.

So, I fixed more DOM nodes leaks. I did not manage to get the same DOM count before and after running the script, but I cannot see more indications on what is leaking. Can you run your test with updated source and tell me if you have any more clue?

Thanks

rosenfeld commented 11 years ago

Hi Guillaume, I just woke up. I've already migrated from jQRangeSlider to a custom component based on jQuery UI slider: http://jsbin.com/iqomom/1

But I'll try to help you anyway by providing as much guidance as I can on what I've learned about the Chrome Developer Toolbar with regards to the Timeline and Profiles tabs for inspecting memory leaks.

First, you don't need to expose the gc, just click on the trash icon in the bottom of the Timeline tab before stop recording. For the Profiles tab, Chrome will always run gc before taking a snapshot. I usually find the leaking nodes by taking a heap snapshot, creating and destroying the component, taking another heap snapshot and trying to find the created objects between them. Then I sort by class and try to find the created divs. The ones shown in red are detached ones and most probably they're leaking. Then, when I hover those elements I can see more details about them, like the classes, so that I can identify which elements are leaking from jQRangeSlider.

Unfortunately I find somewhat confusing to understand exactly what object is preventing some detached node from being collected by using their Profiles tab. Sometimes it is easier, but sometimes I have no clue...

I'll try to see what remaining divs are leaking with your latest version and will let you know.

rosenfeld commented 11 years ago

Also, I always investigate memory leaks in an anonymous session where all extensions are disabled to avoid any distraction.

rosenfeld commented 11 years ago

Also, filter your classes in the Profiles tab for "t.(anonymous function).(anonymous function)". Those objects are created by jQRange Slider and there's an object from this class still leaking.

rosenfeld commented 11 years ago

"b.fn.b" is also from jQRangeSlider. All those proxied functions make it a bit hard to understand your memory leaks... Maybe they could be even the cause for some of them...

rosenfeld commented 11 years ago

Also, notice that I can't click the Leak button in my jsbin anymore as it will throw many exceptions like "remoteWindow is not defined" and "Cannot read property 'rangeSliderBar' of null"

rosenfeld commented 11 years ago

Also, I noticed you haven't fixed the leak I pointed out here: https://github.com/ghusse/jQRangeSlider/issues/108#issuecomment-16533902

https://github.com/ghusse/jQRangeSlider/blob/master/jQRangeSlider.js#L71

ghusse commented 11 years ago

Hi,

Thanks for your help and all tips you're giving me.

About the leak you pointed out, I fixed it but not in the same place: https://github.com/ghusse/jQRangeSlider/blob/master/jQRangeSlider.js#L681

I tested your script, it works on my browser. Is it an issue with jsbin? Are you sure that runned scripts are up to date, and not cached versions?

rosenfeld commented 11 years ago

http://pbrd.co/11qJiHw

rosenfeld commented 11 years ago

As you can see, I'm using latest version instead of a cached one: http://pbrd.co/11qJF4Z

rosenfeld commented 11 years ago

Try this: doLeak().rangeSlider('destroy')

rosenfeld commented 11 years ago

Here is why I think it is leaking: http://pbrd.co/11qKLOj

rosenfeld commented 11 years ago

I don't know why it leaks, but my suggested approach doesn't leak. You might try to delete this._bindResize in destroy() and see if the leak stops

ghusse commented 11 years ago

Ok, I understand why: there is a circular reference.

rosenfeld commented 11 years ago

I'm sorry, but I don't think this fixes the leak, although it is certainly smaller:

http://pbrd.co/11sQoeI

ghusse commented 11 years ago

I think I found the main problem. Now, I'm sure to have no listener leak left, and it seems to have fixed the detached nodes issue as well.

rosenfeld commented 11 years ago

The leak doesn't seem fixed to me: http://pbrd.co/179Tzx4, http://pbrd.co/179TM36, http://pbrd.co/179TNnT

ghusse commented 11 years ago

I don't have the same results at all.

I'm running 50 create/destroy in a script, and after that, here are the remaining objects (created between my 2 heap snapshots)

Capture d e cran 2013-04-20 a 21 34 19

Nothing related to jQRangeSlider, except for the constructor of LabelPositionner (which is normal, I suppose).

Nothing related to the function you're pointing out, and no detached DOM node.

rosenfeld commented 11 years ago

Indeed, LabelPositionner is not leaking and memory is not increasing fast as before, you did a great job in this respect. But I still see some leaks like this: http://pbrd.co/Zc7hNZ

But as I don't understand the leak, maybe this is some bug in my Chrome version (28.0.1478.0 dev)

ghusse commented 11 years ago

I'm using Chrome 26.0.1410.65, and as you can see, _resizeProxy is not leaking memory with this version. Maybe it's related to your version of chrome, as you pointed out.

Thanks a lot for your help!

rosenfeld commented 11 years ago

no problem