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

fix($rootScope): fix potential memory leak when removing scope listeners #16161

Merged
merged 2 commits into from
Oct 28, 2017
Merged

fix($rootScope): fix potential memory leak when removing scope listeners #16161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d87a4e4
test($rootScope): test removal of event listeners during event broadcast
jbedard Aug 3, 2017
2f2fa7a
fix($rootScope): fix potential memory leak when removing scope listeners
jbedard Aug 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1207,7 +1207,10 @@ function $RootScopeProvider() {
return function() {
var indexOfListener = namedListeners.indexOf(listener);
if (indexOfListener !== -1) {
namedListeners[indexOfListener] = null;
// Use delete in the hope of the browser deallocating the memory for the array entry,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...in the hope of the browser deallocating...

Why wouldn't it? 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is implementation dependent, not part of any spec etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be a more descriptive comment here, e.g. "depending on browser implementations, delete should deallocate the memory"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it "should" though. Is there anything that actually says it should? I think browsers (some? all?) do this just to be more efficient. I kind of like the "in the hope of" so I'm having trouble finding a better description...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we ever find a conclusive answer to the actual implementation in specific browsers? Or is this based on benchmarks? Either info could be listed here and then maybe a note that we assume other browsers do it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on benchmarks Chrome and FF seem to both use a more memory efficient array implementation for sparse arrays (which I noted here). But I'm not sure about all browsers, or the details of when Chrome and FF do it, and I'm assuming there is no actual standard for this.

// while not shifting the array indexes of other listeners.
// See issue https://github.com/angular/angular.js/issues/16135
delete namedListeners[indexOfListener];
decrementListenerCount(self, 1, name);
}
};
Expand Down
99 changes: 99 additions & 0 deletions test/ng/rootScopeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1903,6 +1903,105 @@ describe('Scope', function() {
}));


// See issue https://github.com/angular/angular.js/issues/16135
it('should deallocate the listener array entry', inject(function($rootScope) {
var remove1 = $rootScope.$on('abc', noop);
$rootScope.$on('abc', noop);

expect($rootScope.$$listeners['abc'].length).toBe(2);
expect(0 in $rootScope.$$listeners['abc']).toBe(true);

remove1();

expect($rootScope.$$listeners['abc'].length).toBe(2);
expect(0 in $rootScope.$$listeners['abc']).toBe(false);
}));


it('should call next listener after removing the current listener via its own handler', inject(function($rootScope) {
var listener1 = jasmine.createSpy('listener1').and.callFake(function() { remove1(); });
var remove1 = $rootScope.$on('abc', listener1);

var listener2 = jasmine.createSpy('listener2');
var remove2 = $rootScope.$on('abc', listener2);

var listener3 = jasmine.createSpy('listener3');
var remove3 = $rootScope.$on('abc', listener3);

$rootScope.$broadcast('abc');
expect(listener1).toHaveBeenCalled();
expect(listener2).toHaveBeenCalled();
expect(listener3).toHaveBeenCalled();

listener1.calls.reset();
listener2.calls.reset();
listener3.calls.reset();

$rootScope.$broadcast('abc');
expect(listener1).not.toHaveBeenCalled();
expect(listener2).toHaveBeenCalled();
expect(listener3).toHaveBeenCalled();
}));


it('should call all subsequent listeners when a previous listener is removed via a handler', inject(function($rootScope) {
var listener1 = jasmine.createSpy();
var remove1 = $rootScope.$on('abc', listener1);

var listener2 = jasmine.createSpy().and.callFake(remove1);
var remove2 = $rootScope.$on('abc', listener2);

var listener3 = jasmine.createSpy();
var remove3 = $rootScope.$on('abc', listener3);

$rootScope.$broadcast('abc');
expect(listener1).toHaveBeenCalled();
expect(listener2).toHaveBeenCalled();
expect(listener3).toHaveBeenCalled();

listener1.calls.reset();
listener2.calls.reset();
listener3.calls.reset();

$rootScope.$broadcast('abc');
expect(listener1).not.toHaveBeenCalled();
expect(listener2).toHaveBeenCalled();
expect(listener3).toHaveBeenCalled();
}));


it('should not call listener when removed by previous', inject(function($rootScope) {
var listener1 = jasmine.createSpy('listener1');
var remove1 = $rootScope.$on('abc', listener1);

var listener2 = jasmine.createSpy('listener2').and.callFake(function() { remove3(); });
var remove2 = $rootScope.$on('abc', listener2);

var listener3 = jasmine.createSpy('listener3');
var remove3 = $rootScope.$on('abc', listener3);

var listener4 = jasmine.createSpy('listener4');
var remove4 = $rootScope.$on('abc', listener4);

$rootScope.$broadcast('abc');
expect(listener1).toHaveBeenCalled();
expect(listener2).toHaveBeenCalled();
expect(listener3).not.toHaveBeenCalled();
expect(listener4).toHaveBeenCalled();

listener1.calls.reset();
listener2.calls.reset();
listener3.calls.reset();
listener4.calls.reset();

$rootScope.$broadcast('abc');
expect(listener1).toHaveBeenCalled();
expect(listener2).toHaveBeenCalled();
expect(listener3).not.toHaveBeenCalled();
expect(listener4).toHaveBeenCalled();
}));


it('should decrement ancestor $$listenerCount entries', inject(function($rootScope) {
var child1 = $rootScope.$new(),
child2 = child1.$new(),
Expand Down