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

Commit e5fb929

Browse files
jbedardNarretz
jbedard
authored and
Narretz
committed
revert: fix($rootScope): fix potential memory leak when removing scope listeners
This reverts commit 817ac56.
1 parent 41d5c90 commit e5fb929

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

12721272
var self = this;
12731273
return function() {
1274-
var index = arrayRemove(namedListeners, listener);
1275-
if (index >= 0) {
1274+
var indexOfListener = namedListeners.indexOf(listener);
1275+
if (indexOfListener !== -1) {
1276+
namedListeners[indexOfListener] = null;
12761277
decrementListenerCount(self, 1, name);
1277-
// We are removing a listener while iterating over the list of listeners.
1278-
// Update the current $$index if necessary to ensure no listener is skipped.
1279-
if (index <= namedListeners.$$index) {
1280-
namedListeners.$$index--;
1281-
}
12821278
}
12831279
};
12841280
},
@@ -1307,7 +1303,9 @@ function $RootScopeProvider() {
13071303
* @return {Object} Event object (see {@link ng.$rootScope.Scope#$on}).
13081304
*/
13091305
$emit: function(name, args) {
1310-
var scope = this,
1306+
var empty = [],
1307+
namedListeners,
1308+
scope = this,
13111309
stopPropagation = false,
13121310
event = {
13131311
name: name,
@@ -1318,11 +1316,28 @@ function $RootScopeProvider() {
13181316
},
13191317
defaultPrevented: false
13201318
},
1321-
listenerArgs = concat([event], arguments, 1);
1319+
listenerArgs = concat([event], arguments, 1),
1320+
i, length;
13221321

13231322
do {
1324-
invokeListeners(scope, event, listenerArgs, name);
1325-
1323+
namedListeners = scope.$$listeners[name] || empty;
1324+
event.currentScope = scope;
1325+
for (i = 0, length = namedListeners.length; i < length; i++) {
1326+
1327+
// if listeners were deregistered, defragment the array
1328+
if (!namedListeners[i]) {
1329+
namedListeners.splice(i, 1);
1330+
i--;
1331+
length--;
1332+
continue;
1333+
}
1334+
try {
1335+
//allow all listeners attached to the current scope to run
1336+
namedListeners[i].apply(null, listenerArgs);
1337+
} catch (e) {
1338+
$exceptionHandler(e);
1339+
}
1340+
}
13261341
//if any listener on the current scope stops propagation, prevent bubbling
13271342
if (stopPropagation) {
13281343
break;
@@ -1373,11 +1388,28 @@ function $RootScopeProvider() {
13731388

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

1376-
var listenerArgs = concat([event], arguments, 1);
1391+
var listenerArgs = concat([event], arguments, 1),
1392+
listeners, i, length;
13771393

13781394
//down while you can, then up and next sibling or up and next sibling until back at root
13791395
while ((current = next)) {
1380-
invokeListeners(current, event, listenerArgs, name);
1396+
event.currentScope = current;
1397+
listeners = current.$$listeners[name] || [];
1398+
for (i = 0, length = listeners.length; i < length; i++) {
1399+
// if listeners were deregistered, defragment the array
1400+
if (!listeners[i]) {
1401+
listeners.splice(i, 1);
1402+
i--;
1403+
length--;
1404+
continue;
1405+
}
1406+
1407+
try {
1408+
listeners[i].apply(null, listenerArgs);
1409+
} catch (e) {
1410+
$exceptionHandler(e);
1411+
}
1412+
}
13811413

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

14091441
return $rootScope;
14101442

1411-
function invokeListeners(scope, event, listenerArgs, name) {
1412-
var listeners = scope.$$listeners[name];
1413-
if (listeners) {
1414-
if (listeners.$$index !== undefined) {
1415-
throw $rootScopeMinErr('inevt', '{0} already $emit/$broadcast-ing on scope ({1})', name, scope.$id);
1416-
}
1417-
event.currentScope = scope;
1418-
try {
1419-
for (listeners.$$index = 0; listeners.$$index < listeners.length; listeners.$$index++) {
1420-
try {
1421-
//allow all listeners attached to the current scope to run
1422-
listeners[listeners.$$index].apply(null, listenerArgs);
1423-
} catch (e) {
1424-
$exceptionHandler(e);
1425-
}
1426-
}
1427-
} finally {
1428-
listeners.$$index = undefined;
1429-
}
1430-
}
1431-
}
14321443

14331444
function beginPhase(phase) {
14341445
if ($rootScope.$$phase) {

‎test/ng/rootScopeSpec.js

+2-116
Original file line numberDiff line numberDiff line change
@@ -2438,19 +2438,6 @@ describe('Scope', function() {
24382438
}));
24392439

24402440

2441-
// See issue https://github.com/angular/angular.js/issues/16135
2442-
it('should deallocate the listener array entry', inject(function($rootScope) {
2443-
var remove1 = $rootScope.$on('abc', noop);
2444-
$rootScope.$on('abc', noop);
2445-
2446-
expect($rootScope.$$listeners['abc'].length).toBe(2);
2447-
2448-
remove1();
2449-
2450-
expect($rootScope.$$listeners['abc'].length).toBe(1);
2451-
}));
2452-
2453-
24542441
it('should call next listener after removing the current listener via its own handler', inject(function($rootScope) {
24552442
var listener1 = jasmine.createSpy('listener1').and.callFake(function() { remove1(); });
24562443
var remove1 = $rootScope.$on('abc', listener1);
@@ -2583,107 +2570,6 @@ describe('Scope', function() {
25832570
expect($rootScope.$$listenerCount).toEqual({abc: 1});
25842571
expect(child.$$listenerCount).toEqual({abc: 1});
25852572
}));
2586-
2587-
2588-
it('should throw on recursive $broadcast', inject(function($rootScope) {
2589-
$rootScope.$on('e', function() { $rootScope.$broadcast('e'); });
2590-
2591-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2592-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2593-
}));
2594-
2595-
2596-
it('should throw on nested recursive $broadcast', inject(function($rootScope) {
2597-
$rootScope.$on('e2', function() { $rootScope.$broadcast('e'); });
2598-
$rootScope.$on('e', function() { $rootScope.$broadcast('e2'); });
2599-
2600-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2601-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2602-
}));
2603-
2604-
2605-
it('should throw on recursive $emit', inject(function($rootScope) {
2606-
$rootScope.$on('e', function() { $rootScope.$emit('e'); });
2607-
2608-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2609-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2610-
}));
2611-
2612-
2613-
it('should throw on nested recursive $emit', inject(function($rootScope) {
2614-
$rootScope.$on('e2', function() { $rootScope.$emit('e'); });
2615-
$rootScope.$on('e', function() { $rootScope.$emit('e2'); });
2616-
2617-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2618-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2619-
}));
2620-
2621-
2622-
it('should throw on recursive $broadcast on child listener', inject(function($rootScope) {
2623-
var child = $rootScope.$new();
2624-
child.$on('e', function() { $rootScope.$broadcast('e'); });
2625-
2626-
expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
2627-
expect(function() { child.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
2628-
}));
2629-
2630-
2631-
it('should throw on nested recursive $broadcast on child listener', inject(function($rootScope) {
2632-
var child = $rootScope.$new();
2633-
child.$on('e2', function() { $rootScope.$broadcast('e'); });
2634-
child.$on('e', function() { $rootScope.$broadcast('e2'); });
2635-
2636-
expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
2637-
expect(function() { child.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)');
2638-
}));
2639-
2640-
2641-
it('should throw on recursive $emit parent listener', inject(function($rootScope) {
2642-
var child = $rootScope.$new();
2643-
$rootScope.$on('e', function() { child.$emit('e'); });
2644-
2645-
expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2646-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2647-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2648-
}));
2649-
2650-
2651-
it('should throw on nested recursive $emit parent listener', inject(function($rootScope) {
2652-
var child = $rootScope.$new();
2653-
$rootScope.$on('e2', function() { child.$emit('e'); });
2654-
$rootScope.$on('e', function() { child.$emit('e2'); });
2655-
2656-
expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2657-
expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2658-
expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)');
2659-
}));
2660-
2661-
2662-
it('should clear recursive state of $broadcast if $exceptionHandler rethrows', function() {
2663-
module(function($exceptionHandlerProvider) {
2664-
$exceptionHandlerProvider.mode('rethrow');
2665-
});
2666-
inject(function($rootScope) {
2667-
var throwingListener = jasmine.createSpy('thrower').and.callFake(function() {
2668-
throw new Error('Listener Error!');
2669-
});
2670-
var secondListener = jasmine.createSpy('second');
2671-
2672-
$rootScope.$on('e', throwingListener);
2673-
$rootScope.$on('e', secondListener);
2674-
2675-
expect(function() { $rootScope.$broadcast('e'); }).toThrowError('Listener Error!');
2676-
expect(throwingListener).toHaveBeenCalled();
2677-
expect(secondListener).not.toHaveBeenCalled();
2678-
2679-
throwingListener.calls.reset();
2680-
secondListener.calls.reset();
2681-
2682-
expect(function() { $rootScope.$broadcast('e'); }).toThrowError('Listener Error!');
2683-
expect(throwingListener).toHaveBeenCalled();
2684-
expect(secondListener).not.toHaveBeenCalled();
2685-
});
2686-
});
26872573
});
26882574
});
26892575

@@ -2773,7 +2659,7 @@ describe('Scope', function() {
27732659
expect(spy1).toHaveBeenCalledOnce();
27742660
expect(spy2).toHaveBeenCalledOnce();
27752661
expect(spy3).toHaveBeenCalledOnce();
2776-
expect(child.$$listeners['evt'].length).toBe(2);
2662+
expect(child.$$listeners['evt'].length).toBe(3); // cleanup will happen on next $emit
27772663

27782664
spy1.calls.reset();
27792665
spy2.calls.reset();
@@ -2807,7 +2693,7 @@ describe('Scope', function() {
28072693
expect(spy1).toHaveBeenCalledOnce();
28082694
expect(spy2).toHaveBeenCalledOnce();
28092695
expect(spy3).toHaveBeenCalledOnce();
2810-
expect(child.$$listeners['evt'].length).toBe(2);
2696+
expect(child.$$listeners['evt'].length).toBe(3); //cleanup will happen on next $broadcast
28112697

28122698
spy1.calls.reset();
28132699
spy2.calls.reset();

0 commit comments

Comments
 (0)