Skip to content

Commit 00f1d27

Browse files
jbedardjbedard
committed
Revert "fix($rootScope): fix potential memory leak when removing scope listeners"
This reverts commit 817ac56.
1 parent 8b69d91 commit 00f1d27

File tree

3 files changed

+47
-172
lines changed

3 files changed

+47
-172
lines changed

‎docs/content/error/$rootScope/inevt.ngdoc

-22
This file was deleted.

‎src/ng/rootScope.js

+45-34
Original file line numberDiff line numberDiff line change
@@ -1181,14 +1181,10 @@ function $RootScopeProvider() {
11811181

11821182
var self = this;
11831183
return function() {
1184-
var index = arrayRemove(namedListeners, listener);
1185-
if (index >= 0) {
1184+
var indexOfListener = namedListeners.indexOf(listener);
1185+
if (indexOfListener !== -1) {
1186+
namedListeners[indexOfListener] = null;
11861187
decrementListenerCount(self, 1, name);
1187-
// We are removing a listener while iterating over the list of listeners.
1188-
// Update the current $$index if necessary to ensure no listener is skipped.
1189-
if (index <= namedListeners.$$index) {
1190-
namedListeners.$$index--;
1191-
}
11921188
}
11931189
};
11941190
},
@@ -1217,7 +1213,9 @@ function $RootScopeProvider() {
12171213
* @return {Object} Event object (see {@link ng.$rootScope.Scope#$on}).
12181214
*/
12191215
$emit: function(name, args) {
1220-
var scope = this,
1216+
var empty = [],
1217+
namedListeners,
1218+
scope = this,
12211219
stopPropagation = false,
12221220
event = {
12231221
name: name,
@@ -1228,11 +1226,28 @@ function $RootScopeProvider() {
12281226
},
12291227
defaultPrevented: false
12301228
},
1231-
listenerArgs = concat([event], arguments, 1);
1229+
listenerArgs = concat([event], arguments, 1),
1230+
i, length;
12321231

12331232
do {
1234-
invokeListeners(scope, event, listenerArgs, name);
1235-
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+
}
12361251
//if any listener on the current scope stops propagation, prevent bubbling
12371252
if (stopPropagation) {
12381253
break;
@@ -1283,11 +1298,28 @@ function $RootScopeProvider() {
12831298

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

1286-
var listenerArgs = concat([event], arguments, 1);
1301+
var listenerArgs = concat([event], arguments, 1),
1302+
listeners, i, length;
12871303

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

12921324
// Insanity Warning: scope depth-first traversal
12931325
// yes, this code is a bit crazy, but it works and we have tests to prove it!
@@ -1317,27 +1349,6 @@ function $RootScopeProvider() {
13171349

13181350
return $rootScope;
13191351

1320-
function invokeListeners(scope, event, listenerArgs, name) {
1321-
var listeners = scope.$$listeners[name];
1322-
if (listeners) {
1323-
if (listeners.$$index !== undefined) {
1324-
throw $rootScopeMinErr('inevt', '{0} already $emit/$broadcast-ing on scope ({1})', name, scope.$id);
1325-
}
1326-
event.currentScope = scope;
1327-
try {
1328-
for (listeners.$$index = 0; listeners.$$index < listeners.length; listeners.$$index++) {
1329-
try {
1330-
//allow all listeners attached to the current scope to run
1331-
listeners[listeners.$$index].apply(null, listenerArgs);
1332-
} catch (e) {
1333-
$exceptionHandler(e);
1334-
}
1335-
}
1336-
} finally {
1337-
listeners.$$index = undefined;
1338-
}
1339-
}
1340-
}
13411352

13421353
function beginPhase(phase) {
13431354
if ($rootScope.$$phase) {

‎test/ng/rootScopeSpec.js

+2-116
Original file line numberDiff line numberDiff line change
@@ -2316,19 +2316,6 @@ 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-
23322319
it('should call next listener after removing the current listener via its own handler', inject(function($rootScope) {
23332320
var listener1 = jasmine.createSpy('listener1').and.callFake(function() { remove1(); });
23342321
var remove1 = $rootScope.$on('abc', listener1);
@@ -2461,107 +2448,6 @@ describe('Scope', function() {
24612448
expect($rootScope.$$listenerCount).toEqual({abc: 1});
24622449
expect(child.$$listenerCount).toEqual({abc: 1});
24632450
}));
2464-
2465-
2466-
it('should throw on recursive $broadcast', inject(function($rootScope) {
2467-
$rootScope.$on('e', function() { $rootScope.$broadcast('e'); });
2468-
2469-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2470-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2471-
}));
2472-
2473-
2474-
it('should throw on nested recursive $broadcast', inject(function($rootScope) {
2475-
$rootScope.$on('e2', function() { $rootScope.$broadcast('e'); });
2476-
$rootScope.$on('e', function() { $rootScope.$broadcast('e2'); });
2477-
2478-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2479-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2480-
}));
2481-
2482-
2483-
it('should throw on recursive $emit', inject(function($rootScope) {
2484-
$rootScope.$on('e', function() { $rootScope.$emit('e'); });
2485-
2486-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2487-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2488-
}));
2489-
2490-
2491-
it('should throw on nested recursive $emit', inject(function($rootScope) {
2492-
$rootScope.$on('e2', function() { $rootScope.$emit('e'); });
2493-
$rootScope.$on('e', function() { $rootScope.$emit('e2'); });
2494-
2495-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2496-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2497-
}));
2498-
2499-
2500-
it('should throw on recursive $broadcast on child listener', inject(function($rootScope) {
2501-
var child = $rootScope.$new();
2502-
child.$on('e', function() { $rootScope.$broadcast('e'); });
2503-
2504-
expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
2505-
expect(function() { child.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
2506-
}));
2507-
2508-
2509-
it('should throw on nested recursive $broadcast on child listener', inject(function($rootScope) {
2510-
var child = $rootScope.$new();
2511-
child.$on('e2', function() { $rootScope.$broadcast('e'); });
2512-
child.$on('e', function() { $rootScope.$broadcast('e2'); });
2513-
2514-
expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
2515-
expect(function() { child.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
2516-
}));
2517-
2518-
2519-
it('should throw on recursive $emit parent listener', inject(function($rootScope) {
2520-
var child = $rootScope.$new();
2521-
$rootScope.$on('e', function() { child.$emit('e'); });
2522-
2523-
expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2524-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2525-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2526-
}));
2527-
2528-
2529-
it('should throw on nested recursive $emit parent listener', inject(function($rootScope) {
2530-
var child = $rootScope.$new();
2531-
$rootScope.$on('e2', function() { child.$emit('e'); });
2532-
$rootScope.$on('e', function() { child.$emit('e2'); });
2533-
2534-
expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2535-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2536-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2537-
}));
2538-
2539-
2540-
it('should clear recursive state of $broadcast if $exceptionHandler rethrows', function() {
2541-
module(function($exceptionHandlerProvider) {
2542-
$exceptionHandlerProvider.mode('rethrow');
2543-
});
2544-
inject(function($rootScope) {
2545-
var throwingListener = jasmine.createSpy('thrower').and.callFake(function() {
2546-
throw new Error('Listener Error!');
2547-
});
2548-
var secondListener = jasmine.createSpy('second');
2549-
2550-
$rootScope.$on('e', throwingListener);
2551-
$rootScope.$on('e', secondListener);
2552-
2553-
expect(function() { $rootScope.$broadcast('e'); }).toThrowError('Listener Error!');
2554-
expect(throwingListener).toHaveBeenCalled();
2555-
expect(secondListener).not.toHaveBeenCalled();
2556-
2557-
throwingListener.calls.reset();
2558-
secondListener.calls.reset();
2559-
2560-
expect(function() { $rootScope.$broadcast('e'); }).toThrowError('Listener Error!');
2561-
expect(throwingListener).toHaveBeenCalled();
2562-
expect(secondListener).not.toHaveBeenCalled();
2563-
});
2564-
});
25652451
});
25662452
});
25672453

@@ -2651,7 +2537,7 @@ describe('Scope', function() {
26512537
expect(spy1).toHaveBeenCalledOnce();
26522538
expect(spy2).toHaveBeenCalledOnce();
26532539
expect(spy3).toHaveBeenCalledOnce();
2654-
expect(child.$$listeners['evt'].length).toBe(2);
2540+
expect(child.$$listeners['evt'].length).toBe(3); // cleanup will happen on next $emit
26552541

26562542
spy1.calls.reset();
26572543
spy2.calls.reset();
@@ -2685,7 +2571,7 @@ describe('Scope', function() {
26852571
expect(spy1).toHaveBeenCalledOnce();
26862572
expect(spy2).toHaveBeenCalledOnce();
26872573
expect(spy3).toHaveBeenCalledOnce();
2688-
expect(child.$$listeners['evt'].length).toBe(2);
2574+
expect(child.$$listeners['evt'].length).toBe(3); //cleanup will happen on next $broadcast
26892575

26902576
spy1.calls.reset();
26912577
spy2.calls.reset();

0 commit comments

Comments
 (0)