diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cb071d25d59..f0241b97f56 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -399,11 +399,6 @@ jobs: run: npm ci env: PUPPETEER_SKIP_DOWNLOAD: true - - name: Setup cache for browser binaries - uses: actions/cache@v3 - with: - path: packages/browsers/test/cache - key: browsers-${{ hashFiles('packages/browsers/tools/downloadTestBrowsers.mjs') }}-${{ hashFiles('packages/browsers/test/src/versions.ts') }} - name: Run tests run: npm run test --workspace @puppeteer/browsers diff --git a/packages/browsers/package.json b/packages/browsers/package.json index dd478ac84d2..039d521bd2e 100644 --- a/packages/browsers/package.json +++ b/packages/browsers/package.json @@ -60,7 +60,7 @@ ] }, "test": { - "command": "node tools/downloadTestBrowsers.mjs && mocha", + "command": "node tools/downloadTestBrowsers.mjs && cross-env DEBUG=puppeteer:* mocha", "files": [ ".mocharc.cjs" ], diff --git a/packages/browsers/src/Cache.ts b/packages/browsers/src/Cache.ts index 231a6fbebbb..255c5a7b0f5 100644 --- a/packages/browsers/src/Cache.ts +++ b/packages/browsers/src/Cache.ts @@ -62,7 +62,7 @@ export class Cache { force: true, recursive: true, maxRetries: 10, - retryDelay: 200, + retryDelay: 500, }); } } diff --git a/packages/browsers/src/fetch.ts b/packages/browsers/src/fetch.ts index 6e09fe2f4ba..970a042fe73 100644 --- a/packages/browsers/src/fetch.ts +++ b/packages/browsers/src/fetch.ts @@ -33,6 +33,22 @@ import {downloadFile, headHttpRequest} from './httpUtil.js'; const debugFetch = debug('puppeteer:browsers:fetcher'); +const times = new Map(); +function debugTime(label: string) { + times.set(label, process.hrtime()); +} + +function debugTimeEnd(label: string) { + const end = process.hrtime(); + const start = times.get(label); + if (!start) { + return; + } + const duration = + end[0] * 1000 + end[1] / 1e6 - (start[0] * 1000 + start[1] / 1e6); // calculate duration in milliseconds + debugFetch(`Duration for ${label}: ${duration}ms`); +} + /** * @public */ @@ -121,7 +137,9 @@ export async function fetch(options: Options): Promise { }; } debugFetch(`Downloading binary from ${url}`); + debugTime('download'); await downloadFile(url, archivePath, options.downloadProgressCallback); + debugTimeEnd('download'); return { path: archivePath, browser: options.browser, @@ -145,9 +163,22 @@ export async function fetch(options: Options): Promise { } try { debugFetch(`Downloading binary from ${url}`); - await downloadFile(url, archivePath, options.downloadProgressCallback); + try { + debugTime('download'); + await downloadFile(url, archivePath, options.downloadProgressCallback); + } finally { + debugTimeEnd('download'); + } + debugFetch(`Installing ${archivePath} to ${outputPath}`); - await unpackArchive(archivePath, outputPath); + try { + debugTime('extract'); + await unpackArchive(archivePath, outputPath); + } finally { + debugTimeEnd('extract'); + } + } catch (err) { + debugFetch(`Error during installation`, err); } finally { if (existsSync(archivePath)) { await unlink(archivePath); diff --git a/packages/browsers/src/launcher.ts b/packages/browsers/src/launcher.ts index 7d9a495a0f7..c41168c231c 100644 --- a/packages/browsers/src/launcher.ts +++ b/packages/browsers/src/launcher.ts @@ -160,28 +160,34 @@ class Process { opts.handleSIGINT ??= true; opts.handleSIGTERM ??= true; opts.handleSIGHUP ??= true; - opts.detached ??= true; + // On non-windows platforms, `detached: true` makes child process a + // leader of a new process group, making it possible to kill child + // process tree with `.kill(-pid)` command. @see + // https://nodejs.org/api/child_process.html#child_process_options_detached + opts.detached ??= process.platform !== 'win32'; const stdio = this.#configureStdio({ pipe: opts.pipe, dumpio: opts.dumpio, }); - debugLaunch(`Launching ${this.#executablePath} ${this.#args.join(' ')}`); + debugLaunch(`Launching ${this.#executablePath} ${this.#args.join(' ')}`, { + detached: opts.detached, + env: opts.env, + stdio, + }); this.#browserProcess = childProcess.spawn( this.#executablePath, this.#args, { - // On non-windows platforms, `detached: true` makes child process a - // leader of a new process group, making it possible to kill child - // process tree with `.kill(-pid)` command. @see - // https://nodejs.org/api/child_process.html#child_process_options_detached detached: opts.detached, env: opts.env, stdio, } ); + + debugLaunch(`Launched ${this.#browserProcess.pid}`); if (opts.dumpio) { this.#browserProcess.stderr?.pipe(process.stderr); this.#browserProcess.stdout?.pipe(process.stdout); @@ -201,6 +207,7 @@ class Process { } this.#browserProcessExiting = new Promise((resolve, reject) => { this.#browserProcess.once('exit', async () => { + debugLaunch(`Browser process ${this.#browserProcess.pid} onExit`); this.#clearListeners(); this.#exited = true; try { @@ -281,6 +288,7 @@ class Process { } kill(): void { + debugLaunch(`Trying to kill ${this.#browserProcess.pid}`); // If the process failed to launch (for example if the browser executable path // is invalid), then the process does not get a pid assigned. A call to // `proc.kill` would error, as the `pid` to-be-killed can not be found. @@ -290,12 +298,17 @@ class Process { pidExists(this.#browserProcess.pid) ) { try { + debugLaunch(`Browser process ${this.#browserProcess.pid} exists`); if (process.platform === 'win32') { try { childProcess.execSync( `taskkill /pid ${this.#browserProcess.pid} /T /F` ); } catch (error) { + debugLaunch( + `Killing ${this.#browserProcess.pid} using taskkill failed`, + error + ); // taskkill can fail to kill the process e.g. due to missing permissions. // Let's kill the process via Node API. This delays killing of all child // processes of `this.proc` until the main Node.js process dies. @@ -309,6 +322,10 @@ class Process { try { process.kill(processGroupId, 'SIGKILL'); } catch (error) { + debugLaunch( + `Killing ${this.#browserProcess.pid} using process.kill failed`, + error + ); // Killing the process group can fail due e.g. to missing permissions. // Let's kill the process via Node API. This delays killing of all child // processes of `this.proc` until the main Node.js process dies. diff --git a/packages/browsers/test/src/chrome/fetch.spec.ts b/packages/browsers/test/src/chrome/fetch.spec.ts index 826e2d76675..3a2db243d27 100644 --- a/packages/browsers/test/src/chrome/fetch.spec.ts +++ b/packages/browsers/test/src/chrome/fetch.spec.ts @@ -176,7 +176,7 @@ describe('Chrome fetch', () => { }); it('can fetch via a proxy', async function () { - this.timeout(60000); + this.timeout(120000); const expectedOutputPath = path.join( tmpDir, 'chrome', diff --git a/packages/browsers/test/src/chrome/launcher.spec.ts b/packages/browsers/test/src/chrome/launcher.spec.ts index 26d4e40de97..9aef9079a7f 100644 --- a/packages/browsers/test/src/chrome/launcher.spec.ts +++ b/packages/browsers/test/src/chrome/launcher.spec.ts @@ -26,9 +26,8 @@ import { fetch, Browser, BrowserPlatform, - Cache, } from '../../../lib/cjs/main.js'; -import {getServerUrl, setupTestServer} from '../utils.js'; +import {getServerUrl, setupTestServer, clearCache} from '../utils.js'; import {testChromeBuildId} from '../versions.js'; describe('Chrome', () => { @@ -64,9 +63,44 @@ describe('Chrome', () => { }); afterEach(() => { - new Cache(tmpDir).clear(); + clearCache(tmpDir); }); + function getArgs() { + return [ + '--allow-pre-commit-input', + '--disable-background-networking', + '--disable-background-timer-throttling', + '--disable-backgrounding-occluded-windows', + '--disable-breakpad', + '--disable-client-side-phishing-detection', + '--disable-component-extensions-with-background-pages', + '--disable-component-update', + '--disable-default-apps', + '--disable-dev-shm-usage', + '--disable-extensions', + '--disable-features=Translate,BackForwardCache,AcceptCHFrame,MediaRouter,OptimizationHints,DialMediaRouteProvider', + '--disable-hang-monitor', + '--disable-ipc-flooding-protection', + '--disable-popup-blocking', + '--disable-prompt-on-repost', + '--disable-renderer-backgrounding', + '--disable-sync', + '--enable-automation', + '--enable-features=NetworkServiceInProcess2', + '--export-tagged-pdf', + '--force-color-profile=srgb', + '--headless=new', + '--metrics-recording-only', + '--no-first-run', + '--password-store=basic', + '--remote-debugging-port=9222', + '--use-mock-keychain', + `--user-data-dir=${path.join(tmpDir, 'profile')}`, + 'about:blank', + ]; + } + it('should launch a Chrome browser', async () => { const executablePath = computeExecutablePath({ cacheDir: tmpDir, @@ -75,12 +109,7 @@ describe('Chrome', () => { }); const process = launch({ executablePath, - args: [ - '--headless=new', - '--use-mock-keychain', - '--disable-features=DialMediaRouteProvider', - `--user-data-dir=${path.join(tmpDir, 'profile')}`, - ], + args: getArgs(), }); await process.close(); }); @@ -93,38 +122,7 @@ describe('Chrome', () => { }); const process = launch({ executablePath, - args: [ - '--allow-pre-commit-input', - '--disable-background-networking', - '--disable-background-timer-throttling', - '--disable-backgrounding-occluded-windows', - '--disable-breakpad', - '--disable-client-side-phishing-detection', - '--disable-component-extensions-with-background-pages', - '--disable-component-update', - '--disable-default-apps', - '--disable-dev-shm-usage', - '--disable-extensions', - '--disable-features=Translate,BackForwardCache,AcceptCHFrame,MediaRouter,OptimizationHints,DialMediaRouteProvider', - '--disable-hang-monitor', - '--disable-ipc-flooding-protection', - '--disable-popup-blocking', - '--disable-prompt-on-repost', - '--disable-renderer-backgrounding', - '--disable-sync', - '--enable-automation', - '--enable-features=NetworkServiceInProcess2', - '--export-tagged-pdf', - '--force-color-profile=srgb', - '--headless=new', - '--metrics-recording-only', - '--no-first-run', - '--password-store=basic', - '--remote-debugging-port=9222', - '--use-mock-keychain', - `--user-data-dir=${path.join(tmpDir, 'profile')}`, - 'about:blank', - ], + args: getArgs(), }); const url = await process.waitForLineOutput(CDP_WEBSOCKET_ENDPOINT_REGEX); await process.close(); diff --git a/packages/browsers/test/src/chromium/launcher.spec.ts b/packages/browsers/test/src/chromium/launcher.spec.ts index 2787424adfd..cd9c2ac817c 100644 --- a/packages/browsers/test/src/chromium/launcher.spec.ts +++ b/packages/browsers/test/src/chromium/launcher.spec.ts @@ -26,9 +26,8 @@ import { fetch, Browser, BrowserPlatform, - Cache, } from '../../../lib/cjs/main.js'; -import {getServerUrl, setupTestServer} from '../utils.js'; +import {getServerUrl, setupTestServer, clearCache} from '../utils.js'; import {testChromiumBuildId} from '../versions.js'; describe('Chromium', () => { @@ -47,7 +46,7 @@ describe('Chromium', () => { describe('launcher', function () { setupTestServer(); - this.timeout(60000); + this.timeout(120000); let tmpDir = '/tmp/puppeteer-browsers-test'; @@ -64,10 +63,45 @@ describe('Chromium', () => { }); afterEach(() => { - new Cache(tmpDir).clear(); + clearCache(tmpDir); }); - it('should launch a Chrome browser', async () => { + function getArgs() { + return [ + '--allow-pre-commit-input', + '--disable-background-networking', + '--disable-background-timer-throttling', + '--disable-backgrounding-occluded-windows', + '--disable-breakpad', + '--disable-client-side-phishing-detection', + '--disable-component-extensions-with-background-pages', + '--disable-component-update', + '--disable-default-apps', + '--disable-dev-shm-usage', + '--disable-extensions', + '--disable-features=Translate,BackForwardCache,AcceptCHFrame,MediaRouter,OptimizationHints,DialMediaRouteProvider', + '--disable-hang-monitor', + '--disable-ipc-flooding-protection', + '--disable-popup-blocking', + '--disable-prompt-on-repost', + '--disable-renderer-backgrounding', + '--disable-sync', + '--enable-automation', + '--enable-features=NetworkServiceInProcess2', + '--export-tagged-pdf', + '--force-color-profile=srgb', + '--headless=new', + '--metrics-recording-only', + '--no-first-run', + '--password-store=basic', + '--remote-debugging-port=9222', + '--use-mock-keychain', + `--user-data-dir=${path.join(tmpDir, 'profile')}`, + 'about:blank', + ]; + } + + it('should launch a Chromium browser', async () => { const executablePath = computeExecutablePath({ cacheDir: tmpDir, browser: Browser.CHROMIUM, @@ -75,12 +109,7 @@ describe('Chromium', () => { }); const process = launch({ executablePath, - args: [ - '--headless=new', - '--use-mock-keychain', - '--disable-features=DialMediaRouteProvider', - `--user-data-dir=${path.join(tmpDir, 'profile')}`, - ], + args: getArgs(), }); await process.close(); }); @@ -93,13 +122,7 @@ describe('Chromium', () => { }); const process = launch({ executablePath, - args: [ - '--headless=new', - '--use-mock-keychain', - '--disable-features=DialMediaRouteProvider', - '--remote-debugging-port=9222', - `--user-data-dir=${path.join(tmpDir, 'profile')}`, - ], + args: getArgs(), }); const url = await process.waitForLineOutput(CDP_WEBSOCKET_ENDPOINT_REGEX); await process.close(); diff --git a/packages/browsers/test/src/firefox/fetch.spec.ts b/packages/browsers/test/src/firefox/fetch.spec.ts index 8b4a7787b1f..97548b757f5 100644 --- a/packages/browsers/test/src/firefox/fetch.spec.ts +++ b/packages/browsers/test/src/firefox/fetch.spec.ts @@ -19,8 +19,8 @@ import fs from 'fs'; import os from 'os'; import path from 'path'; -import {fetch, Browser, BrowserPlatform, Cache} from '../../../lib/cjs/main.js'; -import {setupTestServer, getServerUrl} from '../utils.js'; +import {fetch, Browser, BrowserPlatform} from '../../../lib/cjs/main.js'; +import {setupTestServer, getServerUrl, clearCache} from '../utils.js'; import {testFirefoxBuildId} from '../versions.js'; /** @@ -37,7 +37,7 @@ describe('Firefox fetch', () => { }); afterEach(() => { - new Cache(tmpDir).clear(); + clearCache(tmpDir); }); it('should download a buildId that is a bzip2 archive', async function () { diff --git a/packages/browsers/test/src/firefox/launcher.spec.ts b/packages/browsers/test/src/firefox/launcher.spec.ts index b6e39dc3cdf..83c6151c891 100644 --- a/packages/browsers/test/src/firefox/launcher.spec.ts +++ b/packages/browsers/test/src/firefox/launcher.spec.ts @@ -15,7 +15,6 @@ */ import assert from 'assert'; -import {execSync} from 'child_process'; import fs from 'fs'; import os from 'os'; import path from 'path'; @@ -26,10 +25,9 @@ import { fetch, Browser, BrowserPlatform, - Cache, createProfile, } from '../../../lib/cjs/main.js'; -import {setupTestServer, getServerUrl} from '../utils.js'; +import {setupTestServer, getServerUrl, clearCache} from '../utils.js'; import {testFirefoxBuildId} from '../versions.js'; describe('Firefox', () => { @@ -65,14 +63,7 @@ describe('Firefox', () => { }); afterEach(() => { - try { - new Cache(tmpDir).clear(); - } catch (err) { - if (os.platform() === 'win32') { - console.log(execSync('tasklist').toString('utf-8')); - } - throw err; - } + clearCache(tmpDir); }); it('should launch a Firefox browser', async () => { diff --git a/packages/browsers/test/src/utils.ts b/packages/browsers/test/src/utils.ts index 7ff28ac7ae1..ee77f5dbc12 100644 --- a/packages/browsers/test/src/utils.ts +++ b/packages/browsers/test/src/utils.ts @@ -14,12 +14,17 @@ * limitations under the License. */ +import {execSync} from 'child_process'; +import os from 'os'; import path from 'path'; import * as readline from 'readline'; import {Writable, Readable} from 'stream'; import {TestServer} from '@pptr/testserver'; +import {isErrorLike} from '../../lib/cjs/launcher.js'; +import {Cache} from '../../lib/cjs/main.js'; + export function createMockedReadlineInterface( input: string ): readline.Interface { @@ -62,3 +67,19 @@ export function setupTestServer(): void { export function getServerUrl(): string { return `http://localhost:${state.server!.port}`; } + +export function clearCache(tmpDir: string): void { + try { + new Cache(tmpDir).clear(); + } catch (err) { + if (os.platform() === 'win32') { + console.log(execSync('tasklist').toString('utf-8')); + // Sometimes on Windows the folder cannot be removed due to unknown reasons. + // We suppress the error to avoud flakiness. + if (isErrorLike(err) && err.message.includes('EBUSY')) { + return; + } + } + throw err; + } +} diff --git a/packages/browsers/tools/downloadTestBrowsers.mjs b/packages/browsers/tools/downloadTestBrowsers.mjs index 5127278412b..1af9906e554 100644 --- a/packages/browsers/tools/downloadTestBrowsers.mjs +++ b/packages/browsers/tools/downloadTestBrowsers.mjs @@ -38,7 +38,6 @@ function getBrowser(str) { } const cacheDir = path.normalize(path.join('.', 'test', 'cache')); -const promises = []; for (const version of Object.keys(versions)) { const browser = getBrowser(version); @@ -50,37 +49,31 @@ for (const version of Object.keys(versions)) { const buildId = versions[version]; for (const platform of Object.values(BrowserPlatform)) { - promises.push( - (async function download(buildId, platform) { - const targetPath = path.join( - cacheDir, - 'server', - ...downloadPaths[browser](platform, buildId) - ); - - if (fs.existsSync(targetPath)) { - return; - } - - const result = await fetch({ - browser, - buildId, - platform, - cacheDir: path.join(cacheDir, 'tmp'), - install: false, - }); - - fs.mkdirSync(path.dirname(targetPath), { - recursive: true, - }); - fs.copyFileSync(result.path, targetPath); - })(buildId, platform) + const targetPath = path.join( + cacheDir, + 'server', + ...downloadPaths[browser](platform, buildId) ); + + if (fs.existsSync(targetPath)) { + continue; + } + + const result = await fetch({ + browser, + buildId, + platform, + cacheDir: path.join(cacheDir, 'tmp'), + install: false, + }); + + fs.mkdirSync(path.dirname(targetPath), { + recursive: true, + }); + fs.copyFileSync(result.path, targetPath); } } -await Promise.all(promises); - fs.rmSync(path.join(cacheDir, 'tmp'), { recursive: true, force: true,