fix(close): ensure close() can be called twice (#2744)

... without any exceptions.

src/browser.ts

@@ -49,6 +49,7 @@ export abstract class BrowserBase extends EventEmitter implements Browser {
   readonly _options: BrowserOptions;
   private _downloads = new Map<string, Download>();
   _defaultContext: BrowserContextBase | null = null;
+  private _startedClosing = false;
 
   constructor(options: BrowserOptions) {
     super();
@@ -87,12 +88,21 @@ export abstract class BrowserBase extends EventEmitter implements Browser {
     this._downloads.delete(uuid);
   }
 
+  _didClose() {
+    for (const context of this.contexts())
+      (context as BrowserContextBase)._browserClosed();
+    this.emit(Events.Browser.Disconnected);
+  }
+
   async close() {
-    if (this._options.ownedServer) {
-      await this._options.ownedServer.close();
-    } else {
-      await Promise.all(this.contexts().map(context => context.close()));
-      this._disconnect();
+    if (!this._startedClosing) {
+      this._startedClosing = true;
+      if (this._options.ownedServer) {
+        await this._options.ownedServer.close();
+      } else {
+        await Promise.all(this.contexts().map(context => context.close()));
+        this._disconnect();
+      }
     }
     if (this.isConnected())
       await new Promise(x => this.once(Events.Browser.Disconnected, x));

src/browserContext.ts

@@ -61,7 +61,8 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser
   readonly _pageBindings = new Map<string, PageBinding>();
   readonly _options: BrowserContextOptions;
   _routes: { url: types.URLMatch, handler: network.RouteHandler }[] = [];
-  _closed = false;
+  private _isPersistentContext: boolean;
+  private _startedClosing = false;
   readonly _closePromise: Promise<Error>;
   private _closePromiseFulfill: ((error: Error) => void) | undefined;
   readonly _permissions = new Map<string, string[]>();
@@ -70,12 +71,13 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser
   readonly _apiLogger: Logger;
   private _debugController: DebugController | undefined;
 
-  constructor(browserBase: BrowserBase, options: BrowserContextOptions) {
+  constructor(browserBase: BrowserBase, options: BrowserContextOptions, isPersistentContext: boolean) {
     super();
     this._browserBase = browserBase;
     this._options = options;
     const loggers = options.logger ? new Loggers(options.logger) : browserBase._options.loggers;
     this._apiLogger = loggers.api;
+    this._isPersistentContext = isPersistentContext;
     this._closePromise = new Promise(fulfill => this._closePromiseFulfill = fulfill);
   }
 
@@ -103,16 +105,13 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser
   _browserClosed() {
     for (const page of this.pages())
       page._didClose();
-    this._didCloseInternal(true);
+    this._didCloseInternal();
   }
 
-  async _didCloseInternal(omitDeleteDownloads = false) {
-    this._closed = true;
-    this.emit(Events.BrowserContext.Close);
-    this._closePromiseFulfill!(new Error('Context closed'));
-    if (!omitDeleteDownloads)
-      await Promise.all([...this._downloads].map(d => d.delete()));
+  private _didCloseInternal() {
     this._downloads.clear();
+    this._closePromiseFulfill!(new Error('Context closed'));
+    this.emit(Events.BrowserContext.Close);
   }
 
   // BrowserContext methods.
@@ -131,7 +130,7 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser
   abstract _doExposeBinding(binding: PageBinding): Promise<void>;
   abstract route(url: types.URLMatch, handler: network.RouteHandler): Promise<void>;
   abstract unroute(url: types.URLMatch, handler?: network.RouteHandler): Promise<void>;
-  abstract close(): Promise<void>;
+  abstract _doClose(): Promise<void>;
 
   async cookies(urls: string | string[] | undefined = []): Promise<types.NetworkCookie[]> {
     if (urls && !Array.isArray(urls))
@@ -222,6 +221,22 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser
     if (username && password)
       this._options.httpCredentials = { username, password };
   }
+
+  async close() {
+    if (this._isPersistentContext) {
+      // Default context is only created in 'persistent' mode and closing it should close
+      // the browser.
+      await this._browserBase.close();
+      return;
+    }
+    if (!this._startedClosing) {
+      this._startedClosing = true;
+      await this._doClose();
+      await Promise.all([...this._downloads].map(d => d.delete()));
+      this._didCloseInternal();
+    }
+    await this._closePromise;
+  }
 }
 
 export function assertBrowserContextIsNotOwned(context: BrowserContextBase) {

src/chromium/crBrowser.ts

@@ -89,11 +89,7 @@ export class CRBrowser extends BrowserBase {
     super(options);
     this._connection = connection;
     this._session = this._connection.rootSession;
-    this._connection.on(ConnectionEvents.Disconnected, () => {
-      for (const context of this._contexts.values())
-        context._browserClosed();
-      this.emit(CommonEvents.Browser.Disconnected);
-    });
+    this._connection.on(ConnectionEvents.Disconnected, () => this._didClose());
     this._session.on('Target.attachedToTarget', this._onAttachedToTarget.bind(this));
     this._session.on('Target.detachedFromTarget', this._onDetachedFromTarget.bind(this));
   }
@@ -281,7 +277,7 @@ export class CRBrowserContext extends BrowserContextBase {
   readonly _evaluateOnNewDocumentSources: string[];
 
   constructor(browser: CRBrowser, browserContextId: string | null, options: types.BrowserContextOptions) {
-    super(browser, options);
+    super(browser, options, !browserContextId);
     this._browser = browser;
     this._browserContextId = browserContextId;
     this._evaluateOnNewDocumentSources = [];
@@ -425,18 +421,10 @@ export class CRBrowserContext extends BrowserContextBase {
       await (page._delegate as CRPage).updateRequestInterception();
   }
 
-  async close() {
-    if (this._closed)
-      return;
-    if (!this._browserContextId) {
-      // Default context is only created in 'persistent' mode and closing it should close
-      // the browser.
-      await this._browser.close();
-      return;
-    }
+  async _doClose() {
+    assert(this._browserContextId);
     await this._browser._session.send('Target.disposeBrowserContext', { browserContextId: this._browserContextId });
     this._browser._contexts.delete(this._browserContextId);
-    await this._didCloseInternal();
   }
 
   backgroundPages(): Page[] {

src/firefox/ffBrowser.ts

@@ -53,11 +53,7 @@ export class FFBrowser extends BrowserBase {
     this._connection = connection;
     this._ffPages = new Map();
     this._contexts = new Map();
-    this._connection.on(ConnectionEvents.Disconnected, () => {
-      for (const context of this._contexts.values())
-        context._browserClosed();
-      this.emit(Events.Browser.Disconnected);
-    });
+    this._connection.on(ConnectionEvents.Disconnected, () => this._didClose());
     this._eventListeners = [
       helper.addEventListener(this._connection, 'Browser.attachedToTarget', this._onAttachedToTarget.bind(this)),
       helper.addEventListener(this._connection, 'Browser.detachedFromTarget', this._onDetachedFromTarget.bind(this)),
@@ -147,7 +143,7 @@ export class FFBrowserContext extends BrowserContextBase {
   readonly _browserContextId: string | null;
 
   constructor(browser: FFBrowser, browserContextId: string | null, options: types.BrowserContextOptions) {
-    super(browser, options);
+    super(browser, options, !browserContextId);
     this._browser = browser;
     this._browserContextId = browserContextId;
     this._authenticateProxyViaHeader();
@@ -320,17 +316,9 @@ export class FFBrowserContext extends BrowserContextBase {
       await this._browser._connection.send('Browser.setRequestInterception', { browserContextId: this._browserContextId || undefined, enabled: false });
   }
 
-  async close() {
-    if (this._closed)
-      return;
-    if (!this._browserContextId) {
-      // Default context is only created in 'persistent' mode and closing it should close
-      // the browser.
-      await this._browser.close();
-      return;
-    }
+  async _doClose() {
+    assert(this._browserContextId);
     await this._browser._connection.send('Browser.removeBrowserContext', { browserContextId: this._browserContextId });
     this._browser._contexts.delete(this._browserContextId);
-    await this._didCloseInternal();
   }
 }

src/page.ts

@@ -89,7 +89,7 @@ type PageState = {
 };
 
 export class Page extends EventEmitter {
-  private _closed = false;
+  private _closedState: 'open' | 'closing' | 'closed' = 'open';
   private _closedCallback: () => void;
   private _closedPromise: Promise<void>;
   private _disconnected = false;
@@ -145,8 +145,8 @@ export class Page extends EventEmitter {
   }
 
   _didClose() {
-    assert(!this._closed, 'Page closed twice');
-    this._closed = true;
+    assert(this._closedState !== 'closed', 'Page closed twice');
+    this._closedState = 'closed';
     this.emit(Events.Page.Close);
     this._closedCallback();
   }
@@ -460,11 +460,13 @@ export class Page extends EventEmitter {
   }
 
   async close(options?: { runBeforeUnload?: boolean }) {
-    if (this._closed)
+    if (this._closedState === 'closed')
       return;
-    assert(!this._disconnected, 'Protocol error: Connection closed. Most likely the page has been closed.');
     const runBeforeUnload = !!options && !!options.runBeforeUnload;
-    await this._delegate.closePage(runBeforeUnload);
+    if (this._closedState !== 'closing') {
+      assert(!this._disconnected, 'Protocol error: Connection closed. Most likely the page has been closed.');
+      await this._delegate.closePage(runBeforeUnload);
+    }
     if (!runBeforeUnload)
       await this._closedPromise;
     if (this._ownedContext)
@@ -472,7 +474,7 @@ export class Page extends EventEmitter {
   }
 
   isClosed(): boolean {
-    return this._closed;
+    return this._closedState === 'closed';
   }
 
   private _attributeToPage<T>(func: () => T): T {

src/webkit/wkBrowser.ts

@@ -68,11 +68,7 @@ export class WKBrowser extends BrowserBase {
   _onDisconnect() {
     for (const wkPage of this._wkPages.values())
       wkPage.dispose();
-    for (const context of this._contexts.values())
-      context._browserClosed();
-    // Note: previous method uses pages to issue 'close' event on them, so we clear them after.
-    this._wkPages.clear();
-    this.emit(Events.Browser.Disconnected);
+    this._didClose();
   }
 
   async newContext(options: BrowserContextOptions = {}): Promise<BrowserContext> {
@@ -203,7 +199,7 @@ export class WKBrowserContext extends BrowserContextBase {
   readonly _evaluateOnNewDocumentSources: string[];
 
   constructor(browser: WKBrowser, browserContextId: string | undefined, options: types.BrowserContextOptions) {
-    super(browser, options);
+    super(browser, options, !browserContextId);
     this._browser = browser;
     this._browserContextId = browserContextId;
     this._evaluateOnNewDocumentSources = [];
@@ -337,17 +333,9 @@ export class WKBrowserContext extends BrowserContextBase {
       await (page._delegate as WKPage).updateRequestInterception();
   }
 
-  async close() {
-    if (this._closed)
-      return;
-    if (!this._browserContextId) {
-      // Default context is only created in 'persistent' mode and closing it should close
-      // the browser.
-      await this._browser.close();
-      return;
-    }
+  async _doClose() {
+    assert(this._browserContextId);
     await this._browser._browserSession.send('Playwright.deleteContext', { browserContextId: this._browserContextId });
     this._browser._contexts.delete(this._browserContextId);
-    await this._didCloseInternal();
   }
 }

test/browsercontext.spec.js

@@ -121,6 +121,14 @@ describe('BrowserContext', function() {
     let error = await promise;
     expect(error.message).toContain('Context closed');
   });
+  it('close() should be callable twice', async({browser}) => {
+    const context = await browser.newContext();
+    await Promise.all([
+      context.close(),
+      context.close(),
+    ]);
+    await context.close();
+  });
 });
 
 describe('BrowserContext({userAgent})', function() {

test/launcher.spec.js

@@ -223,6 +223,14 @@ describe('Browser.close', function() {
     await browser.close();
     expect(closed).toBe(true);
   });
+  it('should be callable twice', async({browserType, defaultBrowserOptions}) => {
+    const browser = await browserType.launch(defaultBrowserOptions);
+    await Promise.all([
+      browser.close(),
+      browser.close(),
+    ]);
+    await browser.close();
+  });
 });
 
 describe('browserType.launchServer', function() {

test/page.spec.js

@@ -82,6 +82,14 @@ describe('Page.close', function() {
       expect(message).not.toContain('Timeout');
     }
   });
+  it('should be callable twice', async({context}) => {
+    const newPage = await context.newPage();
+    await Promise.all([
+      newPage.close(),
+      newPage.close(),
+    ]);
+    await newPage.close();
+  });
 });
 
 describe('Page.Events.Load', function() {