Using actions on elements triggered by events piles up listeners

[averydev]

Hi there,

I've found that when elements are created and removed, for instance in an {{if}} statement, and include actions paired with native events, those events pile up (at least in chrome).

If this is happening with a lot of elements at once, for instance showing a list of 200 items, each with 3 inputs and 2 buttons, this can happen quickly, and cause memory problems.

You can also see the heap grow and never decrease on the example below of the "onClick" button.

Ember Twiddle:
https://ember-twiddle.com/fb52fe588830908c7b12a0a396574434?fileTreeShown=false&numColumns=0

Screencast:
https://monosnap.com/file/iWoH8QfcMdDPJTPtpaAq3vmTrt5t5o

In particular the result observed in the performance > memory > listener panel of chrome is quite different between these two:
<button onClick={{action "click"}}>Button With "onClick" Action</button>
and
<button {{action "click"}}>Button w/ simple action</button>
@locks suggested I mention @stefanpenner and @krisselden

Thanks!

[pixelhandler]

@averydev thanks for reporting the issue and providing a reproduction of the issue.

Summary:

Conditional display of element with click handlers will result in creating and destroying elements with their associated handlers.

Though we can use onclick to attach an action handler to a DOM attribute of an element, that event handler will not be torn down upon removal of the element (though the onclick attr will not appear in the DOM) ...

<button onclick={{action "click"}}>Button With "onclick" Action</button>

However using an action not attached to a DOM attribute of an element will not create additional handlers each time the condition is met in order to render an element into the DOM...

<button {{action "click"}}>Button w/ simple action</button>

The difference is that the latter does not attach a function to the DOM element's onclick attribute.

It appears that there is no way to teardown the event handler attached to the onclick attribute. In short, we should not use that style of attaching an event for items that will be destroyed and recreated (e.g. in a long list).

I think that the traditional use of the action (not attaching to onclick DOM attribute) only registers a callback for events that are listented to using "event delegation". Thus, not adding extra event handlers :)

I'm not sure if there is a solution for automatic teardown of onclick function handlers when elements are removed due to a condition in a template.

There are other gotchas with using onclick as well. I think there is a work around. We can avoid use of onclick for elements that need to be created and destroyed often.

The primary benefit from using onclick is to recieve the event as an argument in the action handler.
From the documentation:

To utilize an event object as a function parameter:
Define the event handler in the component (which is designed to receive the browser event object).
Or, assign an action to an inline event handler in the template (which creates a closure action and does receive the event object as an argument).

See https://guides.emberjs.com/v2.16.0/components/handling-events/

Original implementation: If you need to receive the event as an argument use a component and define the event handler in the component, instead of using an action assigned to the onclick attribute.

import Component from '@ember/component';
export default Component.extend({
  click(evt) {
    // This receives the event and will be torn down
  }
});

The above should avoid the leak of bound event handlers, and the below should be used with caution.

import Controller from '@ember/controller';
export default Controller.extend({
  actions: {
    onClick(evt) {
      // receives the event only using `onClick=(action 'onClick')` in the .hbs template but the element's handler lives on even after removing it from the DOM
    }
  }
});

[pixelhandler]

I'm inclined to think that this is not a bug. The Ember framework provides various ways to listen to events and they have their trade-offs. Using a component with an event handler will setup and teardown properly.

[pixelhandler]

Also mentioned is that onClick should be onclick I forked the twiddle example, https://ember-twiddle.com/d371f2880f4d3e5c5096821f72eaadd8?fileTreeShown=false&openFiles=controllers.application.js%2C

But the result is the same regarding adding handlers to the element's attribute.

[pixelhandler]

@averydev I updated the forked twiddle to show using a component's event handler, see https://ember-twiddle.com/d371f2880f4d3e5c5096821f72eaadd8?fileTreeShown=false&openFiles=templates.application.hbs%2Ctemplates.components.x-button.hbs

{{#x-button onClick=(action "toggleXButton")}}X-Button Component{{/x-button}}

import Component from '@ember/component';
export default Component.extend({
  tagName: 'button',
  click(evt) {
    console.log('x-button click', evt); // will be torn down
    this.get('onClick')(evt);
  }
});

[averydev]

Hi there @pixelhandler thanks for looking into this!

The good thing here is that there are plenty of good alternatives to <button onclick={{action 'doSomething'}}> that are likely to be perfect alternatives without the listener downside.

In particular, while re-reading the docs on {{action}} i noticed the following option for the first time: <button {{action "doSomething" on="onclick"}}>

I forked the twiddle again here: https://ember-twiddle.com/00c059ecd222d5312fcb3188a06e71bd?openFiles=templates.application.hbs%2C
and the result of a quick test is that it cleans up after itself nicely. Great!

After thinking about it a bit, i'm not surprised that the onclick={{action}} option doesn't clean up after itself given the output html:

The reason that i would characterize this as a bug is for the following reasons:

This falls into a "Use the framework" type of feature, and it is prevalent throughout the docs. As such, I personally expected it to not leak listeners or memory without additional effort, when in fact it does so.

This formulation is found in docs throughout Ember... In fact the first two examples in the docs for the Action Helper show it in the form onclick={{action ...}}

It is also widely found in addon docs, for instance here's ember-concurrency:

Given the above, I doubt that many Ember developers think that the onclick={{action}} has a downside.

It is a particularly common formulation because it is more explicit than button {{action 'doSomething'}} and seems less magical, which is not a bad thing on its own.

Also, the same listener issue occurs regardless of if you're using a button, li, div etc. Particularly if you need something like mouseenter={{action "doSomething}} mouseleave={{action "somethingElse" or <input onchange={{action "doSomething"}}>

Also, you don't need to be creating and destroying thousands of items for this to be an issue. The reason that I stumbled on this issue was that prolonged use of the app i'm working on currently was gobbling up tremendous amounts of memory to the point that prolonged usage would grind the app to a halt. The reason for this I think is that even a single listener that hasn't been cleaned up will be a reference to the parent leading to detached DOM trees that can't be garbage collected.
The culprit it turned out was a search field that would filter results as the user typed, and there were listeners in the table rows.

I could be wrong, but I'm pretty sure that's what's going on here.

[averydev]

Also, there isn't anything specific about {{#if}} that causes this issue, it would be the same if for any reason an element was removed from the template that had had an onclick={{action...}} for instance:

//.js
results: computed('query', function (){
      const query = this.get('query');
      return this.get('items').filter( d => d.get('field').indexOf(query) !== -1);
}
//.hbs
{{#each results as |result|}}
       <div mousenter={{action 'resultHovered' result}};> {{result.title}}</div>
{{/each}}

[rtablada]

@averydev the if statement just makes it easier to add and remove elements.

[averydev]

@rtablada yep! Just confirming and isolating that the bug exists regardless of the element being removed from the DOM as the result of a change to the evaluation of a conditional in the template.

[stefanpenner]

It appears that there is no way to teardown the event handler attached to the onclick attribute. In short, we should not use that style of attaching an event for items that will be destroyed and recreated (e.g. in a long list).

The browser cleans these up for us. Once an element who had an handler attached, is both:

• detached from the DOM
• unreachable from JavaScript

That elements DOM object and JS Wrapper is collected, at that point the browser can (and at some point will) collect the attached handler

The good thing here is that there are plenty of good alternatives to <button onclick={{action 'doSomething'}}> that are likely to be perfect alternatives without the listener downside.

<button onclick={{action 'doSomething'}}>

There should be no listener downside with ^, in-fact I would strongly encourage the direct property listeners when possible.

[averydev]

I just ran through the examples again, and Chrome seems to have improved GC of these abandoned listeners since this bug was reported! Whew! Previously I've seen abandoned listeners last seemingly forever, but leaving it alone for ~10s was enough to have them GC'd.

I've been using the syntax of <button {{action "doSomething" on="onclick"}}> when lots of listeners are likely to be added/removed, but that is more of a blog post than a bug...

At this point I'd recommend closing this.

[krisselden]

@averydev the memory growth chart can climb simply because there is no GC pressure that does not mean there is a leak. I just did a comparison heap snapshot between a snapshot after toggling on/off 4 times and toggling on/off 100 times, there is no leak of the closure action or dom that was removed.