Angular.js: Nested ng-if's leads to a scope leak

[jhasenfuss]

I'm submitting a ...

• [x] bug report
• [ ] feature request
• [ ] other (Please do not submit support requests here (see above))

Current behavior:

Every ng-if creates a new isolated scope. If a ng-if is set to false, the corresponding scope will be destroyed. But if there are two nested ng-ifs and both created an isolated scope (both == true) and the outer ng-if is set to false, the inner scope won't be destroyed. This results in a massive scope leak.

Expected / new behavior:

I expected, that if the scope of the outer ng-if gets destroyed, the scope of the inner ng-if gets also destroyed.

Minimal reproduction of the problem with instructions:

I've created a simple example:
http://plnkr.co/edit/Ul06ito10wazclZ4htWQ?p=preview
You can see the created and destroyed scopes as well as the total scopes in the js-console.

AngularJS version: every?

Browser: all

Anything else:

I fixed that problem with a modification of the $destroy-implementation:

$destroy: function() {
  // We can't destroy a scope that has been already destroyed.
  if (this.$$destroyed) return;
  var parent = this.$parent;

  this.$broadcast('$destroy');
  this.$$destroyed = true;

  if (this === $rootScope) {
    //Remove handlers attached to window when $rootScope is removed
    $browser.$$applicationDestroyed();
  }

  incrementWatchersCount(this, -this.$$watchersCount);
  for (var eventName in this.$$listenerCount) {
    decrementListenerCount(this, this.$$listenerCount[eventName], eventName);
  }

  /* * START NEW CODE * */
  var destroyCleanup = function (child) {
    if (!child) return;
    if (child.$$nextSibling) {
      destroyCleanup(child.$$nextSibling);
    }
    if (child.$$childHead) {
      destroyCleanup(child.$$childHead);
    }
    child.$destroy();
  };
  destroyCleanup(this.$$childHead);
  /* * END NEW CODE * */

  // sever all the references to parent scopes (after this cleanup, the current scope should
  // not be retained by any of our references and should be eligible for garbage collection)
  if (parent && parent.$$childHead === this) parent.$$childHead = this.$$nextSibling;
  if (parent && parent.$$childTail === this) parent.$$childTail = this.$$prevSibling;
  if (this.$$prevSibling) this.$$prevSibling.$$nextSibling = this.$$nextSibling;
  if (this.$$nextSibling) this.$$nextSibling.$$prevSibling = this.$$prevSibling;

  // Disable listeners, watchers and apply/digest methods
  this.$destroy = this.$digest = this.$apply = this.$evalAsync = this.$applyAsync = noop;
  this.$on = this.$watch = this.$watchGroup = function() { return noop; };
  this.$$listeners = {};

  // Disconnect the next sibling to prevent `cleanUpScope` destroying those too
  this.$$nextSibling = null;
  cleanUpScope(this);
},

I can't create a plunker, cause the modified angular.js-file is too large.

Because i never created a pull request, i created this issue.
Is this a real bug of AngularJS or incorrect usage?
And is this a clever modification or can this cause other problems?

[gkalpak]

Thx for the write-up 👍
I looked into it and this isn't a real bug (afaict). We don't call $destroy() on every child scope when a scope i sdestroyed, but we also shouldn't need to.

Here is what happens:

1. The destroyed scope broadcasts a $destroy event, which will propagate to all child scopes and trigger their $destroy handlers (thus taking care of any clean-up work that needs to be done):

https://github.com/angular/angular.js/blob/bf758d0bef449e07ce57d9c53dcb2a9191c1dc3d/src/ng/rootScope.js#L935

1. The internal $$destroyed flag is set on isolate or transcluded child scopes.

https://github.com/angular/angular.js/blob/bf758d0bef449e07ce57d9c53dcb2a9191c1dc3d/src/ng/rootScope.js#L272

https://github.com/angular/angular.js/blob/bf758d0bef449e07ce57d9c53dcb2a9191c1dc3d/src/ng/rootScope.js#L103

1. The scope is removed from the scope tree (along with its references to other scopes).

https://github.com/angular/angular.js/blob/bf758d0bef449e07ce57d9c53dcb2a9191c1dc3d/src/ng/rootScope.js#L948-L953

https://github.com/angular/angular.js/blob/bf758d0bef449e07ce57d9c53dcb2a9191c1dc3d/src/ng/rootScope.js#L960-L962

https://github.com/angular/angular.js/blob/bf758d0bef449e07ce57d9c53dcb2a9191c1dc3d/src/ng/rootScope.js#L130-L131

These steps ensure that we don't keep references to unused scopes and the whole destroyed scope substree is eligible for garbage collection.

#

In your plnkr, you are (incorrectly) assuming that counting $destroy() calls is an accurate way to meassure destroyed scopes.

What you should be really looking at is memory consumption steadily increasing (after garbage collection) while you toggle the top-level ngIf. This shouldn't happen, but if you find a case where it does, please open a new issue with a minimal reproduction (e.g. a demo in plnkr).

Thx again for taking the time to investigate this and write up your findings!

[jhasenfuss]

Okay, thanks for your reply!

I've tested a bigger example and checked the JS-Heap in chrome dev tools.
It seems like, that the heap gets cleaned out.
http://plnkr.co/edit/Xe3ubOTMUUxxxrnLm1Pn?p=preview
The watcher are also tidied up.

So if i understand it correct, you cut only the subtree and let the GC does his job.
And with my modification, i "cut" every sub-scope and that is too much. Right?

[gkalpak]

you cut only the subtree and let the GC does his job

Exactly. (More or less :smiley:)

I "cut" every sub-scope and that is too much. Right?

In fact, your modifications doesn't do anything as it is, because essentially it calls $destroy() on each scope of the subtree, but that will bail out immediately, since $$destroyed will already be true:

https://github.com/angular/angular.js/blob/bf758d0bef449e07ce57d9c53dcb2a9191c1dc3d/src/ng/rootScope.js#L932

If this weren't the case, then your change would:
a. Clear all methods/properties of each scope in the subtree (e.g. $watch, $digest, $$listeners etc) and remove all references between these scopes (e.g. $$childHead, $parent, etc).
b. Broadcast $destroy N times on each scope (where N is the depth of the scope in the sub-tree) :smiley:

As you correctly mentioned, this is not necessary, because even if there are cross-references between the scopes inside the sub-tree, the whole sub-tree is GC'd anyway, so all memory is claimed back.

[jhasenfuss]

Okay, great. Now it makes Sense 😄
Many thanks! 😃