fix: Fix event listener leaks in Player #4229
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Event listeners were being leaked in Player across load() / unload() cycles. This was fundamentally caused by the difficulty in keeping track of which event listeners to clean up at which stages of the load graph. Everything is cleaned up by Player.destroy() and EventManager.release(), but for a Player with heavy re-use, there was still a small leak. This fixes the leak by splitting the EventManager instance into three instances, each of which is cleaned up in a different part of the load graph life cycle. Listeners which should only live as long as a piece of content is loaded go into loadEventManager_. Listeners which should only live as long as we are attached to the video element go into attachEventManager_. Listeners which should live as long as the Player instance itself go into globalEventManager_. It is now impossible to miss unlistening to a particular event, since we no longer have to unlisten to any individual events at all. The removeAll() method of each event manager will clean up all listeners at the appropriate time.
avelad
approved these changes
May 17, 2022
joeyparrish
added a commit
that referenced
this pull request
May 17, 2022
Event listeners were being leaked in Player across load() / unload() cycles. This was fundamentally caused by the difficulty in keeping track of which event listeners to clean up at which stages of the load graph. Everything is cleaned up by Player.destroy() and EventManager.release(), but for a Player with heavy re-use, there was still a small leak. This fixes the leak by splitting the EventManager instance into three instances, each of which is cleaned up in a different part of the load graph life cycle. Listeners which should only live as long as a piece of content is loaded go into loadEventManager_. Listeners which should only live as long as we are attached to the video element go into attachEventManager_. Listeners which should live as long as the Player instance itself go into globalEventManager_. It is now impossible to miss unlistening to a particular event, since we no longer have to unlisten to any individual events at all. The removeAll() method of each event manager will clean up all listeners at the appropriate time.
joeyparrish
added a commit
that referenced
this pull request
May 17, 2022
Event listeners were being leaked in Player across load() / unload() cycles. This was fundamentally caused by the difficulty in keeping track of which event listeners to clean up at which stages of the load graph. Everything is cleaned up by Player.destroy() and EventManager.release(), but for a Player with heavy re-use, there was still a small leak. This fixes the leak by splitting the EventManager instance into three instances, each of which is cleaned up in a different part of the load graph life cycle. Listeners which should only live as long as a piece of content is loaded go into loadEventManager_. Listeners which should only live as long as we are attached to the video element go into attachEventManager_. Listeners which should live as long as the Player instance itself go into globalEventManager_. It is now impossible to miss unlistening to a particular event, since we no longer have to unlisten to any individual events at all. The removeAll() method of each event manager will clean up all listeners at the appropriate time.
joeyparrish
added a commit
that referenced
this pull request
May 17, 2022
Event listeners were being leaked in Player across load() / unload() cycles. This was fundamentally caused by the difficulty in keeping track of which event listeners to clean up at which stages of the load graph. Everything is cleaned up by Player.destroy() and EventManager.release(), but for a Player with heavy re-use, there was still a small leak. This fixes the leak by splitting the EventManager instance into three instances, each of which is cleaned up in a different part of the load graph life cycle. Listeners which should only live as long as a piece of content is loaded go into loadEventManager_. Listeners which should only live as long as we are attached to the video element go into attachEventManager_. Listeners which should live as long as the Player instance itself go into globalEventManager_. It is now impossible to miss unlistening to a particular event, since we no longer have to unlisten to any individual events at all. The removeAll() method of each event manager will clean up all listeners at the appropriate time.
nyanmisaka
added a commit
to nyanmisaka/shaka-player
that referenced
this pull request
Oct 6, 2022
This reverts commit e6c3b22.
github-actions
bot
added
the
status: archived
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Event listeners were being leaked in Player across load() / unload()
cycles. This was fundamentally caused by the difficulty in keeping
track of which event listeners to clean up at which stages of the load
graph. Everything is cleaned up by Player.destroy() and
EventManager.release(), but for a Player with heavy re-use, there was
still a small leak.
This fixes the leak by splitting the EventManager instance into three
instances, each of which is cleaned up in a different part of the load
graph life cycle. Listeners which should only live as long as a piece
of content is loaded go into loadEventManager_. Listeners which
should only live as long as we are attached to the video element go
into attachEventManager_. Listeners which should live as long as the
Player instance itself go into globalEventManager_. It is now
impossible to miss unlistening to a particular event, since we no
longer have to unlisten to any individual events at all. The
removeAll() method of each event manager will clean up all listeners
at the appropriate time.