fix(screencast): ensure that _videostarted is fired after newPage (#3807

)
microsoft · Sep 9, 2020 · af58c8a · af58c8a
1 parent a5a5636
commit af58c8a
Show file tree

Hide file tree

Showing 5 changed files with 18 additions and 17 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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}});