-
Notifications
You must be signed in to change notification settings - Fork 82
Bind playwright execute page to Chrome's active tab
#333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
eee7745
c7cd73e
ac9769b
5d9bab0
f2de954
57546c8
5195138
6703f7f
eb7424b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ | |
| import { createServer, Socket } from 'net'; | ||
| import { unlinkSync, existsSync } from 'fs'; | ||
| import { transform } from 'esbuild'; | ||
| import { chromium as chromiumPW, Browser } from 'playwright-core'; | ||
| import { chromium as chromiumPW, Browser, BrowserContext, Page } from 'playwright-core'; | ||
| import { chromium as chromiumPR } from 'patchright'; | ||
|
|
||
| const SOCKET_PATH = process.env.PLAYWRIGHT_DAEMON_SOCKET || '/tmp/playwright-daemon.sock'; | ||
|
|
@@ -131,6 +131,58 @@ async function ensureBrowserConnection(): Promise<Browser> { | |
| } | ||
| } | ||
|
|
||
| // Resolves the browser's actual foreground tab via CDP rather than guessing from | ||
| // tab-creation order. Chrome 150+ populates `TargetInfo.embedderData.tabActive` | ||
| // on `tab` targets from the real tab strip state; the Playwright `Page` for that | ||
| // tab is found by relating the tab target to its page target with | ||
| // `Target.autoAttachRelated`. Every session used here is temporary and detached | ||
| // before returning, so this adds no cross-request state to the daemon. | ||
| async function resolveActivePage(browser: Browser, context: BrowserContext): Promise<Page> { | ||
| const root = await browser.newBrowserCDPSession(); | ||
|
|
||
| try { | ||
| const { targetInfos } = await root.send('Target.getTargets', { | ||
| filter: [{ type: 'tab', exclude: false }, { exclude: true }], | ||
| }); | ||
|
|
||
| const activeTab = targetInfos.find(target => (target.embedderData as any)?.tabActive === true); | ||
| if (!activeTab) throw new Error('no foreground tab reported by CDP'); | ||
|
|
||
| const relatedPageIds = new Set<string>(); | ||
| root.on('Target.attachedToTarget', event => { | ||
| if (event.targetInfo.type === 'page' && !event.targetInfo.subtype) { | ||
| relatedPageIds.add(event.targetInfo.targetId); | ||
| } | ||
| }); | ||
|
|
||
| await root.send('Target.autoAttachRelated', { | ||
| targetId: activeTab.targetId, | ||
| waitForDebuggerOnStart: false, | ||
| filter: [{ type: 'page', exclude: false }, { exclude: true }], | ||
| }); | ||
|
|
||
| for (const page of context.pages()) { | ||
| try { | ||
| const session = await context.newCDPSession(page); | ||
| try { | ||
| const { targetInfo } = await session.send('Target.getTargetInfo'); | ||
| if (relatedPageIds.has(targetInfo.targetId)) return page; | ||
| } finally { | ||
| await session.detach().catch(() => {}); | ||
| } | ||
| } catch { | ||
| // A crashed or closing page can fail CDP session setup/queries; skip it | ||
| // rather than aborting the search for the real foreground tab. | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| throw new Error('foreground tab has no matching Playwright page'); | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| } finally { | ||
| await root.detach().catch(() => {}); | ||
| } | ||
| } | ||
|
|
||
| async function executeCode(request: ExecuteRequest, signal: AbortSignal): Promise<ExecuteResponse> { | ||
| const { id, code } = request; | ||
|
|
||
|
|
@@ -172,7 +224,11 @@ async function executeCode(request: ExecuteRequest, signal: AbortSignal): Promis | |
| const contexts = browserInstance.contexts(); | ||
| const context = contexts.length > 0 ? contexts[0] : await browserInstance.newContext(); | ||
| const pages = context.pages(); | ||
| const page = pages.length > 0 ? pages[0] : await context.newPage(); | ||
| // Bind `page` to the actual foreground tab (see resolveActivePage). Using | ||
| // pages[0] bound `page` to the oldest tab regardless of which was active, so | ||
| // calls like page.pdf() operated on the wrong tab whenever more than one was | ||
| // open. | ||
| const page = pages.length > 0 ? await resolveActivePage(browserInstance, context) : await context.newPage(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Active tab ignored across contextsMedium Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit f2de954. Configure here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving this as-is: contexts()[0] is the pre-existing selection this whole endpoint has always used for the 'context' variable exposed to user code — it predates this PR and applies equally to the empty-context newPage() path a few lines up. resolveActivePage searching only within that same context is consistent with what 'context' means for this daemon, not a new limitation this PR introduces. If the true foreground tab lives in a different browser context, the old heuristic would have silently bound 'page' to the wrong context's tab; this now fails loudly instead, which is arguably safer. Multi-context support for this endpoint would be a bigger, separate change to how 'context' itself is resolved. |
||
|
|
||
| const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor; | ||
| const userFunction = new AsyncFunction('page', 'context', 'browser', jsCode); | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing pages may not attach
High Severity
resolveActivePagefillsrelatedPageIdsonly fromTarget.attachedToTargetafterTarget.autoAttachRelated. CDP documents that command as monitoring related-target creation and reporting newly created related targets. For already-open tabs, the set can stay empty, so the loop never matches and execution fails withforeground tab has no matching Playwright page.Reviewed by Cursor Bugbot for commit 5d9bab0. Configure here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified this doesn't reproduce against the image's actual Chrome (152.0.7977.42): the e2e test added in this PR (TestPlaywrightExecuteAPI, https://github.com/kernel/kernel-images/actions/runs/32164219559/job/95800815565) explicitly covers a pre-existing related target — it opens a second tab, then several execute calls later brings the original tab (created well before the autoAttachRelated call in question, not a target created in that same request) back to the foreground and asserts the daemon binds to it. That assertion passed in CI. In practice, autoAttachRelated on this target's browser session fires attachedToTarget for already-open related targets at call time, not just future creations, so relatedPageIds is populated correctly for this case. Leaving as-is; will revisit if this surfaces on a real session.