fix(chromium): background pages on persistent close error (#7118)

This is a speculative fix that moves "background pages cleanup"
to `_didCloseInternal` so that it is only run once, but on both
context closure and browser closure.

Symptom from a flaky test:

```log
browserContext.close: page@18087c372d32819222707ca5e8fd1030 is sending "close" event after being disposed
    at PageDispatcher._dispatchEvent (D:\a\playwright\playwright\src\dispatchers\dispatcher.ts:86:15)
    at Page.<anonymous> (D:\a\playwright\playwright\src\dispatchers\pageDispatcher.ts:59:12)
    at Page.emit (events.js:314:20)
    at Page._didClose (D:\a\playwright\playwright\src\server\page.ts:220:10)
    at CRPage.didClose (D:\a\playwright\playwright\src\server\chromium\crPage.ts:165:16)
    at CRBrowserContext._onClosePersistent (D:\a\playwright\playwright\src\server\chromium\crBrowser.ts:476:24)
    at CRBrowserContext.close (D:\a\playwright\playwright\src\server\browserContext.ts:288:20)
```
This commit is contained in:
Dmitry Gozman 2021-06-14 21:55:55 -07:00 committed by GitHub
parent 970bb6a70d
commit 2041aab010
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 8 additions and 5 deletions

View File

@ -131,6 +131,8 @@ export abstract class BrowserContext extends SdkObject {
this._closedStatus = 'closed';
this._deleteAllDownloads();
this._downloads.clear();
if (this._isPersistentContext)
this._onClosePersistent();
this._closePromiseFulfill!(new Error('Context closed'));
this.emit(BrowserContext.Events.Close);
}
@ -151,7 +153,7 @@ export abstract class BrowserContext extends SdkObject {
abstract _doExposeBinding(binding: PageBinding): Promise<void>;
abstract _doUpdateRequestInterception(): Promise<void>;
abstract _doClose(): Promise<void>;
abstract _onClosePersistent(): Promise<void>;
abstract _onClosePersistent(): void;
abstract _doCancelDownload(uuid: string): Promise<void>;
async cookies(urls: string | string[] | undefined = []): Promise<types.NetworkCookie[]> {
@ -285,7 +287,6 @@ export abstract class BrowserContext extends SdkObject {
// Close all the pages instead of the context,
// because we cannot close the default context.
await Promise.all(this.pages().map(page => page.close(metadata)));
await this._onClosePersistent();
} else {
// Close the context.
await this._doClose();

View File

@ -470,7 +470,9 @@ export class CRBrowserContext extends BrowserContext {
}
}
async _onClosePersistent() {
_onClosePersistent() {
// When persistent context is closed, we do not necessary get Target.detachedFromTarget
// for all the background pages.
for (const [targetId, backgroundPage] of this._browser._backgroundPages.entries()) {
if (backgroundPage._browserContext === this && backgroundPage._initializedPage) {
backgroundPage.didClose();

View File

@ -323,7 +323,7 @@ export class FFBrowserContext extends BrowserContext {
await this._browser._connection.send('Browser.setRequestInterception', { browserContextId: this._browserContextId, enabled: !!this._requestInterceptor });
}
async _onClosePersistent() {}
_onClosePersistent() {}
async _doClose() {
assert(this._browserContextId);

View File

@ -319,7 +319,7 @@ export class WKBrowserContext extends BrowserContext {
await (page._delegate as WKPage).updateRequestInterception();
}
async _onClosePersistent() {}
_onClosePersistent() {}
async _doClose() {
assert(this._browserContextId);