From 2041aab01043e974232a579b99cbe4b932943368 Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Mon, 14 Jun 2021 21:55:55 -0700
Subject: [PATCH] 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)
```
---
 src/server/browserContext.ts     | 5 +++--
 src/server/chromium/crBrowser.ts | 4 +++-
 src/server/firefox/ffBrowser.ts  | 2 +-
 src/server/webkit/wkBrowser.ts   | 2 +-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/server/browserContext.ts b/src/server/browserContext.ts
index 71c7a01e3f..9d5c7bf29e 100644
--- a/src/server/browserContext.ts
+++ b/src/server/browserContext.ts
@@ -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();
diff --git a/src/server/chromium/crBrowser.ts b/src/server/chromium/crBrowser.ts
index de158fd2ca..da984ff572 100644
--- a/src/server/chromium/crBrowser.ts
+++ b/src/server/chromium/crBrowser.ts
@@ -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();
diff --git a/src/server/firefox/ffBrowser.ts b/src/server/firefox/ffBrowser.ts
index 7d0bc7c015..6b0fc9d080 100644
--- a/src/server/firefox/ffBrowser.ts
+++ b/src/server/firefox/ffBrowser.ts
@@ -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);
diff --git a/src/server/webkit/wkBrowser.ts b/src/server/webkit/wkBrowser.ts
index 0c3531094f..98900b0995 100644
--- a/src/server/webkit/wkBrowser.ts
+++ b/src/server/webkit/wkBrowser.ts
@@ -319,7 +319,7 @@ export class WKBrowserContext extends BrowserContext {
       await (page._delegate as WKPage).updateRequestInterception();
   }
 
-  async _onClosePersistent() {}
+  _onClosePersistent() {}
 
   async _doClose() {
     assert(this._browserContextId);