DOMRenderer listener leaks

[jd-carroll]

Currently there is no facility which removes listeners from the actual DOM element. When we subscribe to a DOM event, the DOMRenderer will add a listener by:

        // FIXME Add to content DIV if available
        var target = eventMap[type][1] ? this._root : this._target;
        target.listeners[type] = this._boundTriggerEvent;
        target.element.addEventListener(type, this._boundTriggerEvent);

If the associated node is removed/dismounted and the underlying DOM element is freed, we will never stop listening to the subscribed events on that element.

[alexanderGugel]

1. Most of the time we don't even add an event listener to the element (event delegation is being used pretty much everywhere).
2. We control if the events are being "catched" via the subscribe object in ElementCache (see https://github.com/Famous/engine/blob/master/dom-renderers/DOMRenderer.js#L111).
3. Because of 2. we never need to remove the event listener to stop listening for events.
4. Dismounting nodes currently only hides the ElementCache. It doesn't actually remove it from the DOM, but it gets reused if an element is being inserted at the same path at a later point in time.
5. The cleanup logic in DOMElement needs to handle this (onDismount). Currently it doesn't. Should be fixed in fix: Refactor DOMElement #266. Let me check if the problem persists there.

[jd-carroll]

@alexanderGugel I'm not sure I follow everything you said: (stated in the form of questions)

1. Most of the time we don't even add an event listener to the element (event delegation is being used pretty much everywhere).

Correct, most elements will not have an event listener, but any where we call .addUIEvent an event listener will always be added to the element

2 We control if the events are being "catched" via the subscribe object in ElementCache (see https://github.com/Famous/engine/blob/master/dom-renderers/DOMRenderer.js#L111).

Understood, but there isn't a unsubscribe

3 Because of 2. we never need to remove the event listener to stop listening for events.

You lost me... As long as the element stays associated with the same Node this will be the case. However, as soon as it is associated with a different Node those listeners will need to be removed. Or are you saying you will just keep them there and not publish those DOM event to the Node (if so sounds inefficient)

4 Dismounting nodes currently only hides the ElementCache. It doesn't actually remove it from the DOM, but it gets reused if an element is being inserted at the same path at a later point in time.

This is only relative to the level, and has no relation to which parent it is under.. ?

5 The cleanup logic in DOMElement needs to handle this (onDismount). Currently it doesn't. Should be fixed in #266. Let me check if the problem persists there.

The logic to remove DOM event listeners?

I probably made this sound a little more catastrophic than I needed and will correct that. Was simply trying to say that the listeners are never removed so if the DOM element was ever destroyed it wouldn't be GC'd because of the listener references.

[alexanderGugel]

Correct, most elements will not have an event listener, but any where we call .addUIEvent an event listener will always be added to the element

No. We use event delegation in most cases. The event listener is only being added once per event type. E.g. if I add an event listener for "click" to node1 and node2, there will only be one listener.

Understood, but there isn't a unsubscribe

• Node#onRemoveUIEvent
• DOMRenderer#unsubscribe
• Node#removeUIEvent

Needs to be merged in: https://github.com/Famous/engine/pull/318/files

You lost me... As long as the element stays associated with the same Node this will be the case. However, as soon as it is associated with a different Node those listeners will need to be removed. Or are you saying you will just keep them there and not publish those DOM event to the Node (if so sounds inefficient)

We are using event delegation. Listening for a new event doesn't necessarily mean adding a new event listener. If the event bubbles, we simply catch it as it bubbles up to the root element, where there is only one event listener per type. What you say is only true for non-bubbling events (such as mouseenter).

5 The cleanup logic in DOMElement needs to handle this (onDismount). Currently it doesn't. Should be fixed in #266. Let me check if the problem persists there.

The general logic needed for cleaning up the DOMElement, including - but not limited to - removing event listeners that have been attached to the element.

I probably made this sound a little more catastrophic than I needed and will correct that. Was simply trying to say that the listeners are never removed so if the DOM element was ever destroyed it wouldn't be GC'd because of the listener references.

I probably made this sound a little more catastrophic.

You raised a couple of good points that are worth being discussed. Currently the documentation on how we are actually attaching event listeners, why we do it (performance, GC) etc. is... non existent. I apologize for any confusion arising from that. I'm gonna write a blog post or something about that.

AFAIK this is only an issue in ancient versions of IE. If we would remove them from the DOM, we probably also wouldn't have to clean them up.

I will elaborate on my points on Monday. I just wanted to give a quick response.

[jd-carroll]

@alexanderGugel the article you shared is actually really good. I did not realize that javascript only counts the references to the object, and not the references from the object, before collecting.

Closing as you are correct even if the element is removed, any listener reference would not prevent the collection.