fix(tracing): improve captureStackTrace (#8236)

- Simplify by only considering client/ vs non-client/
- Fix stack traces when calling from other playwright code, e.g. from the cli
- Account for re-entrant calls that happen when
  instrumenting context creation/desctruction
- Add tests
- Fix StackTraceView on Windows

src/utils/stackTrace.ts

@@ -31,21 +31,9 @@ export function rewriteErrorMessage(e: Error, newMessage: string): Error {
   return e;
 }
 
-const PW_LIB_DIRS = [
-  'playwright',
-  'playwright-chromium',
-  'playwright-firefox',
-  'playwright-webkit',
-  path.join('@playwright', 'test'),
-].map(packageName => path.sep + packageName);
-
-const runnerNpmPkgLib = path.join('@playwright', 'test', 'lib', 'test');
-const runnerLib = path.join('lib', 'test');
-const runnerSrc = path.join('src', 'test');
-
-function includesFileInPlaywrightSubDir(subDir: string, fileName: string) {
-  return PW_LIB_DIRS.map(p => path.join(p, subDir)).some(libDir => fileName.includes(libDir));
-}
+const ROOT_DIR = path.resolve(__dirname, '..', '..');
+const CLIENT_LIB = path.join(ROOT_DIR, 'lib', 'client');
+const CLIENT_SRC = path.join(ROOT_DIR, 'src', 'client');
 
 export type ParsedStackTrace = {
   frames: StackFrame[];
@@ -59,44 +47,52 @@ export function captureStackTrace(): ParsedStackTrace {
   const error = new Error();
   const stack = error.stack!;
   Error.stackTraceLimit = stackTraceLimit;
-  const frames: StackFrame[] = [];
-  const frameTexts: string[] = [];
-  const lines = stack.split('\n').reverse();
-  let apiName = '';
 
-  const isTesting = !!process.env.PWTEST_CLI_ALLOW_TEST_COMMAND || isUnderTest();
-
-  for (const line of lines) {
+  const isTesting = isUnderTest();
+  type ParsedFrame = {
+    frame: StackFrame;
+    frameText: string;
+    inClient: boolean;
+  };
+  let parsedFrames = stack.split('\n').map(line => {
     const frame = stackUtils.parseLine(line);
     if (!frame || !frame.file)
-      continue;
+      return null;
     if (frame.file.startsWith('internal'))
-      continue;
+      return null;
     const fileName = path.resolve(process.cwd(), frame.file);
     if (isTesting && fileName.includes(path.join('playwright', 'tests', 'config', 'coverage.js')))
-      continue;
-    if (isFilePartOfPlaywright(isTesting, fileName)) {
+      return null;
+    const parsed: ParsedFrame = {
+      frame: {
+        file: fileName,
+        line: frame.line,
+        column: frame.column,
+        function: frame.function,
+      },
+      frameText: line,
+      inClient: fileName.startsWith(CLIENT_LIB) || fileName.startsWith(CLIENT_SRC),
+    };
+    return parsed;
+  }).filter(frame => !!frame) as ParsedFrame[];
+
+  let apiName = '';
+  // Deepest transition between non-client code calling into client code
+  // is the api entry.
+  for (let i = 0; i < parsedFrames.length - 1; i++) {
+    if (parsedFrames[i].inClient && !parsedFrames[i + 1].inClient) {
+      const frame = parsedFrames[i].frame;
       apiName = frame.function ? frame.function[0].toLowerCase() + frame.function.slice(1) : '';
+      parsedFrames = parsedFrames.slice(i + 1);
       break;
     }
-    frameTexts.push(line);
-    frames.push({
-      file: fileName,
-      line: frame.line,
-      column: frame.column,
-      function: frame.function,
-    });
   }
-  frames.reverse();
-  frameTexts.reverse();
-  return { frames, frameTexts, apiName };
-}
 
-function isFilePartOfPlaywright(isTesting: boolean, fileName: string): boolean {
-  const isPlaywrightTest = fileName.includes(runnerNpmPkgLib);
-  const isLocalPlaywright = isTesting && (fileName.includes(runnerSrc) || fileName.includes(runnerLib));
-  const isInPlaywright = (includesFileInPlaywrightSubDir('src', fileName) || includesFileInPlaywrightSubDir('lib', fileName));
-  return !isPlaywrightTest && !isLocalPlaywright && isInPlaywright;
+  return {
+    frames: parsedFrames.map(p => p.frame),
+    frameTexts: parsedFrames.map(p => p.frameText),
+    apiName
+  };
 }
 
 export function splitErrorMessage(message: string): { name: string, message: string } {

src/web/traceViewer/ui/stackTrace.tsx

@@ -26,6 +26,8 @@ export const StackTraceView: React.FunctionComponent<{
   const frames = action?.metadata.stack || [];
   return <div className='stack-trace'>{
     frames.map((frame, index) => {
+      // Windows frames are E:\path\to\file
+      const pathSep = frame.file[1] === ':' ? '\\' : '/';
       return <div
         key={index}
         className={'stack-trace-frame' + (selectedFrame === index ? ' selected' : '')}
@@ -37,7 +39,7 @@ export const StackTraceView: React.FunctionComponent<{
           {frame.function || '(anonymous)'}
         </span>
         <span className='stack-trace-frame-location'>
-          {frame.file.split('/').pop()}
+          {frame.file.split(pathSep).pop()}
         </span>
         <span className='stack-trace-frame-line'>
           {':' + frame.line}

tests/trace-viewer/trace-viewer.spec.ts

@@ -46,6 +46,14 @@ class TraceViewerPage {
     await this.page.click(`.snapshot-tab .tab-label:has-text("${name}")`);
   }
 
+  async showConsoleTab() {
+    await this.page.click('text="Console"');
+  }
+
+  async showSourceTab() {
+    await this.page.click('text="Source"');
+  }
+
   async callLines() {
     await this.page.waitForSelector('.call-line:visible');
     return await this.page.$$eval('.call-line:visible', ee => ee.map(e => e.textContent));
@@ -78,6 +86,11 @@ class TraceViewerPage {
     return await this.page.$$eval('.console-stack:visible', ee => ee.map(e => e.textContent));
   }
 
+  async sourceStack() {
+    await this.page.waitForSelector('.stack-trace-frame:visible');
+    return await this.page.$$eval('.stack-trace-frame:visible', ee => ee.map(e => (e as HTMLElement).innerText.replace(/\s+/g, ' ')));
+  }
+
   async snapshotSize() {
     return this.page.$eval('.snapshot-container', e => {
       const style = window.getComputedStyle(e);
@@ -102,7 +115,7 @@ const test = playwrightTest.extend<{ showTraceViewer: (trace: string) => Promise
 
 let traceFile: string;
 
-test.beforeAll(async ({ browser, browserName }, workerInfo) => {
+test.beforeAll(async function recordTrace({ browser, browserName, browserType }, workerInfo) {
   const context = await browser.newContext();
   await context.tracing.start({ name: 'test', screenshots: true, snapshots: true });
   const page = await context.newPage();
@@ -115,15 +128,27 @@ test.beforeAll(async ({ browser, browserName }, workerInfo) => {
     setTimeout(() => { throw new Error('Unhandled exception'); }, 0);
     return 'return ' + a;
   }, { a: 'paramA', b: 4 });
-  await page.click('"Click"');
+
+  async function doClick() {
+    await page.click('"Click"');
+  }
+  await doClick();
+
   await Promise.all([
     page.waitForNavigation(),
     page.waitForTimeout(200).then(() => page.goto('data:text/html,<html>Hello world 2</html>'))
   ]);
   await page.setViewportSize({ width: 500, height: 600 });
-  await page.close();
-  traceFile = path.join(workerInfo.project.outputDir, browserName, 'trace.zip');
-  await context.tracing.stop({ path: traceFile });
+
+  // Go through instrumentation to exercise reentrant stack traces.
+  (browserType as any)._onWillCloseContext = async () => {
+    await page.hover('body');
+    await page.close();
+    traceFile = path.join(workerInfo.project.outputDir, browserName, 'trace.zip');
+    await context.tracing.stop({ path: traceFile });
+  };
+  await context.close();
+  (browserType as any)._onWillCloseContext = undefined;
 });
 
 test('should show empty trace viewer', async ({ showTraceViewer }, testInfo) => {
@@ -141,6 +166,7 @@ test('should open simple trace viewer', async ({ showTraceViewer }) => {
     'page.waitForNavigation',
     'page.gotodata:text/html,<html>Hello world 2</html>',
     'page.setViewportSize',
+    'page.hoverbody',
   ]);
 });
 
@@ -163,7 +189,7 @@ test('should render console', async ({ showTraceViewer, browserName }) => {
   test.fixme(browserName === 'firefox', 'Firefox generates stray console message for page error');
   const traceViewer = await showTraceViewer(traceFile);
   await traceViewer.selectAction('page.evaluate');
-  await traceViewer.page.click('"Console"');
+  await traceViewer.showConsoleTab();
 
   const events = await traceViewer.consoleLines();
   expect(events).toEqual(['Info', 'Warning', 'Error', 'Unhandled exception']);
@@ -203,3 +229,22 @@ test('should have correct snapshot size', async ({ showTraceViewer }) => {
   await traceViewer.selectSnapshot('After');
   expect(await traceViewer.snapshotSize()).toEqual({ width: '500px', height: '600px' });
 });
+
+test('should have correct stack trace', async ({ showTraceViewer }) => {
+  const traceViewer = await showTraceViewer(traceFile);
+
+  await traceViewer.selectAction('page.click');
+  await traceViewer.showSourceTab();
+  const stack1 = await traceViewer.sourceStack();
+  expect(stack1.slice(0, 2)).toEqual([
+    'doClick trace-viewer.spec.ts :133',
+    'recordTrace trace-viewer.spec.ts :135',
+  ]);
+
+  await traceViewer.selectAction('page.hover');
+  await traceViewer.showSourceTab();
+  const stack2 = await traceViewer.sourceStack();
+  expect(stack2.slice(0, 1)).toEqual([
+    'BrowserType.browserType._onWillCloseContext trace-viewer.spec.ts :145',
+  ]);
+});