Memory issues in 3.8

[adamjmcgrath]

We're having memory retention issues upgrading from 3.4 to 3.8. When we use string based event listeners they get retained on the meta of the object that's being listened to. eg

// "3.8.2"
var person = Ember.Object.extend(Ember.Evented, {
  greet() {
    this.trigger('greet');
  }
}).create();
var Foo = { hello() { console.log('hello') }  }
person.on('greet', Foo, 'hello')
person.greet()
// hello
Ember.meta(person)._listeners
 //  (4) ["greet", {…}, "hello", false]
person.off('greet', Foo, 'hello')
Ember.meta(person)._listeners
// [{event: "greet", target: {…}, method: "hello", kind: 2}] '_listeners' still has the event

After person.off('greet', Foo, 'hello') we would expect Ember.meta(person)._listeners to be an empty array - this is the behaviour we see in 3.4 and is also the behaviour we see when we use function listeners:

person.on('greet', Foo, Foo.hello)
Ember.meta(person)._listeners
 //  (4) ["greet", {…}, "hello", false]
person.off('greet', Foo, Foo.hello)
Ember.meta(person)._listeners
// [] An empty array

It looks like the root cause is in a change that was added to retain these events, and instead give them a kind property of ListenerFlags.REMOVE (in https://github.com/emberjs/ember.js/pull/16874/files#diff-1c610ca34cd0e59afd9814e4e3b92806R471).

A later change was added to remove function listeners all together - presumably to fix issues with function listeners being retained (in 7164bd6), but this was not done for string listeners.

This behaviour is causing large memory retention issues in our application and stopping us from upgrading. Should string listeners be cleaned up as well or should we change these all to function listeners to avoid the retention issues?

Have also tested this in 3.10 and see the same issue.

[adamjmcgrath]

I can also reproduce the same memory leak issue on https://try.discourse.org (they're on 3.8.0)

Detached HTML elements are being retained when toggling between the 'Top' and 'Categories' routes

[pzuraq]

Hey @adamjmcgrath, I worked on those changes so I think I can give a bit of context here. The refactors were done in order to make listeners lazy, there actually were some pretty serious bugs that could occur by using them in the wrong order, and these bugs were exacerbated by native classes.

Function listeners, in particular, were tricky because inheriting them was hard. It didn't really make sense in most cases either. However, we did not deprecate non-inherited function listeners (e.g. instance listeners). Function listeners will be fully removed too, as compared to string listeners, so you should be able to use switch to them to solve your problem in the short term.

In the longer term, I'm not sure if it's possible for us to remove string listeners on instances either. There are some funky edge cases where people may be using them for inheritance on POJOs, which was the reason we didn't before I believe. I'll talk with @krisselden about it to try to think through it again and figure out why we made that decision.

Also, I do recommend in general moving away from listeners. They don't exist on Glimmer components and don't work particularly well with native classes in general (and Ember doesn't ship decorators for them) so I think minimizing their use would be a good idea.

[adamjmcgrath]

Hi @pzuraq, thanks for the explanation. It feels like bit of a gotcha that string listeners are removed lazily and function listeners aren't - albeit only from a performance standpoint. That said, it shouldn't be too hard for us to move away from listeners or at least change a few to function listeners in the interim.

FWIW, I noticed that LinkedIn also has the lazy removeListeners change in their version of Ember and I see detached HTML Elements being retained when I toggle to and from the Profile page.

[krisselden]

The intention was string listeners is what should be used for inheritance on prototypes and they should point to methods not functions with non static state and on instances function listeners added/removed should be removed. If the latter is not happening then it is a bug.

[krisselden]

Basically the only case that should leak is if you add a listener to a prototype that closes over instance state but there is no reasonable use case for that, inheriting listeners should only be for methods and use strings. All add/remove of listeners to a non-prototype should not leak memory and it is a bug if they do.