From af58c8acb277dbb989f38b282398cff773cd8b48 Mon Sep 17 00:00:00 2001
From: Yury Semikhatsky <yurys@chromium.org>
Date: Tue, 8 Sep 2020 17:01:00 -0700
Subject: [PATCH] fix(screencast): ensure that _videostarted is fired after
 newPage (#3807)

---
 src/server/browser.ts         | 9 +++++++--
 src/server/chromium/crPage.ts | 6 +-----
 src/server/firefox/ffPage.ts  | 6 +-----
 src/server/webkit/wkPage.ts   | 6 +-----
 test/screencast.spec.ts       | 8 ++++++++
 5 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/src/server/browser.ts b/src/server/browser.ts
index 85d0477b01..1ff02fd5c7 100644
--- a/src/server/browser.ts
+++ b/src/server/browser.ts
@@ -21,6 +21,7 @@ import { EventEmitter } from 'events';
 import { Download } from './download';
 import { ProxySettings } from './types';
 import { ChildProcess } from 'child_process';
+import { makeWaitForNextTask } from '../utils/utils';
 
 export interface BrowserProcess {
   onclose: ((exitCode: number | null, signal: string | null) => void) | undefined;
@@ -87,10 +88,14 @@ export abstract class Browser extends EventEmitter {
     this._downloads.delete(uuid);
   }
 
-  _videoStarted(videoId: string, file: string): Video {
+  _videoStarted(videoId: string, file: string, pageOrError: Promise<Page | Error>) {
     const video = new Video(file);
     this._idToVideo.set(videoId, video);
-    return video;
+    pageOrError.then(pageOrError => {
+      // Emit the event in another task to ensure that newPage response is handled before.
+      if (pageOrError instanceof Page)
+        makeWaitForNextTask()(() => pageOrError.emit(Page.Events.VideoStarted, video));
+    });
   }
 
   _videoFinished(videoId: string) {
diff --git a/src/server/chromium/crPage.ts b/src/server/chromium/crPage.ts
index d4327734f6..a7ee7495c3 100644
--- a/src/server/chromium/crPage.ts
+++ b/src/server/chromium/crPage.ts
@@ -762,11 +762,7 @@ class FrameSession {
       this._screencastState = 'started';
       this._videoRecorder = videoRecorder;
       this._screencastId = screencastId;
-      const video = this._crPage._browserContext._browser._videoStarted(screencastId, options.outputFile);
-      this._crPage.pageOrError().then(pageOrError => {
-        if (pageOrError instanceof Page)
-          pageOrError.emit(Page.Events.VideoStarted, video);
-      }).catch(() => {});
+      this._crPage._browserContext._browser._videoStarted(screencastId, options.outputFile, this._crPage.pageOrError());
     } catch (e) {
       videoRecorder.stop().catch(() => {});
       throw e;
diff --git a/src/server/firefox/ffPage.ts b/src/server/firefox/ffPage.ts
index 5fa1ab35ec..665a2be191 100644
--- a/src/server/firefox/ffPage.ts
+++ b/src/server/firefox/ffPage.ts
@@ -258,11 +258,7 @@ export class FFPage implements PageDelegate {
   }
 
   _onScreencastStarted(event: Protocol.Page.screencastStartedPayload) {
-    const video = this._browserContext._browser._videoStarted(event.screencastId, event.file);
-    this.pageOrError().then(pageOrError => {
-      if (pageOrError instanceof Page)
-        pageOrError.emit(Page.Events.VideoStarted, video);
-    }).catch(() => {});
+    this._browserContext._browser._videoStarted(event.screencastId, event.file, this.pageOrError());
   }
 
   async exposeBinding(binding: PageBinding) {
diff --git a/src/server/webkit/wkPage.ts b/src/server/webkit/wkPage.ts
index f1590929b0..1c57640fff 100644
--- a/src/server/webkit/wkPage.ts
+++ b/src/server/webkit/wkPage.ts
@@ -721,11 +721,7 @@ export class WKPage implements PageDelegate {
         width: options.width,
         height: options.height,
       }) as any;
-      const video = this._browserContext._browser._videoStarted(screencastId, options.outputFile);
-      this.pageOrError().then(pageOrError => {
-        if (pageOrError instanceof Page)
-          pageOrError.emit(Page.Events.VideoStarted, video);
-      }).catch(() => {});
+      this._browserContext._browser._videoStarted(screencastId, options.outputFile, this.pageOrError());
     } catch (e) {
       this._recordingVideoFile = null;
       throw e;
diff --git a/test/screencast.spec.ts b/test/screencast.spec.ts
index a866400d5d..24a7153a1a 100644
--- a/test/screencast.spec.ts
+++ b/test/screencast.spec.ts
@@ -281,6 +281,14 @@ describe('screencast', suite => {
     await browser.close();
   });
 
+  it('should fire striclty after context.newPage', async ({browser, tmpDir}) => {
+    const context = await browser.newContext({_recordVideos: {width: 320, height: 240}});
+    const page = await context.newPage();
+    // Should not hang.
+    await page.waitForEvent('_videostarted');
+    await context.close();
+  });
+
   it('should fire start event for popups', async ({browserType, tmpDir, server}) => {
     const browser = await browserType.launch({_videosPath: tmpDir});
     const context = await browser.newContext({_recordVideos: {width: 320, height: 240}});