Clear out pending errors on player disposal. #1481
Conversation
Source selection errors are dispatched asynchronously so that there is an opportunity to override the error message. If the player is disposed during that period, the error timeout wasn't being cleared properly. Fix for videojs#1480.
| @@ -1155,7 +1155,7 @@ vjs.Player.prototype.src = function(source){ | |||
| * @private | |||
| */ | |||
| vjs.Player.prototype.sourceList_ = function(sources){ | |||
| var sourceTech = this.selectSource(sources); | |||
| var sourceTech = this.selectSource(sources), errorTimeout; | |||
There was a problem hiding this comment.
There are plenty of other instances of multiple variable declarations on the same line in this file. Is you objection because one of the variables is also defined here?
There was a problem hiding this comment.
While I'm not saying this file is shining example of consistency, this would be the only place a var definition (as opposed to just declarations) wasn't on it's own line. :)
|
@mmcc should review this one. What if we just checked if this.el() existed before firing the error? If that's not clear enough we might set Just wondering, does anyone know of an alternative pattern we might use for handling a situation like this? Where you want to listen for an even that may have already happened. The only other one I can think of is how ready() listeners are handled, where if ready already happened the listener is fired. Not sure if I like that for this case. |
|
While checking whether the el exists is a good idea, it isn't be the only thing used here. We should definitely clear out any timeouts and intervals that are created on a player dispose. |
|
There are plenty of alternative ways to fix this. I chose this path because it's consistent with timeout handling elsewhere in this file. |
|
Yeah, after a second look I like how it is. |
|
Just curious.. does this relate to the vjs.hasData function getting called repeatedly and el being null? Was going to open an issue with this, but it's a bit odd to replicate.. (Using AngularJS app) |
| var clock = sinon.useFakeTimers(), player; | ||
|
|
||
| player = PlayerTest.makePlayer(); | ||
| player.src({ |
There was a problem hiding this comment.
Everywhere else in the player spec any options are set in the options object for makePlayer. Any reason you didn't go that route here?
There was a problem hiding this comment.
I encountered this issue originally through calling src() after the player was constructed so I reproduced that scenario for my test case. I'm fairly certain the same error would occur if the sources came in through options. Do you want me to adjust the test?
|
@Manbearpixel I doubt it? Is there any chance you're destroying a video element without disposing the player? We should take that to a new issue so as to avoid hijacking this thread, because I doubt it's related to this. LGTM, but just out of curiosity, how would this happen in the wild? |
|
@mmcc it showed up for me because I have tests that create a player, run some synchronous tests against it, and then immediately dispose it. In the wild, it could happen if you updated the player src programmatically (auto-advancing playlists, for instance) and the viewer took some action that caused the player to be disposed at the same time. I'm sure it would be pretty rare, but it could happen. |
|
Yeah I figured it initially surfaced in a test. Totally agree it should be fixed, I was just curious. |
When defining variables inline with declarations, stick to one variable per line.
|
Pushed to stable |
Source selection errors are dispatched asynchronously so that there is an opportunity to override the error message. If the player is disposed during that period, the error timeout wasn't being cleared properly. Fix for #1480.