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

jbedard · 2017-08-10T03:36:33Z

This attempts to fix #16135, but is assuming that browsers will use some type of memory efficient array implementation for sparse arrays (which they appear to do, see 1, 2, 3). In the worst case this ~~should have no change~~ may cause some extra swapping of array implementations and/or not fix the bug, in the best case it will be more memory efficient in this edge case.

Also added a couple tests that show why we don't simply use splice.

Thanks @jvilk for investigating this delete solution.

Narretz · 2017-08-10T10:36:30Z

The errors look weird. An incompatibility between yarn and dgeni-packages? Strange because I don't think we changed anything.

gkalpak

A minor suggestion, but LGTM 👍

gkalpak · 2017-08-10T16:54:39Z

src/ng/rootScope.js

@@ -1180,7 +1180,10 @@ function $RootScopeProvider() {
        return function() {
          var indexOfListener = namedListeners.indexOf(listener);
          if (indexOfListener !== -1) {
-            namedListeners[indexOfListener] = null;
+            // Use delete in the hope of the browser deallocating the memory for the array entry,


...in the hope of the browser deallocating...

Why wouldn't it? 😁

Because it is implementation dependent, not part of any spec etc.

I think there should be a more descriptive comment here, e.g. "depending on browser implementations, delete should deallocate the memory"

I'm not sure if it "should" though. Is there anything that actually says it should? I think browsers (some? all?) do this just to be more efficient. I kind of like the "in the hope of" so I'm having trouble finding a better description...

Did we ever find a conclusive answer to the actual implementation in specific browsers? Or is this based on benchmarks? Either info could be listed here and then maybe a note that we assume other browsers do it as well.

Based on benchmarks Chrome and FF seem to both use a more memory efficient array implementation for sparse arrays (which I noted here). But I'm not sure about all browsers, or the details of when Chrome and FF do it, and I'm assuming there is no actual standard for this.

gkalpak · 2017-08-10T16:59:14Z

test/ng/rootScopeSpec.js

+          $rootScope.$broadcast('abc');
+          expect(listener2).toHaveBeenCalled();
+          expect(listener3).toHaveBeenCalled();
+        }));


You could run a second round (it this and the subsequent tests) to make sure the listener will be called correctly and nothing will break after the initial run. E.g.:

listener2.calls.reset(); listener3.calls.reset(); $rootScope.$broadcast('abc'); expect(listener2).toHaveBeenCalled(); expect(listener3).toHaveBeenCalled();

I've added this to all 3 tests

Narretz · 2017-09-06T14:32:36Z

@jbedard can you please rebase this? The tests currently don't run because of a Travis problem at the branch point.

jbedard · 2017-09-10T01:39:53Z

Rebased. Adjusted the tests a bit like @gkalpak suggested.

jbedard · 2017-09-17T21:23:37Z

Another good read about array storage/optimizations: https://www.html5rocks.com/en/tutorials/speed/v8/#toc-topic-numbers

Again v8 specific, but this confirms what we've been talking about (and what @jvilk measured) with array vs dictionary versions of an array. Using delete could cause a switch to the dictionary implementation which forces memory reallocation/copying, but then (for sparse arrays) it will use less memory.

jbedard · 2017-10-14T23:20:58Z

test/ng/rootScopeSpec.js

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


I just realized there are two similar tests to this one where the current listener gets removed for $emit and $broadcast (e6966e0) under describe('$emit'. Maybe those two should move here to replace this one? Or just keep this extra one?

jbedard · 2017-10-15T01:35:54Z

Actually... I came up with an alternative that might be better, essentially doing the same as $$digestWatchIndex. Basically making the $$listeners iteration index global (per event type per scope) so it can be adjusted, if necessary, when a listener is removed. The only constraint is this means you can not iterator over $$listeners recursively which I think is already a restriction due to the prevoius array cleanup that essentially had the same restriction.

See jbedard@68247ee - WDYT?

Narretz · 2017-10-16T08:55:16Z

The only constraint is this means you can not iterator over $$listeners recursively

You mean iterate over $$listeners and modify while you iterate over them? That sounds like an acceptable tradeoff.

jbedard · 2017-10-16T18:22:17Z

I think it would be recursive $emit or $broadcast calls of the same event name. I'm pretty sure this was already broken in some cases, but this (storing the iteration index on the array) would break it in all cases.

Narretz · 2017-10-17T12:53:56Z

Since I don't really use events in AngularJs, I can't say if there are valid use cases for recursive emission and broadcasting. It's probably a BC for someone ...

gkalpak · 2017-10-17T13:28:04Z

We decided to land this PR as is for 1.6.0 (to avoid any breaking changes and still avoid the memory leak).
For 1.7.0, we will implement something like jbedard/angular.js@68247ee (which avoids making iterations slower as the array grows in length) with an explicit check to prevent nested listener iterations and a breaking change notice.

Narretz · 2017-10-17T14:49:02Z

SGTM

Narretz · 2017-10-23T09:51:18Z

Don't forget to change the branch target to v1.6.x for this one

jbedard · 2017-10-26T04:56:30Z

I've rebased this onto v1.6.x. Also updated the first commit to include the changes from #16293.

gkalpak

Still LGTM 😃

When removing listeners they are removed from the array but the array size is not changed until the event is fired again. If the event is never fired but listeners are added/removed then the array will continue growing. By changing the listener removal to `delete` the array entry instead of setting it to `null` browsers can potentially deallocate the memory for the entry. Fixes angular#16135 Closes angular#16161

jbedard · 2017-10-28T03:59:30Z

I've merged this to v1.6.x and cherry-picked the first commit (extra tests, not specifically for this bug) into master.

When removing listeners they are removed from the array but the array size is not changed until the event is fired again. If the event is never fired but listeners are added/removed then the array will continue growing. By changing the listener removal to `delete` the array entry instead of setting it to `null` browsers can potentially deallocate the memory for the entry. Fixes angular#16135 Closes angular#16161

When removing listeners they are removed from the array but the array size is not changed until the event is fired again. If the event is never fired but listeners are added/removed then the array will continue growing. By changing the listener removal to `delete` the array entry instead of setting it to `null` browsers can potentially deallocate the memory for the entry. Fixes #16135 Closes #16161

googlebot added the cla: yes label Aug 10, 2017

gkalpak approved these changes Aug 10, 2017

View reviewed changes

jvilk mentioned this pull request Aug 10, 2017

Loomio Frontend Impacted by Minor AngularJS Memory Leak loomio/loomio#4249

Closed

Narretz added this to the 1.6.x milestone Sep 6, 2017

Narretz added the component: scopes label Sep 6, 2017

jbedard force-pushed the 16135 branch 3 times, most recently from 8490e74 to 396566f Compare September 10, 2017 01:32

jbedard mentioned this pull request Oct 8, 2017

Small Memory Leak in $rootScope.$on #16135

Closed

3 tasks

jbedard commented Oct 14, 2017

View reviewed changes

jbedard force-pushed the 16135 branch from 396566f to 824b2a2 Compare October 14, 2017 23:51

jbedard mentioned this pull request Oct 21, 2017

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

Merged

jbedard changed the base branch from master to v1.6.x October 26, 2017 04:54

jbedard force-pushed the 16135 branch from 824b2a2 to ddb5c7d Compare October 26, 2017 04:54

gkalpak approved these changes Oct 26, 2017

View reviewed changes

test($rootScope): test removal of event listeners during event broadcast

d87a4e4

jbedard force-pushed the 16135 branch 2 times, most recently from 7adfbcc to b3023b6 Compare October 27, 2017 16:04

jbedard force-pushed the 16135 branch from b3023b6 to 887f0f4 Compare October 28, 2017 03:26

jbedard force-pushed the 16135 branch from 887f0f4 to 2f2fa7a Compare October 28, 2017 03:33

jbedard merged commit 358a69f into angular:v1.6.x Oct 28, 2017

dependencies bot mentioned this pull request Dec 5, 2017

Update 26 dependencies from npm JetBrains/ring-ui#30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jbedard commented Aug 10, 2017 •

edited

Loading

Narretz commented Aug 10, 2017

gkalpak left a comment

gkalpak Aug 10, 2017

jbedard Aug 10, 2017

Narretz Oct 10, 2017

jbedard Oct 14, 2017

Narretz Oct 14, 2017

jbedard Oct 14, 2017

gkalpak Aug 10, 2017

jbedard Sep 9, 2017

Narretz commented Sep 6, 2017

jbedard commented Sep 10, 2017

jbedard commented Sep 17, 2017

jbedard Oct 14, 2017

jbedard commented Oct 15, 2017

Narretz commented Oct 16, 2017

jbedard commented Oct 16, 2017

Narretz commented Oct 17, 2017

gkalpak commented Oct 17, 2017

Narretz commented Oct 17, 2017

Narretz commented Oct 23, 2017

jbedard commented Oct 26, 2017 •

edited

Loading

gkalpak left a comment

jbedard commented Oct 28, 2017

		}));


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

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

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

Conversation

jbedard commented Aug 10, 2017 • edited Loading

Narretz commented Aug 10, 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

Narretz commented Sep 6, 2017

jbedard commented Sep 10, 2017

jbedard commented Sep 17, 2017

Choose a reason for hiding this comment

jbedard commented Oct 15, 2017

Narretz commented Oct 16, 2017

jbedard commented Oct 16, 2017

Narretz commented Oct 17, 2017

gkalpak commented Oct 17, 2017

Narretz commented Oct 17, 2017

Narretz commented Oct 23, 2017

jbedard commented Oct 26, 2017 • edited Loading

gkalpak left a comment

Choose a reason for hiding this comment

jbedard commented Oct 28, 2017

jbedard commented Aug 10, 2017 •

edited

Loading

jbedard commented Oct 26, 2017 •

edited

Loading