Skip to content

Commit 0896d4d

Browse files
jbedardjbedard
committed
Revert "fix($rootScope): fix potential memory leak when removing scope listeners"
This reverts commit 817ac56.
1 parent a61c5d3 commit 0896d4d

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
@@ -1178,14 +1178,10 @@ function $RootScopeProvider() {
11781178

11791179
var self = this;
11801180
return function() {
1181-
var index = arrayRemove(namedListeners, listener);
1182-
if (index >= 0) {
1181+
var indexOfListener = namedListeners.indexOf(listener);
1182+
if (indexOfListener !== -1) {
1183+
namedListeners[indexOfListener] = null;
11831184
decrementListenerCount(self, 1, name);
1184-
// We are removing a listener while iterating over the list of listeners.
1185-
// Update the current $$index if necessary to ensure no listener is skipped.
1186-
if (index <= namedListeners.$$index) {
1187-
namedListeners.$$index--;
1188-
}
11891185
}
11901186
};
11911187
},
@@ -1214,7 +1210,9 @@ function $RootScopeProvider() {
12141210
* @return {Object} Event object (see {@link ng.$rootScope.Scope#$on}).
12151211
*/
12161212
$emit: function(name, args) {
1217-
var scope = this,
1213+
var empty = [],
1214+
namedListeners,
1215+
scope = this,
12181216
stopPropagation = false,
12191217
event = {
12201218
name: name,
@@ -1225,11 +1223,28 @@ function $RootScopeProvider() {
12251223
},
12261224
defaultPrevented: false
12271225
},
1228-
listenerArgs = concat([event], arguments, 1);
1226+
listenerArgs = concat([event], arguments, 1),
1227+
i, length;
12291228

12301229
do {
1231-
invokeListeners(scope, event, listenerArgs, name);
1232-
1230+
namedListeners = scope.$$listeners[name] || empty;
1231+
event.currentScope = scope;
1232+
for (i = 0, length = namedListeners.length; i < length; i++) {
1233+
1234+
// if listeners were deregistered, defragment the array
1235+
if (!namedListeners[i]) {
1236+
namedListeners.splice(i, 1);
1237+
i--;
1238+
length--;
1239+
continue;
1240+
}
1241+
try {
1242+
//allow all listeners attached to the current scope to run
1243+
namedListeners[i].apply(null, listenerArgs);
1244+
} catch (e) {
1245+
$exceptionHandler(e);
1246+
}
1247+
}
12331248
//if any listener on the current scope stops propagation, prevent bubbling
12341249
if (stopPropagation) {
12351250
break;
@@ -1280,11 +1295,28 @@ function $RootScopeProvider() {
12801295

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

1283-
var listenerArgs = concat([event], arguments, 1);
1298+
var listenerArgs = concat([event], arguments, 1),
1299+
listeners, i, length;
12841300

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

12891321
// Insanity Warning: scope depth-first traversal
12901322
// yes, this code is a bit crazy, but it works and we have tests to prove it!
@@ -1314,27 +1346,6 @@ function $RootScopeProvider() {
13141346

13151347
return $rootScope;
13161348

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

13391350
function beginPhase(phase) {
13401351
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)