fix(browser): close native execution races

Author: waleedlatif1

apps/desktop/src/main/browser-agent/cdp.test.ts

@@ -596,4 +596,56 @@ describe('browser-agent screenshot capture', () => {
     expect(shot.viewport).toBeNull()
     expect(shot.imageSize).toEqual({ width: 1024, height: 512 })
   })
+
+  it.each([
+    [
+      'dimensions',
+      { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024 } },
+      { cssLayoutViewport: { clientWidth: 1024, clientHeight: 512 } },
+    ],
+    [
+      'metric units',
+      { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024 } },
+      { layoutViewport: { clientWidth: 2048, clientHeight: 1024 } },
+    ],
+    [
+      'horizontal scroll offset',
+      { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024, pageX: 0, pageY: 20 } },
+      { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024, pageX: 10, pageY: 20 } },
+    ],
+    [
+      'vertical scroll offset',
+      { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024, pageX: 10, pageY: 20 } },
+      { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024, pageX: 10, pageY: 30 } },
+    ],
+    [
+      'offset validity',
+      { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024, pageX: 0, pageY: 0 } },
+      {
+        cssLayoutViewport: {
+          clientWidth: 2048,
+          clientHeight: 1024,
+          pageX: 0,
+          pageY: Number.NaN,
+        },
+      },
+    ],
+    ['availability', {}, {}],
+  ])(
+    'rejects a capture when viewport %s change during CDP capture',
+    async (_label, before, after) => {
+      const { contents } = captureFixture({ width: 1024, height: 512 })
+      let metricsRead = 0
+      vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => {
+        if (method === 'Page.getLayoutMetrics') {
+          metricsRead++
+          return Promise.resolve(metricsRead === 1 ? before : after)
+        }
+        if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' })
+        return Promise.resolve(undefined)
+      })
+
+      await expect(captureScreenshot(contents)).rejects.toThrow(/viewport changed/)
+    }
+  )
 })

apps/desktop/src/main/browser-agent/cdp.ts

@@ -382,6 +382,14 @@ const SCREENSHOT_CAPTURE_QUALITY = 90
 interface CdpViewport {
   clientWidth: number
   clientHeight: number
+  pageX?: number
+  pageY?: number
+}
+
+interface ScreenshotViewportMetrics extends ScreenshotSize {
+  pageX: number | null
+  pageY: number | null
+  unit: 'css' | 'device'
 }
 
 interface ScreenshotSize {
@@ -396,6 +404,51 @@ export interface ScreenshotCapture {
   imageSize: ScreenshotSize | null
 }
 
+function screenshotViewportMetrics(
+  metrics: {
+    cssLayoutViewport?: CdpViewport
+    layoutViewport?: CdpViewport
+  } | null
+): ScreenshotViewportMetrics | null {
+  const viewport = metrics?.cssLayoutViewport ?? metrics?.layoutViewport
+  const width = viewport?.clientWidth ?? 0
+  const height = viewport?.clientHeight ?? 0
+  if (width <= 0 || height <= 0) return null
+  const pageX = viewport?.pageX
+  const pageY = viewport?.pageY
+  const hasPagePosition = pageX !== undefined || pageY !== undefined
+  if (
+    hasPagePosition &&
+    (pageX === undefined ||
+      pageY === undefined ||
+      !Number.isFinite(pageX) ||
+      !Number.isFinite(pageY))
+  ) {
+    return null
+  }
+  return {
+    width,
+    height,
+    pageX: pageX ?? null,
+    pageY: pageY ?? null,
+    unit: metrics?.cssLayoutViewport ? 'css' : 'device',
+  }
+}
+
+function sameScreenshotViewport(
+  before: ScreenshotViewportMetrics | null,
+  after: ScreenshotViewportMetrics | null
+): boolean {
+  if (!before || !after) return false
+  return (
+    before.unit === after.unit &&
+    before.width === after.width &&
+    before.height === after.height &&
+    before.pageX === after.pageX &&
+    before.pageY === after.pageY
+  )
+}
+
 /**
  * Screenshot via CDP (works while the view is hidden), bounded in resolution.
  *
@@ -419,9 +472,9 @@ export async function captureScreenshot(contents: WebContents): Promise<Screensh
     layoutViewport?: CdpViewport
   }>(contents, 'Page.getLayoutMetrics').catch(() => null)
 
-  const captureViewport = metrics?.cssLayoutViewport ?? metrics?.layoutViewport
-  const width = captureViewport?.clientWidth ?? 0
-  const height = captureViewport?.clientHeight ?? 0
+  const captureViewport = screenshotViewportMetrics(metrics)
+  const width = captureViewport?.width ?? 0
+  const height = captureViewport?.height ?? 0
   const cssWidth = metrics?.cssLayoutViewport?.clientWidth ?? 0
   const cssHeight = metrics?.cssLayoutViewport?.clientHeight ?? 0
   const cssViewport = cssWidth > 0 && cssHeight > 0 ? { width: cssWidth, height: cssHeight } : null
@@ -432,6 +485,13 @@ export async function captureScreenshot(contents: WebContents): Promise<Screensh
     format: 'jpeg',
     quality: SCREENSHOT_CAPTURE_QUALITY,
   })
+  const metricsAfterCapture = await send<{
+    cssLayoutViewport?: CdpViewport
+    layoutViewport?: CdpViewport
+  }>(contents, 'Page.getLayoutMetrics').catch(() => null)
+  if (!sameScreenshotViewport(captureViewport, screenshotViewportMetrics(metricsAfterCapture))) {
+    throw new Error('The page viewport changed or could not be verified during screenshot capture')
+  }
   const captured = `data:image/jpeg;base64,${result.data}`
 
   const targetWidth = Math.round(width * scale)

apps/desktop/src/main/browser-agent/driver.test.ts

@@ -2481,6 +2481,45 @@ describe('credential protection', () => {
     ).toBe(true)
   })
 
+  it('accepts stable truncated page identity with deprecated device metrics', async () => {
+    const contents = await openPage()
+    const fullUrl = `https://example.com/${'u'.repeat(5000)}`
+    const fullTitle = `Example ${'t'.repeat(600)}`
+    vi.mocked(contents.getURL).mockReturnValue(fullUrl)
+    vi.mocked(contents.getTitle).mockReturnValue(fullTitle)
+    mockScreenshotImage({ width: 1024, height: 512 })
+    vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => {
+      if (method === 'Page.getLayoutMetrics') {
+        return Promise.resolve({ layoutViewport: { clientWidth: 2048, clientHeight: 1024 } })
+      }
+      if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' })
+      return Promise.resolve(undefined)
+    })
+    respondWith(contents, {
+      getViewportInfo: {
+        url: fullUrl.slice(0, 4096),
+        title: fullTitle.slice(0, 500),
+        width: 1024,
+        height: 512,
+      },
+    })
+
+    const result = await driver.executeTool('chat-test', 'browser_screenshot', {})
+
+    expect(result).toMatchObject({
+      ok: true,
+      result: {
+        scale: 1,
+        viewport: {
+          url: fullUrl.slice(0, 4096),
+          title: fullTitle.slice(0, 500),
+          width: 1024,
+          height: 512,
+        },
+      },
+    })
+  })
+
   it('rejects an undecodable screenshot instead of returning an unverified scale', async () => {
     const contents = await openPage()
     mockScreenshotImage(null)
@@ -2522,7 +2561,9 @@ describe('credential protection', () => {
     const contents = await openPage()
     mockScreenshotImage({ width: 1024, height: 256 })
     vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => {
-      if (method === 'Page.getLayoutMetrics') return Promise.resolve({})
+      if (method === 'Page.getLayoutMetrics') {
+        return Promise.resolve({ layoutViewport: { clientWidth: 1024, clientHeight: 256 } })
+      }
       if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' })
       return Promise.resolve(undefined)
     })
@@ -2540,4 +2581,58 @@ describe('credential protection', () => {
     expect(result.ok).toBe(false)
     expect(result.error).toMatch(/viewport changed while the screenshot was captured/)
   })
+
+  it('rejects a screenshot when the document navigates during capture', async () => {
+    const contents = await openPage()
+    mockScreenshotImage({ width: 1024, height: 512 })
+    vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => {
+      if (method === 'Page.getLayoutMetrics') {
+        return Promise.resolve({
+          cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024 },
+        })
+      }
+      if (method === 'Page.captureScreenshot') {
+        emitContentsEvent(contents, 'did-navigate')
+        return Promise.resolve({ data: 'c2lt' })
+      }
+      return Promise.resolve(undefined)
+    })
+
+    const result = await driver.executeTool('chat-test', 'browser_screenshot', {})
+
+    expect(result.ok).toBe(false)
+    expect(result.error).toMatch(/page changed while its screenshot was being captured/)
+  })
+
+  it.each(['url', 'title'] as const)(
+    'rejects a screenshot when the page %s changes during capture',
+    async (identityField) => {
+      const contents = await openPage()
+      mockScreenshotImage({ width: 1024, height: 512 })
+      const initialUrl = contents.getURL()
+      const initialTitle = contents.getTitle()
+      let currentUrl = initialUrl
+      let currentTitle = initialTitle
+      vi.mocked(contents.getURL).mockImplementation(() => currentUrl)
+      vi.mocked(contents.getTitle).mockImplementation(() => currentTitle)
+      vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => {
+        if (method === 'Page.getLayoutMetrics') {
+          return Promise.resolve({
+            cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024 },
+          })
+        }
+        if (method === 'Page.captureScreenshot') {
+          if (identityField === 'url') currentUrl = 'https://example.com/changed'
+          else currentTitle = 'Changed title'
+          return Promise.resolve({ data: 'c2lt' })
+        }
+        return Promise.resolve(undefined)
+      })
+
+      const result = await driver.executeTool('chat-test', 'browser_screenshot', {})
+
+      expect(result.ok).toBe(false)
+      expect(result.error).toMatch(/page changed while its screenshot was being captured/)
+    }
+  )
 })

apps/desktop/src/main/browser-agent/driver.ts

@@ -2127,13 +2127,37 @@ async function executeToolInner(
     }
 
     case 'browser_screenshot': {
-      const contents = session.requireAutomationTab().view.webContents
+      const capturedTab = session.requireAutomationTab()
+      const contents = capturedTab.view.webContents
+      const capturedNavigationEpoch = navigationEpoch(contents)
+      const capturedUrl = contents.getURL()
+      const capturedTitle = contents.getTitle()
+      const capturedViewportUrl = capturedUrl.slice(0, 4096)
+      const capturedViewportTitle = capturedTitle.slice(0, 500)
+      const captureIsCurrent = (): boolean => {
+        const activeTab = session.automationTab()
+        return (
+          activeTab?.id === capturedTab.id &&
+          activeTab.view.webContents === contents &&
+          !contents.isDestroyed() &&
+          navigationEpoch(contents) === capturedNavigationEpoch &&
+          contents.getURL() === capturedUrl &&
+          contents.getTitle() === capturedTitle
+        )
+      }
+      const assertCaptureIsCurrent = (): void => {
+        if (captureIsCurrent()) return
+        throw new ToolError(
+          'The page changed while its screenshot was being captured. Retry browser_screenshot before using image coordinates.'
+        )
+      }
       const shot = await cdp.captureScreenshot(contents).catch(() => null)
       if (shot === null) {
         throw new ToolError(
           'Could not capture the page. Use browser_snapshot or browser_read_text instead.'
         )
       }
+      assertCaptureIsCurrent()
       if (shot.dataUrl.length > 8_000_000) {
         throw new ToolError(
           'The screenshot result was too large to return safely. Use browser_snapshot or browser_read_text instead.'
@@ -2146,11 +2170,21 @@ async function executeToolInner(
       }
       const viewport = shot.viewport
         ? {
-            url: contents.getURL().slice(0, 4096),
-            title: contents.getTitle().slice(0, 500),
+            url: capturedViewportUrl,
+            title: capturedViewportTitle,
             ...shot.viewport,
           }
         : await execInPage(contents, getViewportInfo, []).catch(() => null)
+      assertCaptureIsCurrent()
+      if (
+        !shot.viewport &&
+        isRecordLike(viewport) &&
+        (viewport.url !== capturedViewportUrl || viewport.title !== capturedViewportTitle)
+      ) {
+        throw new ToolError(
+          'The page changed while its screenshot viewport was being verified. Retry browser_screenshot before using image coordinates.'
+        )
+      }
       let scale = shot.scale
       const viewportWidth =
         isRecordLike(viewport) && typeof viewport.width === 'number' ? viewport.width : 0