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

Clear out pending errors on player disposal. #1481

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -1155,7 +1155,8 @@ vjs.Player.prototype.src = function(source){
* @private
*/
vjs.Player.prototype.sourceList_ = function(sources){
var sourceTech = this.selectSource(sources);
var sourceTech = this.selectSource(sources),
errorTimeout;

if (sourceTech) {
if (sourceTech.tech === this.techName) {
Expand All @@ -1167,13 +1168,17 @@ vjs.Player.prototype.sourceList_ = function(sources){
}
} else {
// We need to wrap this in a timeout to give folks a chance to add error event handlers
setTimeout(vjs.bind(this, function() {
errorTimeout = setTimeout(vjs.bind(this, function() {
this.error({ code: 4, message: this.localize(this.options()['notSupportedMessage']) });
}), 0);

// we could not find an appropriate tech, but let's still notify the delegate that this is it
// this needs a better comment about why this is needed
this.triggerReady();

this.on('dispose', function() {
clearTimeout(errorTimeout);
});
}
};

Expand Down
4 changes: 2 additions & 2 deletions test/unit/mediafaker.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ vjs.MediaFaker = vjs.MediaTechController.extend({
}
});

// Support everything
// Support everything except for "video/unsupported-format"
vjs.MediaFaker.isSupported = function(){ return true; };
vjs.MediaFaker.canPlaySource = function(srcObj){ return true; };
vjs.MediaFaker.canPlaySource = function(srcObj){ return srcObj.type !== 'video/unsupported-format'; };

vjs.MediaFaker.prototype.createEl = function(){
var el = vjs.MediaTechController.prototype.createEl.call(this, 'div', {
Expand Down
17 changes: 17 additions & 0 deletions test/unit/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -601,3 +601,20 @@ test('should honor disabled inactivity timeout', function() {

clock.restore();
});

test('should clear pending errors on disposal', function() {
var clock = sinon.useFakeTimers(), player;

player = PlayerTest.makePlayer();
player.src({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

src: 'http://example.com/movie.unsupported-format',
type: 'video/unsupported-format'
});
player.dispose();
try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is a try needed here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, is it to catch an error, if it's thrown?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

clock.tick(5000);
} catch (e) {
return ok(!e, 'threw an error: ' + e.message);
}
ok(true, 'did not throw an error after disposal');
});