ngAnimate: memory leak in disabledElementsLookup Map

[vertazzar]

All begins with this line of the code:

angular.js/src/ngAnimate/animateQueue.js

Line 306 in 494277e

disabledElementsLookup.set(node, !bool);

Reproducable: always
Browsers: Any
Operating system: Any
Versions: All that use the hash map

Steps to reproduce:

Available in codepen: https://codepen.io/anon/pen/yqVwwE

click on "leaker" n times to get n detached nodes as seen in the screenshot

Solution: listen for element scope destroy event, or element.on('remove') and delete the node from disabledElementsLookup Map ?

Note: If this is "working as intended" at least provide public api to disabledElementsLookup so we can cleanup after directive exits

[gkalpak]

If we only had WeakMaps 😃

I am not sure, but I guess the reason for choosing a map over storing the value on the DOM element itself was speed. Since we only set a value when calling $animate.enabled(element, true/false) - which is not very common I presume - I think it would be fine storing that info on the node (instead of a Map).

(Note to self: This should be independent of .data(), so that it is not affected by .removeData() and also so that it can be independently optimized for speed.)

Listening for the element's $destroy event might also work. (Not the scope's $destroy, because not all elements have a scope and not the element's remove because it does not cover all cases (e.g. replaceWith()).)

[vertazzar]

You are right about trying to tap into weird environments ($destroy, .data() etc). Then it would be something that must be explicitly called, eg:

$animate.unregisterEnabledState(element)

$animate.enabled(element, null)

[gkalpak]

For the record, I said listening for the element's $destroy event might work 😁
Exposing a public API to "unregister" the element would be very straight forward to implement, but feels weird. Ideally, we shouldn't be introducing memory leaks in the first place 😁

Let's see what others think. (Cc @Narretz who has more experience with ngAnimate.)

[vertazzar]

aha, I misunderstood what you meant (exhaustion :D)

[Narretz]

I would probably go with the event listener. I assume that setting on the node directly is pretty slow on older IEs.

[gkalpak]

Note that adding an event listener also assigns stuff on the node under the hood 😁
But given that $animate.enabled(element, true/false) is rare, I think this is a good trade-off.

@vertazzar, would you be interested in taking a stub at fixing this?

[vertazzar]

@gkalpak

Easiest solution was just to write something like:

            if (!element.data('hasAnimateDestroyListener')) {
                element.data('hasAnimateDestroyListener', true);
                var scope = element.scope();
                scope.$on('$destroy', function () {
                    disabledElementsLookup.delete(node);
                });
            }

the .data() is to avoid constant listener registration when running the $animate.enabled.

For the element lifecycle I'm not sure if the value of element.scope(); can change for the element at any point in runtime before the $destroy event. If that's the case, you might want to add element.data('hasAnimateDestroyListener' + scope.$id) to add support for multiple scopes grabbing the element.

[gkalpak]

As mentioned in #16637 (comment), the scope's $destroy event is not a good option (because not all elements have their own scope).

I created a PR that bind to the element's $destroy event: #16649