diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d91a0eb127..3375a15e93 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -249,7 +249,7 @@ jobs: - "packages/cli/src/utils/**" - "packages/cli/src/ui/**" - "packages/cli/src/{background-removal,registry,telemetry}/**" - - "packages/cli/src/capture/{captureCompositionFrame,readBoundedResponse}.ts" + - "packages/cli/src/capture/{captureCompositionFrame,captureTimeout,navigateForCapture,readBoundedResponse}.ts" - "packages/cli/src/media-use/lib/config-lock.mjs" - "packages/cli/scripts/**" - "packages/core/stubs/**" diff --git a/packages/cli/src/capture/captureCompositionFrame.browserFallback.test.ts b/packages/cli/src/capture/captureCompositionFrame.browserFallback.test.ts index fa929722f3..0639649ff5 100644 --- a/packages/cli/src/capture/captureCompositionFrame.browserFallback.test.ts +++ b/packages/cli/src/capture/captureCompositionFrame.browserFallback.test.ts @@ -57,6 +57,11 @@ function fakeBrowser() { evaluateOnNewDocument: vi.fn(async () => undefined), setViewport: vi.fn(async () => undefined), goto: vi.fn(async () => undefined), + createCDPSession: async () => ({ + send: async () => undefined, + on: () => undefined, + detach: async () => undefined, + }), waitForFunction: vi.fn(async () => undefined), evaluate: vi.fn(async () => undefined), setRequestInterception: vi.fn(async (_on: boolean) => undefined), diff --git a/packages/cli/src/capture/captureCompositionFrame.ts b/packages/cli/src/capture/captureCompositionFrame.ts index 95e9ded62d..a029db5ca8 100644 --- a/packages/cli/src/capture/captureCompositionFrame.ts +++ b/packages/cli/src/capture/captureCompositionFrame.ts @@ -18,6 +18,7 @@ import { windowsChromeCrashRemediation } from "../browser/windowsCrash.js"; import { resolveCompositionViewportFromHtml } from "../utils/compositionViewport.js"; import { normalizeErrorMessage } from "../utils/errorMessage.js"; import { resolveDiagnosticNavigationTimeoutMs } from "../utils/renderArgs.js"; +import { gotoWhenSent } from "./navigateForCapture.js"; const SHADER_TRANSITIONS_TIMEOUT_MS = 90_000; const CAPTURE_SETTLE_MS = 1500; @@ -265,10 +266,11 @@ export async function openSettledCompositionPage( await page.setViewport(viewport); if (answer) await answerPageInProcess(page, url, answer); await options.beforeNavigate?.(page); - await page.goto(url, { - waitUntil: "domcontentloaded", - timeout: resolveDiagnosticNavigationTimeoutMs(process.env, options.navigationTimeoutMs), - }); + await gotoWhenSent( + page, + url, + resolveDiagnosticNavigationTimeoutMs(process.env, options.navigationTimeoutMs), + ); await assertWebGpuAdapterAvailable(page, requiresWebGpu, softwareWebGpu); const renderReadyTimedOut = !(await waitForCompositionSettle(page, options)); return { browser: chromeBrowser, page, renderReadyTimedOut }; diff --git a/packages/cli/src/capture/captureTimeout.ts b/packages/cli/src/capture/captureTimeout.ts index e171a59b74..fe48e6e087 100644 --- a/packages/cli/src/capture/captureTimeout.ts +++ b/packages/cli/src/capture/captureTimeout.ts @@ -58,9 +58,10 @@ export async function withRemainingBudget( work: Promise, remainingMs: number, label: string, + timeoutError: () => Error = () => new StageBudgetTimeoutError(label, Math.max(0, remainingMs)), ): Promise { if (!(remainingMs > 0)) { - throw new StageBudgetTimeoutError(label, Math.max(0, remainingMs)); + throw timeoutError(); } let timer: ReturnType | undefined; @@ -69,7 +70,7 @@ export async function withRemainingBudget( work, new Promise((_resolve, reject) => { timer = setTimeout(() => { - reject(new StageBudgetTimeoutError(label, remainingMs)); + reject(timeoutError()); }, remainingMs); }), ]); diff --git a/packages/cli/src/capture/navigateForCapture.browser.test.ts b/packages/cli/src/capture/navigateForCapture.browser.test.ts new file mode 100644 index 0000000000..28d9507248 --- /dev/null +++ b/packages/cli/src/capture/navigateForCapture.browser.test.ts @@ -0,0 +1,60 @@ +import { once } from "node:events"; +import { createServer, type ServerResponse } from "node:http"; +import type { AddressInfo } from "node:net"; +import puppeteer, { type Browser } from "puppeteer-core"; +import { afterAll, beforeAll, expect, it } from "vitest"; +import { ensureBrowser } from "../browser/manager.js"; +import { documentRequestSent } from "./navigateForCapture.js"; + +let browser: Browser; +let held: ServerResponse | undefined; +let requested: () => void = () => undefined; +const server = createServer((_req, res) => { + held = res; + requested(); +}); + +beforeAll(async () => { + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { executablePath } = await ensureBrowser(); + browser = await puppeteer.launch({ executablePath, headless: true, args: ["--no-sandbox"] }); +}, 60_000); + +afterAll(async () => { + const closed = once(server.close(), "close"); + server.closeAllConnections(); + await Promise.all([browser?.close(), closed]); +}); + +it("settles when Chrome sends the document request, before the server answers", async () => { + const page = await browser.newPage(); + const cdp = await page.createCDPSession(); + await cdp.send("Network.enable"); + const serverGotRequest = new Promise((resolve) => (requested = resolve)); + const sent = documentRequestSent(cdp); + const navigation = page.goto(`http://127.0.0.1:${(server.address() as AddressInfo).port}/`, { + waitUntil: "domcontentloaded", + timeout: 0, + }); + await serverGotRequest; + await sent; + held?.end("

page

"); + await navigation; + expect(await page.content()).toContain("page"); + await page.close(); +}); + +it("settles for a page answered by interception, which never reaches the network", async () => { + const page = await browser.newPage(); + const cdp = await page.createCDPSession(); + await cdp.send("Network.enable"); + await page.setRequestInterception(true); + page.on("request", (request) => + request.respond({ status: 200, contentType: "text/html", body: "

answered

" }), + ); + const sent = documentRequestSent(cdp); + await page.goto("http://in-process.invalid/", { waitUntil: "domcontentloaded", timeout: 0 }); + await sent; + expect(await page.content()).toContain("answered"); + await page.close(); +}); diff --git a/packages/cli/src/capture/navigateForCapture.test.ts b/packages/cli/src/capture/navigateForCapture.test.ts index 98c9517a9a..aa65f0b821 100644 --- a/packages/cli/src/capture/navigateForCapture.test.ts +++ b/packages/cli/src/capture/navigateForCapture.test.ts @@ -1,6 +1,8 @@ -import { describe, expect, it, vi } from "vitest"; -import { TimeoutError } from "puppeteer-core"; +import { EventEmitter } from "node:events"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { TimeoutError, type Page } from "puppeteer-core"; import { + gotoWhenSent, isNavigationTimeoutError, navigateForCapture, networkIdleAttemptTimeoutMs, @@ -117,3 +119,81 @@ describe("navigateForCapture", () => { }); }); }); + +/** A page whose goto keeps Puppeteer's contract: a non-zero timeout rejects the navigation. */ +function fakeChromePage() { + const session = Object.assign(new EventEmitter(), { + send: async () => undefined, + detach: vi.fn(async () => undefined), + }); + let load: (response: unknown) => void = () => undefined; + const page = { + createCDPSession: async () => session, + goto: (_url: string, { timeout }: { timeout: number }) => + new Promise((resolve, reject) => { + load = resolve; + if (timeout > 0) { + setTimeout( + () => reject(new TimeoutError(`Navigation timeout of ${timeout} ms exceeded`)), + timeout, + ); + } + }), + } as unknown as Page; + return { + page, + session, + navigate: () => + session.emit("Network.requestWillBeSent", { requestId: "doc", type: "Document" }), + send: () => session.emit("Network.requestWillBeSentExtraInfo", { requestId: "doc" }), + load: (response: unknown) => load(response), + }; +} + +describe("gotoWhenSent", () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it("gives the page its whole budget after a cold Chrome is slow to send the request", async () => { + vi.useFakeTimers(); + const chrome = fakeChromePage(); + const response = { status: () => 200 }; + const opened = gotoWhenSent(chrome.page, "http://127.0.0.1:1/", 10_000); + await vi.advanceTimersByTimeAsync(0); + chrome.navigate(); + await vi.advanceTimersByTimeAsync(25_000); + chrome.send(); + await vi.advanceTimersByTimeAsync(9_000); + chrome.load(response); + await expect(opened).resolves.toBe(response); + expect(chrome.session.detach).toHaveBeenCalledOnce(); + }); + + it("fails as a navigation timeout when Chrome never sends the request", async () => { + vi.useFakeTimers(); + const chrome = fakeChromePage(); + const opened = gotoWhenSent(chrome.page, "http://127.0.0.1:1/", 10_000); + const failed = expect(opened).rejects.toSatisfy( + (error: unknown) => + isNavigationTimeoutError(error) && + String(error).includes("did not send the page request within 30000 ms"), + ); + await vi.advanceTimersByTimeAsync(0); + chrome.navigate(); + await vi.advanceTimersByTimeAsync(30_000); + await failed; + }); + + it("fails when the page does not load within its budget after the request is sent", async () => { + vi.useFakeTimers(); + const chrome = fakeChromePage(); + const opened = gotoWhenSent(chrome.page, "http://127.0.0.1:1/", 10_000); + const failed = expect(opened).rejects.toThrow("Navigation timeout of 10000 ms exceeded"); + await vi.advanceTimersByTimeAsync(0); + chrome.send(); + chrome.navigate(); + await vi.advanceTimersByTimeAsync(10_000); + await failed; + }); +}); diff --git a/packages/cli/src/capture/navigateForCapture.ts b/packages/cli/src/capture/navigateForCapture.ts index 92b6597599..68ff4793b8 100644 --- a/packages/cli/src/capture/navigateForCapture.ts +++ b/packages/cli/src/capture/navigateForCapture.ts @@ -1,4 +1,5 @@ -import { isNavigationTimeoutError } from "./captureTimeout.js"; +import { TimeoutError, type CDPSession, type HTTPResponse, type Page } from "puppeteer-core"; +import { isNavigationTimeoutError, withRemainingBudget } from "./captureTimeout.js"; export { isNavigationTimeoutError }; @@ -61,3 +62,61 @@ export async function navigateForCapture( fellBackFromNetworkIdle: true, }; } + +const PUPPETEER_LAUNCH_DEFAULT_TIMEOUT_MS = 30_000; +const SENT_OR_ANSWERED_WITHOUT_NETWORK = [ + "Network.requestWillBeSentExtraInfo", + "Network.responseReceived", +] as const; + +export async function gotoWhenSent( + page: Page, + url: string, + timeoutMs: number, + coldChromeFirstRequestTimeoutMs = PUPPETEER_LAUNCH_DEFAULT_TIMEOUT_MS, +): Promise { + const cdp = await page.createCDPSession(); + try { + await cdp.send("Network.enable"); + const sent = documentRequestSent(cdp); + const navigation = page.goto(url, { waitUntil: "domcontentloaded", timeout: 0 }); + const navigationAwaitedBelow = navigation.catch(() => undefined); + await withRemainingBudget( + Promise.race([sent, navigationAwaitedBelow]), + coldChromeFirstRequestTimeoutMs, + "sending the page request", + () => + new TimeoutError( + `Navigation timeout: Chrome did not send the page request within ${coldChromeFirstRequestTimeoutMs} ms`, + ), + ); + return await withRemainingBudget( + navigation, + timeoutMs, + "loading the page", + () => new TimeoutError(`Navigation timeout of ${timeoutMs} ms exceeded`), + ); + } finally { + await cdp.detach().catch(() => undefined); + } +} + +export function documentRequestSent(cdp: CDPSession): Promise { + const documents = new Set(); + const sent = new Set(); + return new Promise((resolve) => { + const settle = (requestId: string) => { + if (documents.has(requestId) && sent.has(requestId)) resolve(); + }; + cdp.on("Network.requestWillBeSent", ({ requestId, type }) => { + if (type === "Document") documents.add(requestId); + settle(requestId); + }); + for (const event of SENT_OR_ANSWERED_WITHOUT_NETWORK) { + cdp.on(event, ({ requestId }) => { + sent.add(requestId); + settle(requestId); + }); + } + }); +} diff --git a/packages/cli/src/commands/deprecationTestHarness.ts b/packages/cli/src/commands/deprecationTestHarness.ts index ba66b6e3da..f2a57ee373 100644 --- a/packages/cli/src/commands/deprecationTestHarness.ts +++ b/packages/cli/src/commands/deprecationTestHarness.ts @@ -19,6 +19,10 @@ const FAKE_PROJECT = { indexPath: "/fake-project/index.html", }; +export async function fakeCdpSession() { + return { send: async () => undefined, on: () => undefined, detach: async () => undefined }; +} + export function resolveProjectMock() { return { resolveProject: vi.fn(() => FAKE_PROJECT) }; } diff --git a/packages/cli/src/commands/layout.ts b/packages/cli/src/commands/layout.ts index d8c14e4b3b..194d8977e7 100644 --- a/packages/cli/src/commands/layout.ts +++ b/packages/cli/src/commands/layout.ts @@ -8,6 +8,7 @@ import type { Example } from "./_examples.js"; import { c } from "../ui/colors.js"; import { resolveProject } from "../utils/project.js"; import { resolveDiagnosticNavigationTimeoutMs } from "../utils/renderArgs.js"; +import { gotoWhenSent } from "../capture/navigateForCapture.js"; import { normalizeErrorMessage } from "../utils/errorMessage.js"; import { serveStaticProjectHtml } from "../utils/staticProjectServer.js"; import { printDeprecationNotice, withMeta } from "../utils/updateCheck.js"; @@ -184,10 +185,7 @@ async function alignViewportToComposition( }); await page.setViewport(size); - await page.goto(url, { - waitUntil: "domcontentloaded", - timeout: resolveDiagnosticNavigationTimeoutMs(), - }); + await gotoWhenSent(page, url, resolveDiagnosticNavigationTimeoutMs()); } async function runLayoutAudit( @@ -237,10 +235,7 @@ async function runLayoutAudit( const page = await chromeBrowser.newPage(); await installPageFunctionGuard(page); await page.setViewport({ width: 1920, height: 1080 }); - await page.goto(server.url, { - waitUntil: "domcontentloaded", - timeout: resolveDiagnosticNavigationTimeoutMs(), - }); + await gotoWhenSent(page, server.url, resolveDiagnosticNavigationTimeoutMs()); await assertWebGpuAdapterAvailable(page, requiresWebGpu, softwareWebGpu); await alignViewportToComposition(page, server.url); await waitForRuntimeReady(page, opts.timeout); diff --git a/packages/cli/src/commands/motionShot.browserClose.test.ts b/packages/cli/src/commands/motionShot.browserClose.test.ts index 0ab81a1d2c..0315cfad62 100644 --- a/packages/cli/src/commands/motionShot.browserClose.test.ts +++ b/packages/cli/src/commands/motionShot.browserClose.test.ts @@ -2,10 +2,12 @@ import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, expect, it, vi } from "vitest"; +import { fakeCdpSession } from "./deprecationTestHarness.js"; const browser = vi.hoisted(() => ({ close: vi.fn(async () => undefined), newPage: vi.fn(async () => ({ + createCDPSession: fakeCdpSession, setViewport: async () => undefined, goto: async () => undefined, evaluate: async () => undefined, diff --git a/packages/cli/src/commands/motionShot.ts b/packages/cli/src/commands/motionShot.ts index b70e59ffa0..8a07008f0e 100644 --- a/packages/cli/src/commands/motionShot.ts +++ b/packages/cli/src/commands/motionShot.ts @@ -14,6 +14,7 @@ import { launchManagedBrowser, resolveManagedGpuMode } from "../browser/launch.j import { mkdirSync, statSync, writeFileSync } from "node:fs"; import { dirname, resolve } from "node:path"; import { resolveDiagnosticNavigationTimeoutMs } from "../utils/renderArgs.js"; +import { gotoWhenSent } from "../capture/navigateForCapture.js"; import { resolveCompositionViewportFromHtml } from "../utils/compositionViewport.js"; import { waitForRuntimeReady } from "../capture/captureCompositionFrame.js"; import { @@ -425,9 +426,8 @@ async function openCompositionPage( }); try { const page = await browser.newPage(); - const navigationTimeout = resolveDiagnosticNavigationTimeoutMs(); await page.setViewport(size); - await page.goto(url, { waitUntil: "domcontentloaded", timeout: navigationTimeout }); + await gotoWhenSent(page, url, resolveDiagnosticNavigationTimeoutMs()); await assertWebGpuAdapterAvailable(page, requiresWebGpu, softwareWebGpu); await waitForRuntimeReady(page, 10000); await page diff --git a/packages/cli/src/commands/validate.browserClose.test.ts b/packages/cli/src/commands/validate.browserClose.test.ts index a6418380df..4320c6bc5e 100644 --- a/packages/cli/src/commands/validate.browserClose.test.ts +++ b/packages/cli/src/commands/validate.browserClose.test.ts @@ -1,5 +1,9 @@ import { afterEach, expect, it, vi } from "vitest"; -import { resolveProjectMock, runAndCaptureStdio } from "./deprecationTestHarness.js"; +import { + fakeCdpSession, + resolveProjectMock, + runAndCaptureStdio, +} from "./deprecationTestHarness.js"; const browser = vi.hoisted(() => ({ close: vi.fn(async () => undefined), @@ -8,6 +12,7 @@ const browser = vi.hoisted(() => ({ on: () => undefined, goto: async () => undefined, evaluate: async () => undefined, + createCDPSession: fakeCdpSession, })), }));