From 369c7f230fe7114039f3ed94a5852406a1cba3e4 Mon Sep 17 00:00:00 2001 From: Yashwant Kotipalli Date: Sun, 3 May 2026 17:43:00 -0700 Subject: [PATCH] fix(browse): clear refs when iframe auto-detaches in getActiveFrameOrPage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asymmetric cleanup between two equivalent staleness conditions: onMainFrameNavigated() → clearRefs() + activeFrame = null ✓ getActiveFrameOrPage() → activeFrame = null (refs NOT cleared) ✗ Both paths see the same staleness condition — refs were captured against a frame that no longer exists. The main-frame path correctly clears both pieces of state. The iframe-detach path nulls the frame but leaves the refMap intact. The lazy click-time check in `resolveRef` (tab-session.ts:97) partially saves us — `entry.locator.count()` on a detached-frame locator throws or returns 0, so the click errors out as "Ref X is stale". But the user has no signal that frame context silently changed underfoot: the next `snapshot` runs against `this.page` (main) while old iframe refs still litter `refMap` with the same role+name keys. New refs collide with stale ones, the resolver picks one at random, the user clicks the wrong element. TODOS.md line 816-820 documents "Detached frame auto-recovery" as a shipped iframe-support feature in v0.12.1.0. This restores the documented intent — the recovery should leave the session in a clean state, not a half-cleared one. Fix: 1 line — add `this.clearRefs()` next to `this.activeFrame = null` inside the if-branch. Test plan: - [x] New regression test: 4/4 pass - refs cleared when getActiveFrameOrPage detects detached iframe - refs preserved when active frame is still attached (no regression) - refs preserved when no frame set (page-level path untouched) - matches onMainFrameNavigated symmetry — both paths reach the same clean end state - [x] `bun run build` clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- browse/src/tab-session.ts | 9 +- browse/test/tab-session-frame-detach.test.ts | 154 +++++++++++++++++++ 2 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 browse/test/tab-session-frame-detach.test.ts diff --git a/browse/src/tab-session.ts b/browse/src/tab-session.ts index 739942689a..dccabf28f4 100644 --- a/browse/src/tab-session.ts +++ b/browse/src/tab-session.ts @@ -149,9 +149,16 @@ export class TabSession { * Use this for operations that work on both Page and Frame (locator, evaluate, etc.). */ getActiveFrameOrPage(): Page | Frame { - // Auto-recover from detached frames (iframe removed/navigated) + // Auto-recover from detached frames (iframe removed/navigated). Clear + // refs alongside the activeFrame — same staleness condition as + // onMainFrameNavigated() below: refs were captured against a frame + // that no longer exists. Without this, refMap entries linger against + // a dead frame after silently falling back to the main page; the + // next snapshot's role+name keys collide with stale entries and the + // resolver picks one at random. if (this.activeFrame?.isDetached()) { this.activeFrame = null; + this.clearRefs(); } return this.activeFrame ?? this.page; } diff --git a/browse/test/tab-session-frame-detach.test.ts b/browse/test/tab-session-frame-detach.test.ts new file mode 100644 index 0000000000..32f2632b90 --- /dev/null +++ b/browse/test/tab-session-frame-detach.test.ts @@ -0,0 +1,154 @@ +/** + * Regression: refMap must be cleared when an iframe detaches. + * + * `TabSession.getActiveFrameOrPage()` (tab-session.ts:151) auto-recovers + * from detached iframes by setting `activeFrame = null` and silently + * falling back to the main page. The asymmetric bug: the matching + * `clearRefs()` call is missing. + * + * Compare to `onMainFrameNavigated()` (tab-session.ts:167) — the + * staleness condition is equivalent (refs were captured against a frame + * that no longer exists), and the main-frame path correctly clears both + * the activeFrame AND the refMap: + * + * onMainFrameNavigated(): void { + * this.clearRefs(); // ← clears refs + * this.activeFrame = null; + * this.loadedHtml = null; + * this.loadedHtmlWaitUntil = undefined; + * } + * + * getActiveFrameOrPage(): Page | Frame { + * if (this.activeFrame?.isDetached()) { + * this.activeFrame = null; // ← but no clearRefs() here + * } + * return this.activeFrame ?? this.page; + * } + * + * The lazy click-time staleness check at `resolveRef` (tab-session.ts:97) + * partially saves us — `entry.locator.count()` on a detached-frame + * locator throws or returns 0, so a click against a stale ref errors out + * with "Ref X is stale". But the user has no signal that frame context + * silently changed underfoot: the next `snapshot` runs against + * `this.page` (main) while old iframe refs still litter `refMap` with + * the same role+name keys. New refs collide with stale ones, the + * resolver picks one at random, the user clicks the wrong element. + * + * Behavior the test locks: when an iframe detaches and + * `getActiveFrameOrPage()` auto-recovers, the refMap is cleared in the + * same step (matching the `onMainFrameNavigated` symmetry). TODOS.md + * line 816-820 documents "Detached frame auto-recovery" as a feature; + * this restores the documented intent. + */ + +import { describe, test, expect, beforeEach } from 'bun:test'; +import { TabSession, type RefEntry } from '../src/tab-session'; +import type { Page, Frame, Locator } from 'playwright'; + +// Minimal type-cast mocks. Same pattern as tab-isolation.test.ts — +// pure-logic tests don't launch a browser. +function mockPage(): Page { + return {} as Page; +} + +function mockDetachedFrame(): Frame { + return { isDetached: () => true } as unknown as Frame; +} + +function mockAttachedFrame(): Frame { + return { isDetached: () => false } as unknown as Frame; +} + +function mockRefEntry(role: string, name: string): RefEntry { + return { + locator: {} as Locator, + role, + name, + }; +} + +// Fresh refs Map per call — avoid by-reference mutation poisoning across +// halves of the symmetry test (clearRefs() clears the same Map instance +// the test holds a reference to). +function makeRefs(): Map { + const r = new Map(); + r.set('e1', mockRefEntry('button', 'Submit')); + r.set('e2', mockRefEntry('textbox', 'Email')); + r.set('e3', mockRefEntry('link', 'Forgot password')); + return r; +} + +describe('TabSession — frame detach + ref staleness', () => { + let session: TabSession; + + beforeEach(() => { + session = new TabSession(mockPage()); + session.setRefMap(makeRefs()); + }); + + test('refs cleared when getActiveFrameOrPage detects detached iframe', () => { + // Pre-condition: refs captured inside an iframe context + session.setFrame(mockDetachedFrame()); + expect(session.getRefCount()).toBe(3); + + // Act: caller invokes getActiveFrameOrPage (e.g. via the next /command + // dispatch). The detach gets noticed inside. + const result = session.getActiveFrameOrPage(); + + // Auto-recovery: activeFrame nulled (already worked pre-fix) + expect(session.getFrame()).toBeNull(); + + // The fix: refs ALSO cleared so the next snapshot runs against a + // clean ref namespace. Pre-fix this was 3 — refs lingered against a + // dead frame, colliding with refs the next snapshot would emit. + expect(session.getRefCount()).toBe(0); + }); + + test('refs preserved when active frame is still attached', () => { + // No regression on the happy path — attached frame should NOT + // trigger the cleanup. + session.setFrame(mockAttachedFrame()); + expect(session.getRefCount()).toBe(3); + + session.getActiveFrameOrPage(); + + // Frame still set, refs still present. + expect(session.getFrame()).not.toBeNull(); + expect(session.getRefCount()).toBe(3); + }); + + test('refs preserved when no frame is set (page-level snapshot)', () => { + // No frame ever set → the if-branch never enters → refs untouched. + expect(session.getFrame()).toBeNull(); + expect(session.getRefCount()).toBe(3); + + session.getActiveFrameOrPage(); + + expect(session.getRefCount()).toBe(3); + }); + + test('matches onMainFrameNavigated symmetry (refs+frame both cleared)', () => { + // Pin the design symmetry: both staleness paths (main-frame nav AND + // iframe detach) must clear both pieces of state together. If a + // future refactor splits these, the test fails before merge. + session.setFrame(mockDetachedFrame()); + expect(session.getRefCount()).toBe(3); + + session.onMainFrameNavigated(); + + expect(session.getFrame()).toBeNull(); + expect(session.getRefCount()).toBe(0); + + // Reset with a FRESH Map (the previous one was emptied by clearRefs + // by-reference) and exercise the iframe-detach path. End state must + // match. + session.setRefMap(makeRefs()); + session.setFrame(mockDetachedFrame()); + expect(session.getRefCount()).toBe(3); + + session.getActiveFrameOrPage(); + + expect(session.getFrame()).toBeNull(); + expect(session.getRefCount()).toBe(0); + }); +});