From 7c91f526d893b198f2644924b77abe3e6f7ab0a8 Mon Sep 17 00:00:00 2001 From: Mark Date: Sat, 16 May 2026 10:59:17 +0200 Subject: [PATCH] fix(01-review): sweep #2+#3+#4 recorder lifecycle hardening (re-entrance + start throw + dual-track teardown) --- src/offscreen/recorder.ts | 119 +++++++++++++++++++++++++++++++++----- 1 file changed, 105 insertions(+), 14 deletions(-) diff --git a/src/offscreen/recorder.ts b/src/offscreen/recorder.ts index c396206..a226982 100644 --- a/src/offscreen/recorder.ts +++ b/src/offscreen/recorder.ts @@ -241,6 +241,13 @@ async function startRecording(): Promise { * MediaStream, навешиваем обработчики, стартуем без timeslice (один * dataavailable на остановку → один blob на сегмент → один keyframe в * заголовке за счёт fresh-encoder-инициализации). + * + * Sweep #3 hardening: the MediaRecorder constructor and .start() may + * throw (codec mid-session unavailability, GPU/driver hot-swap, etc.). + * Without a guard the rotation chain would silently die — onSegmentStopped + * is never called again, no new RECORDING_ERROR is emitted, the popup + * shows green while nothing is recording. Catch + classify + emit + + * tear down the session so the operator gets actionable feedback. */ function startNewSegment(): void { if (mediaStream === null) { @@ -248,20 +255,50 @@ function startNewSegment(): void { return; } currentChunks = []; - videoRecorder = new MediaRecorder(mediaStream, { - mimeType: VIDEO_MIME, - videoBitsPerSecond: VIDEO_BITRATE, - }); - videoRecorder.ondataavailable = onDataAvailable; - videoRecorder.onstop = onSegmentStopped; - videoRecorder.onerror = (event) => logger.error('MediaRecorder error:', event); - // Без timeslice: одно событие dataavailable придёт по .stop() — это - // ровно один blob, содержащий целиком сегмент (EBML-заголовок + - // кластеры). Так каждый сегмент гарантированно декодируется - // независимо. Если когда-то потребуется живая стат-телеметрия, - // можно дать timeslice назад без изменения семантики ротации. - videoRecorder.start(); - scheduleRotation(); + try { + videoRecorder = new MediaRecorder(mediaStream, { + mimeType: VIDEO_MIME, + videoBitsPerSecond: VIDEO_BITRATE, + }); + videoRecorder.ondataavailable = onDataAvailable; + videoRecorder.onstop = onSegmentStopped; + videoRecorder.onerror = (event) => logger.error('MediaRecorder error:', event); + // Без timeslice: одно событие dataavailable придёт по .stop() — это + // ровно один blob, содержащий целиком сегмент (EBML-заголовок + + // кластеры). Так каждый сегмент гарантированно декодируется + // независимо. Если когда-то потребуется живая стат-телеметрия, + // можно дать timeslice назад без изменения семантики ротации. + videoRecorder.start(); + scheduleRotation(); + } catch (err) { + // Sweep #3 fix: MediaRecorder construction / start failed mid-session. + // Most common cause is the codec becoming unavailable (GPU hot-swap, + // driver change). Classify, notify, and tear down so the operator + // sees an actionable error instead of silent recording cessation. + const code = classifyCaptureError(err); + const msg = err instanceof Error ? err.message : String(err); + logger.warn('startNewSegment failed (raw):', msg); + logger.error('startNewSegment failed (code):', code); + chrome.runtime.sendMessage({ type: 'RECORDING_ERROR', error: code }); + // Tear down — same shape as onUserStoppedSharing's cleanup so the + // SW-side state machine doesn't get a half-recorded session. + const streamToStop = mediaStream; + mediaStream = null; + videoRecorder = null; + if (rotationTimerId !== null) { + clearTimeout(rotationTimerId); + rotationTimerId = null; + } + if (streamToStop !== null) { + streamToStop.getTracks().forEach((t) => { + try { + t.stop(); + } catch (terr) { + logger.warn('track.stop() during startNewSegment cleanup failed:', terr); + } + }); + } + } } /** @@ -340,7 +377,23 @@ function onDataAvailable(event: BlobEvent): void { currentChunks.push(event.data); } +// Sweep #4 fix: onUserStoppedSharing is registered with `{ once: true }` +// on EACH track of the captured stream. With multiple tracks (video + +// audio — audio currently always disabled per D-13 but the registration +// walks `getTracks()` defensively), the handler could fire twice if +// both tracks emit `ended` in close succession. `resetBuffer()` and the +// stream-cleanup steps are idempotent, but the `chrome.runtime.sendMessage` +// would double-emit RECORDING_ERROR — same double-emit pattern WR-02 +// fixed for the codec/getDisplayMedia path. Guard with a flag that +// gates the broadcast + cleanup. +let teardownInProgress = false; + function onUserStoppedSharing(): void { + if (teardownInProgress) { + logger.log('onUserStoppedSharing already ran — second track ended, ignoring'); + return; + } + teardownInProgress = true; logger.log('Operator stopped sharing — clearing buffer'); resetBuffer(); if (videoRecorder !== null && videoRecorder.state !== 'inactive') { @@ -356,6 +409,14 @@ function onUserStoppedSharing(): void { } videoRecorder = null; chrome.runtime.sendMessage({ type: 'RECORDING_ERROR', error: 'user-stopped-sharing' }); + // Reset the guard so a future startRecording → onUserStoppedSharing + // cycle works correctly. Place AFTER the broadcast so a same-tick + // second invocation is still gated. + // Use a microtask deferral (queueMicrotask) so the reset happens after + // every synchronous re-entrant invocation in the same dispatcher tick. + queueMicrotask(() => { + teardownInProgress = false; + }); } function stopRecording(): void { @@ -441,7 +502,28 @@ function onPortMessage(message: unknown): void { // Any unknown port message type is silently dropped (T-1-04 defense-in-depth). } +// Sweep #2 fix: in-flight guard against re-entrant encodeAndSendBuffer. +// The SW only issues one REQUEST_BUFFER per saveArchive in production, but +// nothing in the API design forbids back-to-back REQUEST_BUFFER messages. +// Without this guard, two concurrent encode passes would: +// (a) interleave `++segmentSeq` increments — each request's segments +// end up with non-contiguous timestamps that look like gaps from +// the SW sort perspective (benign but noisy) +// (b) both call getSegments() against the same buffer snapshot, so +// the SW would receive two BUFFERs with overlapping content if +// it accidentally combined them +// (c) inflate base64-encode CPU cost unnecessarily during the +// encode latency window (~150 ms for 3 segments) +// The guard drops the second concurrent call with a warn log; the SW +// timeout fires cleanly and the next saveArchive retries on the fresh +// post-completion state. +let encodeInFlight = false; + async function encodeAndSendBuffer(): Promise { + if (encodeInFlight) { + logger.warn('encodeAndSendBuffer already running — dropping concurrent call'); + return; + } // CR-01 fix: capture the port identity BEFORE the await. If `keepalivePort` // is replaced by a fresh reconnect during base64 encoding, posting on the // new port would silently leak the BUFFER to a stranger — the SW's @@ -454,6 +536,15 @@ async function encodeAndSendBuffer(): Promise { logger.warn('encodeAndSendBuffer called without an active port — drop'); return; } + encodeInFlight = true; + try { + await doEncodeAndSendBuffer(portAtRequest); + } finally { + encodeInFlight = false; + } +} + +async function doEncodeAndSendBuffer(portAtRequest: chrome.runtime.Port): Promise { // WR-09 fix: an in-flight segment lacks the Matroska finalization that // MediaRecorder.stop() performs (SegmentSize, Cues) — splicing it onto // a finalized tail re-introduces the "File ended prematurely" symptom