Skip to content

Handle Playwright active tabs across contexts - #347

Open
ehfeng wants to merge 6 commits into
mainfrom
hypeship/fix-cross-context-page
Open

Handle Playwright active tabs across contexts#347
ehfeng wants to merge 6 commits into
mainfrom
hypeship/fix-cross-context-page

Conversation

@ehfeng

@ehfeng ehfeng commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • resolve Chrome's active tab against pages from every Playwright browser context
  • inject the selected page's owning context, retry target lookup once, and fall back to an open page instead of failing execution
  • add cross-context regression coverage and document the selection/fallback behavior

Validation

  • rebuilt the daemon with the affected image's compiler, hotpatched it through the browser File API, and restarted Chromium
  • reproduced the original cross-context failure on the baseline bundle
  • passed 30/30 consecutive patched cycles; every cycle recreated and foregrounded a second context before a fresh daemon probe
  • verified same-context focus still selects a newly opened tab and then an older refocused tab
  • go vet ./e2e/...
  • non-e2e race suite passed except for one transient devtoolsproxy temp-directory cleanup failure; the focused rerun passed
  • the container e2e target could not start because its local test image was unavailable; no test logic ran
  • CI passed the headful/headless image builds, server unit suite, packaged-image e2e suite, and static/security checks

Note

Medium Risk
Changes core Playwright daemon tab binding used by every execute call; wrong page/context could affect automation, though fallback reduces hard failures.

Overview
Playwright execute now picks the injected page from Chrome’s CDP active-tab signal across all Playwright browser contexts, not only the first context’s pages. The injected context is the BrowserContext that owns that page.

resolveActivePage was refactored to collect active tab targets, map them to Playwright pages in any context, retry once on races, and fall back to the last open page (or a new page) instead of failing when CDP lookup doesn’t match. OpenAPI docs were updated to describe context, retry, and fallback; use browser.contexts() for explicit selection.

E2E adds a cross-context scenario (second context foregrounded with a newer blank tab behind it) so page must be the foreground tab, not the newest-page fallback.

Reviewed by Cursor Bugbot for commit 3034b5a. Bugbot is set up for automated code reviews on this repo. Configure here.

@ehfeng
ehfeng marked this pull request as ready for review August 21, 2026 22:28
@ehfeng
ehfeng requested review from masnwilliams and rgarcia August 21, 2026 22:28
Comment thread server/e2e/e2e_playwright_test.go Outdated

@rgarcia rgarcia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed incrementally against CUS-580 (the #333 hard-fail regression). overall direction is right: cross-context resolution, retry-once, and fallback all match what the ticket asked for, and the e2e genuinely fails against #333-as-shipped via its Success assertions. a few things worth a look before ship:

test gaps

  • server/e2e/e2e_playwright_test.go:170page.context() === context can never be false: executeCode derives context from page.context() (playwright-daemon.ts:263), so this passes no matter which page gets injected (bugbot flagged this too). assert page identity instead — e.g. return page.url() and require it contains second-context.
  • server/e2e/e2e_playwright_test.go:146-171 — even a url assertion is masked by the fallback today: secondPage is also the newest page, so findLast returns it too whenever resolution silently misses. to pin the mechanism: open a third page in context 1 after secondPage (making it newest), foreground secondPage, then assert the url — correct resolution → data: url, fallback → about:blank. that's the assertion distinguishing "resolution won" from "fallback bailed us out", and it guards exactly the silent-wrong-tab mode.
  • note: cus-580's intermittent variant isn't exercised — with one window / two contexts there's a single deterministic tabActive target; the multi-window lottery (one active tab per window, unspecified order) needs 2 windows. fine not to chase in ci, just acknowledging.
  • nit: require.Equal(t, true, ...)require.True(...)

daemon

  • server/runtime/playwright-daemon.ts:211 — total resolution failure now degrades silently to newest-page binding. the fallback itself matches the cus-580 proposal, but a future chrome dropping embedderData.tabActive would be invisible. worth a console.error('[playwright-daemon] active-tab resolution failed; falling back') before returning null (the daemon logs this way elsewhere).
  • server/runtime/playwright-daemon.ts:192-211 — retry/fallback is hard to follow: magic 2, two bare catches whose meanings differ (inner = skip this tab target, outer = re-snapshot and retry the attempt) but aren't stated. behavior is defensible; structure needs a comment pass or the extraction below.
  • server/runtime/playwright-daemon.ts:150-189 — session churn: each execute burns 1 + W browser-level cdp sessions (activeTabTargetIds, then one per active tab) plus up-to-P throwaway per-page sessions, ×2 attempts worst case. one root session can serve the whole pass, and page→targetId is immutable so it memoizes cleanly in a WeakMap.
  • question: cus-580 also asked to skip resolution entirely for ops that never touch page (listPages etc.). this pr answers that only via the fallback — every call still pays full cdp resolution. intentional?

suggested shape

same semantics, cheaper and (we think) easier to follow — the core idea is an explicit join between playwright's page registry and chrome's tab strip, keyed by cdp page-target id:

═══ MODULE STATE ══════════════════════════════════════════════
targetIdMemo : WeakMap<Page, string>   # Page → immutable cdp page-target id;
                                       # WeakMap lets closed Pages gc

═══ PER EXECUTE CALL ══════════════════════════════════════════
execute(code):
  contexts       = browserInstance.contexts()
  defaultContext = contexts[0] ?? await browserInstance.newContext()
  pages          = contexts.flatMap(c => c.pages())      # snapshot, ALL contexts

  page =
    resolveActivePage(browserInstance)              # the join ↓
    ?? pages.findLast(p => !p.isClosed())           # fallback 1: newest open page
    ?? await defaultContext.newPage()               # fallback 2: blank page
  context = page.context()                          # derived — pair cannot mismatch

resolveActivePage(browser) → Page | null:
  root = await browser.newBrowserCDPSession()   # ONE session for the whole pass
  try:
    for attempt in 1..2:                        # focus can shift mid-join; retry
      page = findActivePage(browser, root).catch(() => null)   # re-snapshots both sides
      if page: return page
    console.error('[playwright-daemon] active-tab resolution failed; falling back')
    return null
  finally:
    detach(root)

findActivePage(browser, root) → Page | null:
  # joins two views by cdp page-target id:
  #   LEFT  — playwright's live registry (what user code must receive)
  #   RIGHT — chrome's tab strip (sole source of truth for foreground,
  #           embedderData.tabActive, one per window)
  # null when the join finds nothing: no active tabs reported, active tabs'
  # related pages closed/prerender-only/unmatched (focus shifted mid-scan),
  # or zero open pages. callers treat null as "fall back".
  pageByTargetId = {}
  for page in browser.contexts().flatMap(c => c.pages()):
    if page.isClosed(): continue
    pageByTargetId[targetIdOf(page)] = page     # LEFT side (memoized)

  {targetInfos} = root.send('Target.getTargets', filter tabs)
  for tab in targetInfos where embedderData.tabActive == true:
    for id in pageTargetIdsForTab(root, tab.targetId):
      if pageByTargetId[id]:                    # ← THE JOIN
        return pageByTargetId[id]
  return null

targetIdOf(page) → string:
  if targetIdMemo.has(page): return cached                 # steady state: free
  s = page.context().newCDPSession(page)   # page-scoped channel over the SAME
                                           # connection; getTargetInfo() w/o args
                                           # = "who am i", only answerable while
                                           # attached to that page
  {targetInfo} = s.send('Target.getTargetInfo')
  detach(s)
  targetIdMemo.set(page, targetInfo.targetId)
  return targetInfo.targetId

pageTargetIdsForTab(root, tabId) → ids[]:
  # tab targets and page targets have DIFFERENT ids and chrome exposes no direct
  # tab→page query. autoAttachRelated(tabId) is the only bridge: attaching fires
  # Target.attachedToTarget naming each page target under the tab.
  listen → collect ids where type=='page' && !subtype          # skips prerender
  root.send('Target.autoAttachRelated', { targetId: tabId })
  stop listening; return ids

known trade-offs: first-ever call probes all P pages up front instead of early-exiting (worst case identical to today; steady state ≈ free); the WeakMap is technically cross-request state, though it caches an immutable string rather than a live session — #333's "no state between calls" comment was about sessions specifically.

@masnwilliams masnwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting one change before this goes into the image release: make the cross-context regression test deterministic.

The current assertion, page.context() === context, cannot verify the behavior under test because executeCode derives context from page.context(). Repeating that probe 30 times against unchanged browser state does not add coverage. I ran the new setup/probe against the exact pre-PR daemon and it passed 30/30, so this test does not fail on the implementation that this PR is intended to fix.

A deterministic regression case is available without relying on Chrome's unspecified ordering across active windows:

  1. Create a second context and navigate its page to a unique data: URL.
  2. Close every page in the first context while leaving that context open.
  3. Execute a fresh script.
  4. Assert that the injected page has the unique URL and that its owning context is at index 1.

The old resolver searches only browser.contexts()[0], so it fails this setup; this PR's resolver searches the second context and succeeds. The 30-iteration loop can then be removed.

The target revalidation race, sequential CDP session churn, and fallback observability are worth follow-ups, but I would not block this release on them because the main active-tab resolver already had those characteristics. Once the regression test reliably distinguishes old and new behavior and CI/BugBot are green, this is good to ship.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9aea03f. Configure here.

Comment thread server/e2e/e2e_playwright_test.go
@ehfeng
ehfeng requested review from masnwilliams and rgarcia August 24, 2026 21:59
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.

3 participants