Merge pull request #352 from NianJiuZst/codex/fix-emulation-lifecycle

fix(emulate): scope remembered overrides to the session and attachment

.github/workflows/ci.yml

@@ -215,6 +215,12 @@ jobs:
           BSK_CLICK_CHROME: google-chrome
         run: pnpm exec vitest run src/tools/__tests__/click.browser.test.ts src/tools/__tests__/observation-layers.browser.test.ts src/debug/__tests__/*.browser.test.ts
 
+      - name: Run emulation-lifecycle browser regression
+        working-directory: apps/extension
+        env:
+          BSK_CLICK_CHROME: google-chrome
+        run: pnpm exec vitest run src/tools/__tests__/emulation-lifecycle.browser.test.ts
+
   node-scripts:
     name: Node script tests
     runs-on: ubuntu-latest

apps/extension/src/tools/__tests__/emulate.test.ts

@@ -89,6 +89,66 @@ describe("handleEmulate", () => {
     resetEmulateStatesForTests();
   });
 
+  it.each([
+    "new-session",
+    "same-id",
+    "detach",
+    "reattach",
+    "new-runner",
+  ])("does not restore an old device profile after %s", async (mode) => {
+    const sm = new SessionManager({ agentWindow: fakeAgentWindow([100, 100]) });
+    await sm.start("aa11");
+    const deps = makeDeps();
+    let attachment: string | undefined = "first";
+    deps.cdp.getAttachmentId = () => attachment;
+    await handleEmulate(sm, { session_id: "aa11", overrides: fullOverrides }, deps);
+    let sessionId = "aa11";
+    if (mode === "new-session" || mode === "same-id") {
+      await sm.stop("aa11");
+      sessionId = mode === "same-id" ? "aa11" : "bb22";
+      await sm.start(sessionId);
+    } else if (mode === "detach") attachment = undefined;
+    else if (mode === "reattach") attachment = "second";
+    const next = mode === "new-runner" ? makeDeps() : deps;
+    const result = await handleEmulate(
+      sm,
+      {
+        session_id: sessionId,
+        overrides: { width: 1200, height: 800 },
+      },
+      next,
+    );
+    expect(result).toMatchObject({ applied: { width: 1200, height: 800 } });
+    expect("applied" in result && result.applied).toEqual({ width: 1200, height: 800 });
+    expect(next.calls.metrics.at(-1)?.metrics).toEqual({
+      width: 1200,
+      height: 800,
+      deviceScaleFactor: 0,
+      mobile: false,
+    });
+    expect(next.calls.ua).toHaveLength(mode === "new-runner" ? 0 : 1);
+    expect(next.calls.touch).toHaveLength(mode === "new-runner" ? 0 : 1);
+  });
+
+  it("keeps partial updates on the same live attachment", async () => {
+    const sm = await makeManager();
+    const deps = makeDeps();
+    // The first emulation call itself can establish the attachment.
+    let attachment: string | undefined;
+    deps.cdp.getAttachmentId = () => attachment;
+    vi.mocked(deps.cdp.setDeviceMetricsOverride).mockImplementation(async (tabId, metrics) => {
+      attachment = "live";
+      deps.calls.metrics.push({ tabId, metrics });
+    });
+    await handleEmulate(sm, { session_id: "aa11", overrides: fullOverrides }, deps);
+    const result = await handleEmulate(
+      sm,
+      { session_id: "aa11", overrides: { width: 500, height: 900 } },
+      deps,
+    );
+    expect(result).toMatchObject({ applied: { ...fullOverrides, width: 500, height: 900 } });
+  });
+
   it("applies viewport, UA and touch overrides to the active tab", async () => {
     const sm = await makeManager();
     const deps = makeDeps();

apps/extension/src/tools/__tests__/emulation-lifecycle.browser.test.ts

@@ -0,0 +1,137 @@
+// @vitest-environment node
+// Opt in with BSK_CLICK_CHROME; the harness owns an isolated browser/profile.
+import { describe, expect, it } from "vitest";
+import { type CdpDebuggerApi, ChromiumCdp } from "@/browser-driver/chromium-cdp";
+import { SessionManager } from "@/session-manager/manager";
+import { handleEmulate, resetEmulateStatesForTests } from "../emulate";
+
+type Send = <T = Record<string, unknown>>(
+  method: string,
+  params?: object,
+  sessionId?: string,
+) => Promise<T>;
+
+async function browser(
+  run: (send: Send, cdp: ChromiumCdp, sessionId: () => string) => Promise<void>,
+) {
+  const { withChrome } = await import(
+    new URL(
+      "../../../../../evals/browser/cases/regression/snapshot-coordinates/chrome.mjs",
+      import.meta.url,
+    ).href
+  );
+  await withChrome(
+    {
+      executable: process.env.BSK_CLICK_CHROME,
+      deviceScale: 1,
+      zoom: 1,
+      startupTimeout: 30_000,
+    },
+    async (send: Send) => {
+      const { targetId } = await send<{ targetId: string }>("Target.createTarget", {
+        url: "about:blank",
+      });
+      let activeSession = "";
+      const api: CdpDebuggerApi = {
+        attach: async () => {
+          const reply = await send<{ sessionId: string }>("Target.attachToTarget", {
+            targetId,
+            flatten: true,
+          });
+          activeSession = reply.sessionId;
+        },
+        detach: async () => {
+          await send("Target.detachFromTarget", { sessionId: activeSession });
+          activeSession = "";
+        },
+        sendCommand: async (_target, method, params) => send(method, params, activeSession),
+        onEvent: { addListener() {}, removeListener() {} } as unknown as CdpDebuggerApi["onEvent"],
+        onDetach: {
+          addListener() {},
+          removeListener() {},
+        } as unknown as CdpDebuggerApi["onDetach"],
+      };
+      const cdp = new ChromiumCdp(api);
+      try {
+        await run(send, cdp, () => activeSession);
+      } finally {
+        await cdp.detach(7);
+      }
+    },
+  );
+}
+
+function manager() {
+  return new SessionManager({
+    agentWindow: {
+      create: async () => ({ windowId: 100, initialTabIds: [7] }),
+      remove: async () => {},
+      ensureActiveTab: async () => 7,
+    },
+  });
+}
+const tab = { id: 7, windowId: 100, active: true, url: "about:blank" } as chrome.tabs.Tab;
+const tabsApi = { get: async () => tab, query: async () => [tab] };
+
+describe.skipIf(!process.env.BSK_CLICK_CHROME)("emulation lifecycle", () => {
+  it("a new session's viewport-only emulation does not restore the previous owner's UA or touch state", async () => {
+    resetEmulateStatesForTests();
+    try {
+      await browser(async (_send, cdp) => {
+        const sessions = manager();
+        const first = await sessions.start("emulate-first");
+        const read = async () =>
+          (
+            await cdp.send<{ result: { value: { ua: string; touch: number } } }>(
+              7,
+              "Runtime.evaluate",
+              {
+                expression: "({ua: navigator.userAgent, touch: navigator.maxTouchPoints})",
+                returnByValue: true,
+              },
+            )
+          ).result.value;
+        const original = await read();
+        const firstReply = await handleEmulate(
+          sessions,
+          {
+            session_id: first.sessionId,
+            tab_id: 7,
+            overrides: {
+              width: 390,
+              height: 844,
+              mobile: true,
+              device_scale_factor: 3,
+              user_agent: "BSK-AUDIT-OLD-SESSION",
+              touch: true,
+              max_touch_points: 5,
+            },
+          },
+          { cdp, tabsApi },
+        );
+        expect(firstReply).not.toHaveProperty("code");
+        expect((await read()).ua).toBe("BSK-AUDIT-OLD-SESSION");
+        // This is the production return-tab cleanup path. The Chrome tab survives.
+        await cdp.releaseSessionTab(first.sessionId, 7);
+        await sessions.stop(first.sessionId);
+        const afterReturn = await read();
+        expect(afterReturn).toEqual(original);
+        const second = await sessions.start("emulate-second");
+        const secondReply = await handleEmulate(
+          sessions,
+          { session_id: second.sessionId, tab_id: 7, overrides: { width: 1200, height: 800 } },
+          { cdp, tabsApi },
+        );
+        expect(secondReply).not.toHaveProperty("code");
+        const afterSecond = await read();
+        console.log(
+          "EMULATION_PROOF",
+          JSON.stringify({ original, afterReturn, afterSecond, secondReply }),
+        );
+        expect(afterSecond).toEqual(original);
+      });
+    } finally {
+      resetEmulateStatesForTests();
+    }
+  }, 40_000);
+});

apps/extension/src/tools/emulate.ts

@@ -11,7 +11,7 @@
 
 import type { DeviceMetricsOverride, UserAgentOverride } from "@/browser-driver/chromium-cdp";
 import { ChromiumCdp } from "@/browser-driver/chromium-cdp";
-import type { SessionManager } from "@/session-manager/manager";
+import type { SessionContext, SessionManager } from "@/session-manager/manager";
 import type {
   EmulateOverrides,
   EmulateParams,
@@ -39,6 +39,7 @@ export interface EmulateCdpRunner {
   setUserAgentOverride(tabId: number, override: UserAgentOverride): Promise<void>;
   setTouchEmulationEnabled(tabId: number, enabled: boolean, maxTouchPoints?: number): Promise<void>;
   trackSessionTab?(sessionId: string, tabId: number): void;
+  getAttachmentId?(tabId: number): string | undefined;
 }
 
 export interface EmulateDeps {
@@ -79,24 +80,21 @@ function invalidParams(message: string): RpcError {
   return { code: "invalid_params", message };
 }
 
-/**
- * Per-tab record of the emulation state applied so far. New requests
- * are merged onto it field by field (see `mergeEmulateOverrides`) and
- * the merged state is applied as a whole, so e.g. a later
- * `--width/--height` does not silently reset the dpr/mobile of an
- * earlier `--device` preset.
- *
- * The record is best-effort: an entry left behind by a closed tab is
- * simply overwritten by the next emulate call that targets the same tab
- * id, and the whole map is lost when the extension service worker
- * reloads — after a reload the next emulate is equivalent to a full
- * (re)set of just the fields it carries.
- */
-const tabEmulationStates = new Map<number, EmulateOverrides>();
+interface EmulationState {
+  cdp: EmulateCdpRunner;
+  attachmentId: string | undefined;
+  overrides: EmulateOverrides;
+}
 
-/** Test hook: drop every per-tab emulation state record. */
+// A session's partial updates belong to one live debugger attachment. Chrome
+// clears overrides on detach, so neither a new owner nor a reattached owner
+// may resurrect the previous device profile. Weak keys also retire stopped
+// sessions without retaining them in a browser-global tab-id map.
+let sessionEmulationStates = new WeakMap<SessionContext, Map<number, EmulationState>>();
+
+/** Test hook: drop every remembered emulation state. */
 export function resetEmulateStatesForTests(): void {
-  tabEmulationStates.clear();
+  sessionEmulationStates = new WeakMap();
 }
 
 /**
@@ -241,7 +239,7 @@ export async function handleEmulate(
     } catch (err) {
       return cdpFailed(err);
     }
-    tabEmulationStates.delete(target.tabId);
+    sessionEmulationStates.get(ctx)?.delete(target.tabId);
     return { tab_id: target.tabId, cleared: true, note: EMULATE_SCOPE_NOTE };
   }
 
@@ -254,7 +252,11 @@ export async function handleEmulate(
 
   // Fields absent from this request keep their previously applied
   // values; the merged state is what gets applied (and echoed back).
-  const merged = mergeEmulateOverrides(tabEmulationStates.get(target.tabId), overrides);
+  const states = sessionEmulationStates.get(ctx) ?? new Map<number, EmulationState>();
+  const stored = states.get(target.tabId);
+  const current =
+    stored?.cdp === deps.cdp && stored.attachmentId === deps.cdp.getAttachmentId?.(target.tabId);
+  const merged = mergeEmulateOverrides(current ? stored.overrides : undefined, overrides);
   try {
     if (deps.signal?.aborted) return { code: "cancelled", message: "emulate aborted" };
     deps.cdp.trackSessionTab?.(ctx.sessionId, target.tabId);
@@ -292,7 +294,12 @@ export async function handleEmulate(
     return cdpFailed(err);
   }
   // Record the merged state only once it was fully applied.
-  tabEmulationStates.set(target.tabId, merged);
+  states.set(target.tabId, {
+    cdp: deps.cdp,
+    attachmentId: deps.cdp.getAttachmentId?.(target.tabId),
+    overrides: merged,
+  });
+  sessionEmulationStates.set(ctx, states);
   return {
     tab_id: target.tabId,
     cleared: false,