Password field causes memory leak in production builds #12420

dcodrin · 2018-03-21T21:13:31Z

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.

The text was updated successfully, but these errors were encountered:

nhunzaker · 2018-03-22T11:51:51Z

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?

dcodrin · 2018-03-22T13:58:04Z

@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.

gaearon · 2018-03-22T18:04:32Z

Have you tried a similar test without React?

testerez · 2018-03-22T20:24:56Z

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>

aweary · 2018-03-22T22:58:27Z

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.

gaearon · 2018-04-02T15:14:21Z

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?

react/packages/react-reconciler/src/ReactFiberCommitWork.js

Lines 494 to 498 in 6b99c6f

    
           // Cut off the return pointers to disconnect it from the tree. Ideally, we 
        
           // should clear the child pointer of the parent alternate to let this 
        
           // get GC:ed but we don't know which for sure which parent is the current 
        
           // one so we'll settle for GC:ing the subtree of this child. This child 
        
           // itself will be GC:ed when the parent updates the next time.

aweary · 2018-04-02T17:03:01Z

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?

gaearon · 2018-04-02T17:22:37Z

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.

aweary · 2018-04-02T17:27:13Z

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.

jacksongabbard · 2018-05-02T18:59:11Z

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.

And

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.

gaearon · 2018-08-02T19:39:41Z

Seems like there's no leak then.

alivedise · 2018-09-12T09:47:55Z

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

gaearon · 2018-09-12T12:07:26Z

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?

aweary added Resolution: Needs More Information Type: Needs Investigation and removed Resolution: Needs More Information labels Mar 27, 2018

aweary mentioned this issue Apr 2, 2018

Fix IE9 Undo Bug #12505

Closed

gaearon closed this as completed Aug 2, 2018

levinqdl mentioned this issue Nov 20, 2018

Input of password type causes memory leak #14284

Closed

gaearon mentioned this issue Jan 18, 2019

componentDidMount inside a class causing a memory leak #14630

Closed

gaearon mentioned this issue Feb 8, 2019

Memory leak on React > 16.2.5 #14732

Closed

richie-south mentioned this issue Feb 21, 2019

Old references retained by memoizedProps in fiber #14380

Closed

hamlim mentioned this issue Feb 27, 2019

Password input type causes memory leak #14962

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Password field causes memory leak in production builds #12420

Password field causes memory leak in production builds #12420

dcodrin commented Mar 21, 2018

nhunzaker commented Mar 22, 2018

dcodrin commented Mar 22, 2018 •

edited

Loading

gaearon commented Mar 22, 2018

testerez commented Mar 22, 2018

aweary commented Mar 22, 2018

gaearon commented Apr 2, 2018

aweary commented Apr 2, 2018

gaearon commented Apr 2, 2018 •

edited

Loading

aweary commented Apr 2, 2018

jacksongabbard commented May 2, 2018

gaearon commented Aug 2, 2018

alivedise commented Sep 12, 2018

gaearon commented Sep 12, 2018

Password field causes memory leak in production builds #12420

Password field causes memory leak in production builds #12420

Comments

dcodrin commented Mar 21, 2018

nhunzaker commented Mar 22, 2018

dcodrin commented Mar 22, 2018 • edited Loading

gaearon commented Mar 22, 2018

testerez commented Mar 22, 2018

aweary commented Mar 22, 2018

gaearon commented Apr 2, 2018

aweary commented Apr 2, 2018

gaearon commented Apr 2, 2018 • edited Loading

aweary commented Apr 2, 2018

jacksongabbard commented May 2, 2018

gaearon commented Aug 2, 2018

alivedise commented Sep 12, 2018

gaearon commented Sep 12, 2018

dcodrin commented Mar 22, 2018 •

edited

Loading

gaearon commented Apr 2, 2018 •

edited

Loading