Skip to content

Commit 09e46b2

Browse files
jbedardjbedard
committed
fix($rootScope): fix potential memory leak when removing scope listeners
Previously the array entry for listeners was set to null but the array size was not trimmed until the event was broadcasted again (see angular@e6966e0). By keeping track of the listener iteration index globally it can be adjusted if a listener removal effects the index. Fixes angular#16135 BREAKING CHANGE: `$emit`/`$broadcast` listeners of a specific event name on a scope can no longer be recursivly invoked.
1 parent e04b833 commit 09e46b2

File tree

2 files changed

+82
-55
lines changed

2 files changed

+82
-55
lines changed

‎src/ng/rootScope.js

+32-53
Original file line numberDiff line numberDiff line change
@@ -1167,7 +1167,7 @@ function $RootScopeProvider() {
11671167
$on: function(name, listener) {
11681168
var namedListeners = this.$$listeners[name];
11691169
if (!namedListeners) {
1170-
this.$$listeners[name] = namedListeners = [];
1170+
this.$$listeners[name] = namedListeners = extend([], {$$index: -1});
11711171
}
11721172
namedListeners.push(listener);
11731173

@@ -1181,10 +1181,12 @@ function $RootScopeProvider() {
11811181

11821182
var self = this;
11831183
return function() {
1184-
var indexOfListener = namedListeners.indexOf(listener);
1185-
if (indexOfListener !== -1) {
1186-
namedListeners[indexOfListener] = null;
1184+
var index = arrayRemove(namedListeners, listener);
1185+
if (index >= 0) {
11871186
decrementListenerCount(self, 1, name);
1187+
if (index <= namedListeners.$$index) {
1188+
namedListeners.$$index--;
1189+
}
11881190
}
11891191
};
11901192
},
@@ -1213,9 +1215,7 @@ function $RootScopeProvider() {
12131215
* @return {Object} Event object (see {@link ng.$rootScope.Scope#$on}).
12141216
*/
12151217
$emit: function(name, args) {
1216-
var empty = [],
1217-
namedListeners,
1218-
scope = this,
1218+
var scope = this,
12191219
stopPropagation = false,
12201220
event = {
12211221
name: name,
@@ -1226,36 +1226,14 @@ function $RootScopeProvider() {
12261226
},
12271227
defaultPrevented: false
12281228
},
1229-
listenerArgs = concat([event], arguments, 1),
1230-
i, length;
1229+
listenerArgs = concat([event], arguments, 1);
12311230

12321231
do {
1233-
namedListeners = scope.$$listeners[name] || empty;
1234-
event.currentScope = scope;
1235-
for (i = 0, length = namedListeners.length; i < length; i++) {
1236-
1237-
// if listeners were deregistered, defragment the array
1238-
if (!namedListeners[i]) {
1239-
namedListeners.splice(i, 1);
1240-
i--;
1241-
length--;
1242-
continue;
1243-
}
1244-
try {
1245-
//allow all listeners attached to the current scope to run
1246-
namedListeners[i].apply(null, listenerArgs);
1247-
} catch (e) {
1248-
$exceptionHandler(e);
1249-
}
1250-
}
1251-
//if any listener on the current scope stops propagation, prevent bubbling
1252-
if (stopPropagation) {
1253-
event.currentScope = null;
1254-
return event;
1255-
}
1232+
invokeListeners(scope, event, listenerArgs, name);
1233+
12561234
//traverse upwards
1257-
scope = scope.$parent;
1258-
} while (scope);
1235+
//if any listener on the current scope stops propagation, prevent bubbling
1236+
} while (!stopPropagation && (scope = scope.$parent));
12591237

12601238
event.currentScope = null;
12611239

@@ -1299,28 +1277,11 @@ function $RootScopeProvider() {
12991277

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

1302-
var listenerArgs = concat([event], arguments, 1),
1303-
listeners, i, length;
1280+
var listenerArgs = concat([event], arguments, 1);
13041281

13051282
//down while you can, then up and next sibling or up and next sibling until back at root
13061283
while ((current = next)) {
1307-
event.currentScope = current;
1308-
listeners = current.$$listeners[name] || [];
1309-
for (i = 0, length = listeners.length; i < length; i++) {
1310-
// if listeners were deregistered, defragment the array
1311-
if (!listeners[i]) {
1312-
listeners.splice(i, 1);
1313-
i--;
1314-
length--;
1315-
continue;
1316-
}
1317-
1318-
try {
1319-
listeners[i].apply(null, listenerArgs);
1320-
} catch (e) {
1321-
$exceptionHandler(e);
1322-
}
1323-
}
1284+
invokeListeners(current, event, listenerArgs, name);
13241285

13251286
// Insanity Warning: scope depth-first traversal
13261287
// yes, this code is a bit crazy, but it works and we have tests to prove it!
@@ -1350,6 +1311,24 @@ function $RootScopeProvider() {
13501311

13511312
return $rootScope;
13521313

1314+
function invokeListeners(scope, event, listenerArgs, name) {
1315+
var listeners = scope.$$listeners[name];
1316+
if (listeners) {
1317+
if (listeners.$$index !== -1) {
1318+
throw $rootScopeMinErr('inevt', '{0} already $emit/$broadcast-ing', name);
1319+
}
1320+
event.currentScope = scope;
1321+
for (listeners.$$index = 0; listeners.$$index < listeners.length; listeners.$$index++) {
1322+
try {
1323+
//allow all listeners attached to the current scope to run
1324+
listeners[listeners.$$index].apply(null, listenerArgs);
1325+
} catch (e) {
1326+
$exceptionHandler(e);
1327+
}
1328+
}
1329+
listeners.$$index = -1;
1330+
}
1331+
}
13531332

13541333
function beginPhase(phase) {
13551334
if ($rootScope.$$phase) {

‎test/ng/rootScopeSpec.js

+50-2
Original file line numberDiff line numberDiff line change
@@ -2316,6 +2316,19 @@ describe('Scope', function() {
23162316
}));
23172317

23182318

2319+
// See issue https://github.com/angular/angular.js/issues/16135
2320+
it('should deallocate the listener array entry', inject(function($rootScope) {
2321+
var remove1 = $rootScope.$on('abc', noop);
2322+
$rootScope.$on('abc', noop);
2323+
2324+
expect($rootScope.$$listeners['abc'].length).toBe(2);
2325+
2326+
remove1();
2327+
2328+
expect($rootScope.$$listeners['abc'].length).toBe(1);
2329+
}));
2330+
2331+
23192332
it('should call next listener when removing current', inject(function($rootScope) {
23202333
var listener1 = jasmine.createSpy().and.callFake(function() { remove1(); });
23212334
var remove1 = $rootScope.$on('abc', listener1);
@@ -2442,6 +2455,41 @@ describe('Scope', function() {
24422455
expect($rootScope.$$listenerCount).toEqual({abc: 1});
24432456
expect(child.$$listenerCount).toEqual({abc: 1});
24442457
}));
2458+
2459+
2460+
it('should throw on recursive $broadcast', inject(function($rootScope) {
2461+
$rootScope.$on('e', function() { $rootScope.$broadcast('e'); });
2462+
2463+
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing');
2464+
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing');
2465+
}));
2466+
2467+
2468+
it('should throw on recursive $emit', inject(function($rootScope) {
2469+
$rootScope.$on('e', function() { $rootScope.$emit('e'); });
2470+
2471+
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing');
2472+
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing');
2473+
}));
2474+
2475+
2476+
it('should throw on recursive $broadcast on child listener', inject(function($rootScope) {
2477+
var child = $rootScope.$new();
2478+
child.$on('e', function() { $rootScope.$broadcast('e'); });
2479+
2480+
expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing');
2481+
expect(function() { child.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing');
2482+
}));
2483+
2484+
2485+
it('should throw on recursive $emit parent listener', inject(function($rootScope) {
2486+
var child = $rootScope.$new();
2487+
$rootScope.$on('e', function() { child.$emit('e'); });
2488+
2489+
expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing');
2490+
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing');
2491+
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing');
2492+
}));
24452493
});
24462494
});
24472495

@@ -2531,7 +2579,7 @@ describe('Scope', function() {
25312579
expect(spy1).toHaveBeenCalledOnce();
25322580
expect(spy2).toHaveBeenCalledOnce();
25332581
expect(spy3).toHaveBeenCalledOnce();
2534-
expect(child.$$listeners['evt'].length).toBe(3); // cleanup will happen on next $emit
2582+
expect(child.$$listeners['evt'].length).toBe(2);
25352583

25362584
spy1.calls.reset();
25372585
spy2.calls.reset();
@@ -2565,7 +2613,7 @@ describe('Scope', function() {
25652613
expect(spy1).toHaveBeenCalledOnce();
25662614
expect(spy2).toHaveBeenCalledOnce();
25672615
expect(spy3).toHaveBeenCalledOnce();
2568-
expect(child.$$listeners['evt'].length).toBe(3); //cleanup will happen on next $broadcast
2616+
expect(child.$$listeners['evt'].length).toBe(2);
25692617

25702618
spy1.calls.reset();
25712619
spy2.calls.reset();

0 commit comments

Comments
 (0)