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

fix: prevent dispose error and text track duplicate listeners #6984

Merged
merged 4 commits into from
Jan 21, 2021
Merged
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
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we also return early in this function if the mode is the same as the current mode? Is there a reason we trigger modechange when a track goes from disabled to disabled again?

Copy link
Member

Choose a reason for hiding this comment

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

Making this change makes sense. I think we'd want to see whether doing that causes any issues for the captions menu, though, I doubt it.


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