Leak of ResizeManager and event listeners in 7.0.5

[davidstelter]

Description

While troubleshooting a memory leak in our app which was using videojs 7.0.3, I came across an apparent memory leak resulting from some flaw in Player.dispose() or one of the components it deferred disposal duties to. I upgraded to 7.0.5 to see if the issue just got better and it did! But then there was a different leak. Using Chrome's heap snapshot comparison feature, I found that there were lots of "Detached EventListeners" piling up. Digging into the leaked objects I found a number of ResizeManager instances, which were coming from inside videojs. I was able to get it to stop just by supplying resizeManager = false in the options. We are properly calling Player.dispose() when we're finished with a player instance and then releasing our reference to it.

Our use case involves creating and disposing lots of (sometimes quite short) videos, so we might be a pretty good canary for this leak. I've seen it climb to around 1GB in a few hours!

Is there some additional cleanup required by the user when using the default ResizeManager on a Player that I'm just missing? Or should I create a reduced test case?

Steps to reproduce

Explain in detail the exact steps necessary to reproduce the issue.

1. Take a heap snapshot in Chrome
2. Create & dispose lots of players in a loop, leave it running for 10-100 cycles
3. Take another heap snapshot & compare
4. look for objects with deltas close to the number of disposed videos & find "Detached EventListeners"
5. Also note overall memory use climbing

Results

Expected

Dispose should clean up all resources allocated by a Player.

Actual

Player.dispose() leaks ResizeManager objects possibly via EventListeners, or vice versa. Consumes a gradually ballooning amount of memory.

Error output

Eventually Chrome will Aww, Snap as it does when memory is exhausted.

Additional Information

Please include any additional information necessary here. Including the following:

versions

videojs

7.0.5

browsers

Chrome (latest) at a minimum, but I expect others

OSes

Would expect all, observed on OS X

plugins

No plugins

[welcome]

👋 Thanks for opening your first issue here! 👋

If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
To help make it easier for us to investigate your issue, please follow the contributing guidelines.

[gkatsev]

Theoretically, the user shouldn't need to do anything and video.js should take care of everything on dispose. The only requirement is that the elements shouldn't be removed from the DOM until Video.js is done disposing.

A reduced test case would be great!

Also, if you find any other leaks, we'd be happy to take a look and work with you to fix them!

[gkatsev]

Also, would be good to look and see if the dispose method of the ResizeManager is working properly and whether things are getting unbound:

video.js/src/js/resize-manager.js

Lines 98 to 118 in e637768

	dispose() {
	if (this.resizeObserver_) {
	if (this.player_.el()) {
	this.resizeObserver_.unobserve(this.player_.el());
	}
	this.resizeObserver_.disconnect();
	}
	if (this.el_ && this.el_.contentWindow) {
	Events.off(this.el_.contentWindow, 'resize', this.debouncedHandler_);
	}
	if (this.loadListener_) {
	this.off('load', this.loadListener_);
	}
	this.ResizeObserver = null;
	this.resizeObserver = null;
	this.debouncedHandler_ = null;
	this.loadListener_ = null;
	}

[davidstelter]

OK, pretty sure dispose is called before anything happens to the DOM on our side, I'll try to get a reduced test case together this week. Thanks!

[gkatsev]

@davidstelter it's possible we found out the source of the leak and it should be fixed in #5369. Should be out in our next release.

[gkatsev]

If you still have issues, please let us know.