Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggle called from unmounted Tooltips #1255

Closed
fmrl-island opened this issue Oct 8, 2018 · 27 comments · Fixed by #1562 · May be fixed by largerock/largerock.com#5
Closed

Toggle called from unmounted Tooltips #1255

fmrl-island opened this issue Oct 8, 2018 · 27 comments · Fixed by #1562 · May be fixed by largerock/largerock.com#5

Comments

@fmrl-island
Copy link

  • components: UncontrolledTooltip
  • reactstrap version #6.5.0
  • react version #16.3.2

What is happening?

Error message is being printed in the console because setState is being called on an unmounted component (UncontrolledTooltip).

What should be happening?

No error should be printed. The tooltip component methods "show" and "hide" should each check that the component is still mounted before calling "toggle" when they are called via "setTimeout", because "setTimeout" could trigger after the component has already been unmounted. This happens specifically with UncontrolledTooltip because it manages whether or not it is open using setState, however, in practice this could occur on any component which uses a Tooltip.

Steps to reproduce issue

  1. Add UncontrolledTooltip to page
  2. Cause tooltip to show
  3. Perform an action which causes the tooltip to hide, then quickly perform an action which removes the tooltip entirely.
  4. Observe the error in the logs.

Error message in console

warning.js:33 Warning: Can't call setState (or forceUpdate) on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
in UncontrolledTooltip (created by [redacted])
[redacted]
printWarning @ warning.js:33
warning @ warning.js:57
warnAboutUpdateOnUnmounted @ react-dom.development.js:11186
scheduleWorkImpl @ react-dom.development.js:12130
scheduleWork @ react-dom.development.js:12082
enqueueSetState @ react-dom.development.js:6644
Component.setState @ react.development.js:238
toggle @ reactstrap.es.js:5337
toggle @ reactstrap.es.js:4180
hide @ reactstrap.es.js:4123
setTimeout (async)
onMouseLeaveTooltip @ reactstrap.es.js:4077

Thanks!

@TheSharpieOne
Copy link
Member

This is most likely caused by the timeouts used to delay the hiding and showing of the tooltip. In this case, the hide is triggered which sets a timer. Before that timer expires and executes the hide, the component is removed.

To fix this, would might need to track the component mounting and unmounting a little better.

@jeff-nz
Copy link
Contributor

jeff-nz commented Oct 9, 2018

Hi @TheSharpieOne ,

I have created a fix for this issue.

Here's my pull request:
#1256

Regards,
Jeff

@jeff-nz
Copy link
Contributor

jeff-nz commented Oct 9, 2018

I forgot to mention. I'll try my best to help.

So to replicate the issue mentioned on this ticket, I utilised this repo (reactstrap) then temporarily modified the file for testing:
./reactstrap/docs/lib/examples/TooltipUncontrolled.js

On your browser you can view this under Uncontrolled Tooltip seciont of the page below:
http://localhost:8080/components/tooltips/

With the code below applied, after compiling, hover on the link to pop component then do a sudden click on the paragraph tag. This should give you the same error mentioned above. Once the fix is applied, this should prevent the error on the unmounted component.

/* eslint react/no-multi-comp: 0, react/prop-types: 0 */
import React from 'react';
import { Tooltip } from 'reactstrap';

export default class Example extends React.Component{
  constructor(props) {
    super(props);
    this.handleClick = this.handleClick.bind(this);
    this.toggle = this.toggle.bind(this);
    this.state = {
      hideControl: false,
      isOpen: false
    };
  }

  handleClick() {
    this.setState({
      hideControl: true
    });
  }

  toggle() {
    this.setState({ isOpen: !this.state.isOpen });
  }

  render() {
    return (
      <div>
        <p onClick={this.handleClick}>Somewhere in here is a.</p>
        <span style={{
          textDecoration: "underline",
          color: "blue"
        }} href="#"
              id="UncontrolledTooltipExample">tooltip</span>
        {!this.state.hideControl &&
        <Tooltip isOpen={this.state.isOpen} toggle={this.toggle} placement="right" target="UncontrolledTooltipExample">
          Hello world!
        </Tooltip>
        }
      </div>
    );
  }
}

I hope this helps.

@fmrl-island
Copy link
Author

I believe the fix needs to go into Tooltip, not into UncontrolledTooltip. If it is only patched in UncontrolledTooltip, then any user of Tooltip also needs to put similar code into whatever code they are using to maintain the open/closed state.

@jeff-nz
Copy link
Contributor

jeff-nz commented Oct 9, 2018

The issue is not in the Tooltip component but in the UncontrolledTooltip component. I did a test before creating my fix to find the root cause of this issue.

The Tooltip component does not set or use any state. This component is only passing props.

However, UncontrolledTooltip has its own state which is causing the issue that you've reported.

Here's my code simulating the same scenario above but calling Tooltip direct:

/* eslint react/no-multi-comp: 0, react/prop-types: 0 */
import React from 'react';
import { Tooltip } from 'reactstrap';

export default class Example extends React.Component{
  constructor(props) {
    super(props);
    this.handleClick = this.handleClick.bind(this);
    this.toggle = this.toggle.bind(this);
    this.state = {
      hideControl: false,
      isOpen: false
    };
  }

  handleClick() {
    this.setState({
      hideControl: true
    });
  }

  toggle() {
    this.setState({ isOpen: !this.state.isOpen });
  }

  render() {
    return (
      <div>
        <p onClick={this.handleClick}>Somewhere in here is a.</p>
        <span style={{
          textDecoration: "underline",
          color: "blue"
        }} href="#"
              id="UncontrolledTooltipExample">tooltip</span>
        {!this.state.hideControl &&
        <Tooltip isOpen={this.state.isOpen} toggle={this.toggle} placement="right" target="UncontrolledTooltipExample">
          Hello world!
        </Tooltip>
        }
      </div>
    );
  }
}

I hope it helps.

@jeff-nz
Copy link
Contributor

jeff-nz commented Oct 9, 2018

It's before calling the Tooltip Component that the error is triggered. The error is about setting state on component that is unmounted

Error message that looked like:

Error message in console
warning.js:33 Warning: Can't call setState (or forceUpdate) on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
in UncontrolledTooltip (created by [redacted])
[redacted]
printWarning @ warning.js:33
warning @ warning.js:57
warnAboutUpdateOnUnmounted @ react-dom.development.js:11186
scheduleWorkImpl @ react-dom.development.js:12130
scheduleWork @ react-dom.development.js:12082
enqueueSetState @ react-dom.development.js:6644
Component.setState @ react.development.js:238
toggle @ reactstrap.es.js:5337
toggle @ reactstrap.es.js:4180
hide @ reactstrap.es.js:4123
setTimeout (async)
onMouseLeaveTooltip @ reactstrap.es.js:4077

Here's a link on a similar issue to this:
https://stackoverflow.com/questions/50428842/cant-call-setstate-or-forceupdate-on-an-unmounted-component

@fmrl-island
Copy link
Author

I would expect your second test case to not throw the error, even in the absence of a fix, because the component which manages the implementation of toggle (and the state) is not being unmounted. It is possible for components other than UncontrolledTooltip to become unmounted in this manner.

You are correct that Tooltip does not manage any state, therefore, the error will never actually occur within Tooltip. However, the error being thrown in the wrapping component is a direct consequence of the way that the toggle callback is called after unmounting.

Here is an example which causes the error without the use of UncontrolledTooltip:

import React from 'react';
import {Button, Tooltip} from 'reactstrap';

export class ButtonAndTooltip extends React.Component {

	constructor(props) {
		super(props);
		this.state = {
			isOpen: false,
		};
		this.toggle = this.toggle.bind(this);
	}

	toggle() {
		this.setState({ isOpen: !this.state.isOpen });
	}

	render() {
		return (
			<div>
				<Button id="UncontrolledTooltipExample">Hover over me, then click the button above me</Button>
				<Tooltip isOpen={this.state.isOpen} toggle={this.toggle} placement="right" target="UncontrolledTooltipExample" delay={{show: 0, hide: 1000}}>
					Hello world!
				</Tooltip>
			</div>
		);
	}
}
import React from 'react';
import {Button} from 'reactstrap';
import {ButtonAndTooltip} from './buttonAndTooltip';

export class Example extends React.Component {

	constructor(props) {
		super(props);
		this.onShowHide = this.onShowHide.bind(this);

		this.state = {
			isHidden: false,
		};
	}

	onShowHide() {
		this.setState({ isHidden: !this.state.isHidden });
	}

	render() {
		const button = (this.state.isHidden) ? null : <ButtonAndTooltip onHide={this.onHide}/>;
		return (
			<div>
				<Button onClick={this.onShowHide}>Click me to hide button below</Button>
				{button}
			</div>
		);
	}
}

Thanks!

@jeff-nz
Copy link
Contributor

jeff-nz commented Oct 9, 2018

Then your ButtonAndTooltip component should be written like this:

import React from 'react';
import {Button, Tooltip} from 'reactstrap';

export class ButtonAndTooltip extends React.Component {

	constructor(props) {
		super(props);
		this.state = {
			isOpen: false,
		};
		this.toggle = this.toggle.bind(this);
	}

       componentDidMount() {
           this._isMounted = true;
       }

       componentWillUnmount() {
           this._isMounted = false;
       }

	toggle() {
           if (this._isMounted) {
		this.setState({ isOpen: !this.state.isOpen });
           }
	}

	render() {
		return (
			<div>
				<Button id="UncontrolledTooltipExample">Hover over me, then click the button above me</Button>
				<Tooltip isOpen={this.state.isOpen} toggle={this.toggle} placement="right" target="UncontrolledTooltipExample" delay={{show: 0, hide: 1000}}>
					Hello world!
				</Tooltip>
			</div>
		);
	}
}

Tooltip is a stateless function/component. If you don't write it like the one above, you'll the same error

Can't call setState (or forceUpdate) on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
in UncontrolledTooltip (created by [redacted])

Similar issue to:
https://stackoverflow.com/questions/50428842/cant-call-setstate-or-forceupdate-on-an-unmounted-component

I hope it helps

@jeff-nz
Copy link
Contributor

jeff-nz commented Oct 9, 2018

Sorry I forgot to explain, the error happens before re-rendering Tooltip. This error happens during this.setState on the unmounted component (ButtonAndTooltip).

@jeff-nz
Copy link
Contributor

jeff-nz commented Oct 9, 2018

Hi Guys ,

I found an article from the react site that talks about an issue similar to this one and the solution. This issue is complicated, I hope the article can help.

Here's the link:
https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html

@fmrl-island
Copy link
Author

I was going to get around to this point. I think that the setTimeout should be cleared inside of Tooltip's unmounting (in lieu of an ismounted flag) so that the toggle callback (and transitively, the parent component), can be garbage collected (if otherwise possible).

Another bug which exists and would not be fixed if not fixed inside of tooltip: Imagine the parent component is not unmounted and a new tooltip is mounted while the previous tooltip has a toggle pending. That pending toggle will eventually trigger and show/hide the new tooltip.

@jeff-nz
Copy link
Contributor

jeff-nz commented Oct 9, 2018

In my humble opinion, clearing the timeout doesn't fix the issue here.

A stateless component like Tooltip only reads and returns props from an external source. If the Tooltip ever triggers a callback because it is in transition of closing but object is already unmounted, the tool tip does not care about your parent's state. This component only calls the toggle property you have entered outside the Tooltip component.

I hope it helps.

@fmrl-island
Copy link
Author

It is true that Tooltip is a "stateless" component, but that is a bit of a misnomer as it truly does have state, just not in the React sense of the word (state which affects rendering). Because each tooltip has a toggle function passed in from the parent, it effectively captures a reference to the parent's data. Because the timeout has a reference to the toggle function when a hide is pending, the parents data cannot be garbage collected even if the parent and the child are both unmounted.

I haven't tried fixing it Tooltip to prevent the memory leak, but using the following ButtonAndTooltip, along with the previous example code, you can run out of memory in your browser. Obviously this is a bit of a contrived example because each parent component is very heavy memory wise and in practice no one would have a hide delay of an hour, but a memory leak is a memory leak.

To trigger an out of memory leak, click the top button, hover over the bottom button to make the tooltip show, and then click the top button again. Repeat until out of memory.

import React from 'react';
import {Button, Tooltip} from 'reactstrap';

export class ButtonAndTooltip extends React.Component {

	constructor(props) {
		super(props);
		this.state = {
			isOpen: false,
		};
		this.toggle = this.toggle.bind(this);

		this.someGarbageData = [];
		for (let i = 0; i < 10_000_000; i++) {
			this.someGarbageData.push(1);
		}
	}

	toggle() {
		this.setState({ isOpen: !this.state.isOpen });
	}

	render() {
		return (
			<div>
				<Button id="UncontrolledTooltipExample">Hover over me, then click the button above me</Button>
				<Tooltip isOpen={this.state.isOpen} toggle={this.toggle} placement="right" target="UncontrolledTooltipExample" delay={{show: 0, hide: 60*60*1000}}>
					Hello world!
				</Tooltip>
			</div>
		);
	}
}

@jeff-nz
Copy link
Contributor

jeff-nz commented Oct 10, 2018

Ok cool. I will leave this with you guys then.

It'll be interesting to see how the current Tooltip pickup the mounting state of ButtonAndTooltip.

@jeff-nz
Copy link
Contributor

jeff-nz commented Oct 19, 2018

Hey @StephenLucasKnutson ,

Have you had any chance on testing your theory yet?

You are correct on the statement "parents data cannot be garbage collected even if the parent and the child are both unmounted"

This is why I made a the fix like:
#1256

As per reactjs's suggested fix, my pull request should do the job.

Let me know if you need me to reopen my PR.

black-jk added a commit to black-jk/reactstrap that referenced this issue Jan 28, 2019
@black-jk
Copy link

black-jk commented Jan 28, 2019

black-jk@3e8bf36
Just clear all timeout when component unmount !?

@tommyalvarez
Copy link

@black-jk what's the status on this?

@black-jk
Copy link

black-jk commented Mar 7, 2019

@black-jk what's the status on this?
You can reproduce problem by this:

`...
render() {
return ;
}
...

// ====================================================================================================
import React from 'react';
import { Tooltip, Button } from "reactstrap";

class Demo extends React.Component {
state = { hideButton: false };

render() {
if (this.state.hideButton) return null;
return <TooltipButton onClick={() => { this.setState({ hideButton: true }); }}>;
}
}

// ----------------------------------------------------------------------------------------------------

class TooltipButton extends React.Component {
state = { tooltipOpen: false };

render() {
return (


<Button id="object" size="xsmall" onClick={() => { this.props.onClick(); }}>Click Me!
<Tooltip target={"object"} placement="right" delay={{ show: 50, hide: 50 }} isOpen={this.state.tooltipOpen} toggle={this._toggleTooltip}>This is Tooltip!

);
}

// --------------------------------------------------

_toggleTooltip = () => {
this.setState({
tooltipOpen: !this.state.tooltipOpen,
});
}
}`

Get warning message when click:

Warning: Can't perform a React state update on an unmounted component.
This is a no-op, but it indicates a memory leak in your application.
To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
in TooltipButton (created by Demo)

@siliconeidolon
Copy link

Any chance of this going in?

@Zev-Engineer
Copy link

Is there any update on this? I have been having the same problem.

Or, if no fix is forthcoming- does this actually cause a memory leak, or does it simply trigger a warning? Is it safe to leave UncontrolledToolTip in my code, or will I need to find another solution?

@Zev-Engineer
Copy link

Zev-Engineer commented Apr 18, 2019

I have found a solution. I created a slightly altered version of the UncontrolledTooltip.js file (hence the altered file path for 'omit' below). Rendered using the same syntax as the original UncontrolledTooltip component, this version provided my project with the needed functionality and did not cause any warnings or errors.

I added componentDidMount and componentWillUnmount methods which set a boolean variable named 'readyToUpdate.' I also added a check to the toggle method so that it will not attempt a setState if componentWillUnmount has been called.

Can anyone else confirm that this solves the issue for them, as well? If so, would this be a viable replacement for the current implementation of UncontrolledTooltip?

/*******************************************************************************************************/

`
import React, { Component } from 'react';
import PropTypes from 'prop-types';
import {Tooltip} from 'reactstrap';
import { omit } from '../../../node_modules/reactstrap/src/utils' // file path may vary, depending on project

const omitKeys = ['defaultOpen'];

export default class UncontrolledTooltip extends Component {
constructor(props) {
super(props);
``
this.state = { isOpen: props.defaultOpen || false};
this.toggle = this.toggle.bind(this);
}

componentDidMount() {
this.readyToUpdate = true;
}

componentWillUnmount() {
this.readyToUpdate = false;
}

toggle() {
if (this.readyToUpdate)
{
this.setState({ isOpen: !this.state.isOpen });
}
}

render() {
return <Tooltip isOpen={this.state.isOpen} toggle={this.toggle} {...omit(this.props, omitKeys)} />;
}
}

UncontrolledTooltip.propTypes = {
defaultOpen: PropTypes.bool,
...Tooltip.propTypes
};
`

@ghost
Copy link

ghost commented Apr 24, 2019

I found safe solution for myself, will leave it here for newcomers. The only problem is that instead of using UncontrolledTooltip component you will need to use Tooltip component and manage open state by yourself.

With usage of React refs
import React, { Component } from 'react'
import { Tooltip } from 'reactstrap'

class YourComponent extends Component {
  mounted = false

  state = {
    isTooltipOpen: false,
    tooltipContainerRef: null,
    tooltipTargetRef: null
  }

  handleTooltipToggle = () => {
    const { isTooltipOpen } = this.state

    if (!this.mounted) {
      return
    }

    return this.setState({ isTooltipOpen: !isTooltipOpen })
  }

  setTooltipContainerRef = (tooltipContainerRef) => {
    return this.setState({ tooltipContainerRef })
  }

  setTooltipTargetRef = (tooltipTargetRef) => {
    return this.setState({ tooltipTargetRef })
  }

  componentDidMount () {
    this.mounted = true
  }

  componentWillUnmount () {
    this.mounted = false
  }

  render () {
    const { isTooltipOpen, tooltipContainerRef, tooltipTargetRef } = this.state

    return (
      <div ref={this.setTooltipContainerRef}>
        <button ref={this.setTooltipTargetRef}>
          Hover me (if you can)!
        </button>
        {tooltipContainerRef && tooltipTargetRef && (
          <Tooltip
            container={tooltipContainerRef}
            isOpen={isTooltipOpen}
            target={tooltipTargetRef}
            toggle={this.handleTooltipToggle}
          >
            Hovering this button you will see magic
          </Tooltip>
        )}
      </div>
    )
  }
}

export default YourComponent
With usage of id attribute
import React, { Component } from 'react'
import { Tooltip } from 'reactstrap'

class YourComponent extends Component {
  mounted = false

  state = {
    isTooltipOpen: false
  }

  handleTooltipToggle = () => {
    const { isTooltipOpen } = this.state

    if (!this.mounted) {
      return
    }

    return this.setState({ isTooltipOpen: !isTooltipOpen })
  }

  componentDidMount () {
    this.mounted = true
  }

  componentWillUnmount () {
    this.mounted = false
  }

  render () {
    const { isTooltipOpen } = this.state

    return (
      <div id="YourComponent__TooltipContainer">
        <button id="YourComponent__TooltipTarget">
          Hover me (if you can)!
        </button>
        <Tooltip
          container="YourComponent__TooltipContainer"
          isOpen={isTooltipOpen}
          target="YourComponent__TooltipTarget"
          toggle={this.handleTooltipToggle}
        >
          Hovering this button you will see magic
        </Tooltip>
      </div>
    )
  }
}

export default YourComponent

@amillward
Copy link
Contributor

Is there any progress on a fix for this issue?

@jinixx
Copy link
Contributor

jinixx commented Jul 8, 2019

Hi @TheSharpieOne

I've added a PR to this issue.
#1562

Please let me know if you need anything changed.

Thanks!

@amillward
Copy link
Contributor

amillward commented Jul 9, 2019

@jinixx Thank you - is there a way to use this fix in my project yet or do I need to wait for the next release? Installing it directly (npm i -s 'git://github.com:reactstrap/reactstrap.git#e85238b3fc32610d5c27340f87377a0a54df49a5') doesn't seem to work

@jinixx
Copy link
Contributor

jinixx commented Jul 9, 2019

@amillward try remove first before install again from github

@TheSharpieOne
Copy link
Member

You'll have to wait for the next release since we only publish compiled assets to npm and never check them into github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment