Display the frame line number in tooltip backtraces (#5512)

[Production](https://share.firefox.dev/4kDaBxg) | [Deploy
preview](https://deploy-preview-5512--perf-html.netlify.app/public/73df76308b29355bd1b42a56b99b7a293b244b18/marker-chart/?globalTrackOrder=013w52&hiddenGlobalTracks=013w5&implementation=js&markerSearch=Reflow&range=1515m27&thread=3&timelineType=category&v=11)

[Production](https://share.firefox.dev/44M7w8c) | [Deploy
preview](https://deploy-preview-5512--perf-html.netlify.app/public/fpcrs77zkn48n4b4zj6z5evg7tecpzevth5mc2g/marker-chart/?globalTrackOrder=x20wx1&hiddenGlobalTracks=1wx2&hiddenLocalTracksByPid=2261-0wa&markerSearch=NotifyObs&profileName=delete_cookies%2Ffilter.py%20macos%20timeout&range=3209m2403~3278m335&thread=0&v=11)

Fixes #1150.

Author: mstange

src/components/shared/Backtrace.js

@@ -6,11 +6,7 @@
 
 import React from 'react';
 import classNames from 'classnames';
-import { filterCallNodeAndCategoryPathByImplementation } from 'firefox-profiler/profile-logic/transforms';
-import {
-  getFuncNamesAndOriginsForPath,
-  convertStackToCallNodeAndCategoryPath,
-} from 'firefox-profiler/profile-logic/profile-data';
+import { getBacktraceItemsForStack } from 'firefox-profiler/profile-logic/transforms';
 
 import type {
   CategoryList,
@@ -34,15 +30,11 @@ type Props = {|
 export function Backtrace(props: Props) {
   const { stackIndex, thread, implementationFilter, maxStacks, categories } =
     props;
-  const callNodePath = filterCallNodeAndCategoryPathByImplementation(
-    thread,
+  const funcNamesAndOrigins = getBacktraceItemsForStack(
+    stackIndex,
     implementationFilter,
-    convertStackToCallNodeAndCategoryPath(thread, stackIndex)
-  );
-  const funcNamesAndOrigins = getFuncNamesAndOriginsForPath(
-    callNodePath,
     thread
-  ).reverse();
+  );
 
   if (funcNamesAndOrigins.length) {
     return (

src/components/shared/MarkerContextMenu.js

@@ -42,11 +42,7 @@ import type {
 import type { ConnectedProps } from 'firefox-profiler/utils/connect';
 import { getImplementationFilter } from 'firefox-profiler/selectors/url-state';
 
-import { filterCallNodeAndCategoryPathByImplementation } from 'firefox-profiler/profile-logic/transforms';
-import {
-  convertStackToCallNodeAndCategoryPath,
-  getFuncNamesAndOriginsForPath,
-} from 'firefox-profiler/profile-logic/profile-data';
+import { getBacktraceItemsForStack } from 'firefox-profiler/profile-logic/transforms';
 import { getThreadSelectorsFromThreadsKey } from 'firefox-profiler/selectors/per-thread';
 
 import './MarkerContextMenu.css';
@@ -161,13 +157,12 @@ class MarkerContextMenuImpl extends PureComponent<Props> {
       return '';
     }
 
-    const path = filterCallNodeAndCategoryPathByImplementation(
-      thread,
+    const funcNamesAndOrigins = getBacktraceItemsForStack(
+      stack,
       implementationFilter,
-      convertStackToCallNodeAndCategoryPath(thread, stack)
-    );
+      thread
+    ).reverse();
 
-    const funcNamesAndOrigins = getFuncNamesAndOriginsForPath(path, thread);
     return funcNamesAndOrigins
       .map(({ funcName, origin }) => `${funcName} [${origin}]`)
       .join('\n');

src/components/shared/SampleTooltipContents.js

@@ -9,8 +9,7 @@ import { Backtrace } from './Backtrace';
 import { TooltipDetailSeparator } from '../tooltip/TooltipDetails';
 import {
   getCategoryPairLabel,
-  getFuncNamesAndOriginsForPath,
-  convertStackToCallNodeAndCategoryPath,
+  isSampleWithNonEmptyStack,
 } from 'firefox-profiler/profile-logic/profile-data';
 import { getFormattedTimelineValue } from 'firefox-profiler/profile-logic/committed-ranges';
 import {
@@ -26,7 +25,6 @@ import type {
   Milliseconds,
 } from 'firefox-profiler/types';
 import type { CpuRatioInTimeRange } from './thread/ActivityGraphFills';
-import { ensureExists } from '../../utils/flow';
 
 type CPUProps = CpuRatioInTimeRange;
 
@@ -134,21 +132,10 @@ export class SampleTooltipContents extends React.PureComponent<Props> {
     let hasStack = false;
     let formattedSampleTime = null;
     if (sampleIndex !== null) {
-      const { samples, stackTable } = rangeFilteredThread;
+      const { samples } = rangeFilteredThread;
       const sampleTime = samples.time[sampleIndex];
-      const stackIndex = samples.stack[sampleIndex];
-      const hasSamples = samples.length > 0 && stackTable.length > 1;
-
-      if (hasSamples) {
-        const stack = getFuncNamesAndOriginsForPath(
-          convertStackToCallNodeAndCategoryPath(
-            rangeFilteredThread,
-            ensureExists(stackIndex)
-          ),
-          rangeFilteredThread
-        );
-        hasStack = stack.length > 1 || stack[0].funcName !== '(root)';
-      }
+
+      hasStack = isSampleWithNonEmptyStack(sampleIndex, rangeFilteredThread);
 
       formattedSampleTime = getFormattedTimelineValue(
         sampleTime - zeroAt,

src/profile-logic/profile-data.js

@@ -71,7 +71,6 @@ import type {
   PageList,
   CallNodeTable,
   CallNodePath,
-  CallNodeAndCategoryPath,
   IndexIntoCallNodeTable,
   AccumulatedCounterSamples,
   SamplesLikeTable,
@@ -2148,30 +2147,6 @@ export function processEventDelays(
   };
 }
 
-/**
- * This function converts a stack information into a call node and
- * category path structure.
- */
-export function convertStackToCallNodeAndCategoryPath(
-  thread: Thread,
-  stack: IndexIntoStackTable
-): CallNodeAndCategoryPath {
-  const { stackTable, frameTable } = thread;
-  const path = [];
-  for (
-    let stackIndex = stack;
-    stackIndex !== null;
-    stackIndex = stackTable.prefix[stackIndex]
-  ) {
-    const index = stackTable.frame[stackIndex];
-    path.push({
-      category: stackTable.category[stackIndex],
-      func: frameTable.func[index],
-    });
-  }
-  return path.reverse();
-}
-
 /**
  * Compute maximum depth of call stack for a given thread, and return maxDepth+1.
  * This value can be used as the length for any per-depth arrays.
@@ -2883,34 +2858,32 @@ export function reserveFunctionsInThread(
 }
 
 /**
- * From a valid call node path, this function returns a list of information
- * about each function in this path: their names and their origins.
+ * Returns whether the given sample has a stack which is non-null and not just
+ * a single function with the name '(root)'.
  */
-export function getFuncNamesAndOriginsForPath(
-  path: CallNodeAndCategoryPath,
+export function isSampleWithNonEmptyStack(
+  sampleIndex: IndexIntoSamplesTable,
   thread: Thread
-): Array<{
-  funcName: string,
-  category: IndexIntoCategoryList,
-  isFrameLabel: boolean,
-  origin: string,
-}> {
-  const { funcTable, stringTable, resourceTable } = thread;
+): boolean {
+  const { samples, stackTable, frameTable, funcTable, stringTable } = thread;
 
-  return path.map((frame) => {
-    const { category, func } = frame;
-    return {
-      funcName: stringTable.getString(funcTable.name[func]),
-      category: category,
-      isFrameLabel: funcTable.resource[func] === -1,
-      origin: getOriginAnnotationForFunc(
-        func,
-        funcTable,
-        resourceTable,
-        stringTable
-      ),
-    };
-  });
+  const stackIndex = samples.stack[sampleIndex];
+  if (stackIndex === null) {
+    return false;
+  }
+
+  if (stackTable.prefix[stackIndex] !== null) {
+    // Stack contains at least two frames.
+    return true;
+  }
+
+  // Stack is only a single frame. Is it the '(root)' frame that Firefox puts
+  // in its profiles?
+  const frameIndex = stackTable.frame[stackIndex];
+  const funcIndex = frameTable.func[frameIndex];
+  const funcNameStringIndex = funcTable.name[funcIndex];
+  const funcName = stringTable.getString(funcNameStringIndex);
+  return funcName !== '(root)';
 }
 
 /**

src/profile-logic/transforms.js

@@ -12,6 +12,7 @@ import {
   updateThreadStacks,
   updateThreadStacksByGeneratingNewStackColumns,
   getMapStackUpdater,
+  getOriginAnnotationForFunc,
 } from './profile-data';
 import { timeCode } from '../utils/time-code';
 import { assertExhaustiveCheck, convertToTransformType } from '../utils/flow';
@@ -32,7 +33,6 @@ import type {
   IndexIntoStackTable,
   IndexIntoResourceTable,
   CallNodePath,
-  CallNodeAndCategoryPath,
   CallNodeTable,
   StackType,
   ImplementationFilter,
@@ -1563,15 +1563,72 @@ export function filterCallNodePathByImplementation(
   );
 }
 
-export function filterCallNodeAndCategoryPathByImplementation(
-  thread: Thread,
+// User-facing properties about a stack frame.
+export type BacktraceItem = {|
+  // The function name of the stack frame.
+  funcName: string,
+  // The frame category of the stack frame.
+  category: IndexIntoCategoryList,
+  // Whether this frame is a label frame.
+  isFrameLabel: boolean,
+  // A string which is usually displayed after the function name, and which
+  // describes, in some way, where this function or frame came from.
+  // If known, this contains the file name of the function, and the line and
+  // column number of the frame, i.e. the spot within the function that was
+  // being executed.
+  // If the source file name is not known, this might be the name of a native
+  // library instead.
+  // May also be empty.
+  origin: string,
+|};
+
+/**
+ * Convert the stack into an array of "backtrace items" for each stack frame.
+ * The returned array is ordered from callee-most to caller-most, i.e. the root
+ * caller is at the end.
+ */
+export function getBacktraceItemsForStack(
+  stack: IndexIntoStackTable,
   implementationFilter: ImplementationFilter,
-  path: CallNodeAndCategoryPath
-): CallNodeAndCategoryPath {
+  thread: Thread
+): BacktraceItem[] {
+  const { funcTable, stringTable, resourceTable } = thread;
+
+  const { stackTable, frameTable } = thread;
+  const unfilteredPath = [];
+  for (
+    let stackIndex = stack;
+    stackIndex !== null;
+    stackIndex = stackTable.prefix[stackIndex]
+  ) {
+    const frameIndex = stackTable.frame[stackIndex];
+    unfilteredPath.push({
+      category: stackTable.category[stackIndex],
+      funcIndex: frameTable.func[frameIndex],
+      frameLine: frameTable.line[frameIndex],
+      frameColumn: frameTable.column[frameIndex],
+    });
+  }
+
   const funcMatchesImplementation = FUNC_MATCHES[implementationFilter];
-  return path.filter((funcIndex) =>
-    funcMatchesImplementation(thread, funcIndex.func)
+  const path = unfilteredPath.filter(({ funcIndex }) =>
+    funcMatchesImplementation(thread, funcIndex)
   );
+  return path.map(({ category, funcIndex, frameLine, frameColumn }) => {
+    return {
+      funcName: stringTable.getString(funcTable.name[funcIndex]),
+      category: category,
+      isFrameLabel: funcTable.resource[funcIndex] === -1,
+      origin: getOriginAnnotationForFunc(
+        funcIndex,
+        funcTable,
+        resourceTable,
+        stringTable,
+        frameLine,
+        frameColumn
+      ),
+    };
+  });
 }
 
 /**

src/test/unit/profile-data.test.js

@@ -16,7 +16,6 @@ import {
   getInvertedCallNodeInfo,
   filterThreadByImplementation,
   getSampleIndexClosestToStartTime,
-  convertStackToCallNodeAndCategoryPath,
   getSampleIndexToCallNodeIndex,
   getTreeOrderComparator,
   getSamplesSelectedStates,
@@ -46,6 +45,7 @@ import {
 import {
   funcHasDirectRecursiveCall,
   funcHasRecursiveCall,
+  getBacktraceItemsForStack,
 } from '../../profile-logic/transforms';
 
 import type { Thread, IndexIntoStackTable } from 'firefox-profiler/types';
@@ -949,24 +949,6 @@ describe('funcHasDirectRecursiveCall and funcHasRecursiveCall', function () {
   });
 });
 
-describe('convertStackToCallNodeAndCategoryPath', function () {
-  it('correctly returns a call node path for a stack', function () {
-    const profile = getCallNodeProfile();
-    const { derivedThreads } = getProfileWithDicts(profile);
-    const [thread] = derivedThreads;
-    const stack1 = thread.samples.stack[0];
-    const stack2 = thread.samples.stack[1];
-    if (stack1 === null || stack2 === null) {
-      // Makes flow happy
-      throw new Error("stack shouldn't be null");
-    }
-    let callNodePath = convertStackToCallNodeAndCategoryPath(thread, stack1);
-    expect(callNodePath.map((f) => f.func)).toEqual([0, 1, 2, 3, 4]);
-    callNodePath = convertStackToCallNodeAndCategoryPath(thread, stack2);
-    expect(callNodePath.map((f) => f.func)).toEqual([0, 1, 2, 3, 5]);
-  });
-});
-
 describe('getSamplesSelectedStates', function () {
   function setup(textSamples) {
     const {
@@ -1566,3 +1548,43 @@ describe('getNativeSymbolInfo', function () {
     });
   });
 });
+
+describe('getBacktraceItemsForStack', function () {
+  function getBacktraceString(thread, sampleIndex): string {
+    return getBacktraceItemsForStack(
+      ensureExists(thread.samples.stack[sampleIndex]),
+      'combined',
+      thread
+    )
+      .map(({ funcName, origin }) => `${funcName} ${origin}`)
+      .join('\n');
+  }
+
+  it('returns backtrace items in the right order and with frame line numbers', function () {
+    const { derivedThreads } = getProfileFromTextSamples(`
+      A[file:one.js][line:20]  A[file:one.js][line:21]  A[file:one.js][line:20]
+      B[file:one.js][line:30]  D[file:one.js][line:50]  B[file:one.js][line:31]
+      C[file:two.js][line:10]  C[file:two.js][line:11]  C[file:two.js][line:12]
+                                                        D[file:one.js][line:51]
+    `);
+
+    const [thread] = derivedThreads;
+
+    expect(getBacktraceString(thread, 0)).toMatchInlineSnapshot(`
+     "C two.js:10
+     B one.js:30
+     A one.js:20"
+    `);
+    expect(getBacktraceString(thread, 1)).toMatchInlineSnapshot(`
+     "C two.js:11
+     D one.js:50
+     A one.js:21"
+    `);
+    expect(getBacktraceString(thread, 2)).toMatchInlineSnapshot(`
+     "D one.js:51
+     C two.js:12
+     B one.js:31
+     A one.js:20"
+    `);
+  });
+});

src/types/profile-derived.js

@@ -431,13 +431,6 @@ export type AddressProof = {|
  */
 export type CallNodePath = IndexIntoFuncTable[];
 
-export type CallNodeAndCategory = {|
-  func: IndexIntoFuncTable,
-  category: IndexIntoCategoryList,
-|};
-
-export type CallNodeAndCategoryPath = CallNodeAndCategory[];
-
 /**
  * This type contains the first derived `Marker[]` information, plus an IndexedArray
  * to get back to the RawMarkerTable.