Event listener leak

[hally9k]

Do you want to request a feature or report a bug?
bug

What is the current behavior?
Accumulating listeners after interacting with input elements and then un-mounting them. If the elements are not interacted with the listener is garbage collected correctly.

Here is a screenshot of the accumulation in a production build:

If the current behaviour is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

Use either of the following repros and follow the reproduction instructions in the comments/readme.

For convenience it is reproduced here:
https://codesandbox.io/s/317j2lz8l6

But it is better illustrated by pulling this repro as there is less going on:
https://github.com/hally9k/listener-leak-test

What is the expected behavior?
No accumulation of event listeners.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
macOS@10.13.4
Chrome@66.0.3359.181
react@16.4
react-dom@16.4

[whs-dot-hk]

@hally9k Why are you doing this?

https://github.com/hally9k/listener-leak-test/blob/8d71aa0a33508b7e3ce020c80d891fd87dc2320e/src/index.js#L26

Puting

() => ({ edit: !this.state.edit })

inside setState()

The above happened because of this

https://reactjs.org/docs/faq-state.html#what-is-the-difference-between-passing-an-object-or-a-function-in-setstate

[whs-dot-hk]

Is this fix ok?

hally9k/listener-leak-test#2

[hally9k]

@whs-dot-hk I have changed the setState calls from:
() => ({ edit: !this.state.edit })
to:
prevState => ({ edit: !prevState.edit })
As I realise I shouldn't have been referencing this.state directly from the function.

Otherwise there isn't anything wrong with that piece of code as far as I can tell. My understanding is that if you want to reference the component's internal state in a setState call then a function must be used.

This is unsafe:

this.setState({
   edit: !this.state.edit
})

I pulled your changes and it doesn't stop the leak, there are instructions on the repo explaining how to reproduce the leak in chrome dev tools. Have a go at testing if your fix works and note the accumulation of listeners in the performance chart.

[hally9k]

I had a first read of the source yesterday. I think I found where the event listeners are getting added but failed to find the code that tidies them up.

Here is where I assume the listeners for events that do not bubble up are added to elements. Apologies if I'm way off, first timer here. 😬

react/packages/react-dom/src/client/ReactDOMFiberComponent.js

Line 873 in 65ab536

trapBubbledEvent(TOP_INVALID, domElement);

react/packages/react-dom/src/events/ReactDOMEventListener.js

Line 148 in 65ab536

addEventBubbleListener(

react/packages/react-dom/src/events/EventListener.js

Line 15 in 65ab536

element.addEventListener(eventType, listener, false);

If anyone could help show me where these listeners are removed when the component is unmounted that would be awesome.

[gaearon]

These event listeners aren't explicitly removed. GC will do that when nothing references the DOM element. If something keeps a reference then it sounds like a bug. Generally React is expected to clean up a reference to something that was deleted during the next update.

[gaearon]

@hally9k Want to look into what's holding onto the element?

[hally9k]

@gaearon Sure I can have a look, would you mark this as a "Good beginner bug" though? I literally started reading the source yesterday. Either way I'll get stuck in and see how we go.

[nhunzaker]

@hally9k Happy to help answer any questions there. Please don't hesitate to reach out!

[aweary]

@hally9k are you profiling with any extensions enabled? I tried running your test app locally, and while I do see that the listener count increases over time, they do eventually get GC'd.

[aweary]

The Performance tab is indicating that there's an increase in the Node count, even after GC, but I've taken a few heap snapshots and there's no clear indicator that DOM nodes aren't being cleaned up. The only positive delta is for things like (array)

[BTMPL]

I can see an Detached DOM tree when transitioning between states

this matches the count of new nodes recorded in performance tab.

I've updated my code to be a bit smaller:

{this.state.edit ? <b>lol</b> : <p>view</p>}

in order to eliminate any native listeners on input fields etc.

[aweary]

I think a few detached nodes would be expected, since the Fiber may store a reference to the previous node for a short time. After the next update the node should be dereferenced though. I would expect a constant number of detached DOM nodes.

[BTMPL]

Yeah, that looks to be the case. Multiple state changes after that and the delta between previous snapshot is the same (2).

The only change I see (that is meaningfull) is array and system.

Maybe it's an error in the Chrome profiller? Anyone checked with Firebug?