fix($rootScope): fix potential memory leak when removing scope listeners (v2) #16293

jbedard · 2017-10-21T21:36:43Z

This is the alternative to #16161 that we discussed.

This simplifies the listener cleanup and avoids creating sparse listener arrays (unlike #16161) but introduces a new restriction that disallows recursive event $broadcast/$emit-ing. We could remove this restriction but it would complicate everything event related by requiring a stack for the index.

gkalpak

A couple of minor comments.
LGTM as soon as Travis is happy 😉

gkalpak · 2017-10-23T08:22:35Z

src/ng/rootScope.js

+      var listeners = scope.$$listeners[name];
+      if (listeners) {
+        if (listeners.$$index !== -1) {
+          throw $rootScopeMinErr('inevt', '{0} already $emit/$broadcast-ing', name);


Maybe worth mentioning the scope $id (or even whether it was being $emited/$broadcasted) to aid in debugging).

Also, we need an error page for $rootScope:inevt.

gkalpak · 2017-10-23T08:28:54Z

src/ng/rootScope.js

+            //allow all listeners attached to the current scope to run
+            listeners[listeners.$$index].apply(null, listenerArgs);
+          } catch (e) {
+            $exceptionHandler(e);


If $exceptionHandler throws, listeners.$$index will be never reset and the event will be never allowed to be emitted/broadcasted any more. I wonder if that would break some usecase unnecessarily.

Yeah I'd like to fix this somehow...

Could you just wrap the loop in a try...finally and put the listerners.$$index = -1 statement in the finally block?

I added the try...finally and added a test that fails without that fix.

gkalpak · 2017-10-23T08:32:13Z

test/ng/rootScopeSpec.js

+
+
+        it('should call next listener when removing current', inject(function($rootScope) {
+          var listener1 = jasmine.createSpy().and.callFake(function() { remove1(); });


Nit: Giving spies a name makes it a little easier to identify the error (in case the test breaks).

Note: We are relying upon scope hoisting of the var remove1 here.

If we felt that is bad, one way around this could be:

var listener1 = jasmine.createSpy(); var remove1 = $rootScope.$on('abc', listener1); listener1.and.callFake(function() { remove1(); };

gkalpak · 2017-10-23T08:35:16Z

test/ng/rootScopeSpec.js

+          var remove2 = $rootScope.$on('abc', listener2);
+
+          var listener3 = jasmine.createSpy();
+          var remove3 = $rootScope.$on('abc', listener3);


It might be worth adding a 4th listener to make sure the rest of the queue is not messed up.

petebacondarwin · 2017-10-23T08:52:54Z

src/ng/rootScope.js

            decrementListenerCount(self, 1, name);
+            if (index <= namedListeners.$$index) {
+              namedListeners.$$index--;
+            }


A comment here explaining the logic will help in the future. E.g. "we are removing a listener while iterating over the list of listeners. Update the current $index if necessary to ensure no listener is skipped."

petebacondarwin · 2017-10-23T09:13:01Z

test/ng/rootScopeSpec.js

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


Yeah I like this too 👍

petebacondarwin · 2017-10-23T09:49:35Z

test/ng/rootScopeSpec.js

+        }));
+
+
+        it('should call next listener when removing current', inject(function($rootScope) {


should not skip next listener when removing current one?

or not to be negative:

should call next listener after removing the current listener via its own handler

petebacondarwin · 2017-10-23T09:55:55Z

test/ng/rootScopeSpec.js

+        }));
+
+
+        it('should call all listeners when removing previous', inject(function($rootScope) {


should call all subsequent listeners when a previous listener is removed via a handler

petebacondarwin · 2017-10-23T10:17:07Z

test/ng/rootScopeSpec.js

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


Is it worth testing cyclic recursion to? E.g. handler for event "a" broadcasts event "b", whose hander broadcasts event "a" again?

jbedard · 2017-10-28T04:04:20Z

#16161 was merged to v1.6.x and I cherry-picked the test commit from there into master (c15c8a1). So this PR no longer has that that commit.

For this fix... I've added the error docs. Please let me know if they need more info or more examples etc.

jbedard · 2017-10-28T04:05:44Z

src/ng/rootScope.js

+      var listeners = scope.$$listeners[name];
+      if (listeners) {
+        if (listeners.$$index !== -1) {
+          throw $rootScopeMinErr('inevt', '{0} already $emit/$broadcast-ing on scope ({1})', name, scope.$id);


I added the scope.$id here.

Also any better ideas for the error code inevt?

Previously the array entry for listeners was set to null but the array size was not trimmed until the event was broadcasted again (see e6966e0). By keeping track of the listener iteration index globally it can be adjusted if a listener removal effects the index. Fixes angular#16135 Closes angular#16293 BREAKING CHANGE: Recursively invoking `$emit` or `$broadcast` with the same event name is no longer supported. This will now throw a `inevt` minErr.

jbedard · 2017-10-28T17:33:43Z

src/ng/rootScope.js

@@ -1167,7 +1167,7 @@ function $RootScopeProvider() {
      $on: function(name, listener) {
        var namedListeners = this.$$listeners[name];
        if (!namedListeners) {
-          this.$$listeners[name] = namedListeners = [];
+          this.$$listeners[name] = namedListeners = extend([], {$$index: -1});


I think I'll drop this line and use undefined instead of -1 as the "not looping right now" value

Previously the array entry for listeners was set to null but the array size was not trimmed until the event was broadcasted again (see e6966e0). By keeping track of the listener iteration index globally it can be adjusted if a listener removal effects the index. Fixes angular#16135 Closes angular#16293 BREAKING CHANGE: Recursively invoking `$emit` or `$broadcast` with the same event name is no longer supported. This will now throw a `inevt` minErr.

jbedard · 2017-10-31T04:29:23Z

Updated. I think this is ready...

petebacondarwin · 2017-10-31T11:13:20Z

src/ng/rootScope.js

-            }
-          }
+          invokeListeners(scope, event, listenerArgs, name);
+
          //if any listener on the current scope stops propagation, prevent bubbling
          if (stopPropagation) {
            event.currentScope = null;


not related to this PR but why not just do break; here? Since the two lines here are identical to those directly after the loop.

That was bugging me too. I actually originally made that change in this PR but reverted since it isn't related. I'll probably do that once this PR merges...

petebacondarwin · 2017-10-31T11:14:56Z

src/ng/rootScope.js

+      if (listeners) {
+        if (listeners.$$index !== undefined) {
+          throw $rootScopeMinErr('inevt', '{0} already $emit/$broadcast-ing on scope ({1})', name, scope.$id);
+        }


shouldn't the try...finally block wrap this if block too?
Otherwise the listeners.$$index doesn't get cleaned up if the inevt error is thrown.

Or will the original emit/broadcast catch it in its own try...finally?

The original should reset it in its own try...finally

petebacondarwin

Couple of questions

See #16293 (comment) Closes #16339

christopherthielen · 2017-12-04T04:40:51Z

This change breaks a common use case (redirects) for ui-router.

The user wires a $stateChangeStart event listener, checks some conditions, then redirects. To do so, they preventDefault on the event, and start a new transition to a different state, which fires a new $stateChangeStart event.

This all happens in the same digest cycle and triggers the inevt error.

app.run(function($rootScope, $state) {
  $rootScope.$on('$stateChangeStart', function(evt, toState) {
    if (toState.name === 'home.foo' /* e.g., check more conditions */) {
      evt.preventDefault();
      $state.go('other'); // redirect
    }
  })
});

Here's an example: http://plnkr.co/edit/maWQbPvppc0ejlG4QEE2?p=preview

Developers that run into this can work around the problem by wrapping the state.go redirect in a timeout. I understand the reason for this commit, but I'm worried that it will break a lot of apps.

I suspect there might be other libs where this pattern is used (listen for an event, cancel it, fire a new event). I tried writing the same code using ngRoute, but it doesn't show the same behavior somehow.

jbedard · 2017-12-04T04:59:31Z

Thanks @christopherthielen, I always thought we'd find a common case like that, but we never did before merging this one :/

If ng-router still works with this pattern I assume it does the navigation+event async from the go call, but I'm not sure.

My hunch is that we'll have to go with the 1.6 version (358a69f) unless we can think of another way to do this... a stack of indexes instead of one index...?

Narretz · 2017-12-04T12:14:55Z

I'm also in favor of the 1.6 solution, at least as an immediate reaction.

googlebot added the cla: yes label Oct 21, 2017

gkalpak suggested changes Oct 23, 2017

View reviewed changes

gkalpak added component: scopes needs: breaking change severity: memory leak labels Oct 23, 2017

gkalpak added this to the 1.7.0 milestone Oct 23, 2017

petebacondarwin reviewed Oct 23, 2017

View reviewed changes

jbedard force-pushed the 16135-v2 branch 2 times, most recently from 19abca5 to 2c74082 Compare October 26, 2017 04:45

jbedard mentioned this pull request Oct 26, 2017

fix($rootScope): fix potential memory leak when removing scope listeners #16161

Merged

jbedard force-pushed the 16135-v2 branch from 2c74082 to f555340 Compare October 28, 2017 03:58

jbedard commented Oct 28, 2017

View reviewed changes

jbedard force-pushed the 16135-v2 branch from f555340 to cf814e1 Compare October 28, 2017 04:21

jbedard commented Oct 28, 2017

View reviewed changes

jbedard force-pushed the 16135-v2 branch from cf814e1 to 2a0eb2c Compare October 31, 2017 04:27

petebacondarwin reviewed Oct 31, 2017

View reviewed changes

gkalpak approved these changes Oct 31, 2017

View reviewed changes

petebacondarwin approved these changes Oct 31, 2017

View reviewed changes

petebacondarwin added the needs: merging label Oct 31, 2017

jbedard merged commit 817ac56 into angular:master Nov 1, 2017

jbedard mentioned this pull request Nov 22, 2017

refactor($rootScope): simplify $emit stopPropagation handling #16339

Merged

Narretz pushed a commit that referenced this pull request Nov 24, 2017

refactor($rootScope): simplify $emit stopPropagation handling

5838017

See #16293 (comment) Closes #16339

Narretz pushed a commit that referenced this pull request Nov 24, 2017

refactor($rootScope): simplify $emit stopPropagation handling

6c4d270

See #16293 (comment) Closes #16339

jbedard mentioned this pull request Dec 5, 2017

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($rootScope): fix potential memory leak when removing scope listeners (v2) #16293

fix($rootScope): fix potential memory leak when removing scope listeners (v2) #16293

jbedard commented Oct 21, 2017

gkalpak left a comment

gkalpak Oct 23, 2017

gkalpak Oct 23, 2017

jbedard Oct 23, 2017

petebacondarwin Oct 23, 2017

jbedard Oct 28, 2017

gkalpak Oct 23, 2017

petebacondarwin Oct 23, 2017

petebacondarwin Oct 23, 2017

gkalpak Oct 23, 2017

petebacondarwin Oct 23, 2017

petebacondarwin Oct 23, 2017

jbedard Oct 28, 2017

petebacondarwin Oct 23, 2017

petebacondarwin Oct 23, 2017

petebacondarwin Oct 23, 2017

petebacondarwin Oct 23, 2017

jbedard commented Oct 28, 2017

jbedard Oct 28, 2017

jbedard Oct 28, 2017

petebacondarwin Oct 30, 2017

jbedard commented Oct 31, 2017

petebacondarwin Oct 31, 2017

jbedard Oct 31, 2017

petebacondarwin Oct 31, 2017

petebacondarwin Oct 31, 2017

jbedard Oct 31, 2017

petebacondarwin left a comment

christopherthielen commented Dec 4, 2017 •

edited

Loading

jbedard commented Dec 4, 2017 •

edited by Narretz

Loading

Narretz commented Dec 4, 2017



		it('should call next listener when removing current', inject(function($rootScope) {
		var listener1 = jasmine.createSpy().and.callFake(function() { remove1(); });

		}));


		it('should call next listener when removing current', inject(function($rootScope) {

		}));


		it('should call all listeners when removing previous', inject(function($rootScope) {

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

fix($rootScope): fix potential memory leak when removing scope listeners (v2) #16293

fix($rootScope): fix potential memory leak when removing scope listeners (v2) #16293

Conversation

jbedard commented Oct 21, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Oct 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

christopherthielen commented Dec 4, 2017 • edited Loading

jbedard commented Dec 4, 2017 • edited by Narretz Loading

Narretz commented Dec 4, 2017

christopherthielen commented Dec 4, 2017 •

edited

Loading

jbedard commented Dec 4, 2017 •

edited by Narretz

Loading