fix(jqLite): don't attach event handler to comments (memory leak) #7942

petebacondarwin · 2014-06-23T07:54:13Z

No description provided.

This is what jQuery does by default: https://github.com/jquery/jquery/blob/c18c6229c84cd2f0c9fe9f6fc3749e2c93608cc7/src/data/accepts.js#L16 We don't need to set data on text/comment nodes internally and if we don't allow setting data on these nodes, we don't need to worry about cleaning it up. BREAKING CHANGE: previously it was possible to set jqLite data on Text/Comment nodes, but now that is allowed only on Element and Document nodes just like in jQuery. We don't expect that app code actually depends on this accidental feature.

jbedard · 2014-06-23T16:07:29Z

@petebacondarwin I don't think this is a complete solution. jqLite still leaks, just not with events anymore (see my test snippet in a196c8b comments). But the other thing is that https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1005 is still adding a $destroy event to comment nodes which will now do nothing, isn't that still a problem? When running the tests there's about 15 cases where that line adds a $destroy event only to comment nodes. I think it is always nested-transclusion cases and it always adds the $destory event to 2 comment nodes. Yet the tests still pass and I haven't been able to write a failing one (that's what I was trying to do with the ng-if="1<2" but I clearly did that wrong). Is there a reason the $destroy event isn't needed in those nested-transclusion cases?

petebacondarwin · 2014-06-23T16:59:24Z

@jbedard - I agree that there is still potential for a memory leak (as you show in your snippet) but that that particular memory leak does not occur in this case. With this PR in place the line that you are talking about no longer attaches anything to the comment node - jqLite and jQuery will do nothing if you try to attach a handler to a comment.

petebacondarwin · 2014-06-23T17:01:05Z

Just to be clear - with this fix in place we are no longer adding an event handler (even one that does nothing) the on() method just drops out without adding anything if the subject is a comment or text node.

petebacondarwin · 2014-06-23T17:02:05Z

The reason that you are not able to make a failing test is that we attach a valid handler to the non-comment nodes that constitute the clone, and that when these elements are removed, the scope is correctly destroyed.

jbedard · 2014-06-23T17:18:28Z

Where does the handler get attached to the non-comment nodes in the case where the boundTranscluseFn clone is only comment nodes? Or is it not necessary in that case?

petebacondarwin · 2014-06-23T19:01:28Z

As far as I can see at this point they are never only comment nodes. Can you find a situation where they are?

jbedard · 2014-06-23T19:08:17Z

If you just add a snippet in boundTranscludeFn you can see about ~15 cases when you run the tests where there are only comment nodes.

Something such as... (I haven't run this exact code, but I did something similar this morning, and I'm typing this straight into this comment so no guarantee it runs :)

if (clone.length && [].every.call(clone, function(n) { return n.nodeType !== 1 && n.nodeType !== 9; })) {
  console.log( [].map.call(clone, function(n) { return n.textContent; }).join("\n") );
}

petebacondarwin · 2014-06-23T19:26:17Z

Right. I see. Let's take a look at that...

petebacondarwin · 2014-06-23T20:15:58Z

It appears that the only time when you get this situation of the clone only consisting of comments is when you have more than one "element" transclusion directive on the same element.

So I have these two tests that cover this:

    it('should not leak if two transclusions are on the same element', function() {
      var calcCacheSize = function() {
        var size = 0;
        forEach(jqLite.cache, function(item, key) { size++; });
        return size;
      };

      inject(function($compile, $rootScope) {
        expect(calcCacheSize()).toEqual(0);
        $compile('<div><div ng-repeat="x in xs" ng-if="x==1">{{x}}</div></div>')($rootScope);
        expect(calcCacheSize()).toEqual(1);
        $rootScope.$apply('xs = [0,1]');
        expect(calcCacheSize()).toEqual(2);
        $rootScope.$apply('xs = [0]');
        expect(calcCacheSize()).toEqual(1);
        $rootScope.$apply('xs = []');
        expect(calcCacheSize()).toEqual(1);
      });
    });


    it('should not leak if two transclusions are on the same element', function() {
      var calcCacheSize = function() {
        var size = 0;
        forEach(jqLite.cache, function(item, key) { size++; });
        return size;
      };
      inject(function($compile, $rootScope) {
        expect(calcCacheSize()).toEqual(0);
        element = $compile('<div><div ng-repeat="x in xs" ng-if="val">{{x}}</div></div>')($rootScope);

        $rootScope.$apply('xs = [0,1]');
        expect(calcCacheSize()).toEqual(1);

        $rootScope.$apply('val = true');
        expect(calcCacheSize()).toEqual(3);

        $rootScope.$apply('val = false');
        expect(calcCacheSize()).toEqual(1);
      });
    });

They don't seem to leak onto the cache once we stop attaching event handlers to comments.

jbedard · 2014-06-23T20:25:27Z

I see, so those are all cases with more then one element transclusion on the same element, where I was assuming it was nested transclusions. We know it won't leak listeners onto comment nodes now, but I was wondering if there is an issue now that there are no element $destroy listeners on those transcluded elements. So if someone manually removes the elements the scope won't be destroyed? That's what I was trying to reproduce in the 'should remove transclusion scope, when the DOM is destroyed' test...

petebacondarwin · 2014-06-23T20:58:29Z

As far as I can see, all top level elements in a clone will have a $destroy handler attached. Any top level comments in the clone are now ignored.

But comments don't have scope attached, while elements do. So you can remove comments with no issues and if you remove elements then the scope will be destroyed.

One should be aware though that if there are multiple elements in a clone then removing any one of them will destroy the scope, so they should all be removed at once. I'll put together another test that has something along the lines of:

<div ng-if="val0">
  <div ng-if="val1">...</div>
  <div ng-if="val2">...</div>
<div ng-if-end>

IgorMinar · 2014-06-24T06:59:13Z

src/jqLite.js

@@ -750,6 +750,12 @@ forEach({
  on: function onFn(element, type, fn, unsupported){
    if (isDefined(unsupported)) throw jqLiteMinErr('onargs', 'jqLite#on() does not support the `selector` or `eventData` parameters');

+
+    // Do not add event handlers to non-elements because they will not be cleaned up.
+    if ( element.nodeType && element.nodeType !== 1 && element.nodeType !== 9 ) {


we now have this check in at least two places. I think we should have a helper test fn instead.

Here's the jQuery helper test fn: https://github.com/jquery/jquery/blob/master/src/data/accepts.js

I don't think we allow adding data to arbitrary objects so we probably don't need the jquery version. I'll refactor into a simple helper

Actually I take that back, the window object can handle events but has no nodeType property.

If an element contains two "element" transcludes then the initial clone consists of only comment nodes. The concern was that this meant that the transclude scopes would not be cleaned up. But it turns out that in the case that there are only comments then the scope is never attached to anything so we don't need to worry about cleaning it up. Later if a concrete element is created as part of the transclude then these elements will have destroy handlers.

We were attaching handlers to comment nodes when setting up bound transclusion functions. But we don't clean up comments and text nodes when deallocating so there was a memory leak. Closes angular#7913

…ctives

IgorMinar · 2014-06-24T15:25:29Z

test/ng/compileSpec.js

+          expect(calcCacheSize()).toEqual(1);
+
+          $rootScope.$apply('xs = [0,1]');
+          expect(calcCacheSize()).toEqual(2);


btw wouldn't the global after each that checks the state of the cache catch the leaks?

These tests are much more readable, so keep things as they are. I'm just curious.

IgorMinar · 2014-06-24T15:35:38Z

otherwise this looks good. thanks for all the tests

We were attaching handlers to comment nodes when setting up bound transclusion functions. But we don't clean up comments and text nodes when deallocating so there was a memory leak. Closes angular#7913 Closes angular#7942

petebacondarwin added this to the 1.3.0-beta.14 milestone Jun 23, 2014

petebacondarwin assigned petebacondarwin and IgorMinar and unassigned petebacondarwin Jun 23, 2014

petebacondarwin added gh: PR and removed frequency: high labels Jun 23, 2014

IgorMinar reviewed Jun 24, 2014
View reviewed changes

petebacondarwin added 3 commits June 24, 2014 12:07

test($compile): check for memory leaks in nested transclusion

7c06c44

fix(jqLite): don't attach event handlers to comments or text nodes

a3d0964

We were attaching handlers to comment nodes when setting up bound transclusion functions. But we don't clean up comments and text nodes when deallocating so there was a memory leak. Closes angular#7913

petebacondarwin mentioned this pull request Jun 24, 2014

Jqlite data leak #7966

Closed

test(compile): check transclusion/scopes work with multi-element dire…

eca016d

…ctives

IgorMinar reviewed Jun 24, 2014
View reviewed changes

petebacondarwin closed this in 462dbb2 Jun 25, 2014

petebacondarwin deleted the pr7913 branch December 17, 2015 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(jqLite): don't attach event handler to comments (memory leak) #7942

fix(jqLite): don't attach event handler to comments (memory leak) #7942

petebacondarwin commented Jun 23, 2014

jbedard commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

jbedard commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

jbedard commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

jbedard commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

IgorMinar Jun 24, 2014

jbedard Jun 24, 2014

petebacondarwin Jun 24, 2014

petebacondarwin Jun 24, 2014

IgorMinar Jun 24, 2014

IgorMinar commented Jun 24, 2014

fix(jqLite): don't attach event handler to comments (memory leak) #7942

fix(jqLite): don't attach event handler to comments (memory leak) #7942

Conversation

petebacondarwin commented Jun 23, 2014

jbedard commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

jbedard commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

jbedard commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

jbedard commented Jun 23, 2014

petebacondarwin commented Jun 23, 2014

IgorMinar Jun 24, 2014

Choose a reason for hiding this comment

jbedard Jun 24, 2014

Choose a reason for hiding this comment

petebacondarwin Jun 24, 2014

Choose a reason for hiding this comment

petebacondarwin Jun 24, 2014

Choose a reason for hiding this comment

IgorMinar Jun 24, 2014

Choose a reason for hiding this comment

IgorMinar commented Jun 24, 2014