Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Commit

Permalink
fix(Scope): more scope clean up on $destroy to minimize leaks
Browse files Browse the repository at this point in the history
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
Closes #6968
  • Loading branch information
IgorMinar committed Apr 3, 2014
1 parent ba48797 commit d64d41e
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 5 deletions.
31 changes: 28 additions & 3 deletions src/ng/rootScope.js
Original file line number Diff line number Diff line change
Expand Up @@ -731,15 +731,40 @@ function $RootScopeProvider(){

forEach(this.$$listenerCount, bind(null, decrementListenerCount, this));

// 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.$$childHead == this) parent.$$childHead = this.$$nextSibling;
if (parent.$$childTail == this) parent.$$childTail = this.$$prevSibling;
if (this.$$prevSibling) this.$$prevSibling.$$nextSibling = this.$$nextSibling;
if (this.$$nextSibling) this.$$nextSibling.$$prevSibling = this.$$prevSibling;

// This is bogus code that works around Chrome's GC leak
// see: https://github.com/angular/angular.js/issues/1313#issuecomment-10378451

// All of the code below is bogus code that works around V8's memory leak via optimized code
// and inline caches.
//
// see:
// - https://code.google.com/p/v8/issues/detail?id=2073#c26
// - https://github.com/angular/angular.js/issues/6794#issuecomment-38648909
// - https://github.com/angular/angular.js/issues/1313#issuecomment-10378451

this.$parent = this.$$nextSibling = this.$$prevSibling = this.$$childHead =
this.$$childTail = null;
this.$$childTail = this.$root = null;

// don't reset these to null in case some async task tries to register a listener/watch/task
this.$$listeners = {};
this.$$watchers = this.$$asyncQueue = this.$$postDigestQueue = [];

// prevent NPEs since these methods have references to properties we nulled out
this.$destroy = this.$digest = this.$apply = noop;
this.$on = this.$watch = function() { return noop; };


/* jshint -W103 */
// not all browsers have __proto__ so check first
if (this.__proto__) {
this.__proto__ = null;
}
/* jshint +W103 */
},

/**
Expand Down
28 changes: 26 additions & 2 deletions test/ng/rootScopeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -844,13 +844,15 @@ describe('Scope', function() {
expect(log).toBe('123');

first.$destroy();

// once a scope is destroyed apply should not do anything any more
first.$apply();
expect(log).toBe('12323');
expect(log).toBe('123');

first.$destroy();
first.$destroy();
first.$apply();
expect(log).toBe('1232323');
expect(log).toBe('123');
}));


Expand All @@ -874,6 +876,28 @@ describe('Scope', function() {
$rootScope.$broadcast(EVENT);
expect(spy.callCount).toBe(1);
}));


it("should do nothing when a child event listener is registered after parent's destruction",
inject(function($rootScope) {
var parent = $rootScope.$new(),
child = parent.$new();

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


it("should do nothing when a child watch is registered after parent's destruction",
inject(function($rootScope) {
var parent = $rootScope.$new(),
child = parent.$new();

parent.$destroy();
var fn = child.$watch('somePath', function() {});
expect(fn).toBe(noop);
}));
});


Expand Down

0 comments on commit d64d41e

Please sign in to comment.