feat(inspector): pause on page/context close (#5319)

microsoft · Feb 19, 2021 · bb2b296 · bb2b296
1 parent 8a9048c
commit bb2b296
Show file tree

Hide file tree

Showing 13 changed files with 85 additions and 40 deletions.
diff --git a/src/browserServerImpl.ts b/src/browserServerImpl.ts
@@ -171,7 +171,7 @@ class ConnectedBrowser extends BrowserDispatcher {
 
   async close(): Promise<void> {
     // Only close our own contexts.
-    await Promise.all(this._contexts.map(context => context.close()));
+    await Promise.all(this._contexts.map(context => context.close({}, internalCallMetadata())));
     this._didClose();
   }
 

diff --git a/src/dispatchers/browserContextDispatcher.ts b/src/dispatchers/browserContextDispatcher.ts
@@ -65,8 +65,8 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, channel
     });
   }
 
-  async newPage(): Promise<channels.BrowserContextNewPageResult> {
-    return { page: lookupDispatcher<PageDispatcher>(await this._context.newPage()) };
+  async newPage(params: channels.BrowserContextNewPageParams, metadata: CallMetadata): Promise<channels.BrowserContextNewPageResult> {
+    return { page: lookupDispatcher<PageDispatcher>(await this._context.newPage(metadata)) };
   }
 
   async cookies(params: channels.BrowserContextCookiesParams): Promise<channels.BrowserContextCookiesResult> {
@@ -123,8 +123,8 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, channel
     return await this._context.storageState(metadata);
   }
 
-  async close(): Promise<void> {
-    await this._context.close();
+  async close(params: channels.BrowserContextCloseParams, metadata: CallMetadata): Promise<void> {
+    await this._context.close(metadata);
   }
 
   async recorderSupplementEnable(params: channels.BrowserContextRecorderSupplementEnableParams): Promise<void> {

diff --git a/src/dispatchers/pageDispatcher.ts b/src/dispatchers/pageDispatcher.ts
@@ -146,7 +146,7 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageInitializer> i
   }
 
   async close(params: channels.PageCloseParams, metadata: CallMetadata): Promise<void> {
-    await this._page.close(params);
+    await this._page.close(metadata, params);
   }
 
   async setFileChooserInterceptedNoReply(params: channels.PageSetFileChooserInterceptedNoReplyParams, metadata: CallMetadata): Promise<void> {

diff --git a/src/protocol/channels.ts b/src/protocol/channels.ts
@@ -738,6 +738,7 @@ export type BrowserContextPauseResult = void;
 export type BrowserContextRecorderSupplementEnableParams = {
   language?: string,
   startRecording?: boolean,
+  pauseOnNextStatement?: boolean,
   launchOptions?: any,
   contextOptions?: any,
   device?: string,
@@ -747,6 +748,7 @@ export type BrowserContextRecorderSupplementEnableParams = {
 export type BrowserContextRecorderSupplementEnableOptions = {
   language?: string,
   startRecording?: boolean,
+  pauseOnNextStatement?: boolean,
   launchOptions?: any,
   contextOptions?: any,
   device?: string,

diff --git a/src/protocol/protocol.yml b/src/protocol/protocol.yml
@@ -648,6 +648,7 @@ BrowserContext:
       parameters:
         language: string?
         startRecording: boolean?
+        pauseOnNextStatement: boolean?
         launchOptions: json?
         contextOptions: json?
         device: string?

diff --git a/src/protocol/validator.ts b/src/protocol/validator.ts
@@ -363,6 +363,7 @@ export function createScheme(tChannel: (name: string) => Validator): Scheme {
   scheme.BrowserContextRecorderSupplementEnableParams = tObject({
     language: tOptional(tString),
     startRecording: tOptional(tBoolean),
+    pauseOnNextStatement: tOptional(tBoolean),
     launchOptions: tOptional(tAny),
     contextOptions: tOptional(tAny),
     device: tOptional(tString),

diff --git a/src/server/browser.ts b/src/server/browser.ts
@@ -72,13 +72,6 @@ export abstract class Browser extends SdkObject {
   abstract isConnected(): boolean;
   abstract version(): string;
 
-  async newPage(options: types.BrowserContextOptions): Promise<Page> {
-    const context = await this.newContext(options);
-    const page = await context.newPage();
-    page._ownedContext = context;
-    return page;
-  }
-
   _downloadCreated(page: Page, uuid: string, url: string, suggestedFilename?: string) {
     const download = new Download(page, this.options.downloadsPath || '', uuid, url, suggestedFilename);
     this._downloads.set(uuid, download);

diff --git a/src/server/browserContext.ts b/src/server/browserContext.ts
@@ -27,7 +27,7 @@ import { Progress } from './progress';
 import { Selectors, serverSelectors } from './selectors';
 import * as types from './types';
 import path from 'path';
-import { CallMetadata, SdkObject } from './instrumentation';
+import { CallMetadata, internalCallMetadata, SdkObject } from './instrumentation';
 
 export class Video {
   readonly _videoId: string;
@@ -209,8 +209,8 @@ export abstract class BrowserContext extends SdkObject {
       // - chromium fails to change isMobile for existing page;
       // - webkit fails to change locale for existing page.
       const oldPage = pages[0];
-      await this.newPage();
-      await oldPage.close();
+      await this.newPage(progress.metadata);
+      await oldPage.close(progress.metadata);
     }
   }
 
@@ -245,7 +245,7 @@ export abstract class BrowserContext extends SdkObject {
     return this._closedStatus !== 'open';
   }
 
-  async close() {
+  async close(metadata: CallMetadata) {
     if (this._closedStatus === 'open') {
       this.emit(BrowserContext.Events.BeforeClose);
       this._closedStatus = 'closing';
@@ -255,7 +255,7 @@ export abstract class BrowserContext extends SdkObject {
       if (this._isPersistentContext) {
         // Close all the pages instead of the context,
         // because we cannot close the default context.
-        await Promise.all(this.pages().map(page => page.close()));
+        await Promise.all(this.pages().map(page => page.close(metadata)));
       } else {
         // Close the context.
         await this._doClose();
@@ -286,7 +286,7 @@ export abstract class BrowserContext extends SdkObject {
     await this._closePromise;
   }
 
-  async newPage(): Promise<Page> {
+  async newPage(metadata: CallMetadata): Promise<Page> {
     const pageDelegate = await this.newPageDelegate();
     const pageOrError = await pageDelegate.pageOrError();
     if (pageOrError instanceof Page) {
@@ -307,21 +307,22 @@ export abstract class BrowserContext extends SdkObject {
       origins: []
     };
     if (this._origins.size)  {
-      const page = await this.newPage();
+      const internalMetadata = internalCallMetadata();
+      const page = await this.newPage(internalMetadata);
       await page._setServerRequestInterceptor(handler => {
         handler.fulfill({ body: '<html></html>' }).catch(() => {});
       });
       for (const origin of this._origins) {
         const originStorage: types.OriginStorage = { origin, localStorage: [] };
         result.origins.push(originStorage);
         const frame = page.mainFrame();
-        await frame.goto(metadata, origin);
+        await frame.goto(internalMetadata, origin);
         const storage = await frame._evaluateExpression(`({
           localStorage: Object.keys(localStorage).map(name => ({ name, value: localStorage.getItem(name) })),
         })`, false, undefined, 'utility');
         originStorage.localStorage = storage.localStorage;
       }
-      await page.close();
+      await page.close(internalMetadata);
     }
     return result;
   }
@@ -330,7 +331,8 @@ export abstract class BrowserContext extends SdkObject {
     if (state.cookies)
       await this.addCookies(state.cookies);
     if (state.origins && state.origins.length)  {
-      const page = await this.newPage();
+      const internalMetadata = internalCallMetadata();
+      const page = await this.newPage(internalMetadata);
       await page._setServerRequestInterceptor(handler => {
         handler.fulfill({ body: '<html></html>' }).catch(() => {});
       });
@@ -343,7 +345,7 @@ export abstract class BrowserContext extends SdkObject {
               localStorage.setItem(name, value);
           }`, true, originState, 'utility');
       }
-      await page.close();
+      await page.close(internalMetadata);
     }
   }
 

diff --git a/src/server/page.ts b/src/server/page.ts
@@ -428,7 +428,7 @@ export class Page extends SdkObject {
         this._timeoutSettings.timeout(options));
   }
 
-  async close(options?: { runBeforeUnload?: boolean }) {
+  async close(metadata: CallMetadata, options?: { runBeforeUnload?: boolean }) {
     if (this._closedState === 'closed')
       return;
     const runBeforeUnload = !!options && !!options.runBeforeUnload;
@@ -442,7 +442,7 @@ export class Page extends SdkObject {
     if (!runBeforeUnload)
       await this._closedPromise;
     if (this._ownedContext)
-      await this._ownedContext.close();
+      await this._ownedContext.close(metadata);
   }
 
   private _setIsError() {

diff --git a/src/server/supplements/inspectorController.ts b/src/server/supplements/inspectorController.ts
@@ -25,7 +25,7 @@ export class InspectorController implements InstrumentationListener {
 
   async onContextCreated(context: BrowserContext): Promise<void> {
     if (isDebugMode())
-      RecorderSupplement.getOrCreate(context);
+      RecorderSupplement.getOrCreate(context, { pauseOnNextStatement: true });
   }
 
   async onBeforeCall(sdkObject: SdkObject, metadata: CallMetadata): Promise<void> {
@@ -52,12 +52,8 @@ export class InspectorController implements InstrumentationListener {
       }
     }
 
-    if (metadata.method === 'pause') {
-      // Force create recorder on pause.
-      if (!context._browser.options.headful && !isUnderTest())
-        return;
-      RecorderSupplement.getOrCreate(context);
-    }
+    if (shouldOpenInspector(sdkObject, metadata))
+      RecorderSupplement.getOrCreate(context, { pauseOnNextStatement: true });
 
     const recorder = await RecorderSupplement.getNoCreate(context);
     await recorder?.onBeforeCall(sdkObject, metadata);
@@ -104,3 +100,9 @@ export class InspectorController implements InstrumentationListener {
     await recorder?.updateCallLog([metadata]);
   }
 }
+
+function shouldOpenInspector(sdkObject: SdkObject, metadata: CallMetadata): boolean {
+  if (!sdkObject.attribution.browser?.options.headful && !isUnderTest())
+    return false;
+  return metadata.method === 'pause';
+}
diff --git a/src/server/supplements/recorder/recorderApp.ts b/src/server/supplements/recorder/recorderApp.ts
@@ -53,7 +53,7 @@ export class RecorderApp extends EventEmitter {
   }
 
   async close() {
-    await this._page.context().close();
+    await this._page.context().close(internalCallMetadata());
   }
 
   private async _init() {
@@ -85,7 +85,7 @@ export class RecorderApp extends EventEmitter {
 
     this._page.once('close', () => {
       this.emit('close');
-      this._page.context().close().catch(e => console.error(e));
+      this._page.context().close(internalCallMetadata()).catch(e => console.error(e));
     });
 
     const mainFrame = this._page.mainFrame();

diff --git a/src/server/supplements/recorderSupplement.ts b/src/server/supplements/recorderSupplement.ts
@@ -50,7 +50,7 @@ export class RecorderSupplement {
   private _params: channels.BrowserContextRecorderSupplementEnableParams;
   private _currentCallsMetadata = new Map<CallMetadata, SdkObject>();
   private _pausedCallsMetadata = new Map<CallMetadata, () => void>();
-  private _pauseOnNextStatement = false;
+  private _pauseOnNextStatement: boolean;
   private _recorderSources: Source[];
   private _userSources = new Map<string, Source>();
 
@@ -72,6 +72,7 @@ export class RecorderSupplement {
     this._context = context;
     this._params = params;
     this._mode = params.startRecording ? 'recording' : 'none';
+    this._pauseOnNextStatement = !!params.pauseOnNextStatement;
     const language = params.language || context._options.sdkLanguage;
 
     const languages = new Set([
@@ -367,7 +368,7 @@ export class RecorderSupplement {
     this._currentCallsMetadata.set(metadata, sdkObject);
     this._updateUserSources();
     this.updateCallLog([metadata]);
-    if (metadata.method === 'pause' || (this._pauseOnNextStatement && metadata.method === 'goto'))
+    if (shouldPauseOnCall(sdkObject, metadata) || (this._pauseOnNextStatement && shouldPauseOnStep(sdkObject, metadata)))
       await this.pause(metadata);
     if (metadata.params && metadata.params.selector) {
       this._highlightedSelector = metadata.params.selector;
@@ -477,4 +478,14 @@ function languageForFile(file: string) {
   if (file.endsWith('.cs'))
     return 'csharp';
   return 'javascript';
-}
+}
+
+function shouldPauseOnCall(sdkObject: SdkObject, metadata: CallMetadata): boolean {
+  if (!sdkObject.attribution.browser?.options.headful && !isUnderTest())
+    return false;
+  return metadata.method === 'pause';
+}
+
+function shouldPauseOnStep(sdkObject: SdkObject, metadata: CallMetadata): boolean {
+  return metadata.method === 'goto' || metadata.method === 'close';
+}
diff --git a/test/pause.spec.ts b/test/pause.spec.ts
@@ -17,11 +17,20 @@
 import { expect } from 'folio';
 import { Page } from '..';
 import { folio } from './recorder.fixtures';
-const { it, describe} = folio;
+const { afterEach, it, describe } = folio;
 
 describe('pause', (suite, { mode }) => {
   suite.skip(mode !== 'default');
 }, () => {
+  afterEach(async ({ recorderPageGetter }) => {
+    try {
+      const recorderPage = await recorderPageGetter();
+      recorderPage.click('[title=Resume]').catch(() => {});
+    } catch (e) {
+      // Some tests close context.
+    }
+  });
+
   it('should pause and resume the script', async ({ page, recorderPageGetter }) => {
     const scriptPromise = (async () => {
       await page.pause();
@@ -117,7 +126,7 @@ describe('pause', (suite, { mode }) => {
     expect(Math.abs(x1 - x2) < 2).toBeTruthy();
     expect(Math.abs(y1 - y2) < 2).toBeTruthy();
 
-    await recorderPage.click('[title="Step over"]');
+    await recorderPage.click('[title=Resume]');
     await scriptPromise;
   });
 
@@ -196,6 +205,30 @@ describe('pause', (suite, { mode }) => {
     const error = await scriptPromise;
     expect(error.message).toContain('Not a checkbox or radio button');
   });
+
+  it('should pause on page close', async ({ page, recorderPageGetter }) => {
+    const scriptPromise = (async () => {
+      await page.pause();
+      await page.close();
+    })();
+    const recorderPage = await recorderPageGetter();
+    await recorderPage.click('[title="Step over"]');
+    await recorderPage.waitForSelector('.source-line-paused:has-text("page.close();")');
+    await recorderPage.click('[title=Resume]');
+    await scriptPromise;
+  });
+
+  it('should pause on context close', async ({ page, recorderPageGetter }) => {
+    const scriptPromise = (async () => {
+      await page.pause();
+      await page.context().close();
+    })();
+    const recorderPage = await recorderPageGetter();
+    await recorderPage.click('[title="Step over"]');
+    await recorderPage.waitForSelector('.source-line-paused:has-text("page.context().close();")');
+    await recorderPage.click('[title=Resume]');
+    await scriptPromise;
+  });
 });
 
 async function sanitizeLog(recorderPage: Page): Promise<string[]> {