chore: make launch, newContext and newPage progress "strict"

Author: dgozman

packages/playwright-core/src/server/browser.ts

@@ -20,13 +20,15 @@ import { Download } from './download';
 import { SdkObject } from './instrumentation';
 import { Page } from './page';
 import { ClientCertificatesProxy } from './socksClientCertificatesInterceptor';
+import { ProgressController } from './progress';
 
 import type { CallMetadata } from './instrumentation';
 import type * as types from './types';
 import type { ProxySettings } from './types';
 import type { RecentLogsCollector } from './utils/debugLogger';
 import type * as channels from '@protocol/channels';
 import type { ChildProcess } from 'child_process';
+import type { Progress } from './progress';
 
 
 export interface BrowserProcess {
@@ -84,25 +86,26 @@ export abstract class Browser extends SdkObject {
   abstract version(): string;
   abstract userAgent(): string;
 
-  async newContext(metadata: CallMetadata, options: types.BrowserContextOptions): Promise<BrowserContext> {
+  newContextFromMetadata(metadata: CallMetadata, options: types.BrowserContextOptions): Promise<BrowserContext> {
+    const controller = new ProgressController(metadata, this, 'strict');
+    return controller.run(progress => this.newContext(progress, options));
+  }
+
+  async newContext(progress: Progress, options: types.BrowserContextOptions): Promise<BrowserContext> {
     validateBrowserContextOptions(options, this.options);
     let clientCertificatesProxy: ClientCertificatesProxy | undefined;
     if (options.clientCertificates?.length) {
-      clientCertificatesProxy = new ClientCertificatesProxy(options);
+      clientCertificatesProxy = await progress.raceWithCleanup(ClientCertificatesProxy.create(options), proxy => proxy.close());
       options = { ...options };
-      options.proxyOverride = await clientCertificatesProxy.listen();
+      options.proxyOverride = clientCertificatesProxy.proxySettings();
       options.internalIgnoreHTTPSErrors = true;
     }
-    let context;
-    try {
-      context = await this.doCreateNewContext(options);
-    } catch (error) {
-      await clientCertificatesProxy?.close();
-      throw error;
-    }
+    const context = await progress.raceWithCleanup(this.doCreateNewContext(options), context => context.close({ reason: 'Failed to create context' }));
     context._clientCertificatesProxy = clientCertificatesProxy;
+    if ((options as any).__testHookBeforeSetStorageState)
+      await progress.race((options as any).__testHookBeforeSetStorageState());
     if (options.storageState)
-      await context.setStorageState(metadata, options.storageState);
+      await context.setStorageState(progress, options.storageState);
     this.emit(Browser.Events.Context, context);
     return context;
   }
@@ -112,7 +115,7 @@ export abstract class Browser extends SdkObject {
     if (!this._contextForReuse || hash !== this._contextForReuse.hash || !this._contextForReuse.context.canResetForReuse()) {
       if (this._contextForReuse)
         await this._contextForReuse.context.close({ reason: 'Context reused' });
-      this._contextForReuse = { context: await this.newContext(metadata, params), hash };
+      this._contextForReuse = { context: await this.newContextFromMetadata(metadata, params), hash };
       return { context: this._contextForReuse.context, needsReset: false };
     }
     await this._contextForReuse.context.stopPendingOperations('Context recreated');

packages/playwright-core/src/server/browserContext.ts

@@ -218,7 +218,7 @@ export abstract class BrowserContext extends SdkObject {
     // Navigate to about:blank first to ensure no page scripts are running after this point.
     await page?.mainFrame().gotoImpl(progress, 'about:blank', {});
 
-    await this._resetStorage();
+    await this._resetStorage(progress);
     await this.clock.resetForReuse();
     // TODO: following can be optimized to not perform noops.
     if (this._options.permissions)
@@ -379,14 +379,13 @@ export abstract class BrowserContext extends SdkObject {
   async _loadDefaultContextAsIs(progress: Progress): Promise<Page | undefined> {
     if (!this.possiblyUninitializedPages().length) {
       const waitForEvent = helper.waitForEvent(progress, this, BrowserContext.Events.Page);
-      progress.cleanupWhenAborted(() => waitForEvent.dispose);
       // Race against BrowserContext.close
       await Promise.race([waitForEvent.promise, this._closePromise]);
     }
     const page = this.possiblyUninitializedPages()[0];
     if (!page)
       return;
-    const pageOrError = await page.waitForInitializedOrError();
+    const pageOrError = await progress.race(page.waitForInitializedOrError());
     if (pageOrError instanceof Error)
       throw pageOrError;
     await page.mainFrame()._waitForLoadState(progress, 'load');
@@ -402,7 +401,7 @@ export abstract class BrowserContext extends SdkObject {
       // Workaround for:
       // - chromium fails to change isMobile for existing page;
       // - webkit fails to change locale for existing page.
-      await this.newPage(progress.metadata);
+      await this.newPage(progress, false);
       await defaultPage.close();
     }
   }
@@ -511,9 +510,14 @@ export abstract class BrowserContext extends SdkObject {
     await this._closePromise;
   }
 
-  async newPage(metadata: CallMetadata): Promise<Page> {
-    const page = await this.doCreateNewPage(metadata.isServerSide);
-    const pageOrError = await page.waitForInitializedOrError();
+  newPageFromMetadata(metadata: CallMetadata): Promise<Page> {
+    const contoller = new ProgressController(metadata, this, 'strict');
+    return contoller.run(progress => this.newPage(progress, false));
+  }
+
+  async newPage(progress: Progress, isServerSide: boolean): Promise<Page> {
+    const page = await progress.raceWithCleanup(this.doCreateNewPage(isServerSide), page => page.close());
+    const pageOrError = await progress.race(page.waitForInitializedOrError());
     if (pageOrError instanceof Page) {
       if (pageOrError.isClosed())
         throw new Error('Page has been closed.');
@@ -526,7 +530,12 @@ export abstract class BrowserContext extends SdkObject {
     this._origins.add(origin);
   }
 
-  async storageState(indexedDB = false): Promise<channels.BrowserContextStorageStateResult> {
+  storageState(indexedDB = false): Promise<channels.BrowserContextStorageStateResult> {
+    const controller = new ProgressController(serverSideCallMetadata(), this, 'strict');
+    return controller.run(progress => this.storageStateImpl(progress, indexedDB));
+  }
+
+  async storageStateImpl(progress: Progress, indexedDB: boolean): Promise<channels.BrowserContextStorageStateResult> {
     const result: channels.BrowserContextStorageStateResult = {
       cookies: await this.cookies(),
       origins: []
@@ -557,15 +566,14 @@ export abstract class BrowserContext extends SdkObject {
 
     // If there are still origins to save, create a blank page to iterate over origins.
     if (originsToSave.size)  {
-      const internalMetadata = serverSideCallMetadata();
-      const page = await this.newPage(internalMetadata);
-      page.addRequestInterceptor(route => {
+      const page = await this.newPage(progress, true);
+      await progress.race(page.addRequestInterceptor(route => {
         route.fulfill({ body: '<html></html>' }).catch(() => {});
-      }, 'prepend');
+      }, 'prepend'));
       for (const origin of originsToSave) {
         const frame = page.mainFrame();
-        await frame.goto(internalMetadata, origin, { timeout: 0 });
-        const storage: SerializedStorage = await frame.evaluateExpression(collectScript, { world: 'utility' });
+        await frame.gotoImpl(progress, origin, {});
+        const storage: SerializedStorage = await progress.race(frame.evaluateExpression(collectScript, { world: 'utility' }));
         if (storage.localStorage.length || storage.indexedDB?.length)
           result.origins.push({ origin, localStorage: storage.localStorage, indexedDB: storage.indexedDB });
       }
@@ -574,29 +582,27 @@ export abstract class BrowserContext extends SdkObject {
     return result;
   }
 
-  async _resetStorage() {
+  async _resetStorage(progress: Progress) {
     const oldOrigins = this._origins;
     const newOrigins = new Map(this._options.storageState?.origins?.map(p => [p.origin, p]) || []);
     if (!oldOrigins.size && !newOrigins.size)
       return;
     let page = this.pages()[0];
 
-    const internalMetadata = serverSideCallMetadata();
-    page = page || await this.newPage({
-      ...internalMetadata,
-      // Do not mark this page as internal, because we will leave it for later reuse
-      // as a user-visible page.
-      isServerSide: false,
-    });
+    // Do not mark this page as internal, because we will leave it for later reuse
+    // as a user-visible page.
+    page = page || await this.newPage(progress, false);
     const interceptor = (route: network.Route) => {
       route.fulfill({ body: '<html></html>' }).catch(() => {});
     };
-    await page.addRequestInterceptor(interceptor, 'prepend');
+
+    progress.cleanupWhenAborted(() => page.removeRequestInterceptor(interceptor));
+    await progress.race(page.addRequestInterceptor(interceptor, 'prepend'));
 
     for (const origin of new Set([...oldOrigins, ...newOrigins.keys()])) {
       const frame = page.mainFrame();
-      await frame.goto(internalMetadata, origin, { timeout: 0 });
-      await frame.resetStorageForCurrentOriginBestEffort(newOrigins.get(origin));
+      await frame.gotoImpl(progress, origin, {});
+      await progress.race(frame.resetStorageForCurrentOriginBestEffort(newOrigins.get(origin)));
     }
 
     await page.removeRequestInterceptor(interceptor);
@@ -615,27 +621,26 @@ export abstract class BrowserContext extends SdkObject {
     return this._settingStorageState;
   }
 
-  async setStorageState(metadata: CallMetadata, state: NonNullable<channels.BrowserNewContextParams['storageState']>) {
+  async setStorageState(progress: Progress, state: NonNullable<channels.BrowserNewContextParams['storageState']>) {
     this._settingStorageState = true;
     try {
       if (state.cookies)
-        await this.addCookies(state.cookies);
+        await progress.race(this.addCookies(state.cookies));
       if (state.origins && state.origins.length)  {
-        const internalMetadata = serverSideCallMetadata();
-        const page = await this.newPage(internalMetadata);
-        await page.addRequestInterceptor(route => {
+        const page = await this.newPage(progress, true);
+        await progress.race(page.addRequestInterceptor(route => {
           route.fulfill({ body: '<html></html>' }).catch(() => {});
-        }, 'prepend');
+        }, 'prepend'));
         for (const originState of state.origins) {
           const frame = page.mainFrame();
-          await frame.goto(metadata, originState.origin, { timeout: 0 });
+          await frame.gotoImpl(progress, originState.origin, {});
           const restoreScript = `(() => {
             const module = {};
             ${rawStorageSource.source}
             const script = new (module.exports.StorageScript())(${this._browser.options.name === 'firefox'});
             return script.restore(${JSON.stringify(originState)});
           })()`;
-          await frame.evaluateExpression(restoreScript, { world: 'utility' });
+          await progress.race(frame.evaluateExpression(restoreScript, { world: 'utility' }));
         }
         await page.close();
       }

packages/playwright-core/src/server/browserType.ts

@@ -23,7 +23,7 @@ import { debugMode } from './utils/debug';
 import { assert } from '../utils/isomorphic/assert';
 import { ManualPromise } from '../utils/isomorphic/manualPromise';
 import { DEFAULT_PLAYWRIGHT_TIMEOUT } from '../utils/isomorphic/time';
-import { existsAsync } from './utils/fileUtils';
+import { existsAsync, removeFolders } from './utils/fileUtils';
 import { helper } from './helper';
 import { SdkObject } from './instrumentation';
 import { PipeTransport } from './pipeTransport';
@@ -69,7 +69,7 @@ export abstract class BrowserType extends SdkObject {
 
   async launch(metadata: CallMetadata, options: types.LaunchOptions, protocolLogger?: types.ProtocolLogger): Promise<Browser> {
     options = this._validateLaunchOptions(options);
-    const controller = new ProgressController(metadata, this);
+    const controller = new ProgressController(metadata, this, 'strict');
     const browser = await controller.run(progress => {
       const seleniumHubUrl = (options as any).__testHookSeleniumRemoteURL || process.env.SELENIUM_REMOTE_URL;
       if (seleniumHubUrl)
@@ -81,17 +81,16 @@ export abstract class BrowserType extends SdkObject {
 
   async launchPersistentContext(metadata: CallMetadata, userDataDir: string, options: channels.BrowserTypeLaunchPersistentContextOptions & { timeout: number, cdpPort?: number, internalIgnoreHTTPSErrors?: boolean }): Promise<BrowserContext> {
     const launchOptions = this._validateLaunchOptions(options);
-    const controller = new ProgressController(metadata, this);
+    const controller = new ProgressController(metadata, this, 'strict');
     const browser = await controller.run(async progress => {
       // Note: Any initial TLS requests will fail since we rely on the Page/Frames initialize which sets ignoreHTTPSErrors.
       let clientCertificatesProxy: ClientCertificatesProxy | undefined;
       if (options.clientCertificates?.length) {
-        clientCertificatesProxy = new ClientCertificatesProxy(options);
-        launchOptions.proxyOverride = await clientCertificatesProxy?.listen();
+        clientCertificatesProxy = await progress.raceWithCleanup(ClientCertificatesProxy.create(options), proxy => proxy.close());
+        launchOptions.proxyOverride = clientCertificatesProxy.proxySettings();
         options = { ...options };
         options.internalIgnoreHTTPSErrors = true;
       }
-      progress.cleanupWhenAborted(() => clientCertificatesProxy?.close());
       const browser = await this._innerLaunchWithRetries(progress, launchOptions, options, helper.debugProtocolLogger(), userDataDir).catch(e => { throw this._rewriteStartupLog(e); });
       browser._defaultContext!._clientCertificatesProxy = clientCertificatesProxy;
       return browser;
@@ -118,7 +117,7 @@ export abstract class BrowserType extends SdkObject {
     const browserLogsCollector = new RecentLogsCollector();
     const { browserProcess, userDataDir, artifactsDir, transport } = await this._launchProcess(progress, options, !!persistent, browserLogsCollector, maybeUserDataDir);
     if ((options as any).__testHookBeforeCreateBrowser)
-      await (options as any).__testHookBeforeCreateBrowser();
+      await progress.race((options as any).__testHookBeforeCreateBrowser());
     const browserOptions: BrowserOptions = {
       name: this._name,
       isChromium: this._name === 'chromium',
@@ -140,30 +139,24 @@ export abstract class BrowserType extends SdkObject {
     if (persistent)
       validateBrowserContextOptions(persistent, browserOptions);
     copyTestHooks(options, browserOptions);
-    const browser = await this.connectToTransport(transport, browserOptions, browserLogsCollector);
+    const browser = await progress.race(this.connectToTransport(transport, browserOptions, browserLogsCollector));
     (browser as any)._userDataDirForTest = userDataDir;
     // We assume no control when using custom arguments, and do not prepare the default context in that case.
     if (persistent && !options.ignoreAllDefaultArgs)
       await browser._defaultContext!._loadDefaultContext(progress);
     return browser;
   }
 
-  private async _launchProcess(progress: Progress, options: types.LaunchOptions, isPersistent: boolean, browserLogsCollector: RecentLogsCollector, userDataDir?: string): Promise<{ browserProcess: BrowserProcess, artifactsDir: string, userDataDir: string, transport: ConnectionTransport }> {
+  private async _prepareToLaunch(options: types.LaunchOptions, isPersistent: boolean, userDataDir: string | undefined) {
     const {
       ignoreDefaultArgs,
       ignoreAllDefaultArgs,
       args = [],
       executablePath = null,
-      handleSIGINT = true,
-      handleSIGTERM = true,
-      handleSIGHUP = true,
     } = options;
-
-    const env = options.env ? envArrayToObject(options.env) : process.env;
-
     await this._createArtifactDirs(options);
 
-    const tempDirectories = [];
+    const tempDirectories: string[] = [];
     const artifactsDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'playwright-artifacts-'));
     tempDirectories.push(artifactsDir);
 
@@ -178,7 +171,7 @@ export abstract class BrowserType extends SdkObject {
     }
     await this.prepareUserDataDir(options, userDataDir);
 
-    const browserArguments = [];
+    const browserArguments: string[] = [];
     if (ignoreAllDefaultArgs)
       browserArguments.push(...args);
     else if (ignoreDefaultArgs)
@@ -199,15 +192,29 @@ export abstract class BrowserType extends SdkObject {
       await registry.validateHostRequirementsForExecutablesIfNeeded([registryExecutable], this.attribution.playwright.options.sdkLanguage);
     }
 
+    return { executable, browserArguments, userDataDir, artifactsDir, tempDirectories };
+  }
+
+  private async _launchProcess(progress: Progress, options: types.LaunchOptions, isPersistent: boolean, browserLogsCollector: RecentLogsCollector, userDataDir?: string): Promise<{ browserProcess: BrowserProcess, artifactsDir: string, userDataDir: string, transport: ConnectionTransport }> {
+    const {
+      handleSIGINT = true,
+      handleSIGTERM = true,
+      handleSIGHUP = true,
+    } = options;
+
+    const env = options.env ? envArrayToObject(options.env) : process.env;
+    const prepared = await progress.race(this._prepareToLaunch(options, isPersistent, userDataDir));
+    progress.cleanupWhenAborted(() => removeFolders(prepared.tempDirectories));
+
     // Note: it is important to define these variables before launchProcess, so that we don't get
     // "Cannot access 'browserServer' before initialization" if something went wrong.
     let transport: ConnectionTransport | undefined = undefined;
     let browserProcess: BrowserProcess | undefined = undefined;
     const exitPromise = new ManualPromise();
     const { launchedProcess, gracefullyClose, kill } = await launchProcess({
-      command: executable,
-      args: browserArguments,
-      env: this.amendEnvironment(env, userDataDir, executable, browserArguments),
+      command: prepared.executable,
+      args: prepared.browserArguments,
+      env: this.amendEnvironment(env, prepared.userDataDir, prepared.executable, prepared.browserArguments),
       handleSIGINT,
       handleSIGTERM,
       handleSIGHUP,
@@ -216,7 +223,7 @@ export abstract class BrowserType extends SdkObject {
         browserLogsCollector.log(message);
       },
       stdio: 'pipe',
-      tempDirectories,
+      tempDirectories: prepared.tempDirectories,
       attemptToGracefullyClose: async () => {
         if ((options as any).__testHookGracefullyClose)
           await (options as any).__testHookGracefullyClose();
@@ -253,7 +260,7 @@ export abstract class BrowserType extends SdkObject {
       kill
     };
     progress.cleanupWhenAborted(() => closeOrKill(progress.timeUntilDeadline()));
-    const { wsEndpoint } = await Promise.race([
+    const { wsEndpoint } = await progress.race([
       this.waitForReadyState(options, browserLogsCollector),
       exitPromise.then(() => ({ wsEndpoint: undefined })),
     ]);
@@ -263,7 +270,8 @@ export abstract class BrowserType extends SdkObject {
       const stdio = launchedProcess.stdio as unknown as [NodeJS.ReadableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.ReadableStream];
       transport = new PipeTransport(stdio[3], stdio[4]);
     }
-    return { browserProcess, artifactsDir, userDataDir, transport };
+    progress.cleanupWhenAborted(() => transport.close());
+    return { browserProcess, artifactsDir: prepared.artifactsDir, userDataDir: prepared.userDataDir, transport };
   }
 
   async _createArtifactDirs(options: types.LaunchOptions): Promise<void> {