diff --git a/src/browser.ts b/src/browser.ts index bf761360d7..ecab07e731 100644 --- a/src/browser.ts +++ b/src/browser.ts @@ -49,6 +49,7 @@ export abstract class BrowserBase extends EventEmitter implements Browser { readonly _options: BrowserOptions; private _downloads = new Map(); _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)); diff --git a/src/browserContext.ts b/src/browserContext.ts index d9423c9334..9ff9395099 100644 --- a/src/browserContext.ts +++ b/src/browserContext.ts @@ -61,7 +61,8 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser readonly _pageBindings = new Map(); readonly _options: BrowserContextOptions; _routes: { url: types.URLMatch, handler: network.RouteHandler }[] = []; - _closed = false; + private _isPersistentContext: boolean; + private _startedClosing = false; readonly _closePromise: Promise; private _closePromiseFulfill: ((error: Error) => void) | undefined; readonly _permissions = new Map(); @@ -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; abstract route(url: types.URLMatch, handler: network.RouteHandler): Promise; abstract unroute(url: types.URLMatch, handler?: network.RouteHandler): Promise; - abstract close(): Promise; + abstract _doClose(): Promise; async cookies(urls: string | string[] | undefined = []): Promise { 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) { diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 3c57f4bea7..c3e92274a0 100644 --- a/src/chromium/crBrowser.ts +++ b/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[] { diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index e57c4618d9..238a60af24 100644 --- a/src/firefox/ffBrowser.ts +++ b/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(); } } diff --git a/src/page.ts b/src/page.ts index c80a2ed99f..df1f87c556 100644 --- a/src/page.ts +++ b/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; 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(func: () => T): T { diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index e72c8f0a96..715b4dfd8c 100644 --- a/src/webkit/wkBrowser.ts +++ b/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 { @@ -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(); } } diff --git a/test/browsercontext.spec.js b/test/browsercontext.spec.js index fd57d519ce..71ce91ca22 100644 --- a/test/browsercontext.spec.js +++ b/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() { diff --git a/test/launcher.spec.js b/test/launcher.spec.js index f3c431a7a2..610be5f38c 100644 --- a/test/launcher.spec.js +++ b/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() { diff --git a/test/page.spec.js b/test/page.spec.js index c53ca361bc..1758009ea5 100644 --- a/test/page.spec.js +++ b/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() {