fix(browse): clear refs when iframe auto-detaches in getActiveFrameOrPage

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)
This commit is contained in:
Yashwant Kotipalli 2026-05-03 17:43:00 -07:00
parent bf65487162
commit 369c7f230f
2 changed files with 162 additions and 1 deletions

View File

@ -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;
}

View File

@ -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<string, RefEntry> {
const r = new Map<string, RefEntry>();
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);
});
});