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

VR Plugin and associated event handlers are not disposed correctly #118

Closed
ferdaber opened this issue Jul 31, 2018 · 1 comment
Closed

Comments

@ferdaber
Copy link

ferdaber commented Jul 31, 2018

Description

I just learned that according to MDN, when adding an event listener under the capturing phase, removing that event listener must also be under the capturing phase, what this means is that the browser tracks event listeners under the capturing phase separately than listeners under the bubbling phase:

window.addEventListener('resize', myFunc, true)
window.removeEventListener('resize', myFunc, true) // <- true has to be passed in as 3rd arg, otherwise it won't work

In the following code in the plugin, that is not being followed and unfortunately those event listeners are staying around after the player is disposed. This is causing issues in my team's code as the player tries to access elements that are no longer there (and in general is a memory leak)

window.addEventListener('resize', this.handleResize_, true);

window.removeEventListener('resize', this.handleResize_);

My current workaround is to just monkey-patch the reset method:

const VR = window.videojs && window.videojs.getPlugin('vr')
if (VR) {
    const oldReset = VR.prototype.reset
    VR.prototype.reset = function reset() {
        oldReset.apply(this, arguments)
        window.removeEventListener('resize', this.handleResize_, true)
        window.removeEventListener('vrdisplaypresentchange', this.handleResize_, true)
        window.removeEventListener('vrdisplayactivate', this.handleVrDisplayActivate_, true)
        window.removeEventListener('vrdisplaydeactivate', this.handleVrDisplayDeactivate_, true)
    }
}

Steps to reproduce

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

  1. Initialize a video player with VR capabilities
  2. Dispose that video player
  3. Resize that window

Results

Expected

No errors

Actual

A console error is logged as the old listener tries to access a video element that is no longer there
image

versions

videojs

what version of videojs does this occur with? v6.11.0

browsers

what browser are affected? all browsers

OSes

what platforms (operating systems and devices) are affected? all platforms

@brandonocasey
Copy link
Contributor

Should be fixed as of 1.4.4. See #119. Thanks for the report @ferdaber. Feel free to open a PR next time you already had most of the code 👍

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