mirror of https://github.com/garrytan/gstack.git
migrate 3 CDP-session sites to lifecycle helpers
Fixes the CDP-target leak class identified by /codex outside-voice on
the eng review (D11 EXPAND_SCOPE). All three sites called
`page.context().newCDPSession(page)` directly and either forgot the
detach entirely (cdp-bridge cache cleanup), only detached on the
success path (write-commands archive), or detached on framenavigated
but not page-close (cdp-inspector).
- cdp-bridge.ts: `getCdpSession` now delegates to
`getOrCreateCdpSession`, which registers a `page.once('close')` hook
that BOTH removes the cache entry AND calls `session.detach()`.
- cdp-inspector.ts: same migration for the inspector's session pool.
Keeps the existing framenavigated detach (more granular than close
for DOM/CSS state invalidation) plus an inspector-layer close hook
for the initializedPages WeakSet.
- write-commands.ts archive: wraps Page.captureSnapshot in
withCdpSession so the detach runs in `finally`, including the path
where captureSnapshot throws.
The static-grep tripwire (next commit) pins the invariant so future
direct calls to newCDPSession fail CI.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
ec48c176f5
commit
213a234d10
|
|
@ -95,20 +95,14 @@ export async function getOrCreateCdpSession(
|
||||||
|
|
||||||
// ─── $B cdp bridge ─────────────────────────────────────────────
|
// ─── $B cdp bridge ─────────────────────────────────────────────
|
||||||
|
|
||||||
// Per-page CDPSession cache. Created lazily on first allow-listed call,
|
// Per-page CDPSession cache. Lifecycle delegated to getOrCreateCdpSession
|
||||||
// cleaned up when the page closes. TODO(C2): migrate to getOrCreateCdpSession
|
// which registers a close hook that BOTH removes the cache entry AND calls
|
||||||
// so the session also detaches on close (currently only the cache entry is
|
// session.detach() — pre-helper code only did the former, leaving the
|
||||||
// removed; the Chromium-side target lingers).
|
// Chromium-side target attached.
|
||||||
const sessionCache: WeakMap<Page, any> = new WeakMap();
|
const sessionCache: WeakMap<Page, any> = new WeakMap();
|
||||||
|
|
||||||
async function getCdpSession(page: Page): Promise<any> {
|
async function getCdpSession(page: Page): Promise<any> {
|
||||||
let s = sessionCache.get(page);
|
return getOrCreateCdpSession(page, sessionCache);
|
||||||
if (s) return s;
|
|
||||||
s = await page.context().newCDPSession(page);
|
|
||||||
sessionCache.set(page, s);
|
|
||||||
// Clear cache on detach so we don't hold a stale handle.
|
|
||||||
page.once('close', () => sessionCache.delete(page));
|
|
||||||
return s;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface CdpDispatchInput {
|
export interface CdpDispatchInput {
|
||||||
|
|
|
||||||
|
|
@ -13,6 +13,7 @@
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import type { Page } from 'playwright';
|
import type { Page } from 'playwright';
|
||||||
|
import { getOrCreateCdpSession } from './cdp-bridge';
|
||||||
|
|
||||||
// ─── Types ──────────────────────────────────────────────────────
|
// ─── Types ──────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
@ -106,15 +107,23 @@ async function getOrCreateSession(page: Page): Promise<any> {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
session = await page.context().newCDPSession(page);
|
session = await getOrCreateCdpSession(page, cdpSessions);
|
||||||
cdpSessions.set(page, session);
|
|
||||||
|
|
||||||
// Enable DOM and CSS domains
|
// Enable DOM and CSS domains on first init for this page. The session
|
||||||
await session.send('DOM.enable');
|
// itself is cached + close-detached by getOrCreateCdpSession; the
|
||||||
await session.send('CSS.enable');
|
// initializedPages WeakSet is inspector-layer state that needs its
|
||||||
initializedPages.add(page);
|
// own close hook to stay in sync.
|
||||||
|
if (!initializedPages.has(page)) {
|
||||||
|
await session.send('DOM.enable');
|
||||||
|
await session.send('CSS.enable');
|
||||||
|
initializedPages.add(page);
|
||||||
|
page.once('close', () => initializedPages.delete(page));
|
||||||
|
}
|
||||||
|
|
||||||
// Auto-detach on navigation
|
// Auto-detach on navigation — DOM/CSS domain state is tied to the
|
||||||
|
// document. Close-detach (from getOrCreateCdpSession) handles the
|
||||||
|
// tab-close case; framenavigated catches in-tab navigation that
|
||||||
|
// invalidates inspector state without closing the tab.
|
||||||
page.once('framenavigated', () => {
|
page.once('framenavigated', () => {
|
||||||
try {
|
try {
|
||||||
session.detach().catch(() => {});
|
session.detach().catch(() => {});
|
||||||
|
|
|
||||||
|
|
@ -18,6 +18,7 @@ import type { SetContentWaitUntil } from './tab-session';
|
||||||
import { TEMP_DIR, isPathWithin } from './platform';
|
import { TEMP_DIR, isPathWithin } from './platform';
|
||||||
import { SAFE_DIRECTORIES } from './path-security';
|
import { SAFE_DIRECTORIES } from './path-security';
|
||||||
import { modifyStyle, undoModification, resetModifications, getModificationHistory } from './cdp-inspector';
|
import { modifyStyle, undoModification, resetModifications, getModificationHistory } from './cdp-inspector';
|
||||||
|
import { withCdpSession } from './cdp-bridge';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Aggressive page cleanup selectors and heuristics.
|
* Aggressive page cleanup selectors and heuristics.
|
||||||
|
|
@ -1409,9 +1410,10 @@ export async function handleWriteCommand(
|
||||||
validateOutputPath(outputPath);
|
validateOutputPath(outputPath);
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const cdp = await page.context().newCDPSession(page);
|
const data = await withCdpSession(page, async (cdp) => {
|
||||||
const { data } = await cdp.send('Page.captureSnapshot', { format: 'mhtml' });
|
const result = await cdp.send('Page.captureSnapshot', { format: 'mhtml' });
|
||||||
await cdp.detach();
|
return (result as { data: string }).data;
|
||||||
|
});
|
||||||
fs.writeFileSync(outputPath, data);
|
fs.writeFileSync(outputPath, data);
|
||||||
return `Archive saved: ${outputPath} (${Math.round(data.length / 1024)}KB, MHTML)`;
|
return `Archive saved: ${outputPath} (${Math.round(data.length / 1024)}KB, MHTML)`;
|
||||||
} catch (err: any) {
|
} catch (err: any) {
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue