fix(tooltip): make sure tooltip scope is evicted from cache.

[jgrund]

This fix makes sure the tooltip.$scope is cleared from angular.element.cache when $destroy is called, preventing a memory leak.

See: #484

[pkozlowski-opensource]

@jgrund this looks good (after a quick look) but it doesn't merge cleanly I'm afraid. Could you rebase it on the current master?

[joshdmiller]

This is a good catch. We should probably also destroy the inner directive's scope somewhere in the hide(), right?

[jgrund]

@pkozlowski-opensource Rebased on latest master.

[jgrund]

@joshdmiller Correct me if I'm wrong, but I was under the impression that between scope.$destroy and tooltip.remove() all references to the tooltip scope should be taken care of.

[pkozlowski-opensource]

Tests are passing on CI: http://ci.angularjs.org/job/angularui-bootstrap/366/console

@joshdmiller Does it look good to you? It is pre-merged in the pr486 branch.

[joshdmiller]

@pkozlowski-opensource I have a question/some confusion.

hide() removes the tooltip element, so to save compute cycles the existing code calls remove() when the scope is destroyed only when the tooltip is open. This PR makes it so that we call remove() when the tooltip isn't open and therefore doesn't exist. Why do we need to call .remove() on an element previously removed?

So anyway I'm honestly not sure how this solves the memory leak described in the bug, but I'm probably just missing something.

I don't have time to check right now, but @jgrund can you confirm that this test fails without the PR? And if so, can you enlighten me? What am I missing?

[pkozlowski-opensource]

@joshdmiller I didn't look into the details honestly speaking :-) So if you are confused I'm confused as well :-)

[jgrund]

@joshdmiller This test does fail without the PR, as the tooltip scope still exists in the cache.

So .remove is the method responsible for evicting the tooltip data from angular.element.cache.

Simply put, if .remove is not called that data is not evicted, which leads to the leak.

There are code paths where .remove may not have been called. The PR addresses this issue by making sure .remove is called when the scope is destroyed, thus preventing the leak.

hide() removes the tooltip element, so to save compute cycles the existing code calls remove() when the scope is destroyed only when the tooltip is open. This PR makes it so that we call remove() when the tooltip isn't open and therefore doesn't exist. Why do we need to call .remove() on an element previously removed?

The tooltip does exist it's just detached from the DOM. As noted above, .remove may not have been called if the tooltip never had its hide event fired.

[ddoyle]

👍

Same issue here.

[joshdmiller]

@jgrund - After looking further, I can see what you're talking about. Thanks for being patient. The case where this bug exists only affects tooltips that were never shown.

In that case, it is improper to "hide" it. The bug lays in that there is an initial node creation that, in the case that the tooltip was never shown, we want to delete when the scope gets destroyed. The fix should be:

if ( scope.tt_isOpen ) {
  hide();
} else {
  tooltip.remove();
}

The difference is that the solution in this PR uses the hide() function which would cancel timeouts that don't exist as well as potentially delay the destruction of the tooltip node if a delay was specified; in the case above, only the code needed to remove the node is called - and it is guaranteed to be called immediately.

What do you think?

[pkozlowski-opensource]

@jgrund @joshdmiller I had another look at this one as well and while I understand the initial scenario (we've got an element with a tooltip, tooltip is never shown and then the scope associated with the initial element gets destroyed) I'm still a bit confused as of why calling remove() fixes the problem here...

Bear with my guys, there is simply something I don't understand here. What do we leak here, exactly? A tooltip element with its associated scope? I guess both and this happens as we keep a reference to a tooltip element (and an associated scope) in the tooltips linking function.

BUT, looking at the attached test the scenario is that we destroy a scope (in the test it will be $rootScope, right?) without removing associated DOM elements (!). I wonder if / how this could happen in practice.... What I'm trying to say that normally both a DOM element and its scope would be destroyed and not only a scope....

Sorry if I'm talking rubbish, there is an issue here but I can't get to the bottom of it...

[joshdmiller]

@pkozlowski-opensource You're right in that the test is a little contrived; that's a good point. Perhaps a better test would be removing the tooltip element rather than its scope directly, which is what in practice will happen.

@jgrund What are your thoughts?

[jgrund]

@joshdmiller Canceling a $timeout that doesn't exist simply returns false, though I agree checking if the tooltip is open first allows for more efficient execution and could avoid an unnecessary $timeout.

@pkozlowski-opensource The leak this fix addresses is data retained in the angular.element.cache that remains after the parent element is removed. Calling .remove clears the data stored in the angular.element.cache for the element it is called on. Therefore, calling tooltip.remove() removes the data for the tooltip element in the angular.element.cache in the case that tooltip.remove() was not called before.

In regards to calling $destroy instead of .remove in the test, I don't see that as contrived. My understanding is calling .remove on an element will not destroy it's associated scope. I think it should be the responsibility of a parent scope to call $destroy when necessary, which will cascade to the children.

FYI, I have updated the pull request with @joshdmiller recommended change.

[pkozlowski-opensource]

Landed as 9246905. Let's release it as-is since it is a good change. Thnx @jgrund .

Still I need to come back to this one after 0.4.0 release to understand better what it going on here.

[joshdmiller]

Agreed. Thanks for the fix, @jgrund!

[jgrund]

Awesome, thanks for the great project!