fix(profiling): Parse Hermes Bytecode frames virtual address (#3342)

getsentry · Oct 17, 2023 · 5446992 · 5446992
1 parent 21465bc
commit 5446992
Show file tree

Hide file tree

Showing 5 changed files with 107 additions and 33 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 ### Fixes
 
 - Add actual `activeThreadId` to Profiles ([#3338](https://github.com/getsentry/sentry-react-native/pull/3338))
+- Parse Hermes Profiling Bytecode Frames ([#3342](https://github.com/getsentry/sentry-react-native/pull/3342))
 
 ## 5.11.1
 

diff --git a/src/js/profiling/convertHermesProfile.ts b/src/js/profiling/convertHermesProfile.ts
@@ -1,22 +1,17 @@
 import type { FrameId, StackId, ThreadCpuFrame, ThreadCpuSample, ThreadCpuStack, ThreadId } from '@sentry/types';
 import { logger } from '@sentry/utils';
-import { Platform } from 'react-native';
 
-import { ANDROID_DEFAULT_BUNDLE_NAME, IOS_DEFAULT_BUNDLE_NAME } from '../integrations/rewriteframes';
 import type * as Hermes from './hermes';
-import { parseHermesStackFrameFunctionName } from './hermes';
+import { parseHermesJSStackFrame } from './hermes';
 import { MAX_PROFILE_DURATION_MS } from './integration';
 import type { RawThreadCpuProfile } from './types';
 
 const PLACEHOLDER_THREAD_ID_STRING = '0';
 const MS_TO_NS = 1e6;
 const MAX_PROFILE_DURATION_NS = MAX_PROFILE_DURATION_MS * MS_TO_NS;
-const ANONYMOUS_FUNCTION_NAME = 'anonymous';
 const UNKNOWN_STACK_ID = -1;
 const JS_THREAD_NAME = 'JavaScriptThread';
 const JS_THREAD_PRIORITY = 1;
-const DEFAULT_BUNDLE_NAME =
-  Platform.OS === 'android' ? ANDROID_DEFAULT_BUNDLE_NAME : Platform.OS === 'ios' ? IOS_DEFAULT_BUNDLE_NAME : undefined;
 
 /**
  * Converts a Hermes profile to a Sentry profile.
@@ -136,15 +131,7 @@ function mapFrames(hermesStackFrames: Record<Hermes.StackFrameId, Hermes.StackFr
       continue;
     }
     hermesStackFrameIdToSentryFrameIdMap.set(Number(key), frames.length);
-    const hermesFrame = hermesStackFrames[key];
-
-    const functionName = parseHermesStackFrameFunctionName(hermesFrame.name);
-    frames.push({
-      function: functionName || ANONYMOUS_FUNCTION_NAME,
-      file: hermesFrame.category == 'JavaScript' ? DEFAULT_BUNDLE_NAME : undefined,
-      lineno: hermesFrame.line !== undefined ? Number(hermesFrame.line) : undefined,
-      colno: hermesFrame.column !== undefined ? Number(hermesFrame.column) : undefined,
-    });
+    frames.push(parseHermesJSStackFrame(hermesStackFrames[key]));
   }
 
   return {

diff --git a/src/js/profiling/hermes.ts b/src/js/profiling/hermes.ts
@@ -1,3 +1,6 @@
+import { Platform } from 'react-native';
+
+import { ANDROID_DEFAULT_BUNDLE_NAME, IOS_DEFAULT_BUNDLE_NAME } from '../integrations/rewriteframes';
 import { NATIVE } from '../wrapper';
 import { convertToSentryProfile } from './convertHermesProfile';
 import type { RawThreadCpuProfile } from './types';
@@ -28,10 +31,17 @@ export interface Sample {
 }
 
 export interface StackFrame {
+  // Hermes Bytecode
+  funcVirtAddr?: string;
+  offset?: string;
+
+  // JavaScript
   line?: string;
   column?: string;
   funcLine?: string;
   funcColumn?: string;
+
+  // Common
   name: string;
   category: string;
   parent?: number;
@@ -43,15 +53,50 @@ export interface Profile {
   stackFrames: Record<string, StackFrame>;
 }
 
+export interface ParsedHermesStackFrame {
+  function: string;
+  file?: string;
+  lineno?: number;
+  colno?: number;
+}
+
+const DEFAULT_BUNDLE_NAME =
+  Platform.OS === 'android' ? ANDROID_DEFAULT_BUNDLE_NAME : Platform.OS === 'ios' ? IOS_DEFAULT_BUNDLE_NAME : undefined;
+const ANONYMOUS_FUNCTION_NAME = 'anonymous';
+
 /**
- * Hermes Profile Stack Frame Name contains function name and file path.
- *
- * `foo(/path/to/file.js:1:2)` -> `foo`
+ * Parses Hermes StackFrame to Sentry StackFrame.
+ * For native frames only function name is returned, for Hermes bytecode the line and column are calculated.
  */
-export function parseHermesStackFrameFunctionName(hermesName: string): string {
-  const indexOfLeftParenthesis = hermesName.indexOf('(');
-  const name = indexOfLeftParenthesis !== -1 ? hermesName.substring(0, indexOfLeftParenthesis) : hermesName;
-  return name;
+export function parseHermesJSStackFrame(frame: StackFrame): ParsedHermesStackFrame {
+  if (frame.category !== 'JavaScript') {
+    // Native
+    return { function: frame.name };
+  }
+
+  if (frame.funcVirtAddr !== undefined && frame.offset !== undefined) {
+    // Hermes Bytecode
+    return {
+      function: frame.name || ANONYMOUS_FUNCTION_NAME,
+      file: DEFAULT_BUNDLE_NAME,
+      // https://github.com/krystofwoldrich/metro/blob/417e6f276ff9422af6039fc4d1bce41fcf7d9f46/packages/metro-symbolicate/src/Symbolication.js#L298-L301
+      // Hermes lineno is hardcoded 1, currently only one bundle symbolication is supported by metro-symbolicate and thus by us.
+      lineno: 1,
+      // Hermes colno is 0-based, while Sentry is 1-based
+      colno: Number(frame.funcVirtAddr) + Number(frame.offset) + 1,
+    };
+  }
+
+  // JavaScript
+  const indexOfLeftParenthesis = frame.name.indexOf('(');
+  return {
+    function:
+      (indexOfLeftParenthesis !== -1 && (frame.name.substring(0, indexOfLeftParenthesis) || ANONYMOUS_FUNCTION_NAME)) ||
+      frame.name,
+    file: DEFAULT_BUNDLE_NAME,
+    lineno: frame.line !== undefined ? Number(frame.line) : undefined,
+    colno: frame.column !== undefined ? Number(frame.column) : undefined,
+  };
 }
 
 const MS_TO_NS: number = 1e6;

diff --git a/test/profiling/convertHermesProfile.test.ts b/test/profiling/convertHermesProfile.test.ts
@@ -56,7 +56,7 @@ describe('convert hermes profile to sentry profile', () => {
           column: '33',
           funcLine: '1605',
           funcColumn: '14',
-          name: 'fooA(/absolute/path/app:///main.jsbundle:1610:33)',
+          name: 'fooA(app:///main.jsbundle:1610:33)',
           category: 'JavaScript',
           parent: 1,
         },
@@ -65,7 +65,7 @@ describe('convert hermes profile to sentry profile', () => {
           column: '21',
           funcLine: '1614',
           funcColumn: '14',
-          name: 'fooB(/absolute/path/app:///main.jsbundle:1616:21)',
+          name: 'fooB(http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.sampleNewArchitecture:193430:4)',
           category: 'JavaScript',
           parent: 1,
         },
@@ -74,7 +74,7 @@ describe('convert hermes profile to sentry profile', () => {
           column: '18',
           funcLine: '1623',
           funcColumn: '16',
-          name: '(/absolute/path/app:///main.jsbundle:1627:18)',
+          name: '(/Users/distiller/react-native/packages/react-native/sdks/hermes/build_iphonesimulator/lib/InternalBytecode/InternalBytecode.js:139:27)',
           category: 'JavaScript',
           parent: 2,
         },
@@ -83,10 +83,7 @@ describe('convert hermes profile to sentry profile', () => {
     const expectedSentryProfile: RawThreadCpuProfile = {
       frames: [
         {
-          colno: undefined,
-          file: undefined,
           function: '[root]',
-          lineno: undefined,
         },
         {
           colno: 33,

diff --git a/test/profiling/hermes.test.ts b/test/profiling/hermes.test.ts
@@ -1,18 +1,62 @@
-import { parseHermesStackFrameFunctionName } from '../../src/js/profiling/hermes';
+import type { ParsedHermesStackFrame } from '../../src/js/profiling/hermes';
+import { parseHermesJSStackFrame } from '../../src/js/profiling/hermes';
 
 describe('hermes', () => {
   describe('parseHermesStackFrameName', () => {
     test('parses function name and file name', () => {
-      expect(parseHermesStackFrameFunctionName('fooA(/absolute/path/main.jsbundle:1610:33)')).toEqual('fooA');
+      expect(
+        parseHermesJSStackFrame({
+          name: 'fooA(/absolute/path/main.jsbundle:1610:33)',
+          line: '1610',
+          column: '33',
+          category: 'JavaScript',
+        }),
+      ).toEqual(<ParsedHermesStackFrame>{
+        function: 'fooA',
+        file: 'app:///main.jsbundle',
+        lineno: 1610,
+        colno: 33,
+      });
     });
     test('parse hermes root stack frame', () => {
-      expect(parseHermesStackFrameFunctionName('[root]')).toEqual('[root]');
+      expect(
+        parseHermesJSStackFrame({
+          name: '[root]',
+          category: 'root',
+        }),
+      ).toEqual(
+        expect.objectContaining(<ParsedHermesStackFrame>{
+          function: '[root]',
+        }),
+      );
     });
     test('parse only file name', () => {
-      expect(parseHermesStackFrameFunctionName('(/absolute/path/jsbundle:1610:33)')).toEqual('');
+      expect(
+        parseHermesJSStackFrame({
+          name: '(/absolute/path/main.jsbundle:1610:33)',
+          line: '1610',
+          column: '33',
+          category: 'JavaScript',
+        }),
+      ).toEqual(<ParsedHermesStackFrame>{
+        function: 'anonymous',
+        file: 'app:///main.jsbundle',
+        lineno: 1610,
+        colno: 33,
+      });
     });
     test('parse only function name', () => {
-      expect(parseHermesStackFrameFunctionName('fooA')).toEqual('fooA');
+      expect(
+        parseHermesJSStackFrame({
+          name: 'fooA',
+          category: 'JavaScript',
+        }),
+      ).toEqual(
+        expect.objectContaining(<ParsedHermesStackFrame>{
+          function: 'fooA',
+          file: 'app:///main.jsbundle',
+        }),
+      );
     });
   });
 });