React: Password field causes memory leak in production builds

Created on 21 Mar 2018 · 13Comments · Source: facebook/react

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

What is the current behavior?
We're noticing that password-type input fields (as well as their wrapping parents) remain as detached DOM elements when conditionally being rendered and removed from the DOM.

This issue seems to be occurring across several applications using React 16, include a newly created app from Create React App, and we're able to produce Chrome memory heap snapshots in all of these environments to demonstrate this issue.

Observe in the following example, the button adds/removes the input field to the DOM. To reproduce the problem stated in this issue, repeatedly click this button (say 20-30 times), then take a memory heap snapshot. You will see multiple detached DOM elements, the password input field and its wrapping div. Note that this issue does not happen for other types of input fields.

class App extends Component {
  state = { show: false };

  toggle = () => this.setState({show: !this.state.show})

  render() {
    return (
      <div>
        <button onClick={this.toggle}>Toggle password field</button>
        {this.state.show && (
          <div>
            <input type="password"/>
          </div>
       )}
       </div>
    );
  }
}

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
We have tried this issue in React 16.2, 16.1 and React 15.6. Chrome and Canary were the browsers tested in particular.

Needs Investigation

Source

dcodus

👍1

Most helpful comment

Here is a similar vanilla example. I can't see any detached element in the memory snapshot:

<!DOCTYPE html>
<html>
<body>
  <button id="add10">Add 10 input</button>
  <button id="add1000">Add 1000 input</button>
  <button id="add10000">Add 10000 input</button>
  <button id="clear">Clear</button>
  <div id="content"></div>
  <script>
    const content = document.getElementById('content');
    const add = n => () => {
      console.log('click');
      for(let i = 0; i < n; i++){
        const input = document.createElement('input');
        input.setAttribute('type', 'password');
        content.appendChild(input);
      }
    }
    document.getElementById('add10').addEventListener('click', add(10));
    document.getElementById('add1000').addEventListener('click', add(1000));
    document.getElementById('add10000').addEventListener('click', add(10000));
    document.getElementById('clear').addEventListener('click', () => {
      content.innerHTML = '';
    });
  </script>
</body>
</html>

testerez on 22 Mar 2018

👍3

All 13 comments

Interesting! I wonder what's going on. Do you know if this holds true for Firefox or another browser?

To my knowledge, password inputs do not receive any special treatment:

https://github.com/facebook/react/search?utf8=%E2%9C%93&q=password&type=

@aweary or @jquense can you think of anything?

nhunzaker on 22 Mar 2018

@nhunzaker unfortunately i did not fid a way to check for detached dom nodes in firefox or safari 😞
FYI we ran the tests in incognito, autofill disabled and all extensions disabled.

dcodus on 22 Mar 2018

Have you tried a similar test without React?

gaearon on 22 Mar 2018

Here is a similar vanilla example. I can't see any detached element in the memory snapshot:

<!DOCTYPE html>
<html>
<body>
  <button id="add10">Add 10 input</button>
  <button id="add1000">Add 1000 input</button>
  <button id="add10000">Add 10000 input</button>
  <button id="clear">Clear</button>
  <div id="content"></div>
  <script>
    const content = document.getElementById('content');
    const add = n => () => {
      console.log('click');
      for(let i = 0; i < n; i++){
        const input = document.createElement('input');
        input.setAttribute('type', 'password');
        content.appendChild(input);
      }
    }
    document.getElementById('add10').addEventListener('click', add(10));
    document.getElementById('add1000').addEventListener('click', add(1000));
    document.getElementById('add10000').addEventListener('click', add(10000));
    document.getElementById('clear').addEventListener('click', () => {
      content.innerHTML = '';
    });
  </script>
</body>
</html>

testerez on 22 Mar 2018

👍3

I've tried debugging this a little bit, and I don't see any problems with password inputs specifically. Using the example posted, there are a few detached nodes that are referenced from the Fiber instance.

For example, the Fiber for the App component references an old fiber for a div element through lastEffect, and that fiber's stateNode is a div that is no longer in the document. I suspect that effect will eventually be cleared and the reference dropped, but I'm not sure. @gaearon?

I don't see any behavior that would indicate a memory leak though. Taking a profile and performing GC, it looks like the detached nodes eventually get cleaned up.

But it's possible it could be more efficient about storing references to detached elements.

aweary on 22 Mar 2018

👍1

My understanding is that we maintain a reference to detached tree for one more update after it's been removed. On the next update the old node should get cleared. Does that help?

https://github.com/facebook/react/blob/6b99c6f9d376bacbb769264d743c405b495b03ad/packages/react-reconciler/src/ReactFiberCommitWork.js#L494-L498

gaearon on 2 Apr 2018

😕2 👍2

Thanks @gaearon. @dcodus can you verify if this is what you're seeing? A few detached nodes is expected, but they should be GC'd. Can you verify whether there's actually a memory leak by profiling memory over time in the Performance tab?

aweary on 2 Apr 2018

A few detached nodes is expected, but they should be GC'd.

That's not quite accurate. What I'm saying is that the nodes that were unmounted during previous update won't be GC'd until the next update.

So as user interacts with the app, eventually all detached nodes get GC'd but it happens on next update rather than on the update that caused them to unmount.

gaearon on 2 Apr 2018

👍1

Right, that's what I'm saying. I should have been clearer: they should be GC'd _eventually_, assuming an update is processed. Meaning that there shouldn't be a memory leak, since there should be some constant number of detached nodes that get GC'd as the application updates.

aweary on 2 Apr 2018

👍1

I decided to take a crack at this. From what I can tell, React is doing exactly what we'd expect it to do.

I setup a scenario in which the the memory leak should be very pronounced if it exists:

class App extends Component {
  state = {
    toggling: false,
    show: false
  };

  togglingOn = () => this.setState({ toggling: true });
  togglingOff = () => this.setState({ toggling: false });

  render() {
    if (this.state.toggling) {
      setTimeout(() => this.setState({show: !this.state.show}), 18);
    }
    return (
      <div>
        <button onClick={this.togglingOn}>Start toggling</button>
        <button onClick={this.togglingOff}>Stop toggling</button>
        {this.state.show && (
          <div>
            <input type="password" placeholder="password!" />
          </div>
       )}
       </div>
    );
  }
}

Letting this run in Chrome for a bit, I watched the Performance tab for allocations and garbage collections. Things there look about like you'd expect for an eventual garbage collection.

screenshot 2018-05-02 14 50 16

And

screenshot 2018-05-02 14 52 29

These each represent about 60 seconds of self-toggling every 18ms. You can see the GC eventually catching up and reaping the detached nodes. I ran this a few times with the same result. Also, it didn't seem to matter whether it was a text input or a password.

jacksongabbard on 2 May 2018

Seems like there's no leak then.

gaearon on 2 Aug 2018

Calling setState in render to avoid this smells really bad. Don't we have a way to tell react to GC the removed child without using 2-step render? @gaearon

alivedise on 12 Sep 2018

I don’t think anyone suggests actually calling setState in render 🙂

I don’t understand the problem you’re having, @alivedise. If it gets cleared on next update what difference does it make to you? Can you explain a realistic scenario where this matters?

gaearon on 12 Sep 2018

Was this page helpful?

0 / 5 - 0 ratings