Repository navigation
fix: allow textTracks redefinition on repeated HLS source-set - #1086
Merged
Merged
Conversation
Object.defineProperty defaults configurable to false for accessor descriptors. Html5HlsJS() redefines el.textTracks on every source-set, so a second HLS source on the same tech/element (e.g. hls/h265 -> hls/h264 fallback after a 400) threw "Cannot redefine property: textTracks" before hls.attachMedia()/loadSource() ever ran, killing playback silently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ Deploy Preview for cld-vp-esm-pages ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for cld-video-player ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TypeError: Cannot redefine property: textTracksand dead playback when a second HLS source is set on the same player/element — e.g.hls/h265returns 400 and the app falls back tohls/h264on the same instance.Html5HlsJS()callsObject.defineProperty(el, 'textTracks', {...})on every source-set, butconfigurablewas never set, so it defaults tofalse. The second call throws beforehls.attachMedia()/hls.loadSource()ever run, so the fallback source never attaches.configurable: trueto that descriptor.Trade-off
Adding
configurable: trueremoves a fail-fast guard. If some future refactor ever causesHtml5HlsJS()to run twice on the sameelwithout going through video.js's normal dispose-first lifecycle (Tech.prototype.setSource→disposeSourceHandler()→hls.destroy()→ new source handler), it will now silently reconfigure the property instead of throwing where it would previously have been caught in testing/monitoring. Verified viavideo.jscore that the current, real call pattern always disposes the old source handler and reuses the sametechobject before creating the new one — so the getter closure is functionally identical before and after redefinition, and this trade-off does not present a risk in the actual call path today. It's a latent risk for a different, currently-hypothetical bug class only.Verification
docs/adaptive-streaming.html): confirmed the exact reported stack trace (Html5HlsJS→handleSource→Tech.setSource→Player.src_) with the fix reverted, and confirmed clean playback with the fix applied.test/unit/videojs-contrib-hlsjs-texttracks.test.jsas a permanent regression test, exercising the real module via a capturedHlsSourceHandlerreference (mocking onlyhls.js, irrelevant to this bug).Out of scope (flagged separately, not fixed here)
el.addEventListener('error', ...)is re-added on everyHtml5HlsJS()call but never removed bydispose()— a pre-existing stale-listener leak on repeated source-sets, independent of this bug.test/adaptive-streaming.test.jsvia jest-puppeteer) is currently broken in this checkout for unrelated reasons (jest-environment-puppeteerexport-shape mismatch,page.waitForremoved in the installed Puppeteer version) — confirmed the pre-existing tests in that file also fail in isolation, unrelated to this change.Test plan
test/unit/videojs-contrib-hlsjs-texttracks.test.js)