Fix auto-attach failing when launching a new Edge instance

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<Browser> {
+    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<never>((_, 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<never>): Promise<Browser> {
+    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<never>((_, 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,
+        ]);
+    }
+}

src/extension.ts

@@ -270,9 +270,9 @@ export async function launchHtml(fileUri: vscode.Uri): Promise<void> {
         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);
         }

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<IUserConfig> = {}): 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<Browser> {
+export async function launchBrowser(
+    browserPath: string, port: number, targetUrl: string, userDataDir?: string, forceHeadless?: boolean,
+    timeout = SETTINGS_DEFAULT_ATTACH_TIMEOUT,
+): Promise<Browser> {
     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);
 }
 
 /**

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<Browser> = { waitForTarget, disconnect };
+        browser = partialBrowser as Browser;
+        jest.mocked(puppeteer.connect).mockResolvedValue(browser);
+    });
+
+    afterEach(() => {
+        jest.useRealTimers();
+    });
+
+    function launch(timeout = 1000): Promise<Browser> {
+        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<Browser>(() => 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;
+    });
+});

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

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(),
 };