From c2dc615534efcd3d77f7b2c7c5dc68f1cb078019 Mon Sep 17 00:00:00 2001 From: ayushsingh82 Date: Wed, 12 Aug 2026 02:43:33 +0530 Subject: [PATCH] fix: give each profile runtime an unleased anchor page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CloakSessionManager.getPage() used to adopt the context's initial about:blank page as an ordinary leased user page the first time a session asked for one. Releasing the final leased page could then begin browser/context shutdown; a command arriving before that shutdown finished propagating would fail on its first navigation with "Target page, context or browser has been closed". Reserve the initial page as ProfileRuntime.anchorPage instead — never leased out, excluded from listPages/tab selection/page IDs since it's never added to the pages map. Every getPage()/newPage() lease now always creates its own page. If the anchor is ever unexpectedly closed while the context is still healthy, it's recreated (under the existing per-profile page-creation lock) before the next leased page close, so closing the last leased page can never drop the context to zero pages. Fixes #276 --- .../runtime/local-cloak/provider.test.ts | 33 +++-- .../local-cloak/session-manager.test.ts | 119 +++++++++++++++++- .../runtime/local-cloak/session-manager.ts | 66 ++++++++-- 3 files changed, 199 insertions(+), 19 deletions(-) diff --git a/src/browser/runtime/local-cloak/provider.test.ts b/src/browser/runtime/local-cloak/provider.test.ts index 147f7d92..886b02eb 100644 --- a/src/browser/runtime/local-cloak/provider.test.ts +++ b/src/browser/runtime/local-cloak/provider.test.ts @@ -47,17 +47,29 @@ function fakePage(url: string, initialViewport: { width: number; height: number } function makeProviderWithFakePage(initialViewport: { width: number; height: number } | null = { width: 1280, height: 720 }) { - const pages = [fakePage('https://example.com/', initialViewport)]; + // `anchor` stands in for Chromium's own default initial tab — the runtime reserves it as + // ProfileRuntime.anchorPage on launch and never leases it out. `page` is the object the + // first real newPage() call hands back (whether triggered by a session's first getPage or + // an explicit new tab), matching what every test here treats as "the" first real page — + // pages[0] stays the untouched anchor, pages[1] is the first real one, exactly like the + // pre-fix layout, so index-based assertions on multi-tab flows don't need to change. + const anchor = fakePage('about:blank'); + const page = fakePage('https://example.com/', initialViewport); + const pages = [anchor]; + let realPagesCreated = 0; const cdpSession = { send: vi.fn().mockResolvedValue(undefined), detach: vi.fn().mockResolvedValue(undefined) }; const browser = { contexts: vi.fn(() => [context]) }; const context = { browser: vi.fn(() => browser), on: vi.fn(), - pages: vi.fn(() => pages.filter((page) => !page.isClosed())), + pages: vi.fn(() => pages.filter((p) => !p.isClosed())), newPage: vi.fn(async () => { - const page = fakePage('about:blank'); - pages.push(page); - return page; + // A counter, not pages.length — some tests push externally-simulated pages (e.g. a + // popup) into `pages` before any real newPage() call happens. + const created = realPagesCreated === 0 ? page : fakePage('about:blank'); + realPagesCreated += 1; + pages.push(created); + return created; }), newCDPSession: vi.fn().mockResolvedValue(cdpSession), cookies: vi.fn().mockResolvedValue([{ name: 'sid', value: '1', domain: 'example.com', path: '/' }]), @@ -67,7 +79,7 @@ function makeProviderWithFakePage(initialViewport: { width: number; height: numb baseDir: '/tmp/webcmd-test', launchPersistentContext: vi.fn().mockResolvedValue(context), }); - return { provider, browser, page: pages[0], pages, context, cdpSession }; + return { provider, browser, page, pages, context, cdpSession }; } describe('LocalCloakRuntimeProvider', () => { @@ -654,7 +666,9 @@ describe('LocalCloakRuntimeProvider', () => { }); await provider.dispatch({ id: 'exec', action: 'exec', session: 'work', surface: 'browser', code: 'document.readyState', profileId: 'default' }); - expect(pages[1].evaluate).toHaveBeenCalledWith('document.readyState'); + // pages[0] is the anchor, pages[1] is the 'navigate' session's own page, pages[2] is the + // second (index 1 among leased pages) tab that 'tabs new' created and this binds to. + expect(pages[2].evaluate).toHaveBeenCalledWith('document.readyState'); }); it('returns a typed bind error when the requested Cloak tab is missing', async () => { @@ -759,8 +773,11 @@ describe('LocalCloakRuntimeProvider', () => { await expect(provider.dispatch({ id: 'close-window', action: 'close-window', session: 'first', surface: 'browser', page: second.page, profileId: 'default' })) .resolves.toMatchObject({ id: 'close-window', ok: true, data: { closed: true, page: second.page } }); + // pages[0] is the anchor, pages[1] is the 'first' session's own page (left open), pages[2] + // is the 'second' tab that 'tabs new' created and that this closes by identity. expect(pages[0].isClosed()).toBe(false); - expect(pages[1].close).toHaveBeenCalled(); + expect(pages[1].close).not.toHaveBeenCalled(); + expect(pages[2].close).toHaveBeenCalled(); expect(first.page).not.toBe(second.page); }); }); diff --git a/src/browser/runtime/local-cloak/session-manager.test.ts b/src/browser/runtime/local-cloak/session-manager.test.ts index 5b531871..882163ee 100644 --- a/src/browser/runtime/local-cloak/session-manager.test.ts +++ b/src/browser/runtime/local-cloak/session-manager.test.ts @@ -140,7 +140,9 @@ describe('CloakSessionManager', () => { background: true, focus: false, }); - expect(launched.context.newPage).not.toHaveBeenCalled(); + // The runtime's initial page becomes the anchor (never leased), so the first, foreground + // lease creates its own page via newPage; only the explicit background lease uses CDP. + expect(launched.context.newPage).toHaveBeenCalledTimes(1); }); it('creates an explicit background tab without focusing Chromium', async () => { @@ -163,7 +165,9 @@ describe('CloakSessionManager', () => { background: true, focus: false, }); - expect(launched.context.newPage).not.toHaveBeenCalled(); + // The runtime's initial page becomes the anchor (never leased), so the first, foreground + // lease creates its own page via newPage; only the explicit background tab uses CDP. + expect(launched.context.newPage).toHaveBeenCalledTimes(1); }); it('creates an explicit foreground tab through Playwright', async () => { @@ -234,7 +238,9 @@ describe('CloakSessionManager', () => { expect(second.context).toBe(launched.context); expect(first.page).toBe(second.page); expect(first.pageId).toBe(second.pageId); - expect(launched.context.newPage).not.toHaveBeenCalled(); + // Exactly one real page creation for the coalesced lease — the runtime's initial page + // became the anchor, so it's not what the two concurrent callers end up sharing. + expect(launched.context.newPage).toHaveBeenCalledTimes(1); expect(launched.cdp.send).not.toHaveBeenCalled(); }); @@ -731,4 +737,111 @@ describe('CloakSessionManager', () => { }); expect(launchPersistentContext).toHaveBeenCalledTimes(2); }); + + describe('runtime-owned anchor page', () => { + it('keeps the runtime\'s initial page open as the anchor when the only leased page is released', async () => { + const launched = fakeContext(); + const leasedPage = fakeContext().page; + launched.context.newPage.mockResolvedValue(leasedPage); + const manager = new CloakSessionManager({ + baseDir: '/tmp/webcmd-test', + launchPersistentContext: vi.fn().mockResolvedValue(launched.context), + }); + + const lease = await manager.getPage({ profileId: 'default', session: 'work', surface: 'browser' }); + expect(lease.page).toBe(leasedPage); + + await manager.release({ profileId: 'default', session: 'work', surface: 'browser' }); + + expect(leasedPage.close).toHaveBeenCalled(); + expect(launched.page.close).not.toHaveBeenCalled(); + expect(manager.activeProfileIds()).toEqual(['default']); + }); + + it('reuses the same runtime for an immediate getPage after releasing the final leased page', async () => { + const launched = fakeContext(); + const leasedPage = fakeContext().page; + launched.context.newPage.mockResolvedValue(leasedPage); + const launchPersistentContext = vi.fn().mockResolvedValue(launched.context); + const manager = new CloakSessionManager({ baseDir: '/tmp/webcmd-test', launchPersistentContext }); + + await manager.getPage({ profileId: 'default', session: 'work', surface: 'browser' }); + await manager.release({ profileId: 'default', session: 'work', surface: 'browser' }); + const next = await manager.getPage({ profileId: 'default', session: 'work-again', surface: 'browser' }); + + expect(next.context).toBe(launched.context); + expect(launchPersistentContext).toHaveBeenCalledTimes(1); + }); + + it('recreates the anchor before closing the final leased page if the anchor was unexpectedly closed', async () => { + const launched = fakeContext(); + const leasedPage = fakeContext().page; + const recreatedAnchor = fakeContext().page; + launched.context.newPage + .mockResolvedValueOnce(leasedPage) + .mockResolvedValueOnce(recreatedAnchor); + const manager = new CloakSessionManager({ + baseDir: '/tmp/webcmd-test', + launchPersistentContext: vi.fn().mockResolvedValue(launched.context), + }); + + await manager.getPage({ profileId: 'default', session: 'work', surface: 'browser' }); + launched.page.isClosed.mockReturnValue(true); // the anchor closed out from under us + + await manager.release({ profileId: 'default', session: 'work', surface: 'browser' }); + + expect(launched.context.newPage).toHaveBeenCalledTimes(2); // leased page + recreated anchor + expect(leasedPage.close).toHaveBeenCalled(); + }); + + it('never exposes the anchor page through listPages', async () => { + const launched = fakeContext(); + const manager = new CloakSessionManager({ + baseDir: '/tmp/webcmd-test', + launchPersistentContext: vi.fn().mockResolvedValue(launched.context), + }); + + await manager.getPage({ profileId: 'default', session: 'work', surface: 'browser' }); + + expect(await manager.listPages({ profileId: 'default' })).toHaveLength(1); + }); + + it('gives each profile its own independent anchor page', async () => { + const peer = fakeContext(); + const actor = fakeContext(); + peer.context.newPage.mockResolvedValue(fakeContext().page); + actor.context.newPage.mockResolvedValue(fakeContext().page); + const launchPersistentContext = vi.fn() + .mockResolvedValueOnce(peer.context) + .mockResolvedValueOnce(actor.context); + const manager = new CloakSessionManager({ baseDir: '/tmp/webcmd-test', launchPersistentContext }); + + await manager.getPage({ profileId: 'peer', session: 'work', surface: 'browser' }); + await manager.getPage({ profileId: 'actor', session: 'work', surface: 'browser' }); + await manager.release({ profileId: 'peer', session: 'work', surface: 'browser' }); + await manager.release({ profileId: 'actor', session: 'work', surface: 'browser' }); + + expect(peer.page.close).not.toHaveBeenCalled(); + expect(actor.page.close).not.toHaveBeenCalled(); + expect(manager.activeProfileIds().sort()).toEqual(['actor', 'peer']); + }); + + it('does not lease the anchor page out to freshPage requests', async () => { + const launched = fakeContext(); + const leasedPage = fakeContext().page; + launched.context.newPage.mockResolvedValue(leasedPage); + const manager = new CloakSessionManager({ + baseDir: '/tmp/webcmd-test', + launchPersistentContext: vi.fn().mockResolvedValue(launched.context), + }); + const key = { profileId: 'default', session: 'work', surface: 'browser' as const }; + + const first = await manager.getPage(key); + expect(first.page).not.toBe(launched.page); + + const fresh = await manager.getPage({ ...key, freshPage: true }); + expect(fresh.page).not.toBe(launched.page); + expect(launched.page.close).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/browser/runtime/local-cloak/session-manager.ts b/src/browser/runtime/local-cloak/session-manager.ts index 644fb679..ed99433b 100644 --- a/src/browser/runtime/local-cloak/session-manager.ts +++ b/src/browser/runtime/local-cloak/session-manager.ts @@ -83,6 +83,22 @@ interface ProfileRuntime { pages: Map; selectedPageId?: string; lastSeenAt: number; + /** + * An unleased `about:blank` page the runtime itself owns, so the Chromium context never + * drops to zero pages. Without it, releasing the final leased page can begin browser/context + * shutdown; a command arriving before that shutdown finishes propagating then fails with + * "Target page, context or browser has been closed". Never leased to a task or site session, + * and never exposed through listPages, tab selection, page IDs, or network capture. + * + * Undefined only in the (practically nonexistent) case where launchPersistentContext handed + * back a context with zero pages already open — created lazily on demand instead of blocking + * runtime registration on it, so a context close mid-creation can't orphan the whole launch. + */ + anchorPage: PlaywrightPage | undefined; +} + +function anchorPageIsHealthy(runtime: ProfileRuntime): boolean { + return runtime.anchorPage !== undefined && !pageIsClosed(runtime.anchorPage); } export interface CloakSessionManagerOptions { @@ -165,19 +181,18 @@ export class CloakSessionManager { runtime.pages.delete(leaseKey); this.clearIdleTimer(existing); if (runtime.selectedPageId === existing.pageId) runtime.selectedPageId = undefined; + // Already inside withPageCreationLock via the wrapper above — use the lock-free + // variant here, or re-acquiring the same per-profile lock would deadlock. + await this.ensureAnchorPageLocked(runtime); if (!pageIsClosed(existing.page)) await existing.page.close().catch(() => {}); } return this.createPageWithRecoveryAttempt( profileId, input.windowMode, - (candidate) => { - const existingPages = candidate.context.pages(); - // freshPage must never adopt a leftover tab — its whole point is a clean DOM. - return !freshPage && existingPages[0] && candidate.pages.size === 0 - ? existingPages[0] - : this.createPage(candidate.context, input.windowMode); - }, + // Never adopt an existing context page here — the runtime's initial page is reserved + // as its anchor (see launchProfileRuntime), so every lease gets its own fresh page. + (candidate) => this.createPage(candidate.context, input.windowMode), (candidate, page) => { const pageId = nextPageId(); const entry: PageEntry = { page, pageId, session, surface, siteSession: input.siteSession, idleTimeout: input.idleTimeout }; @@ -371,6 +386,7 @@ export class CloakSessionManager { runtime.pages.delete(leaseKey); this.clearIdleTimer(entry); if (runtime.selectedPageId === entry.pageId) runtime.selectedPageId = undefined; + await this.ensureAnchorPage(profileId, runtime); if (!pageIsClosed(entry.page)) await entry.page.close().catch(() => {}); runtime.lastSeenAt = Date.now(); return entry.pageId; @@ -391,6 +407,7 @@ export class CloakSessionManager { this.clearIdleTimer(entry); if (runtime.selectedPageId === entry.pageId) runtime.selectedPageId = undefined; if (entry.siteSession !== 'persistent' && !pageIsClosed(entry.page)) { + await this.ensureAnchorPage(profileId, runtime); await entry.page.close().catch(() => {}); } } @@ -437,12 +454,36 @@ export class CloakSessionManager { if (!isProfileAlreadyInUseError(err) || !(await this.recoverLockedProfile(userDataDir))) throw err; context = await launchPersistentContext(launchOptions); } - const runtime = { context, pages: new Map(), lastSeenAt: Date.now() }; + // Chromium already hands launchPersistentContext its own initial about:blank page; adopt + // that synchronously as the anchor. Registration must not block on creating one when it's + // missing — see the ProfileRuntime.anchorPage doc comment — so a missing anchor is filled + // in lazily by ensureAnchorPage the next time a leased page is about to close. + const runtime: ProfileRuntime = { context, pages: new Map(), lastSeenAt: Date.now(), anchorPage: context.pages()[0] }; this.attachRuntimeLifecycle(profileId, runtime); this.profiles.set(profileId, runtime); return runtime; } + /** + * Recreate the anchor if it's missing or was unexpectedly closed while the context is still + * healthy, so closing the caller's own leased page next can never drop the context to zero + * pages. Callers that are not already inside withPageCreationLock for this profile must use + * ensureAnchorPage instead — this variant does not acquire the lock itself. + */ + private async ensureAnchorPageLocked(runtime: ProfileRuntime): Promise { + if (anchorPageIsHealthy(runtime)) return; + runtime.anchorPage = await this.createPage(runtime.context); + } + + private async ensureAnchorPage(profileId: string, runtime: ProfileRuntime): Promise { + if (anchorPageIsHealthy(runtime)) return; + if (this.profiles.get(profileId) !== runtime) return; + await this.withPageCreationLock(profileId, async () => { + if (this.profiles.get(profileId) !== runtime) return; + await this.ensureAnchorPageLocked(runtime); + }); + } + private invalidateProfileRuntime(profileId: string, runtime: ProfileRuntime): void { if (this.profiles.get(profileId) !== runtime) return; this.profiles.delete(profileId); @@ -557,10 +598,19 @@ export class CloakSessionManager { if (runtime.selectedPageId === entry.pageId) runtime.selectedPageId = undefined; runtime.lastSeenAt = Date.now(); if (entry.siteSession !== 'persistent' && !pageIsClosed(entry.page)) { + const profileId = this.profileIdForRuntime(runtime); + if (profileId) await this.ensureAnchorPage(profileId, runtime); await entry.page.close().catch(() => {}); } } + private profileIdForRuntime(runtime: ProfileRuntime): string | undefined { + for (const [profileId, candidate] of this.profiles.entries()) { + if (candidate === runtime) return profileId; + } + return undefined; + } + private clearIdleTimer(entry: PageEntry): void { if (entry.idleTimer) clearTimeout(entry.idleTimer); entry.idleTimer = undefined;