nested transclusion memory leak

[jbedard]

When the transclude function creates the scope it destroys that scope on the '$destroy' jQuery event. If that element is a comment (because it was also transcluded, for example) then:

• jqLite adds and leaks data on the comment node, but $destroy seems to get called (the added test shows this)
• jQuery does not add/leak data on the comment node, so $destroy does not get called (the modified test shows this)

Here's another example: http://plnkr.co/edit/89BK3LXL6USYoATQhg2c?p=preview

[petebacondarwin]

Ouch! Will take a look. Any thoughts on a fix?

[jbedard]

Can easily make jqLite the same as jQuery (a196c8b should have done that), I think that's a good start to at least fix the big leak and make jqLite/jQuery the same. Not sure about listening to the element $destroy when the element is currently a transclusion comment node...

[petebacondarwin]

The plunk is "not found"

[jbedard]

Oops, I probably forgot to hit save, here's a new one: http://plnkr.co/edit/89BK3LXL6USYoATQhg2c?p=preview

[petebacondarwin]

Does this section of code seem relevant:

https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1561-L1566

            controllerInstance = $controller(controller, locals);
            // For directives with element transclusion the element is a comment,
            // but jQuery .data doesn't support attaching data to comment nodes as it's hard to
            // clean up (http://bugs.jquery.com/ticket/8335).
            // Instead, we save the controllers for the element in a local hash and attach to .data
            // later, once we have the actual element.
            elementControllers[directive.name] = controllerInstance;

Perhaps we need something a bit more generic for element transclusion directives - i.e. rather than specifically fixing it only for controllers, we should fix it for anything that might want to add data to the element when it is a comment?

[petebacondarwin]

It seems that the problem is not actually to do with the directives being nested but instead that ng-if is both using "element" transclusion and is calling the translusion function asynchronously, after compilation has completed. Here is a failing test:

      describe('async "element" transcludes', function() {
        iit('should not leak memory', function() {
          module(function() {
            directive('element', valueFn({
              transclude:'element',
              link: function($scope, $element, $attrs, controller, $transclude) {
                $scope.$evalAsync(function() {
                  $transclude(function(clone) { $element.append(clone); });
                });
              }
            }));
          });

          inject(function($compile, $rootScope) {
            element = jqLite('<div element></div>');
            $compile(element)($rootScope);
            $rootScope.$digest();
          });
        });
      });

[petebacondarwin]

Actually my test there is just wrong ignore it. It is failing for other reasons ...

[caitp]

looking into this...

[caitp]

was this determined to be a regression? I'm not sure this ever worked correctly

[petebacondarwin]

I am pretty sure it is a regression because of e35abc9

[petebacondarwin]

Or at least part of it is a regression

[petebacondarwin]

@jbedard the second "modified" test was not actually valid. It was failing for a different reason than you suggested and so doesn't help us identify a leak when using jQuery. Here is the test, still using ngIf but fixed so it is no longer a false-negative.

      it('should remove transclusion scope, when the DOM is destroyed', function() {
        module(function() {
          directive('box', valueFn({
            transclude: true,
            scope: { name: '=', show: '=' },
            template: '<div><h1>Hello: {{name}}!</h1><div ng-transclude></div></div>',
            link: function(scope, element) {
              scope.$watch(
                  'show',
                  function(show) {
                    if (!show) {
                      element.find('div').find('div').remove();
                    }
                  }
              );
            }
          }));
        });
        inject(function($compile, $rootScope) {
          $rootScope.username = 'Misko';
          $rootScope.select = true;
          element = $compile(
              '<div><div ng-if="true"><div box name="username" show="select">user: {{username}}</div></div></div>')
              ($rootScope);
          $rootScope.$apply();
          expect(element.text()).toEqual('Hello: Misko!user: Misko');

          var ngIfElement = element.find('div').eq(0);
          var ngIfScope = ngIfElement.scope();
          var widgetElement = ngIfElement.find('div').eq(0);
          var widgetScope = widgetElement.isolateScope();
          var transcludeScope = widgetScope.$$nextSibling;

          expect(widgetScope.name).toEqual('Misko');
          expect(widgetScope.$parent).toBe(ngIfScope);
          expect(transcludeScope.$parent).toBe(ngIfScope);

          $rootScope.select = false;
          $rootScope.$apply();
          expect(element.text()).toEqual('Hello: Misko!');
          expect(widgetScope.$$nextSibling).toEqual(null);
        });
      });

[petebacondarwin]

In fact I don't see the memory leak happening when jQuery is loaded. See

http://plnkr.co/edit/JSAQNEcuVpjE4QIeInBV?p=preview

[petebacondarwin]

Reverting e35abc9 removes the memory leak from the jqLite version:

http://plnkr.co/edit/m639uGQU7nNw3rtEYcc7?p=preview

[petebacondarwin]

OK, so the problem with jqLite is that if we have a directive that can span multiple elements, like ngIf and ngRepeat then you get an array back from the translude as the clone here:

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

        var clone = transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn);
        if (scopeCreated) {
          clone.on('$destroy', function() { transcludedScope.$destroy(); });
        }
        return clone;

We then try to attach the $destroy event handler, correctly to the first element, the ngIf, but also incorrectly to the ngIfEnd comment node. We should just change on() not to try to attach handlers to comments.

[jbedard]

I think for ngIf and ngRepeat there is only a comment node, not an array of nodes, so clone.on tries to attach to the comment node. In jqLite that causes a memory leak (because a196c8b is buggy), in jQuery it gets ignored causing removal of the transcluded element to fail. I think a196c8b needs to be fixed so that non-element/document nodes get ignored like jQuery, then the transclusion logic needs to avoid using .on and do something more like controller instance snippet you showed above.

I think the use of .on causing jQuery element removal to not work is an old issue, the jqLite memory leak is new now that jqLite is is trying to be more like jQuery and not supporting data/events on non-element/document nodes.

[jbedard]

The ngIfEnd comment node is another case that will reproduce the jqLite data leak.