Clean up grid on destruction

[andyrooger]

This is intended as a fix to #618. The grid leaks memory in a few places when it is destroyed.

Plunker demonstration of the problem is in the linked issue.

[BertrandDechoux]

I confirm that this is a concern. Any chance that it will be pulled/merged?
( #618 has a longer description of the problem )

[roblarsen]

Yes, I just need to build and test this out. We could also squash those commits into one, but that's more housekeeping.

[andyrooger]

I can squash and rebase to latest master later today. That's if no one is going to mind me disturbing anything they have checked out from this PR.

[andyrooger]

Squashed and rebased, but don't seem to be able to push - my net's having a bad day. There are 2 unit tests failing for me but it looks like the same 2 are failing in master.

[steeeveb]

Any hope for this patch to be included? I have a 30MB memory leak every time I change page in my app... Is there a known workaround for this kind of problem?

[wawyed]

I would like to request some advance with this patch too, it's really hitting the performance specially in ie.

[c0bra]

I plan on taking a look at this shortly. There's more places that event handlers need to be cleaned up.

[taifu]

Great!! Thank you so much! I'm also hanging on to this patch :-)

[c0bra]

What's the minimum needed to reproduce this? Create two pages with ngRouter, both with grids and then swap back and forth between them a bunch? I should see memory use climb in the profiler?

[wawyed]

Yes, exactly. Basically every time the controller gets recreated along with the template there is a memory increase due to watches being redefined without clearing previous ones.

[andyrooger]

I think that should be sufficient. Might be hard to spot unless you have a fairly large dataset in your grid, but I think it's leaving the whole grid DOM behind each time it is destroyed. When I wrote the pull req, it was leaking in window event handlers, and the initial data set if you keep it around. May have changed since though.

[c0bra]

Unfortunately it looks likes the leaks are still occurring. I'm using this plunker as a test bed: http://plnkr.co/edit/j4DHNd?p=preview

I'm going to go through and try to add $destroy handlers to the rest of the places we bind and see if that helps any.

[c0bra]

Still leaking after adding a $destroy handler to every single .on, .bind, .$on, and .$watch.

By doing heap snapshots after a single route change on that plunker I can see the count of ngGrid instances growing by one each time. Not sure why it isn't getting GC'ed.

[steeeveb]

I don't know if it can help, but I found this post http://roubenmeschian.com/rubo/?p=51
The author suggests to look to other possible sources of leaks.

[c0bra]

I've spent the last few days trying to track this down and am stuck. It looks like ng-repeat itself leaks. You can find my issue here:

angular/angular.js#6429

For now I'm closing this as I've taken it as far as I can. If anyone can discover the actual source of these leaks then I'll happily reopen it.

[steeeveb]

In my project the leak is present on firefox too. I can clearly see it watching the about:memory page and using the diff tool.

[saebekassebil]

@c0bra Well wouldn't at least some memory improvements be better than none? This is a major issue in our app and although ng-grid still leaks, this would certainly be better than nothing.

[c0bra]

@saebekassebil I didn't find any actual improvements in my testing. I'm not convinced that the root issue is actually IN ng-grid's code. If someone can demonstrably verify the root cause and provide a fix then I will happily merge it.

[olofd]

Really problematic in my codebase aswell.. ng-grid is really leaky.. tried looking into it but hard to do when you have no knowledge of the codebase.

[dieresys]

FYI, as of version 3.2.9 the memory leak is still there.
You have to make sure that every event you've hooked too is manually destroyed before creating a new instance of the grid, or the grid will hold a reference to the gridApi of the previous instance (including the data).
This is really noticeable when the data set is big (400Mb in my case).

Edit:
OK, I should have waited a couple of minutes before posting.
The problem with my code was that I was passing a scope to the 'on.eventName' that was not being destroyed when the grid was destroyed.
The grid was inside of an ng-repeat directive, and that should have been the scope that I should have used as parameter.