Memory leak in tooltip on scope.$destroy #1133

MWallenberg · 2013-10-07T06:37:30Z

My git-fu is weak, so I will attempt to describe this bug by the ancient technique known as "written text".

The tooltip directive reacts to the $destroy-event by calling tooltip.remove(), as seen on lines 299-306 in tooltip.js:

// Make sure tooltip is destroyed and removed.
scope.$on('$destroy', function onDestroyTooltip() {
if ( scope.tt_isOpen ) {
hide();
} else {
tooltip.remove();
}
});

The problem here is twofold: First, the tooltip-variable still holds a reference to the DOM element. Second, the DOM element still has event listeners attached. This mens that the DOM element will not be garbage collected, and memory is leaked.

To fix this issue, I propose unsetting event listeners and nulling out DOM references:

 // Make sure tooltip is destroyed and removed.
          scope.$on('$destroy', function onDestroyTooltip() {
            tooltip.off();
            if (scope.tt_isOpen) {
              hide();
            } else {
              tooltip.remove();
            }
            tooltip = null;
            $body = null;
          });

pkozlowski-opensource · 2013-10-07T11:18:43Z

@MWallenberg thnx for reporting. Did you manage to illustrate this leak using any of the tools (chrome dev tools or something) in any specific browser? Or is it just based on the code analysis?

MWallenberg · 2013-10-07T11:29:07Z

I used the timeline in Chrome, and noticed the leak there. I have a grid which contained ~500 tooltips, so the memory leak was quite noticable :) Apart from the memory usage, the amount of DOM nodes stored also showed a steady increase. Once I added the above lines in my code, the garbage collection could keep them at a steady level.

One more thing - at the beginning of the show()-method, a null-check is needed:

// Show the tooltip popup element.
function show() {
        if (tooltip == null) { // <-- Right here (and 3 rows down)
          return;
        }

        var position,
            ttWidth,
            ttHeight,
            ttPosition;

This is because I sometimes managed to trigger the show-method right as the grid was re-rendered (it triggered on mouseover) which would cause tooltip to be null once the show()-function kicked off.

pkozlowski-opensource · 2013-10-07T19:44:50Z

@MWallenberg Thnx for the info, this is definitively a bug that should be corrected. Once again, thnx for taking time to report!

Closes #1133

pkozlowski-opensource · 2013-11-17T18:16:45Z

@MWallenberg sorry that it took me so long to come back to you. I was looking into this issue today trying to confirm it before fixing ... and wasn't able to do so! I can see the same memory patterns with a tooltip and without it on a minimal scenario like this:

<!DOCTYPE html>
<html lang="en" ng-app="app">
<head>
    <script src="http://ajax.googleapis.com/ajax/libs/angularjs/1.0.8/angular.js"></script>
    <script src="ui-bootstrap-tpls-0.7.0-SNAPSHOT.js"></script>
    <link href="http://netdna.bootstrapcdn.com/twitter-bootstrap/2.3.1/css/bootstrap.min.css" rel="stylesheet"/>
    <script>
        angular.module('app', []).controller('TTipCtrl', function ($scope) {

            $scope.tips = ['foo', 'bar', 'baz'];

        });
    </script>
</head>
<body ng-controller="TTipCtrl">

<hr/>
<input type="text" ng-model="query"/>
<hr/>
<ul>
    <li ng-repeat="tip in tips | filter:query"><span>{{tip}}</span></li>
</ul>

</body>
</html>

Adding tooltips or not to spans in this example doesn't change anything, neither does code change you are proposing :-( I'm pretty sure that there is something going on here, but I can't put a finger on it... Could you please share a minimal reproduce scenario (both code and steps taken) to observe the leak? Once again, I would like to make sure that we are fixing the right thing... Thnx for your time!

pkozlowski-opensource · 2013-11-17T19:31:04Z

@MWallenberg OK, I think I've tracked down one obscure memory leak, but the one I've found is linked to the way we propagate animation settings. Going to fix the one I've found but would still be interested in your reporoduce scenario so I could confirm if my change fixes your issue as well.

Closes angular-ui#1339 Closes angular-ui#1346 'tooltip' and '$body' have been set to null if scope was destroyed for preventing memory leaks (angular-ui#1133). Deleting both instead will prevent the described errors.

pkozlowski-opensource added a commit that referenced this issue Nov 17, 2013

fix(tooltip): tackle DOM node and event handlers leak

49d520f

Closes #1133

pkozlowski-opensource closed this as completed in 0d810ac Nov 17, 2013

tak-amboss mentioned this issue Dec 4, 2013

fix(tooltip): errors after scope destroyed #1347

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory leak in tooltip on scope.$destroy #1133

Memory leak in tooltip on scope.$destroy #1133

MWallenberg commented Oct 7, 2013

pkozlowski-opensource commented Oct 7, 2013

MWallenberg commented Oct 7, 2013

pkozlowski-opensource commented Oct 7, 2013

pkozlowski-opensource commented Nov 17, 2013

pkozlowski-opensource commented Nov 17, 2013

Memory leak in tooltip on scope.$destroy #1133

Memory leak in tooltip on scope.$destroy #1133

Comments

MWallenberg commented Oct 7, 2013

pkozlowski-opensource commented Oct 7, 2013

MWallenberg commented Oct 7, 2013

pkozlowski-opensource commented Oct 7, 2013

pkozlowski-opensource commented Nov 17, 2013

pkozlowski-opensource commented Nov 17, 2013