Plyr: e.player.elements is null (Firefox Mobile 59, Android 7)

[mhluska]

I'm seeing various issues on latest Firefox Mobile through Sentry after upgrading to v3:

TypeError: e.player.elements is null
  at func(node_modules/plyr/dist/plyr.js:4326:1)
  at wrapped(vendor/raven.js:443:1)

I'm also seeing something on Mobile Safari 11.2.6:

TypeError: null is not an object (evaluating 'this.player.elements.container')
  at value(node_modules/plyr/dist/plyr.js:4101:1)
  at value([native code])
  at wrapped(vendor/raven.js:443:1)

My Plyr version is 3.0.11.

I don't have these devices myself so I haven't been able to reproduce these issues yet. Are these mobile browsers supposed to be supported?

---

This probably deserves it's own ticket but I'm also seeing this sometimes on latest Chrome upon initializing the player:

Uncaught TypeError: Cannot read property 'display' of null
    at r.updateProgress (plyr.js:2630)
    at HTMLVideoElement.<anonymous> (plyr.js:4204)
    at HTMLVideoElement.i (raven.js:443)

[sampotts]

Ah this looks like a regression. I've not seen ant of these come through on the demo site's Sentry but I'll get these fixed up.

[mhluska]

@sampotts thanks for taking a look. To give more detail on the Chrome case, what's happening is I call .destroy() on the plyr instance but it doesn't seem to get fully destroyed. This callback seems to not get cleaned up after destroy:

utils.on(this.player.media, 'progress playing', function (event) {
  return ui.updateProgress.call(_this3.player, event);
});

[paulconlin]

I'm getting this too across the board on desktop Safari, Chrome, Firefox. Agreed that this has something to do with the .destroy() method. I see the issue most when a user is cycling through videos on our app, and event listeners are trying to fire on "destroyed" plyr instances.

@sampotts any idea on how you would like to fix this? I'm willing to spend some time digging into it and put up a PR.

[sampotts]

Are you guys using the latest version?

[mhluska]

I am on 3.0.11 still.

[FlipgridMartin]

On 3.2.0 right now and still seeing it on multiple browsers and operating systems. They all seem to be coming from inside event listeners.

[FlipgridMartin]

Not too familiar with the codebase, but it looks like keyup & keydown listeners get set (https://github.com/sampotts/plyr/blob/master/src/js/listeners.js#L393), and then on non-soft .destroy(), this.listeners.clear() is called which only unsets the keyup & keydown listeners if the global keyboard config is set (https://github.com/sampotts/plyr/blob/master/src/js/listeners.js#L204), but that config property is false by default.

[paulconlin]

It seems the main issue is that there isn't any null checking when accessing things like this.player and this.elements in the ui.js part of the app. This seems to be aggravated by the fact that the .destroy() method isn't clearing all event listeners properly. I'm seeing these errors even after the destroyed callback is called.

[sampotts]

Well the root cause is the listeners still firing. I could add the null checking but that’s not really going to solve the issue. The listeners would still fire and they are only bound initially if the UI is supported anyway. I’ll take another look.

[ondrejrohon]

Seems like event listeners don't get removed by solely removing those elements from dom. I found in code this coment, which explains why there are no removeEventListener calls. I assume that has to mean that there are still some references to those elements, which are not garbage collected and therefore browser doesn't automatically remove event listeners. I will try to find them.

[ondrejrohon]

Unfortunately I don't have enough time to find a proper fix for this issue, but I was able to make a quick one. Have look at this commit in my fork. Hope it helps.

[paulconlin]

Any updates on this one?