diff --git a/src/browserLauncher.ts b/src/browserLauncher.ts new file mode 100644 index 00000000..ff289189 --- /dev/null +++ b/src/browserLauncher.ts @@ -0,0 +1,93 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { spawn } from 'child_process'; +import puppeteer, { Browser, TargetType } from 'puppeteer-core'; + +const RETRY_INTERVAL = 200; + +export async function launchAndConnectToBrowser( + executablePath: string, port: number, args: string[], headless: boolean, timeout: number, +): Promise { + const deadline = Date.now() + timeout; + const browserProcess = spawn(executablePath, await puppeteer.defaultArgs({ args, headless }), { stdio: 'ignore' }); + + let processExited = false; + let rejectOnProcessFailure: (error: Error) => void = () => undefined; + const processFailure = new Promise((_, reject) => { + rejectOnProcessFailure = reject; + }); + // Prevents an unhandled rejection if the process fails after the launch has settled. + processFailure.catch(() => undefined); + + browserProcess.once('error', error => { + processExited = true; + rejectOnProcessFailure(new Error(`Failed to start Microsoft Edge: ${error.message}`, { cause: error })); + }); + // Edge can hand off to another process and exit successfully before its debugging endpoint is ready. + browserProcess.once('exit', (code, signal) => { + processExited = true; + if (code !== 0) { + rejectOnProcessFailure(new Error(`Microsoft Edge exited before connecting (${signal ?? `code ${code}`}).`)); + } + }); + + let browser: Browser | undefined; + try { + browser = await connectBeforeDeadline(port, deadline, processFailure); + await Promise.race([ + browser.waitForTarget(target => target.type() === TargetType.PAGE, { + timeout: Math.max(1, deadline - Date.now()), + }), + processFailure, + ]); + return browser; + } catch (error) { + await browser?.disconnect().catch(() => undefined); + // Only stop the process this launch still owns; after a successful handoff the + // browser belongs to another (possibly pre-existing) Edge process. + if (!processExited) { + browserProcess.kill(); + } + throw error; + } +} + +async function connectBeforeDeadline(port: number, deadline: number, processFailure: Promise): Promise { + const timeoutMessage = `Timed out waiting for Microsoft Edge on port ${port}.`; + let lastError: unknown; + for (;;) { + const remaining = deadline - Date.now(); + if (remaining <= 0) { + throw new Error(timeoutMessage, { cause: lastError }); + } + + let timer: NodeJS.Timeout | undefined; + const timedOut = new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error(timeoutMessage, { cause: lastError })), remaining); + }); + const attempt = puppeteer.connect({ browserURL: `http://127.0.0.1:${port}` }); + try { + const outcome = await Promise.race([ + attempt.then(browser => ({ browser }), (error: unknown) => ({ error })), + processFailure, + timedOut, + ]); + if ('browser' in outcome) { + return outcome.browser; + } + lastError = outcome.error; + } catch (error) { + // The launch was abandoned; don't leak a connection that completes afterwards. + void attempt.then(browser => browser.disconnect(), () => undefined); + throw error; + } finally { + clearTimeout(timer); + } + + await Promise.race([ + new Promise(resolve => setTimeout(resolve, Math.min(RETRY_INTERVAL, Math.max(0, deadline - Date.now())))), + processFailure, + ]); + } +} diff --git a/src/extension.ts b/src/extension.ts index 23c480fd..a9c76a9e 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -270,9 +270,9 @@ export async function launchHtml(fileUri: vscode.Uri): Promise { const url = `file://${vscode.env.remoteName}.localhost/${fileUri.authority.split('+')[1]}/${fileUri.fsPath.replace(/\\/g, '/')}`; edgeDebugConfig.url = url; devToolsAttachConfig.url = url; - const { port, userDataDir } = getRemoteEndpointSettings(); + const { port, userDataDir, timeout } = getRemoteEndpointSettings(); const browserPath = await getBrowserPath(); - await launchBrowser(browserPath, port, url, userDataDir, /** headless */ true).then(() => vscode.debug.startDebugging(undefined, devToolsAttachConfig)); + await launchBrowser(browserPath, port, url, userDataDir, /** headless */ true, timeout).then(() => vscode.debug.startDebugging(undefined, devToolsAttachConfig)); } } @@ -288,9 +288,9 @@ export async function launchScreencast(context: vscode.ExtensionContext, fileUri // Parse the filename from the remoteName, file authority and path e.g. file://wsl.localhost/ubuntu-20.04/test/index.html const url = `file://${vscode.env.remoteName}.localhost/${fileUri.authority.split('+')[1]}/${fileUri.fsPath.replace(/\\/g, '/')}`; edgeDebugConfig.url = url; - const { port, userDataDir } = getRemoteEndpointSettings(); + const { port, userDataDir, timeout } = getRemoteEndpointSettings(); const browserPath = await getBrowserPath(); - await launchBrowser(browserPath, port, url, userDataDir, /** headless */ true).then(() => attach(context, url, undefined, true, true)); + await launchBrowser(browserPath, port, url, userDataDir, /** headless */ true, timeout).then(() => attach(context, url, undefined, true, true)); } } @@ -445,7 +445,7 @@ export async function launch(context: vscode.ExtensionContext, launchUrl?: strin const telemetryProps = { viaConfig: `${!!config}`, browserType, isHeadless}; sendTaxonomyEvent(telemetryReporter, { area: 'command', feature: 'browser', action: 'launch' }, telemetryProps); - const { hostname, port, defaultUrl, userDataDir } = getRemoteEndpointSettings(config); + const { hostname, port, defaultUrl, userDataDir, timeout } = getRemoteEndpointSettings(config); const url = launchUrl || defaultUrl; const target = await openNewTab(hostname, port, url); if (target && target.webSocketDebuggerUrl) { @@ -476,7 +476,7 @@ export async function launch(context: vscode.ExtensionContext, launchUrl?: strin const browserProps = { exe: `${knownBrowser?.toLowerCase()}` }; sendTaxonomyEvent(telemetryReporter, { area: 'command', feature: 'browser', action: 'launch', detail: 'newInstance' }, browserProps); - browserInstance = await launchBrowser(browserPath, port, url, userDataDir); + browserInstance = await launchBrowser(browserPath, port, url, userDataDir, undefined, timeout); if (url !== SETTINGS_DEFAULT_URL) { reportUrlType(url, telemetryReporter); } diff --git a/src/utils.ts b/src/utils.ts index 5a996c5b..59aa2157 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -15,7 +15,8 @@ import { DebugTelemetryReporter } from './debugTelemetryReporter'; import { AriaTelemetryReporter } from './ariaTelemetryReporter'; import { sendTaxonomyEvent } from './telemetryTaxonomy'; -import puppeteer, {Browser} from 'puppeteer-core'; +import type { Browser } from 'puppeteer-core'; +import { launchAndConnectToBrowser } from './browserLauncher'; import { ErrorReporter } from './errorReporter'; import { ErrorCodes } from './common/errorCodes'; @@ -394,8 +395,12 @@ export async function getBrowserPath(config: Partial = {}): Promise * @param targetUrl The url of the page to open * @param userDataDir The user data directory for the launched instance * @param forceHeadless This force overrides the --headless arg for browser launch + * @param timeout Maximum time to wait for the browser to become ready */ -export async function launchBrowser(browserPath: string, port: number, targetUrl: string, userDataDir?: string, forceHeadless?: boolean): Promise { +export async function launchBrowser( + browserPath: string, port: number, targetUrl: string, userDataDir?: string, forceHeadless?: boolean, + timeout = SETTINGS_DEFAULT_ATTACH_TIMEOUT, +): Promise { const args = [ '--no-first-run', '--no-default-browser-check', @@ -417,8 +422,7 @@ export async function launchBrowser(browserPath: string, port: number, targetUrl args.unshift(...browserArgs); } - const browserInstance = await puppeteer.launch({executablePath: browserPath, args, headless}); - return browserInstance; + return launchAndConnectToBrowser(browserPath, port, args, headless, timeout); } /** diff --git a/test/browserLauncher.test.ts b/test/browserLauncher.test.ts new file mode 100644 index 00000000..6a638cd1 --- /dev/null +++ b/test/browserLauncher.test.ts @@ -0,0 +1,99 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { ChildProcess, spawn } from 'child_process'; +import { EventEmitter } from 'events'; +import puppeteer, { Browser } from 'puppeteer-core'; +import { launchAndConnectToBrowser } from '../src/browserLauncher'; + +jest.mock('child_process', () => ({ spawn: jest.fn() })); + +describe('launchAndConnectToBrowser', () => { + let browserProcess: ChildProcess; + let browser: Browser; + let waitForTarget: jest.Mock; + let disconnect: jest.Mock; + const args = ['--remote-debugging-port=9222']; + + beforeEach(() => { + jest.useFakeTimers(); + jest.clearAllMocks(); + browserProcess = Object.assign(new EventEmitter(), { kill: jest.fn() }) as unknown as ChildProcess; + jest.mocked(spawn).mockReturnValue(browserProcess); + jest.mocked(puppeteer.defaultArgs).mockImplementation(async options => options?.args ?? []); + waitForTarget = jest.fn().mockResolvedValue({}); + disconnect = jest.fn().mockResolvedValue(undefined); + const partialBrowser: Partial = { waitForTarget, disconnect }; + browser = partialBrowser as Browser; + jest.mocked(puppeteer.connect).mockResolvedValue(browser); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + function launch(timeout = 1000): Promise { + return launchAndConnectToBrowser('edge.exe', 9222, args, false, timeout); + } + + it('starts Edge with the standard arguments and connects to its debugging endpoint', async () => { + expect(await launch()).toBe(browser); + expect(puppeteer.defaultArgs).toHaveBeenCalledWith({ args, headless: false }); + expect(spawn).toHaveBeenCalledWith('edge.exe', args, { stdio: 'ignore' }); + expect(puppeteer.connect).toHaveBeenCalledWith({ browserURL: 'http://127.0.0.1:9222' }); + expect(waitForTarget).toHaveBeenCalledWith(expect.any(Function), { timeout: 1000 }); + }); + + it('keeps waiting when the launcher process hands off and exits successfully', async () => { + jest.mocked(puppeteer.connect).mockRejectedValueOnce(new Error('ECONNREFUSED')); + const result = launch(); + await jest.advanceTimersByTimeAsync(0); + browserProcess.emit('exit', 0, null); + await jest.advanceTimersByTimeAsync(200); + expect(await result).toBe(browser); + expect(puppeteer.connect).toHaveBeenCalledTimes(2); + }); + + it('reports startup errors', async () => { + jest.mocked(puppeteer.connect).mockRejectedValue(new Error('ECONNREFUSED')); + const result = launch(); + const rejection = expect(result).rejects.toThrow('Failed to start Microsoft Edge: not found'); + await jest.advanceTimersByTimeAsync(0); + browserProcess.emit('error', new Error('not found')); + await jest.advanceTimersByTimeAsync(200); + await rejection; + }); + + it('reports unsuccessful exits', async () => { + jest.mocked(puppeteer.connect).mockRejectedValue(new Error('ECONNREFUSED')); + const result = launch(); + const rejection = expect(result).rejects.toThrow('Microsoft Edge exited before connecting (code 1).'); + await jest.advanceTimersByTimeAsync(0); + browserProcess.emit('exit', 1, null); + await jest.advanceTimersByTimeAsync(200); + await rejection; + }); + + it('times out when the debugging endpoint never becomes available', async () => { + jest.mocked(puppeteer.connect).mockRejectedValue(new Error('ECONNREFUSED')); + const result = launch(500); + const rejection = expect(result).rejects.toThrow('Timed out waiting for Microsoft Edge on port 9222.'); + await jest.advanceTimersByTimeAsync(600); + await rejection; + }); + + it('disconnects and stops the launched process when no page becomes available', async () => { + waitForTarget.mockRejectedValue(new Error('Waiting for target failed')); + await expect(launch()).rejects.toThrow('Waiting for target failed'); + expect(disconnect).toHaveBeenCalledTimes(1); + expect(browserProcess.kill).toHaveBeenCalledTimes(1); + }); + + it('times out when a connection attempt stalls past the deadline', async () => { + jest.mocked(puppeteer.connect).mockReturnValue(new Promise(() => undefined)); + const result = launch(500); + const rejection = expect(result).rejects.toThrow('Timed out waiting for Microsoft Edge on port 9222.'); + await jest.advanceTimersByTimeAsync(500); + await rejection; + }); +}); diff --git a/test/extension.test.ts b/test/extension.test.ts index b55aeed8..eb3af56e 100644 --- a/test/extension.test.ts +++ b/test/extension.test.ts @@ -477,7 +477,8 @@ describe("extension", () => { expect.any(String) /** port */, expectedUrl /** targetUrl */, expect.any(String) /** userDataDir */, - expect.any(Boolean) /** headlessOverride */ + expect.any(Boolean) /** headlessOverride */, + 10000 /** timeout */ ); expect(startDebuggingMock).toHaveBeenCalledWith(undefined, expect.objectContaining({ url: expectedUrl diff --git a/test/puppeteerCoreStub.ts b/test/puppeteerCoreStub.ts index a15fea5f..920b9b00 100644 --- a/test/puppeteerCoreStub.ts +++ b/test/puppeteerCoreStub.ts @@ -27,7 +27,8 @@ export enum TargetType { TAB = 'tab', } -// `src/utils.ts` uses a default import and calls `puppeteer.launch(...)`. +// The launcher uses Puppeteer's standard arguments and connects through CDP. export default { - launch: jest.fn(), + defaultArgs: jest.fn(async ({ args }: { args: string[] }) => args), + connect: jest.fn(), }; diff --git a/test/utils.test.ts b/test/utils.test.ts index 21553955..0c4b8cd8 100644 --- a/test/utils.test.ts +++ b/test/utils.test.ts @@ -588,55 +588,47 @@ describe("utils", () => { describe("launchBrowser", () => { beforeEach(async () => { - jest.mock("puppeteer-core", () => { - return { - launch: jest.fn(), - }; - }); - jest.doMock("path"); - jest.doMock("os"); + jest.doMock("../src/browserLauncher", () => ({ + launchAndConnectToBrowser: jest.fn().mockResolvedValue({}), + })); jest.resetModules(); }); it("spawns the process", async () => { utils = await import("../src/utils"); - const expectedTempPath = "C:\\someTempPath"; - const os = jest.requireMock("os"); - os.tmpdir.mockReturnValue(expectedTempPath); - const executablePath = "somePath"; const expectedPort = 9222; const expectedUrl = "http://example.com"; const expectedUserDataDir = "profile"; const headless = false; await utils.launchBrowser(executablePath, expectedPort, expectedUrl, expectedUserDataDir); - const puppeteer = jest.requireMock("puppeteer-core"); - expect(puppeteer.launch).toHaveBeenCalledWith( - { - args: [ - "--user-data-dir=profile", - "--no-first-run", - "--no-default-browser-check", - `--remote-debugging-port=${expectedPort}`, - expectedUrl, - ], - executablePath, - headless, - }, + const launcher = jest.requireMock("../src/browserLauncher"); + expect(launcher.launchAndConnectToBrowser).toHaveBeenCalledWith( + executablePath, + expectedPort, + [ + "--user-data-dir=profile", + "--no-first-run", + "--no-default-browser-check", + `--remote-debugging-port=${expectedPort}`, + expectedUrl, + ], + headless, + utils.SETTINGS_DEFAULT_ATTACH_TIMEOUT, ); - utils.launchBrowser(executablePath, expectedPort, expectedUrl); - expect(puppeteer.launch).toHaveBeenCalledWith( - { - args: [ - "--no-first-run", - "--no-default-browser-check", - `--remote-debugging-port=${expectedPort}`, - expectedUrl, - ], - executablePath, - headless, - }, + await utils.launchBrowser(executablePath, expectedPort, expectedUrl); + expect(launcher.launchAndConnectToBrowser).toHaveBeenCalledWith( + executablePath, + expectedPort, + [ + "--no-first-run", + "--no-default-browser-check", + `--remote-debugging-port=${expectedPort}`, + expectedUrl, + ], + headless, + utils.SETTINGS_DEFAULT_ATTACH_TIMEOUT, ); }); @@ -652,28 +644,24 @@ describe("utils", () => { vscodeMock.workspace.getConfiguration.mockImplementationOnce(() => configMock); utils = await import("../src/utils"); - const expectedTempPath = "C:\\someTempPath"; - const os = jest.requireMock("os"); - os.tmpdir.mockReturnValue(expectedTempPath); - const executablePath = "somePath"; const expectedPort = 9222; const expectedUrl = "http://example.com"; const expectedUserDataDir = "profile"; await utils.launchBrowser(executablePath, expectedPort, expectedUrl, expectedUserDataDir); - const puppeteer = jest.requireMock("puppeteer-core"); - expect(puppeteer.launch).toHaveBeenCalledWith( - { - args: [ - "--user-data-dir=profile", - "--no-first-run", - "--no-default-browser-check", - `--remote-debugging-port=${expectedPort}`, - expectedUrl, - ], - executablePath, - headless: expectedSettings.headless, - }, + const launcher = jest.requireMock("../src/browserLauncher"); + expect(launcher.launchAndConnectToBrowser).toHaveBeenCalledWith( + executablePath, + expectedPort, + [ + "--user-data-dir=profile", + "--no-first-run", + "--no-default-browser-check", + `--remote-debugging-port=${expectedPort}`, + expectedUrl, + ], + expectedSettings.headless, + utils.SETTINGS_DEFAULT_ATTACH_TIMEOUT, ); }); });