Skip to content

fix: code review round 1 (glm-5.3) - a half-built player, encrypted fallbacks, small fixes - #518

Merged
Nawid3333 merged 2 commits into
mainfrom
fix/review-round1
Oct 21, 2026
Merged

Nawid3333 merged 2 commits into
mainfrom
fix/review-round1

Conversation

@Nawid3333

@Nawid3333 Nawid3333 commented Oct 21, 2026 •

Copy link
Copy Markdown
Owner

Each finding was checked against the code; the false ones were dropped. Every
fix in the player and the background has a test that fails without it.

Player

  • FastStreamClient: a player whose setup() or setSource() throws is destroyed
    and no longer left in client.player, where its video and loader lived on,
    ticked by the main loop, after a load that ended in an error.
  • HLS: a segment loaded by HLSLoader's fallback (one the fragment store has no
    place for) is decrypted too (HLSFragmentRequester.decryptionFor); it reached
    the decoder still encrypted.
  • OPFSManager.setup: a worker whose init fails is terminated and forgotten.
  • The quality menu closes and empties itself when the next video has no
    levels, as the audio and language menus do; a track without a language is
    read out as "Unknown", not "undefined".
  • xml2vtt: a cue without a start is skipped, one without a duration lasts to
    the next cue (or 2 s); NaN timestamps broke the whole track.
  • The speed menu saves the speed 500 ms after its last change, as the volume
    is saved: an arrow key held on the speed button wrote it at every repeat.
  • Logs: a received source and an added source are logged by URL and mode,
    without their headers (the page's Cookie and Authorization).
  • content.js: the visibility observer gets the 0.3 threshold its own check
    uses, so it fires on the way back too.

Background

  • TabTracker.reset() forgets the tab's playing players.
  • VpnProxyMirror guesses a tab's proxy for hosts the page was not seen
    asking only when it is Firefox VPN's (a proxy authorization header): a PAC
    file's per-host choice was overridden for an intranet CDN. A host the page
    itself reached through a proxy is copied as before (the first version
    dropped those too and failed firefox-vpn.e2e.mjs; docs/notes/player.md).
  • A failed tabs.remove or permissions-page tabs.create no longer rejects
    unhandled.

Workflows

  • ci.yml, firefox-beta.yml, firefox-stable.yml: the host log and the e2e logs
    are uploaded for a job that ran out of time (cancelled) too.
  • live-streams.yml: the report names cancelled and timed-out jobs as well.
  • security-alerts.yml: an advisory without a summary no longer breaks the
    table; update-prs.yml: a manifest path with a leading slash matches.

Tools

  • setup-dev-profile resolves .dev-profile from the repository, not the
    working directory; recut-patch refuses an empty resolved file; rebuild's and
    the splicer's doc comments say what the code does; setup.sh states the
    Ubuntu releases it targets (the ones CI runs on).

Tests: lint, typecheck, unit (2807 passed), workflows.

Not changed, for a decision:

  • A speed set with a key (the presets, +/-) is not saved for the next video; only the speed menu saves it.
  • Two security trade-offs from the review (header rules through the player page, signed URL tokens in saved archives) are written up separately.

🤖 Generated with Claude Code

Nawid3333 and others added 2 commits October 21, 2026 22:51
…allbacks, small fixes

Each finding was checked against the code; the false ones were dropped. Every
fix in the player and the background has a test that fails without it.

Player
- FastStreamClient: a player whose setup() or setSource() throws is destroyed
  and no longer left in client.player, where its video and loader lived on,
  ticked by the main loop, after a load that ended in an error.
- HLS: a segment loaded by HLSLoader's fallback (one the fragment store has no
  place for) is decrypted too (HLSFragmentRequester.decryptionFor); it reached
  the decoder still encrypted.
- OPFSManager.setup: a worker whose init fails is terminated and forgotten.
- The quality menu closes and empties itself when the next video has no
  levels, as the audio and language menus do; a track without a language is
  read out as "Unknown", not "undefined".
- xml2vtt: a cue without a start is skipped, one without a duration lasts to
  the next cue (or 2 s); NaN timestamps broke the whole track.
- The speed menu saves the speed 500 ms after its last change, as the volume
  is saved: an arrow key held on the speed button wrote it at every repeat.
- Logs: a received source and an added source are logged by URL and mode,
  without their headers (the page's Cookie and Authorization).
- content.js: the visibility observer gets the 0.3 threshold its own check
  uses, so it fires on the way back too.

Background
- TabTracker.reset() forgets the tab's playing players.
- VpnProxyMirror copies only Firefox VPN's proxy (one with a proxy
  authorization header); a PAC file's choice for the page misrouted
  FastStream's own requests (docs/notes/player.md).
- A failed tabs.remove or permissions-page tabs.create no longer rejects
  unhandled.

Workflows
- ci.yml, firefox-beta.yml, firefox-stable.yml: the host log and the e2e logs
  are uploaded for a job that ran out of time (cancelled) too.
- live-streams.yml: the report names cancelled and timed-out jobs as well.
- security-alerts.yml: an advisory without a summary no longer breaks the
  table; update-prs.yml: a manifest path with a leading slash matches.

Tools
- setup-dev-profile resolves .dev-profile from the repository, not the
  working directory; recut-patch refuses an empty resolved file; rebuild's and
  the splicer's doc comments say what the code does; setup.sh states the
  Ubuntu releases it targets (the ones CI runs on).

Tests: lint, typecheck, unit (2807 passed), workflows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The first version copied no proxy without Firefox VPN's token, and
firefox-vpn.e2e.mjs failed everywhere: its VPN stand-in is a plain http
proxy (Firefox sends the token only to an https one), so the stream's host
was no longer copied and the player offered no VPN button.

A host the page itself reached through a proxy is copied again, token or
not: for that host a PAC file answers the same. Only the tab's proxy, the
guess for hosts the page was not seen asking (noteOwnRequest), is kept for
Firefox VPN's alone, which is where a PAC file's per-host choice was
overridden; a tab whose page goes through a proxy without a token forgets
its VPN. Two unit tests fail on main's code; docs/notes/player.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333
Nawid3333 merged commit c325352 into main Oct 21, 2026
36 checks passed
@Nawid3333
Nawid3333 deleted the fix/review-round1 branch October 21, 2026 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant