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

revert + fix($rootScope): fix potential memory leak when removing scope listeners #16358

Merged
merged 3 commits into from
Dec 13, 2017
Merged

revert + fix($rootScope): fix potential memory leak when removing scope listeners #16358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b5b8d6
revert: fix($rootScope): fix potential memory leak when removing scop…
jbedard Dec 5, 2017
ffb075f
fix($rootScope): fix potential memory leak when removing scope listeners
jbedard Aug 9, 2017
916f3cb
test($rootScope): test recursive event broadcast and emit
jbedard Dec 5, 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
22 changes: 0 additions & 22 deletions docs/content/error/$rootScope/inevt.ngdoc
View file
Open in desktop

This file was deleted.

82 changes: 48 additions & 34 deletions src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1271,14 +1271,13 @@ function $RootScopeProvider() {

var self = this;
return function() {
var index = arrayRemove(namedListeners, listener);
if (index >= 0) {
var indexOfListener = namedListeners.indexOf(listener);
if (indexOfListener !== -1) {
// Use delete in the hope of the browser deallocating the memory for the array entry,
// 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);
// We are removing a listener while iterating over the list of listeners.
// Update the current $$index if necessary to ensure no listener is skipped.
if (index <= namedListeners.$$index) {
namedListeners.$$index--;
}
}
};
},
Expand Down Expand Up @@ -1307,7 +1306,9 @@ function $RootScopeProvider() {
* @return {Object} Event object (see {@link ng.$rootScope.Scope#$on}).
*/
$emit: function(name, args) {
var scope = this,
var empty = [],
namedListeners,
scope = this,
stopPropagation = false,
event = {
name: name,
Expand All @@ -1318,11 +1319,28 @@ function $RootScopeProvider() {
},
defaultPrevented: false
},
listenerArgs = concat([event], arguments, 1);
listenerArgs = concat([event], arguments, 1),
i, length;

do {
invokeListeners(scope, event, listenerArgs, name);

namedListeners = scope.$$listeners[name] || empty;
event.currentScope = scope;
for (i = 0, length = namedListeners.length; i < length; i++) {

// if listeners were deregistered, defragment the array
if (!namedListeners[i]) {
namedListeners.splice(i, 1);
i--;
length--;
continue;
}
try {
//allow all listeners attached to the current scope to run
namedListeners[i].apply(null, listenerArgs);
} catch (e) {
$exceptionHandler(e);
}
}
//if any listener on the current scope stops propagation, prevent bubbling
if (stopPropagation) {
break;
Expand Down Expand Up @@ -1373,11 +1391,28 @@ function $RootScopeProvider() {

if (!target.$$listenerCount[name]) return event;

var listenerArgs = concat([event], arguments, 1);
var listenerArgs = concat([event], arguments, 1),
listeners, i, length;

//down while you can, then up and next sibling or up and next sibling until back at root
while ((current = next)) {
invokeListeners(current, event, listenerArgs, name);
event.currentScope = current;
listeners = current.$$listeners[name] || [];
for (i = 0, length = listeners.length; i < length; i++) {
// if listeners were deregistered, defragment the array
if (!listeners[i]) {
listeners.splice(i, 1);
i--;
length--;
continue;
}

try {
listeners[i].apply(null, listenerArgs);
} catch (e) {
$exceptionHandler(e);
}
}

// Insanity Warning: scope depth-first traversal
// yes, this code is a bit crazy, but it works and we have tests to prove it!
Expand Down Expand Up @@ -1408,27 +1443,6 @@ function $RootScopeProvider() {

return $rootScope;

function invokeListeners(scope, event, listenerArgs, name) {
var listeners = scope.$$listeners[name];
if (listeners) {
if (listeners.$$index !== undefined) {
throw $rootScopeMinErr('inevt', '{0} already $emit/$broadcast-ing on scope ({1})', name, scope.$id);
}
event.currentScope = scope;
try {
for (listeners.$$index = 0; listeners.$$index < listeners.length; listeners.$$index++) {
try {
//allow all listeners attached to the current scope to run
listeners[listeners.$$index].apply(null, listenerArgs);
} catch (e) {
$exceptionHandler(e);
}
}
} finally {
listeners.$$index = undefined;
}
}
}

function beginPhase(phase) {
if ($rootScope.$$phase) {
Expand Down
148 changes: 44 additions & 104 deletions test/ng/rootScopeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2444,10 +2444,12 @@ describe('Scope', function() {
$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(1);
expect($rootScope.$$listeners['abc'].length).toBe(2);
expect(0 in $rootScope.$$listeners['abc']).toBe(false);
}));


Expand Down Expand Up @@ -2583,107 +2585,6 @@ describe('Scope', function() {
expect($rootScope.$$listenerCount).toEqual({abc: 1});
expect(child.$$listenerCount).toEqual({abc: 1});
}));


it('should throw on recursive $broadcast', inject(function($rootScope) {
$rootScope.$on('e', function() { $rootScope.$broadcast('e'); });

expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
}));


it('should throw on nested recursive $broadcast', inject(function($rootScope) {
$rootScope.$on('e2', function() { $rootScope.$broadcast('e'); });
$rootScope.$on('e', function() { $rootScope.$broadcast('e2'); });

expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
}));


it('should throw on recursive $emit', inject(function($rootScope) {
$rootScope.$on('e', function() { $rootScope.$emit('e'); });

expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
}));


it('should throw on nested recursive $emit', inject(function($rootScope) {
$rootScope.$on('e2', function() { $rootScope.$emit('e'); });
$rootScope.$on('e', function() { $rootScope.$emit('e2'); });

expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
}));


it('should throw on recursive $broadcast on child listener', inject(function($rootScope) {
var child = $rootScope.$new();
child.$on('e', function() { $rootScope.$broadcast('e'); });

expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
expect(function() { child.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
}));


it('should throw on nested recursive $broadcast on child listener', inject(function($rootScope) {
var child = $rootScope.$new();
child.$on('e2', function() { $rootScope.$broadcast('e'); });
child.$on('e', function() { $rootScope.$broadcast('e2'); });

expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
expect(function() { child.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
}));


it('should throw on recursive $emit parent listener', inject(function($rootScope) {
var child = $rootScope.$new();
$rootScope.$on('e', function() { child.$emit('e'); });

expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
}));


it('should throw on nested recursive $emit parent listener', inject(function($rootScope) {
var child = $rootScope.$new();
$rootScope.$on('e2', function() { child.$emit('e'); });
$rootScope.$on('e', function() { child.$emit('e2'); });

expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
}));


it('should clear recursive state of $broadcast if $exceptionHandler rethrows', function() {
module(function($exceptionHandlerProvider) {
$exceptionHandlerProvider.mode('rethrow');
});
inject(function($rootScope) {
var throwingListener = jasmine.createSpy('thrower').and.callFake(function() {
throw new Error('Listener Error!');
});
var secondListener = jasmine.createSpy('second');

$rootScope.$on('e', throwingListener);
$rootScope.$on('e', secondListener);

expect(function() { $rootScope.$broadcast('e'); }).toThrowError('Listener Error!');
expect(throwingListener).toHaveBeenCalled();
expect(secondListener).not.toHaveBeenCalled();

throwingListener.calls.reset();
secondListener.calls.reset();

expect(function() { $rootScope.$broadcast('e'); }).toThrowError('Listener Error!');
expect(throwingListener).toHaveBeenCalled();
expect(secondListener).not.toHaveBeenCalled();
});
});
});
});

Expand Down Expand Up @@ -2773,7 +2674,7 @@ describe('Scope', function() {
expect(spy1).toHaveBeenCalledOnce();
expect(spy2).toHaveBeenCalledOnce();
expect(spy3).toHaveBeenCalledOnce();
expect(child.$$listeners['evt'].length).toBe(2);
expect(child.$$listeners['evt'].length).toBe(3); // cleanup will happen on next $emit

spy1.calls.reset();
spy2.calls.reset();
Expand Down Expand Up @@ -2807,7 +2708,7 @@ describe('Scope', function() {
expect(spy1).toHaveBeenCalledOnce();
expect(spy2).toHaveBeenCalledOnce();
expect(spy3).toHaveBeenCalledOnce();
expect(child.$$listeners['evt'].length).toBe(2);
expect(child.$$listeners['evt'].length).toBe(3); //cleanup will happen on next $broadcast

spy1.calls.reset();
spy2.calls.reset();
Expand Down Expand Up @@ -3024,6 +2925,45 @@ describe('Scope', function() {
}));
});
});


it('should allow recursive $emit/$broadcast', inject(function($rootScope) {
var callCount = 0;
$rootScope.$on('evt', function($event, arg0) {
callCount++;
if (arg0 !== 1234) {
$rootScope.$emit('evt', 1234);
$rootScope.$broadcast('evt', 1234);
}
});

$rootScope.$emit('evt');
$rootScope.$broadcast('evt');
expect(callCount).toBe(6);
}));


it('should allow recursive $emit/$broadcast between parent/child', inject(function($rootScope) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this test demonstrates recursive emitting/broadcasting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How so? It does at least one recursive $broadcast + one $emit within $broadcast...

I'm sure we could think of better or more interesting tests though. Let me know if you have any specific ones you think should be added.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it does root.$broadcast inside root.$broadcast (which we don't care about in this test) and child.$emit inside root.$broadcast, so in a way it is recursive. But it doesn't test for root.$broadcast inside child.$emit, for example.

In order to test both root.$broadcast --> child.$emit and child.$emit --> root.$broadcast, I would change it to something like:

var child = $rootScope.$new();
var calls = '';

$rootScope.$on('evt', function($event, arg0) {
  calls += 'r';  // For "root".
  if (arg0 === 'fromChild') {
    $rootScope.$broadcast('evt', 'fromRoot2');
  }
});

child.$on('evt', function($event, arg0) {
  calls += 'c';  // For "child".
  if (arg0 === 'fromRoot1') {
    child.$emit('evt', 'fromChild');
  }
});

$rootScope.$broadcast('evt', 'fromRoot1');
expect(calls).toBe('rccrrc');

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated to use your test

var child = $rootScope.$new();
var calls = '';

$rootScope.$on('evt', function($event, arg0) {
calls += 'r'; // For "root".
if (arg0 === 'fromChild') {
$rootScope.$broadcast('evt', 'fromRoot2');
}
});

child.$on('evt', function($event, arg0) {
calls += 'c'; // For "child".
if (arg0 === 'fromRoot1') {
child.$emit('evt', 'fromChild');
}
});

$rootScope.$broadcast('evt', 'fromRoot1');
expect(calls).toBe('rccrrc');
}));
});

describe('doc examples', function() {
Expand Down