fix(Scope): more clean up scope on $destroy to minimize leaks

[IgorMinar]

Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier
for GC to collect this scope object.

V8 leaks are due to strong references from optimized code (fixed in M34) and inline
caches (fix in works). Inline caches are caches that the virtual machine builds on the
fly to speed up property access for javascript objects. These caches contain strong
references to objects so under certain conditions this can create a leak.

The reason why these leaks are extra bad for Scope instances is that scopes hold on
to ton of stuff, so when a single scope leaks, it makes a ton of other stuff leak.

This change removes references to objects that might be holding other big
objects. This means that even if the destroyed scope leaks, the child scopes
should not leak because we are not explicitly holding onto them.

Additionally in theory we should also help make the current scope eligible for GC
by changing properties of the current Scope object.

I was able to manually verify that this fixes the problem for the following
example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se

Given the nature of the problem I'm not 100% sure that this will work around
the V8 problem in scenarios common for Angular apps, but I guess it's better
than nothing.

This is a second attempt to enhance the cleanup, the first one failed and was
reverted because it was too aggressive and caused problems for existing apps.
See: #6897

[1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073

Closes #6794
Closes #6856

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#6968)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[IgorMinar]

cc: @btesser, @lgalfaso

[lgalfaso]

@IgorMinar the patch adds a small regression as the following test will fail

it('should not throw when trying to listen and release to an event on a child scope of an already destroyed scope', inject(function($rootScope) {
  var parent = $rootScope.$new(),
   child = parent.$new();

  parent.$destroy();
  expect(function() {
    var fn = child.$on('someEvent', angular.noop);
    fn();
  }).not.toThrow();
}));

[IgorMinar]

@lgalfaso thanks for pointing that out. I added your test (slightly modified) and fix for the test.

[lgalfaso]

LGTM

[schmod]

I'm noticing that this doesn't prevent the leaks from occurring in child scopes.

If the parent is destroyed, the children still keep references to their watchers, which in turn keep track of tons of other stuff.

[btesser]

Plnkr?
On Jun 18, 2014 1:46 PM, "Andrew Schmadel" [email protected] wrote:

I'm noticing that this doesn't prevent the leaks from occurring in child
scopes.

If the parent is destroyed, the children still keep references to their
watchers, which in turn keep track of tons of other stuff.

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

[schmod]

Having a tough time isolating this one into a plunkr.

In my app, I notice that (with a specific set of directives and controllers), I start leaking DOM elements and their scopes.

In the screenshot above, $$childScopeClass @268965's parent has been destroyed. However, because scope.$destroy does not traverse the scope's children and wipe out their watchers and $$childHead/Tail references, V8 seems to be keeping the element references around.

Not really sure where to go from here to debug further...

[btesser]

As long as the parent scope is no longer referencing the child scopes there
should not be a leak. Make sure you force a garbage collection and aren't
console logging any objects within those scopes as that will give the
appearance of a leak

[schmod]

Yeah, I stripped all of my console logs before delving into the leaks, and was able to isolate a handful of leaks that were originating in my application.

However, I've seen this particular pattern ($watchers of a child of a destroyed scope, and getterFnCache in the retainer tree) in a few spots in my app, and cannot seem to find any places where they're actually tied back into "live" code.

Curiously, these leaks seem to be Chrome-specific. IE11 appears to GC the scopes properly.

If I have time later this week, I'll come back to this, and try to create a reproducible scenario.

[btesser]

I'll be happy to take a look if you can give me something to look at haha.
Also try doing a forced garbage collection in chrome (trash can icon in
timeline view)

On Mon, Jun 23, 2014 at 10:05 AM, Andrew Schmadel [email protected]
wrote:

Yeah, I stripped all of my console logs before delving into the leaks, and
was able to isolate a handful of leaks that were originating in my
application.

However, I've seen this particular pattern ($watchers of a child of a
destroyed scope, and getterFnCache in the retainer tree) in a few spots
in my app, and cannot seem to find any places where they're actually tied
back into "live" code.

Curiously, these leaks seem to be Chrome-specific. IE11 appears to GC the
scopes properly.

If I have time later this week, I'll come back to this, and try to create
a reproducible scenario.

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