[Detail Bug] Screencast recordings silently lose frames when onFrame is registered after start

[detail-app]

Detail Bug Report

https://app.detail.dev/org_06887db3-bf54-40ab-976d-46c66ab2b840/bugs/bug_9edff65c-aeb3-454e-bb26-3628079b495f

Summary

• Context: The screencast module captures browser frames via Chrome DevTools Protocol, allowing users to record page interactions by registering a callback via onFrame() and starting capture with start().
• Bug: Frames are silently dropped if the onFrame() callback is not registered before start() is called, or if onFrame() is never called at all.
• Actual vs. expected: Frames that arrive before onFrame() is registered are permanently lost without any error or warning, whereas users expect either all frames to be captured once onFrame() is eventually set, or an explicit error if no handler is registered.
• Impact: Silent data loss where users lose screen recording frames with no indication of why, leading to incomplete or empty recordings that are impossible to debug.

Code with Bug

From packages/screencast/src/index.js:

const onScreencastFrame = ({ data, metadata, sessionId }) => {
  cdp.send('Page.screencastFrameAck', { sessionId }).catch(() => {})
  if (metadata.timestamp && onFrame) onFrame(data, metadata)  // <-- BUG 🔴 drops frames when onFrame not set
}

API allows start() before onFrame():

return {
  start: () => {
    attachFrameListener()  // <-- BUG 🔴 starts receiving frames before handler may be registered
    return cdp.send('Page.startScreencast', opts)
  },
  onFrame: fn => (onFrame = fn),
  stop: () => {
    detachFrameListener()
    return cdp.send('Page.stopScreencast').catch(() => {})
  }
}

Explanation

The CDP Page.screencastFrame event handler only forwards frames when onFrame is defined. Since start() attaches the listener immediately, any frames that arrive before onFrame() is registered are acknowledged to CDP but never delivered to user code, with no error/warning.

Failing Test

test('race condition: frames lost when onFrame set after start', async t => {
  const browserless = await getBrowserContext(t)
  const page = await browserless.page()

  let receivedFramesCount = 0
  let cdpEventsCount = 0

  // Monitor CDP events directly
  const cdp = page._client()
  const listener = () => {
    cdpEventsCount++
  }
  cdp.on('Page.screencastFrame', listener)

  const screencast = createScreencast(page, {
    quality: 0,
    format: 'png',
    everyNthFrame: 1
  })

  // Start WITHOUT setting onFrame callback
  await screencast.start()

  // Navigate to trigger frames
  await page.goto('https://example.com', { waitUntil: 'domcontentloaded' })
  await new Promise(resolve => setTimeout(resolve, 1000))

  // NOW set the callback after frames have already arrived
  screencast.onFrame(() => {
    receivedFramesCount++
  })

  await new Promise(resolve => setTimeout(resolve, 500))
  await screencast.stop()

  // Result: CDP received 2 frames, callback received 0 frames
  t.true(cdpEventsCount > 0)  // CDP received frames
  t.is(receivedFramesCount, 0)  // But callback got nothing
})

Test output:

CDP events received: 2
onFrame callback received: 0
✔ race condition: frames lost when onFrame set after start

Recommended Fix

Fail fast if start() is called without an onFrame handler:

start: () => {
  if (!onFrame) {
    throw new Error('onFrame callback must be registered before calling start()')
  }
  attachFrameListener()
  return cdp.send('Page.startScreencast', opts)
}

History

This bug was introduced in commit 0b9ad2b. The commit was intended to improve performance by moving CDP event listeners from eager to lazy registration (attaching on start(), detaching on stop()). However, when refactoring the frame callback, it added a defensive && onFrame check that changed the behavior from "crash if onFrame undefined" to "silently drop frames if onFrame undefined", accidentally introducing the silent failure mode.