perf(rootScope): Fix a memory leak

[realityking]

Closes #11169

[Narretz]

Hi, thanks for the PR. Can you explain why that fixes the memory leak?

[realityking]

The issue occurs in Scope.prototype.$new. The code currently looks like this (I've removed some parts to aid readability):

function(isolate, parent) {
    var child;

    if (isolate) {
      child = new Scope();
      child.$root = this.$root;
    } else {
      // Only create a child scope class if somebody asks for one,
      // but cache it to allow the VM to optimize lookups.
      if (!this.$$ChildScope) {
        this.$$ChildScope = function ChildScope() {
          this.$$watchers = this.$$nextSibling =
              this.$$childHead = this.$$childTail = null;
          this.$$listeners = {};
          this.$$listenerCount = {};
          this.$$watchersCount = 0;
          this.$id = nextUid();
          this.$$ChildScope = null;
        };
        this.$$ChildScope.prototype = this;
      }
      child = new this.$$ChildScope();
    }
    child.$parent = parent;
    child.$$prevSibling = parent.$$childTail;
    if (parent.$$childHead) {
      parent.$$childTail.$$nextSibling = child;
      parent.$$childTail = child;
    } else {
      parent.$$childHead = parent.$$childTail = child;
    }

    if (isolate || parent != this) child.$on('$destroy', destroyChild);

    return child;

    function destroyChild() {
      child.$$destroyed = true;
    }
 }

There are two closures used in this function ChildScope and destroyChild. In V8 closures created in the same lexical scope share one context, which contains all variables used in any of the closures.

Now if we replace all child scopes created in an ngRepeat, ChildScope is cached. Since it shares a context with destroyChild so is the variable child, which is the first child scope created for this scope. The memory would be freed when the parent scope is destroyed.

By not using child in destroyChild we keep it out of the context and thus the memory can be freed earlier. I moved the function outside of $new so it's not recreated for every child scope.

[jbedard]

Thanks for the nice explanation @realityking! I didn't know V8 did that. That could lead to some very interesting memory issues...

[Narretz]

LGTM. For good measure, @petebacondarwin do you think we need a test for this?

[petebacondarwin]

Would an alternative be to move the creation of the ChildScope "class" outside of this function. Something like:

function createChildScopeClass(parent) {
  function ChildScope() {
    this.$$watchers = this.$$nextSibling =
        this.$$childHead = this.$$childTail = null;
    this.$$listeners = {};
    this.$$listenerCount = {};
    this.$$watchersCount = 0;
    this.$id = nextUid();
    this.$$ChildScope = null;
  };
  ChildScope.prototype = parent;
  return ChildScope; 
}

function(isolate, parent) {
    var child;

    if (isolate) {
      child = new Scope();
      child.$root = this.$root;
    } else {
      // Only create a child scope class if somebody asks for one,
      // but cache it to allow the VM to optimize lookups.
      if (!this.$$ChildScope) {
        this.$$ChildScope = createChildScopeClass(this);
      }
      child = new this.$$ChildScope();
    }
    child.$parent = parent;
    child.$$prevSibling = parent.$$childTail;
    if (parent.$$childHead) {
      parent.$$childTail.$$nextSibling = child;
      parent.$$childTail = child;
    } else {
      parent.$$childHead = parent.$$childTail = child;
    }

    if (isolate || parent != this) child.$on('$destroy', function destroyChild() { child.$$destroyed = true; });

    return child;
 }

[shahata]

Nice catch! I've mentioned this leak in #10895 (comment) but I didn't know why it leaked :)

[realityking]

@Narretz I don't know how you'd want to test this. GC behavior is not observable from JS code.

@petebacondarwin That would work too - and IMO is easier to read. Mine has the (probably very small) advantage, that it doesn't create a new destroyChild function for every child scope.

Maybe do both?

[petebacondarwin]

@realityking - Both works for me :-) Let's do that. I also agree that we are unlikely to be able to create a unit test for this...

[realityking]

Alright, I'll update the patch tonight.

[realityking]

I've added a commit with @petebacondarwin's changes