TRANSCLUDE MEMORY LEAK: Memory leak with transclusion, ng:repeat and jQuery

[benoitgrelard]

Hi there,

I believe I have found a memory leak in a very specific use case it seems as it only happens when transcluding ng:repeat generated elements and ONLY if jQuery is loaded.

Here is a use case: http://jsbin.com/nirubi/11/edit?html,js,output

You can try everything first with only jQlite. Follow the number of active scopes and active watches.
Then uncomment the jQuery script tag and try again, you'll see that the number of active scopes and watches creeps up in the case of ng:repeat and ONLY when jQuery is being loaded.

Let me know if I'm doing something wrong or if it's a genuine bug.

Thanks!

[petebacondarwin]

@mzgol - any thoughts on this?

[benoitgrelard]

Hi, additional info, this doesn't happen only with jquery's latest (2.x). We have this issue currently in our app which uses jQuery 1.10.2

[petebacondarwin]

Just to be clear this is only related to 1.2.x and not 1.3.x?

[benoitgrelard]

Interestingly, I have just tried the same example with the latest 1.3.x of angular and the problem is actually worse, as it happens WITH or WITHOUT jquery being loaded. But still only in the case of ng:repeat being used.

[petebacondarwin]

Hmm that is not good.

[petebacondarwin]

Here is the jsbin with latest master: http://jsbin.com/lisodiwuluzo/1/edit?html,js,output

[petebacondarwin]

So the scopes seem to go up by 7 when the ng-if is true and down by only 1 when it is false.
And the watches go up by 6 and down by 0 each time

[petebacondarwin]

Problem with ng-if - ng-if combo too: http://jsbin.com/zimuvo/1/edit

[petebacondarwin]

I have a feeling that some of the recent performance changes have a role to play here, such as b5f7970#diff-a732922b631efed1b9f33a24082ae0dbL1036
cc/ @IgorMinar

[petebacondarwin]

Changing https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1179

from

if (scopeCreated && !elementTransclusion) {

back to

if (scopeCreated) {

Gets us to only one leaked scope on each toggle (rather than 6)

[petebacondarwin]

Current state of master

missing destroy for all the ngRepeat children and the ngTransclude/ngRepeat scope destroy

create new (isolated) scope: 2 parent:  1  { stats }
create new (isolated) scope: 3 parent:  1  { toggle }
create new  scope: 4 parent:  3            { ngIf }
create new  scope: 5 parent:  1            { ngTransclude }
create new  scope: 6 parent:  5            { ngRepeat child "message 0" }
create new  scope: 7 parent:  5            { ngRepeat child "message 1" }
create new  scope: 8 parent:  5            { ngRepeat child "message 2" }
create new  scope: 9 parent:  5            { ngRepeat child "message 3" }
create new  scope: 10 parent:  5           { ngRepeat child "message 4" }
destroy scope: 4 parent:  3                { ngIf }

With element transclusion perf fix removed

still missing the ngTransclude/ngRepeat scope destroy

create new (isolated) scope: 2 parent:  1  { stats }
create new (isolated) scope: 3 parent:  1  { toggle }
create new  scope: 4 parent:  3            { ngIf }
create new  scope: 5 parent:  1            { ngTransclude }
create new  scope: 6 parent:  5            { ngRepeat child "message 0" }
create new  scope: 7 parent:  5            { ngRepeat child "message 1" }
create new  scope: 8 parent:  5            { ngRepeat child "message 2" }
create new  scope: 9 parent:  5            { ngRepeat child "message 3" }
create new  scope: 10 parent:  5           { ngRepeat child "message 4" }
destroy scope: 4 parent:  3                { ngIf }
destroy scope: 6 parent:  5                { ngRepeat child "message 0" }
destroy scope: 7 parent:  5                { ngRepeat child "message 1" }
destroy scope: 8 parent:  5                { ngRepeat child "message 2" }
destroy scope: 9 parent:  5                { ngRepeat child "message 3" }
destroy scope: 10 parent:  5               { ngRepeat child "message 4" }

[petebacondarwin]

Here is a simple plnkr that generates this info: http://plnkr.co/edit/KDHv95XjI6ymNVetYiJq?p=preview

[petebacondarwin]

So if we get ngTransclude to destroy its scope when its element is destroyed and similarly get ngRepeat to destroy its child scopes when it is destroyed then all is good. See http://plnkr.co/edit/KDHv95XjI6ymNVetYiJq?p=preview

[petebacondarwin]

Here is a failing test:

iit('should not leak memory when a directive removes the element containing the transclude directive', function() {

      module(function() {
        directive('toggle', function() {
          return {
            transclude: true,
            template:
              '<section class="toggle">' +
                '<div ng:if="t">' +
                  '<div ng:transclude></div>' +
                '</div>' +
              '</section>'
          };
        });
      });

      inject(function($compile, $rootScope) {

        function getAllScopes() {
          return [$rootScope].concat(
            getChildScopes($rootScope)
          );

          function getChildScopes(scope) {
            var children = [];
            if (!scope.$$childHead) { return children; }
            var childScope = scope.$$childHead;
            do {
              children.push(childScope);
              children = children.concat(getChildScopes(childScope));
            } while ((childScope = childScope.$$nextSibling));
            return children;
          }
        }

        $rootScope.messages = ['message 0'];

        element = $compile(
          '<div toggle>' +
          '  <div ng:repeat="message in messages">{{ message }}</div>' +
          '</div>'
        )($rootScope);

        $rootScope.$apply('t = true');
        expect(element.text()).toContain('message 0');
        expect(getAllScopes().length).toEqual(4);
        $rootScope.$apply('t = false');
        expect(element.text()).not.toContain('message 0');
        expect(getAllScopes().length).toEqual(1);
        $rootScope.$apply('t = true');
        expect(element.text()).toContain('message 0');
        expect(getAllScopes().length).toEqual(4);
        $rootScope.$apply('t = false');
        expect(element.text()).not.toContain('message 0');
        expect(getAllScopes().length).toEqual(1);
      });
    });
  });

[petebacondarwin]

and possibly this is enough to fix it:

var ngTranscludeDirective = ngDirective({
  restrict: 'EAC',
  link: function($scope, $element, $attrs, controller, $transclude) {
    if (!$transclude) {
      throw minErr('ngTransclude')('orphan',
       'Illegal use of ngTransclude directive in the template! ' +
       'No parent directive that requires a transclusion found. ' +
       'Element: {0}',
       startingTag($element));
    }

    $transclude(function(clone, scope) {
      $element.empty();
      $element.append(clone);
      $element.on('$destroy', function() {
        scope.$destroy();
      });
    });
  }
});

[petebacondarwin]

It looks like we don't need to fix ngRepeat

[IgorMinar]

this leak should have been prevented by this code in compile.js:

if (scopeCreated && !elementTransclusion) {
clone.on('$destroy', function() { transcludedScope.$destroy(); });
}

Which generally works, except when repeaters are being transcluded as the root element, because then the clone element is repeater's comment node (the anchor). jQuery and recently also jqLite doesn't support registering listeners on comment nodes, so the listener registration in that case gets quietly ignored.

Since this jqLite behavior changed in 1.3, this is the reason why in the original report the issue was not present with jqLite in 1.2.x.

The fix proposed by Pete is correct for ngTransclude, and it makes the $compile code I referenced earlier obsolete, except for cases when someone is doing manual non-element transclusion via $transclude. In this case the leak will also occur should the template being transcluded contain a repeater as the root element.

I can't think of a proper fix for the manual transclusion case. Maybe we should just say that if someone is doing $transclude they are responsible for destroying the transcluded scope. Document this requirement in docs and breaking commit message and call it done.

[petebacondarwin]

I think I have a more general fix in #9129. This uses the fact that we do indeed know what the future parent element will be and so we can attach the scope destroy handler to this rather than a potential comment.

@IgorMinar - is this likely to break other things? I don't see why the transclusion scope should not be destroyed when the container is destroyed.

[petebacondarwin]

@tbosch and I paired on this today. We realised that neither of the following fixes are sufficient:

• attaching the $destroy handler to the futureParentElement - if the parent is not destroyed but the transcluded content is destroyed then you still get the leak
• using <script> elements as the placeholders instead of what we currently have (comment nodes), which can have handlers attached - if the transcluded content contains no elements then you still get the leak

We decided that the only real solution is to say that if people use $transclude to clone the transcluded content, then you must be responsible for destroying the transcluded scope also.

In this case the memory leak was actually that ngTransclude was not taking responsibility for destroying its transcluded scope when its own scope was destroyed. Adding this in fixes the issue. See http://plnkr.co/edit/1FIvPdbtF7pR4NcLvesP?p=preview.

In addition this ought to allow to remove the clone.on('$destroy') handler declared in createBoundTranscludeFn but we will see the result of testing that...

[petebacondarwin]

@tobias and @IgorMinar - I just awoke with a new idea:

Our problem is that the transclude scope is detached from the scope tree at the point it is used.

rootscope (1)
  - toggle scope (2 - parent 1 - isolate - inherits none)
    - ngIf scope (3 - parent 2 - child - inherits 2)
      - ngRepeat scope (4 - parent 1 - transclude - inherits 1)

What we really need is for transclude scopes really to be children of their containing element's scope while maintaining their prototypical inheritance from the place where they were taken.

This can easily be done by adding a new parent parameter to scope.$new(isolate, parent). Then we would have

rootscope (1)
  - toggle scope (2 - parent 1 - isolate - inherits none)
    - ngIf scope (3 - parent 2 - child - inherits 2)
      - ngRepeat scope (4 - parent 3 - transclude - inherits 1)

And now when the ngIf scope is destroyed the ngRepeat scope is destroyed automatically with no need for the $destroy listener, in either the createBoundTransclude function nor the ngTransclude directive.

Directives like ngIf and ngRepeat who stamp out copies of their transcluded content are still responsible for removing their cloned DOM elements and destroying their cloned scopes but in cases where you just use ngTransclude you would get no memory leaks when the element containing the ngTransclude directive is removed by some directive outside.

Clearly this would be a breaking change for people who are relying upon $scope.$parent on transcluded elements, but this seems like an uncommon and somewhat dubious scenario. (Use of $parent is generally seen as an anti-pattern).

[petebacondarwin]

Of course this change would break the test at https://github.com/angular/angular.js/blob/master/test/ng/compileSpec.js#L4565

But it is arguable that if we are adding and removing elements in the DOM then we should be creating a scope around it and then destroying that scope.

[tbosch]

Yes, I agree. Make it so :-)

[lgalfaso]

@petebacondarwin are you planning on adding a new parameter to the transclude function for the parent scope?

[petebacondarwin]

Yes. I'll have a pr ready in a couple of hours.
On 25 Sep 2014 19:18, "Lucas Galfasó" [email protected] wrote:

@petebacondarwin https://github.com/petebacondarwin are you planning on
adding a new parameter to the transclude function for the parent scope?

—
Reply to this email directly or view it on GitHub
#9095 (comment).