mirror of
https://github.com/Tencent/BrowserSkill.git
synced 2026-09-10 13:01:23 +08:00
fix(extension): pass frame loaderId when waiting for navigation commit
handleNavigate's `Page.frameNavigated` handler called
`acceptLifecycleEvent("commit", frameId)` without the frame's loaderId.
Once `Page.navigate` resolves, the loader guard always carries the new
loaderId, so `eventLoaderIsRelevant(undefined, guard)` rejected the
commit event outright.
The navigation still completed in the common case only because the
readyState probe short-circuited first. On cross-origin navigations the
probe returns null (the old execution context has been destroyed), so
the wait fell back to `frameNavigated` — which was then rejected — and
`wait_until="commit"` timed out for the full `timeout_ms` instead of
resolving on commit.
Pass `p.frame?.loaderId` through, matching the sibling `lifecycleEvent`
branches. Adds a regression test that makes the readyState probe fail
and asserts commit is reached from the `frameNavigated` event; it fails
(reached "timeout") on the previous code.
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -49,6 +49,8 @@ function makeFakeCdp(opts?: {
|
|||||||
beforeLoaderId?: string;
|
beforeLoaderId?: string;
|
||||||
readyState?: string;
|
readyState?: string;
|
||||||
readyStateSequence?: string[];
|
readyStateSequence?: string[];
|
||||||
|
/** Make the readyState probe's `Runtime.evaluate` throw (returns null). */
|
||||||
|
evaluateThrows?: boolean;
|
||||||
}) {
|
}) {
|
||||||
const readyStates = opts?.readyStateSequence ?? (opts?.readyState ? [opts.readyState] : []);
|
const readyStates = opts?.readyStateSequence ?? (opts?.readyState ? [opts.readyState] : []);
|
||||||
let readyStateIdx = 0;
|
let readyStateIdx = 0;
|
||||||
@@ -94,7 +96,10 @@ function makeFakeCdp(opts?: {
|
|||||||
return {};
|
return {};
|
||||||
},
|
},
|
||||||
"Runtime.enable": () => ({}),
|
"Runtime.enable": () => ({}),
|
||||||
"Runtime.evaluate": () => ({ result: { value: nextReadyState() } }),
|
"Runtime.evaluate": () => {
|
||||||
|
if (opts?.evaluateThrows) throw new Error("Cannot execute in a destroyed execution context");
|
||||||
|
return { result: { value: nextReadyState() } };
|
||||||
|
},
|
||||||
"Page.getNavigationHistory": () => ({
|
"Page.getNavigationHistory": () => ({
|
||||||
currentIndex: opts?.historyIndex ?? 1,
|
currentIndex: opts?.historyIndex ?? 1,
|
||||||
entries: opts?.historyEntries ?? [
|
entries: opts?.historyEntries ?? [
|
||||||
@@ -168,8 +173,13 @@ function makeFakeCdp(opts?: {
|
|||||||
const payload = { name, frameId, loaderId };
|
const payload = { name, frameId, loaderId };
|
||||||
for (const listener of [...events]) listener({ tabId: 4 }, "Page.lifecycleEvent", payload);
|
for (const listener of [...events]) listener({ tabId: 4 }, "Page.lifecycleEvent", payload);
|
||||||
},
|
},
|
||||||
fireFrameNavigated(frameId = opts?.navigateFrameId ?? "frame-1") {
|
fireFrameNavigated(
|
||||||
const payload = { frame: { id: frameId, url: opts?.finalUrl ?? "https://example.com/" } };
|
frameId = opts?.navigateFrameId ?? "frame-1",
|
||||||
|
loaderId = opts?.navigateLoaderId ?? "loader-after",
|
||||||
|
) {
|
||||||
|
const payload = {
|
||||||
|
frame: { id: frameId, loaderId, url: opts?.finalUrl ?? "https://example.com/" },
|
||||||
|
};
|
||||||
for (const listener of [...events]) listener({ tabId: 4 }, "Page.frameNavigated", payload);
|
for (const listener of [...events]) listener({ tabId: 4 }, "Page.frameNavigated", payload);
|
||||||
},
|
},
|
||||||
listeners: events,
|
listeners: events,
|
||||||
@@ -457,6 +467,34 @@ describe("handleNavigate", () => {
|
|||||||
expect(res.reached).toBe("commit");
|
expect(res.reached).toBe("commit");
|
||||||
expect(fake.listeners.length).toBe(0);
|
expect(fake.listeners.length).toBe(0);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("resolves wait_until=commit from Page.frameNavigated when the readyState probe fails", async () => {
|
||||||
|
const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) });
|
||||||
|
await sm.start("aa11");
|
||||||
|
// Cross-origin-style navigation: the readyState probe can't read the
|
||||||
|
// (already destroyed) old execution context, so commit must come from
|
||||||
|
// the Page.frameNavigated event. Before the fix this timed out because
|
||||||
|
// the handler dropped the frame's loaderId and the loader guard then
|
||||||
|
// rejected the event.
|
||||||
|
const fake = makeFakeCdp({ evaluateThrows: true });
|
||||||
|
const navP = handleNavigate(
|
||||||
|
sm,
|
||||||
|
{
|
||||||
|
session_id: "aa11",
|
||||||
|
url: "https://example.com/",
|
||||||
|
wait_until: "commit",
|
||||||
|
timeout_ms: 1_000,
|
||||||
|
},
|
||||||
|
{ cdp: fake.cdp, tabsApi: fake.tabsApi },
|
||||||
|
);
|
||||||
|
|
||||||
|
await new Promise((r) => setTimeout(r, 5));
|
||||||
|
fake.fireFrameNavigated("frame-1", "loader-after");
|
||||||
|
const res = await navP;
|
||||||
|
if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`);
|
||||||
|
expect(res.reached).toBe("commit");
|
||||||
|
expect(fake.listeners.length).toBe(0);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("handleNavigateBack / Forward", () => {
|
describe("handleNavigateBack / Forward", () => {
|
||||||
|
|||||||
@@ -325,14 +325,14 @@ function startLifecycleWait(
|
|||||||
if (source.tabId !== expectedTabId) return;
|
if (source.tabId !== expectedTabId) return;
|
||||||
|
|
||||||
if (targetName === "commit" && method === "Page.frameNavigated") {
|
if (targetName === "commit" && method === "Page.frameNavigated") {
|
||||||
const p = params as { frame?: { id?: string; parentId?: string } };
|
const p = params as { frame?: { id?: string; parentId?: string; loaderId?: string } };
|
||||||
const expectedFrameId = currentFrameId();
|
const expectedFrameId = currentFrameId();
|
||||||
if (expectedFrameId.length > 0) {
|
if (expectedFrameId.length > 0) {
|
||||||
if (p.frame?.id !== expectedFrameId) return;
|
if (p.frame?.id !== expectedFrameId) return;
|
||||||
} else if (p.frame?.parentId) {
|
} else if (p.frame?.parentId) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
if (!acceptLifecycleEvent("commit", p.frame?.id)) return;
|
if (!acceptLifecycleEvent("commit", p.frame?.id, p.frame?.loaderId)) return;
|
||||||
finish({ reached: "match", lastLifecycle: "commit" });
|
finish({ reached: "match", lastLifecycle: "commit" });
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user