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

Potential bug/leak: middleware cache grows even if no middleware created #8653

Open
BrainCrumbz opened this issue Mar 22, 2024 · 2 comments · May be fixed by kerasus/soalaa#1707
Open

Potential bug/leak: middleware cache grows even if no middleware created #8653

BrainCrumbz opened this issue Mar 22, 2024 · 2 comments · May be fixed by kerasus/soalaa#1707
Labels
needs: triage This issue needs to be reviewed

Comments

@BrainCrumbz
Copy link
Contributor

BrainCrumbz commented Mar 22, 2024

Description

TL;DR: when videojs players are instantiated and disposed repeatedly on page, middlewareInstances keeps growing with new keys and null values.

We are hunting potential memory leaks in our video-based app. Comparing heap snapshots on Windows Chrome and Android Chrome, we noticed a (small?) amount of property arrays due to middlewareInstances. Snapshot were taken after disposing a player once, and then after instantiating/disposing e.g. 10 times.

Adding logpoints in tech/middleware.js, we noticed that:

  1. getOrCreateFactory is never called, so no new entry is set in middlewareInstances for a new player
  2. clearCacheForPlayer is called every time player is disposed, so a new, null-valued entry is added to middlewareInstances

This happens for videojs v7.20.3. Comparing file with latest revision showed no significant change, so maybe the behaviour is the same in latest code.

If everything is correct, would you consider a tiny PR that addresses this? E.g. clearCacheForPlayer could check if key exists in the first place?

Thanks

Reduced test case

None. Willing to create one if issue is of interest

Steps to reproduce

  1. After page load, create a player with videoPlayer = videojs(videoElem, videoPlayerOptions); and set a source on it with videoPlayer.src({src: ..., type: ...})
  2. After a timeout, dispose player with videoPlayer.dispose();
  3. After a timeout, repeat the process e.g. twice

Errors

None

What version of Video.js are you using?

7.20.3

Video.js plugins used.

None

What browser(s) including version(s) does this occur with?

Chrome 123

What OS(es) and version(s) does this occur with?

Windows 11

@BrainCrumbz BrainCrumbz added the needs: triage This issue needs to be reviewed label Mar 22, 2024
@mister-ben
Copy link
Contributor

A PR would be welcome, thanks.

@BrainCrumbz
Copy link
Contributor Author

PR can be found at #8674

Waiting for comments etc. Bye!

mister-ben added a commit that referenced this issue Jul 6, 2024
## Description
See issue #8653 

## Specific Changes proposed
When in `middleware.js` the function `clearCacheForPlayer` runs, before
setting a value to null in middlware caches, it checks if the key exists
in the first place.

## Requirements Checklist
- [x] Feature implemented / Bug fixed
- [x] If necessary, more likely in a feature request than a bug fix
- [x] Change has been verified in an actual browser (Chrome, Firefox,
IE)
  - [ ] Unit Tests updated or fixed
  - [ ] Docs/guides updated
- [x] Example created ([starter template on
JSBin](https://codepen.io/gkatsev/pen/GwZegv?editors=1000#0))
- [ ] Reviewed by Two Core Contributors

---------

Co-authored-by: Giuseppe Piscopo <g.piscopo@braincrumbz.com>
Co-authored-by: mister-ben <git@misterben.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: triage This issue needs to be reviewed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants