Skip to content

Commit

Permalink
fix: prevent dispose error and text track duplicate listeners (#6984)
Browse files Browse the repository at this point in the history
  • Loading branch information
brandonocasey authored Jan 21, 2021
1 parent ffb690a commit db46578
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 19 deletions.
24 changes: 16 additions & 8 deletions src/js/tracks/text-track.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,10 @@ class TextTrack extends Track {
const activeCues = new TextTrackCueList(this.activeCues_);
let changed = false;
const timeupdateHandler = Fn.bind(this, function() {
if (!this.tech_.isReady_ || this.tech_.isDisposed()) {
return;

}
// Accessing this.activeCues for the side-effects of updating itself
// due to its nature as a getter function. Do not remove or cues will
// stop updating!
Expand All @@ -196,10 +199,13 @@ class TextTrack extends Track {
}
});

const disposeHandler = () => {
this.tech_.off('timeupdate', timeupdateHandler);
};

this.tech_.one('dispose', disposeHandler);
if (mode !== 'disabled') {
this.tech_.ready(() => {
this.tech_.on('timeupdate', timeupdateHandler);
}, true);
this.tech_.on('timeupdate', timeupdateHandler);
}

Object.defineProperties(this, {
Expand Down Expand Up @@ -236,17 +242,19 @@ class TextTrack extends Track {
if (!TextTrackMode[newMode]) {
return;
}
if (mode === newMode) {
return;
}

mode = newMode;
if (!this.preload_ && mode !== 'disabled' && this.cues.length === 0) {
// On-demand load.
loadTrack(this.src, this);
}
this.tech_.off('timeupdate', timeupdateHandler);

if (mode !== 'disabled') {
this.tech_.ready(() => {
this.tech_.on('timeupdate', timeupdateHandler);
}, true);
} else {
this.tech_.off('timeupdate', timeupdateHandler);
this.tech_.on('timeupdate', timeupdateHandler);
}
/**
* An event that fires when mode changes on this track. This allows
Expand Down
6 changes: 4 additions & 2 deletions test/unit/tech/tech.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,9 @@ QUnit.test('dispose() should stop time tracking', function(assert) {
QUnit.test('dispose() should clear all tracks that are passed when its created', function(assert) {
const audioTracks = new AudioTrackList([new AudioTrack(), new AudioTrack()]);
const videoTracks = new VideoTrackList([new VideoTrack(), new VideoTrack()]);
const textTracks = new TextTrackList([new TextTrack({tech: {}}),
new TextTrack({tech: {}})]);
const pretech = new Tech();
const textTracks = new TextTrackList([new TextTrack({tech: pretech}),
new TextTrack({tech: pretech})]);

assert.equal(audioTracks.length, 2, 'should have two audio tracks at the start');
assert.equal(videoTracks.length, 2, 'should have two video tracks at the start');
Expand All @@ -167,6 +168,7 @@ QUnit.test('dispose() should clear all tracks that are passed when its created',
'should hold text tracks that we passed'
);

pretech.dispose();
tech.dispose();

assert.equal(audioTracks.length, 0, 'should have zero audio tracks after dispose');
Expand Down
1 change: 1 addition & 0 deletions test/unit/tracks/html-track-element-list.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const defaultTech = {
textTracks() {},
on() {},
off() {},
one() {},
currentTime() {}
};

Expand Down
2 changes: 1 addition & 1 deletion test/unit/tracks/html-track-element.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ QUnit.test('fires loadeddata when track cues become populated', function(assert)
changes++;
};
const htmlTrackElement = new HTMLTrackElement({
tech() {}
tech: this.tech
});

htmlTrackElement.addEventListener('load', loadHandler);
Expand Down
34 changes: 27 additions & 7 deletions test/unit/tracks/text-track-list-converter.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ if (Html5.supportsNativeTextTracks()) {
tech: {
crossOrigin() {
return null;
}
},
on() {},
one() {}
}
});

Expand All @@ -75,6 +77,7 @@ if (Html5.supportsNativeTextTracks()) {
}
};
},
on() {},
crossOrigin() {
return null;
},
Expand Down Expand Up @@ -108,7 +111,9 @@ if (Html5.supportsNativeTextTracks()) {
tech: {
crossOrigin() {
return null;
}
},
on() {},
one() {}
}
});

Expand Down Expand Up @@ -140,6 +145,7 @@ if (Html5.supportsNativeTextTracks()) {
crossOrigin() {
return null;
},
on() {},
textTracks() {
return tt;
},
Expand Down Expand Up @@ -169,7 +175,9 @@ QUnit.test('trackToJson_ produces correct representation for emulated track obje
tech: {
crossOrigin() {
return null;
}
},
on() {},
one() {}
}
});

Expand All @@ -191,7 +199,9 @@ QUnit.test('textTracksToJson produces good json output for emulated only', funct
tech: {
crossOrigin() {
return null;
}
},
on() {},
one() {}
}
});

Expand All @@ -203,7 +213,9 @@ QUnit.test('textTracksToJson produces good json output for emulated only', funct
tech: {
crossOrigin() {
return null;
}
},
on() {},
one() {}
}
});

Expand All @@ -227,6 +239,8 @@ QUnit.test('textTracksToJson produces good json output for emulated only', funct
crossOrigin() {
return null;
},
on() {},
one() {},
textTracks() {
return tt;
}
Expand Down Expand Up @@ -259,7 +273,9 @@ QUnit.test('jsonToTextTracks calls addRemoteTextTrack on the tech with emulated
tech: {
crossOrigin() {
return null;
}
},
on() {},
one() {}
}
});

Expand All @@ -271,7 +287,9 @@ QUnit.test('jsonToTextTracks calls addRemoteTextTrack on the tech with emulated
tech: {
crossOrigin() {
return null;
}
},
on() {},
one() {}
}
});

Expand All @@ -296,6 +314,8 @@ QUnit.test('jsonToTextTracks calls addRemoteTextTrack on the tech with emulated
crossOrigin() {
return null;
},
on() {},
one() {},
textTracks() {
return tt;
},
Expand Down
2 changes: 1 addition & 1 deletion test/unit/tracks/text-tracks.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ QUnit.test('removes cuechange event when text track is hidden for emulated track
numTextTrackChanges++;
});

tt.mode = 'showing';
tt.mode = 'disabled';
this.clock.tick(1);
assert.equal(
numTextTrackChanges, 1,
Expand Down

0 comments on commit db46578

Please sign in to comment.