Skip to content

Commit

Permalink
fix: Clean up player when a preroll fails and prevent uncaught play p…
Browse files Browse the repository at this point in the history
…romise exceptions. (#470)
  • Loading branch information
misteroneill authored Mar 20, 2019
1 parent d1171ec commit 07946db
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 8 deletions.
29 changes: 24 additions & 5 deletions src/states/Preroll.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ class Preroll extends AdState {
const player = this.player;

if (this.adsReady && !player.ads.inAdBreak() && !this.isContentResuming()) {
player.clearTimeout(this._timeout);
this.clearTimeout(player);
player.ads.adType = 'preroll';
this.waitingForAdBreak = false;
adBreak.start(player);
Expand Down Expand Up @@ -227,16 +227,21 @@ class Preroll extends AdState {
}

resumeAfterNoPreroll(player) {

// Resume to content and unblock play as there is no preroll ad
this.contentResuming = true;
player.ads._shouldBlockPlay = false;
this.cleanupPartial(player);

// Play the content if we had requested play or we paused on 'contentupdate'
// and we haven't played yet. This happens if there was no preroll or if it
// errored, timed out, etc. Otherwise snapshot restore would play.
if (player.paused() &&
(player.ads._playRequested || player.ads._pausedOnContentupdate)) {
player.play();
if (player.paused() && (player.ads._playRequested || player.ads._pausedOnContentupdate)) {
const playPromise = player.play();

if (playPromise && playPromise.then) {
playPromise.then(null, (e) => {});
}
}
}

Expand All @@ -247,12 +252,26 @@ class Preroll extends AdState {
if (!player.ads._hasThereBeenALoadStartDuringPlayerLife) {
videojs.log.warn('Leaving Preroll state before loadstart event can cause issues.');
}
this.cleanupPartial(player);
}

/*
* Performs cleanup tasks without depending on a state transition. This is
* used mainly in cases where a preroll failed.
*/
cleanupPartial(player) {
player.removeClass('vjs-ad-loading');
player.removeClass('vjs-ad-content-resuming');
player.clearTimeout(this._timeout);
this.clearTimeout(player);
}

/*
* Clear the preroll timeout and nulls out the pointer.
*/
clearTimeout(player) {
player.clearTimeout(this._timeout);
this._timeout = null;
}
}

States.registerState('Preroll', Preroll);
Expand Down
17 changes: 14 additions & 3 deletions test/unit/states/test.Preroll.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ QUnit.module('Preroll', {
beforeEach() {
this.events = [];
this.playTriggered = false;
this.classes = [];

this.player = {
ads: {
Expand All @@ -22,8 +23,9 @@ QUnit.module('Preroll', {
},
setTimeout: () => {},
clearTimeout: () => {},
addClass: () => {},
removeClass: () => {},
addClass: (name) => this.classes.push(name),
removeClass: (name) => this.classes.splice(this.classes.indexOf(name), 1),
hasClass: (name) => this.classes.indexOf(name) !== -1,
one: () => {},
trigger: (event) => {
this.events.push(event);
Expand Down Expand Up @@ -120,6 +122,9 @@ QUnit.test('can handle adscanceled', function(assert) {
this.preroll.init(this.player, false, false);
this.preroll.onAdsCanceled(this.player);
assert.equal(this.preroll.isContentResuming(), true);
assert.notOk(this.player.hasClass('vjs-ad-loading'));
assert.notOk(this.player.hasClass('vjs-ad-content-resuming'));
assert.notOk(this.preroll._timeout);
this.preroll.onPlaying(this.player);
assert.equal(this.newState, 'ContentPlayback', 'transitioned to ContentPlayback');
});
Expand All @@ -128,6 +133,9 @@ QUnit.test('can handle adserror', function(assert) {
this.preroll.init(this.player, false, false);
this.preroll.onAdsError(this.player);
assert.equal(this.preroll.isContentResuming(), true);
assert.notOk(this.player.hasClass('vjs-ad-loading'));
assert.notOk(this.player.hasClass('vjs-ad-content-resuming'));
assert.notOk(this.preroll._timeout);
this.preroll.onPlaying(this.player);
assert.equal(this.newState, 'ContentPlayback', 'transitioned to ContentPlayback');
});
Expand All @@ -140,10 +148,13 @@ QUnit.test('can skip linear ad mode', function(assert) {
assert.equal(this.newState, 'ContentPlayback', 'transitioned to ContentPlayback');
});

QUnit.test('plays content after ad timeout', function(assert) {
QUnit.test('can handle adtimeout', function(assert) {
this.preroll.init(this.player, false, false);
this.preroll.onAdTimeout(this.player);
assert.equal(this.preroll.isContentResuming(), true);
assert.notOk(this.player.hasClass('vjs-ad-loading'));
assert.notOk(this.player.hasClass('vjs-ad-content-resuming'));
assert.notOk(this.preroll._timeout);
this.preroll.onPlaying(this.player);
assert.equal(this.newState, 'ContentPlayback', 'transitioned to ContentPlayback');
});
Expand Down

0 comments on commit 07946db

Please sign in to comment.