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

FilePlayer potentially leaks event listeners #616

Closed
me-andre opened this issue Apr 4, 2019 · 2 comments
Closed

FilePlayer potentially leaks event listeners #616

me-andre opened this issue Apr 4, 2019 · 2 comments

Comments

@me-andre
Copy link
Contributor

me-andre commented Apr 4, 2019

https://github.com/CookPete/react-player/blob/master/src/players/FilePlayer.js#L84

When ReactPlayer removes listeners from the <video> element, it calls removeEventListener passing callbacks from current props. But nothing guarantees that they are same functions passed to addEventListener on player initialization. Actually they most likely are another function objects, especially if ReactPlayer was rendered inside a functional component. In that case, calls to removeEventListener are actually no-ops while mistakenly looking like memory-saving destructors.

example:

const dummy = () = {}
const Functional = ({ playing, setPlaying }) => {
  const onPlay = () => setPlaying(true)
  return <ReactPlayer playing={playing} onPlay={onPlay} />
}
render(<Functional setPlaying={dummy} />, domNode)
render(<Functional playing setPlaying={dummy} />, domNode)
unmountComponentAtNode(domNode) // does not really remove onPlay event listener

---
Solution would be to have a correspondent instance method for each cb prop.
@cookpete
Copy link
Owner

cookpete commented Apr 4, 2019

This is true, but only for the callback props that we don't set in Player.js:

https://github.com/CookPete/react-player/blob/14a440226b409b49dcecc5de6beb6aaab0cc353b/src/Player.js#L224-L235

as the functions we set here won't change.

Perhaps the solution is to store an array of listeners that we add, then in removeListeners loop through the array to remove each one?

@me-andre
Copy link
Contributor Author

me-andre commented Apr 4, 2019

Having instance methods that just proxy calls to correspondent props seems to be more flexible.
That would also enable calling an updated prop if the component has received a new cb.
Kinda late binding vs early binding.

albanqoku added a commit to albanqoku/react-player that referenced this issue Feb 24, 2021
Webmaster1116 added a commit to Webmaster1116/video-player that referenced this issue May 20, 2021
webmiraclepro added a commit to webmiraclepro/video-player that referenced this issue Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants