fix(close): fix a race during context.close and page.close (#4018)

There is a race between "close" event coming from the server and
"close" command issued from the client.

This is similar to calling close after disconnect, so added tests.

src/client/browser.ts

@@ -22,6 +22,7 @@ import { Events } from './events';
 import { BrowserContextOptions } from './types';
 import { validateHeaders } from './network';
 import { headersObjectToArray } from '../utils/utils';
+import { isSafeCloseError } from '../utils/errors';
 
 export class Browser extends ChannelOwner<channels.BrowserChannel, channels.BrowserInitializer> {
   readonly _contexts = new Set<BrowserContext>();
@@ -88,9 +89,7 @@ export class Browser extends ChannelOwner<channels.BrowserChannel, channels.Brow
         await this._closedPromise;
       });
     } catch (e) {
-      if (e.message === 'browser.close: Browser has been closed')
-        return;
-      if (e.message === 'browser.close: Target browser or context has been closed')
+      if (isSafeCloseError(e))
         return;
       throw e;
     }

src/client/browserContext.ts

@@ -27,6 +27,7 @@ import { TimeoutSettings } from '../utils/timeoutSettings';
 import { Waiter } from './waiter';
 import { URLMatch, Headers, WaitForEventOptions } from './types';
 import { isUnderTest, headersObjectToArray } from '../utils/utils';
+import { isSafeCloseError } from '../utils/errors';
 
 export class BrowserContext extends ChannelOwner<channels.BrowserContextChannel, channels.BrowserContextInitializer> {
   _pages = new Set<Page>();
@@ -36,7 +37,6 @@ export class BrowserContext extends ChannelOwner<channels.BrowserContextChannel,
   readonly _bindings = new Map<string, frames.FunctionWithSource>();
   _timeoutSettings = new TimeoutSettings();
   _ownerPage: Page | undefined;
-  private _isClosedOrClosing = false;
   private _closedPromise: Promise<void>;
 
   static from(context: channels.BrowserContextChannel): BrowserContext {
@@ -222,19 +222,21 @@ export class BrowserContext extends ChannelOwner<channels.BrowserContextChannel,
   }
 
   async _onClose() {
-    this._isClosedOrClosing = true;
     if (this._browser)
       this._browser._contexts.delete(this);
     this.emit(Events.BrowserContext.Close);
   }
 
   async close(): Promise<void> {
-    return this._wrapApiCall('browserContext.close', async () => {
-      if (!this._isClosedOrClosing) {
-        this._isClosedOrClosing = true;
+    try {
+      await this._wrapApiCall('browserContext.close', async () => {
         await this._channel.close();
-      }
-      await this._closedPromise;
-    });
+        await this._closedPromise;
+      });
+    } catch (e) {
+      if (isSafeCloseError(e))
+        return;
+      throw e;
+    }
   }
 }

src/client/browserType.ts

@@ -29,6 +29,7 @@ import { envObjectToArray } from './clientHelper';
 import { validateHeaders } from './network';
 import { assert, makeWaitForNextTask, headersObjectToArray } from '../utils/utils';
 import { SelectorsOwner, sharedSelectors } from './selectors';
+import { kBrowserClosedError } from '../utils/errors';
 
 export interface BrowserServerLauncher {
   launchServer(options?: LaunchServerOptions): Promise<BrowserServer>;
@@ -127,7 +128,7 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
       connection.onmessage = message => {
         if (ws.readyState !== WebSocket.OPEN) {
           setTimeout(() => {
-            connection.dispatch({ id: (message as any).id, error: serializeError(new Error('Browser has been closed')) });
+            connection.dispatch({ id: (message as any).id, error: serializeError(new Error(kBrowserClosedError)) });
           }, 0);
           return;
         }

src/client/page.ts

@@ -43,6 +43,7 @@ import * as util from 'util';
 import { Size, URLMatch, Headers, LifecycleEvent, WaitForEventOptions, SelectOption, SelectOptionOptions, FilePayload, WaitForFunctionOptions } from './types';
 import { evaluationScript, urlMatches } from './clientHelper';
 import { isString, isRegExp, isObject, mkdirIfNeeded, headersObjectToArray } from '../utils/utils';
+import { isSafeCloseError } from '../utils/errors';
 
 const fsWriteFileAsync = util.promisify(fs.writeFile.bind(fs));
 const mkdirAsync = util.promisify(fs.mkdir);
@@ -451,11 +452,17 @@ export class Page extends ChannelOwner<channels.PageChannel, channels.PageInitia
   }
 
   async close(options: { runBeforeUnload?: boolean } = {runBeforeUnload: undefined}) {
-    return this._wrapApiCall('page.close', async () => {
-      await this._channel.close(options);
-      if (this._ownedContext)
-        await this._ownedContext.close();
-    });
+    try {
+      await this._wrapApiCall('page.close', async () => {
+        await this._channel.close(options);
+        if (this._ownedContext)
+          await this._ownedContext.close();
+      });
+    } catch (e) {
+      if (isSafeCloseError(e))
+        return;
+      throw e;
+    }
   }
 
   isClosed(): boolean {

src/dispatchers/dispatcher.ts

@@ -20,6 +20,7 @@ import { serializeError } from '../protocol/serializers';
 import { createScheme, Validator, ValidationError } from '../protocol/validator';
 import { assert, createGuid, debugAssert, isUnderTest } from '../utils/utils';
 import { tOptional } from '../protocol/validatorPrimitives';
+import { kBrowserOrContextClosedError } from '../utils/errors';
 
 export const dispatcherSymbol = Symbol('dispatcher');
 
@@ -167,7 +168,7 @@ export class DispatcherConnection {
     const { id, guid, method, params, metadata } = message as any;
     const dispatcher = this._dispatchers.get(guid);
     if (!dispatcher) {
-      this.onmessage({ id, error: serializeError(new Error('Target browser or context has been closed')) });
+      this.onmessage({ id, error: serializeError(new Error(kBrowserOrContextClosedError)) });
       return;
     }
     if (method === 'debugScopeState') {

src/utils/errors.ts

@@ -24,3 +24,10 @@ class CustomError extends Error {
 }
 
 export class TimeoutError extends CustomError {}
+
+export const kBrowserClosedError = 'Browser has been closed';
+export const kBrowserOrContextClosedError = 'Target browser or context has been closed';
+
+export function isSafeCloseError(error: Error) {
+  return error.message.endsWith(kBrowserClosedError) || error.message.endsWith(kBrowserOrContextClosedError);
+}

test/browsertype-connect.spec.ts

@@ -211,4 +211,25 @@ describe('connect', (suite, { wire }) => {
     ]);
     await remote.close();
   });
+
+  it('should not throw on context.close after disconnect', async ({browserType, remoteServer, server}) => {
+    const remote = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() });
+    const context = await remote.newContext();
+    await context.newPage();
+    await Promise.all([
+      new Promise(f => remote.on('disconnected', f)),
+      remoteServer.close()
+    ]);
+    await context.close();
+  });
+
+  it('should not throw on page.close after disconnect', async ({browserType, remoteServer, server}) => {
+    const remote = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() });
+    const page = await remote.newPage();
+    await Promise.all([
+      new Promise(f => remote.on('disconnected', f)),
+      remoteServer.close()
+    ]);
+    await page.close();
+  });
 });