Fix #2472: scope frame ID generator to Browser, not Session#2474
Merged
Conversation
CDP target IDs (`FID-{d:0>10}`) must stay unique for the lifetime of
the CDP connection -- Playwright's `CRBrowser._onAttachedToTarget`
asserts on duplicates and the assertion is fatal (the connection is
unusable afterwards).
Before this fix, `Session.frame_id_gen` reset to 0 in two places:
1. `tearDownActivePage` explicitly reset to 0 after every page
teardown (likely intended to mimic pre-pending-page numbering
within a single Session, but invisible there because the
immediately-following `installNewActivePage` typically reuses
the old frame's explicit `frame_id`, see `replaceRootImmediate`).
2. Fresh Sessions started from the field default of 0. Each
`Target.createBrowserContext` calls `Browser.newSession`, which
deinits the old Session and constructs a new one -- so even
without (1), the next BrowserContext's first page would still
get `FID-0000000001`.
(2) is what trips Playwright on the second `browser.newContext()`
on a connection: the second context's first frame re-issues
`FID-0000000001`, identical to the first context's frame, and
Playwright's `CRBrowser._onAttachedToTarget` raises
`Duplicate target FID-0000000001`.
Move `frame_id_gen` (and `nextFrameId`) from `Session` to `Browser`,
which is per-CDP-connection. Existing callers (`Session.createPage`,
`Frame.zig:1327`, `Frame.zig:1437`, `Worker.zig:74`) still go through
`Session.nextFrameId` -- it's now a thin pass-through to
`browser.nextFrameId()` -- so no call sites change. Removed the
explicit reset in `tearDownActivePage`; it was redundant within a
Session (root navigation reuses the old frame_id) and harmful across
Sessions.
`loader_id_gen` stays on Session: Loader IDs (`LID-...`) are scoped
per-frame in CDP and Playwright doesn't track them in the target
registry, so the per-Session reset is correct there.
Repro (`playwright-core@1.58.2`):
for (let i = 1; i <= 3; i++) {
const ctx = await browser.newContext();
await ctx.newPage();
await ctx.close();
}
Before: cycle 2 throws `Duplicate target FID-0000000001`.
After: 5/5 cycles complete cleanly.
Tests: 653/653 pass. Added regression coverage in
`cdp.target: createTarget assigns unique IDs across BrowserContexts
(issue lightpanda-io#2472)` -- verified to fail against the original source
(reverted Browser.zig and Session.zig, kept the test, ran zig build
test: only the new test fails).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2472.
Summary
CDP target IDs (
FID-{d:0>10}) must stay unique for the lifetime of the CDP connection. Playwright'sCRBrowser._onAttachedToTargetrecords every attached target in an in-memory map and asserts on collision; the assertion is fatal and the connection is unusable afterwards.Before this PR,
Session.frame_id_genreset to 0 in two places:tearDownActivePageexplicitly reset to 0 after every page teardown. The reset was likely intended to mimic pre-pending-page numbering within a single Session, but invisible there because the immediately-followinginstallNewActivePagetypically reuses the old frame's explicitframe_id(seereplaceRootImmediate).Fresh Sessions started from the field default of 0. Each
Target.createBrowserContextcallsBrowser.newSession, which deinits the old Session and constructs a new one -- so even without (1), the next BrowserContext's first page would still getFID-0000000001.(2) is the operative cause of the
Duplicate target FID-0000000001collision in the issue: the secondbrowser.newContext()on a connection allocates a fresh Session whose first page re-issues the sameFID-0000000001as the first context's frame.Fix
Move
frame_id_gen(andnextFrameId) fromSessiontoBrowser.Browseris per-CDP-connection (eachCDPholds oneBrowser, eachBrowserholds oneSessionat a time), so the counter now spans the entire connection lifetime as the CDP spec / Chrome behaviour requires.Existing callers (
Session.createPage,Frame.zig:1327,Frame.zig:1437,Worker.zig:74) still go throughSession.nextFrameId-- it's now a thin pass-through tobrowser.nextFrameId()-- so no call sites change. The explicit reset intearDownActivePageis removed; it was redundant within a Session (root navigation reuses the old frame_id) and harmful across Sessions.loader_id_genstays on Session: Loader IDs (LID-...) are per-frame in CDP and Playwright doesn't track them in the target registry, so the per-Session reset is correct there.Verification
Repro from the issue (
playwright-core@1.58.2):Before:
After:
Tests
zig build test: 653 / 653 pass (652 existing + 1 new).Added
cdp.target: createTarget assigns unique IDs across BrowserContexts (issue #2472)-- creates a target, disposes the context, creates another target, and asserts the twotarget_ids differ. Verified the regression test catches the bug:Reverting the Browser.zig + Session.zig changes (keeping only the test) reproduces the failure.
Notes
Independent of #2399 (Playwright connectOverCDP synthetic-STARTUP promotion) but interacts with it. #2399 makes the synthetic STARTUP target's
targetIddeterministically match the first real frame ID (FID-0000000001) so the synthetic-to-real handoff doesn't mismatch in Playwright's registry. That alignment now holds across BrowserContext lifecycle too -- the second context's first frame isFID-0000000002, not a duplicate of the synthetic.The deferred-dispose path in
CDP.disposeBrowserContext(src/cdp/CDP.zig:383) was a possibly-related second finding noted in the issue (script-eval reentrant teardown leavingbrowser_context != null, surfacing asCannot have more than one browser context at a time). It's not addressed here -- I couldn't isolate it in a standalone repro and it may turn out to be the same root cause surfacing differently. Filing separately if it persists once this lands.