refactor: misc refactoring around browsers debugging and stability (#9979)

2023-04-06 11:14:58 +02:00 · 2023-04-06 11:14:58 +02:00 · c874a81445
commit c874a81445
parent 0b4a2635f5
12 changed files with 186 additions and 117 deletions
--- 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

--- 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"
      ],
--- 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,
    });
  }
 }
--- 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<string, [number, number]>();
+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<InstalledBrowser> {
      };
    }
    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<InstalledBrowser> {
  }
  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);
--- 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.
--- 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',
--- 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();
--- 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();
--- 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 () {
--- 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 () => {
--- 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;
+  }
+}
--- 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,