From a769be7ebf9af52939dbea7ce49be85bf0a0ad3c Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Fri, 15 Sep 2023 18:31:04 +0200 Subject: [PATCH 1/7] fix(hermes): Convert Hermes bytecode col pos to 1-based format used by Sentry backend --- src/js/integrations/rewriteframes.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/js/integrations/rewriteframes.ts b/src/js/integrations/rewriteframes.ts index 38773400f1..bf3489562e 100644 --- a/src/js/integrations/rewriteframes.ts +++ b/src/js/integrations/rewriteframes.ts @@ -2,7 +2,7 @@ import { RewriteFrames } from '@sentry/integrations'; import type { StackFrame } from '@sentry/types'; import { Platform } from 'react-native'; -import { isExpo } from '../utils/environment'; +import { isExpo, isHermesEnabled } from '../utils/environment'; export const ANDROID_DEFAULT_BUNDLE_NAME = 'app:///index.android.bundle'; export const IOS_DEFAULT_BUNDLE_NAME = 'app:///main.jsbundle'; @@ -35,6 +35,13 @@ export function createReactNativeRewriteFrames(): RewriteFrames { if (frame.filename === '[native code]' || frame.filename === 'native') { return frame; } + // Is React Native frame + if (isHermesEnabled() && frame.colno !== undefined) { + // https://github.com/facebook/react/issues/21792#issuecomment-873171991 + // hermes columns are 0-based, while v8 and jsc are 1-based + // TODO: This seems to be needed only for Hermes bytecode without debug information + frame.colno += 1; + } // Expo adds hash to the end of bundle names if (isExpo() && Platform.OS === 'android') { From 64125d5867af88a303d9d7e4d5b4d5cedc8c0b1f Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Mon, 18 Sep 2023 16:40:45 +0200 Subject: [PATCH 2/7] Add hermes bytecode check and update tests --- src/js/integrations/rewriteframes.ts | 8 +++-- test/integrations/rewriteframes.test.ts | 41 +++++++++++++------------ 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/js/integrations/rewriteframes.ts b/src/js/integrations/rewriteframes.ts index bf3489562e..77157e2301 100644 --- a/src/js/integrations/rewriteframes.ts +++ b/src/js/integrations/rewriteframes.ts @@ -36,10 +36,12 @@ export function createReactNativeRewriteFrames(): RewriteFrames { return frame; } // Is React Native frame - if (isHermesEnabled() && frame.colno !== undefined) { + + // Check Hermes Bytecode Frame and convert to 1-based column + if (isHermesEnabled() && frame.lineno === 1 && frame.colno !== undefined) { + // hermes bytecode columns are 0-based, while v8 and jsc are 1-based + // Hermes frames without debug info have always line = 1 and col points to a bytecode pos // https://github.com/facebook/react/issues/21792#issuecomment-873171991 - // hermes columns are 0-based, while v8 and jsc are 1-based - // TODO: This seems to be needed only for Hermes bytecode without debug information frame.colno += 1; } diff --git a/test/integrations/rewriteframes.test.ts b/test/integrations/rewriteframes.test.ts index 58867ef345..80e4b6544d 100644 --- a/test/integrations/rewriteframes.test.ts +++ b/test/integrations/rewriteframes.test.ts @@ -4,7 +4,7 @@ import type { Event } from '@sentry/types'; import { Platform } from 'react-native'; import { createReactNativeRewriteFrames } from '../../src/js/integrations/rewriteframes'; -import { isExpo } from '../../src/js/utils/environment'; +import { isExpo, isHermesEnabled } from '../../src/js/utils/environment'; import { mockFunction } from '../testutils'; jest.mock('../../src/js/utils/environment'); @@ -29,6 +29,7 @@ describe('RewriteFrames', () => { beforeEach(() => { mockFunction(isExpo).mockReturnValue(false); + mockFunction(isHermesEnabled).mockReturnValue(false); jest.resetAllMocks(); }); @@ -668,6 +669,8 @@ describe('RewriteFrames', () => { }); it('should parse React Native errors on Android Hermes', async () => { + mockFunction(isHermesEnabled).mockReturnValue(true); + const ANDROID_REACT_NATIVE_HERMES = { message: 'Error: lets throw!', name: 'Error', @@ -714,28 +717,28 @@ describe('RewriteFrames', () => { filename: 'app:///index.android.bundle', function: 'value', lineno: 1, - colno: 31561, + colno: 31562, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'value', lineno: 1, - colno: 32776, + colno: 32777, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'anonymous', lineno: 1, - colno: 31603, + colno: 31604, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'value', lineno: 1, - colno: 33176, + colno: 33177, in_app: true, }, { @@ -747,42 +750,42 @@ describe('RewriteFrames', () => { filename: 'app:///index.android.bundle', function: 'receiveTouches', lineno: 1, - colno: 122512, + colno: 122513, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'Ue', lineno: 1, - colno: 77571, + colno: 77572, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'Ne', lineno: 1, - colno: 77238, + colno: 77239, in_app: true, }, { filename: 'app:///index.android.bundle', function: '_e', lineno: 1, - colno: 127755, + colno: 127756, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'anonymous', lineno: 1, - colno: 77747, + colno: 77748, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'z', lineno: 1, - colno: 74642, + colno: 74643, in_app: true, }, { @@ -794,21 +797,21 @@ describe('RewriteFrames', () => { filename: 'app:///index.android.bundle', function: 'A', lineno: 1, - colno: 74709, + colno: 74710, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'N', lineno: 1, - colno: 74267, + colno: 74268, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'C', lineno: 1, - colno: 74126, + colno: 74127, in_app: true, }, { filename: 'native', function: 'apply', in_app: true }, @@ -816,7 +819,7 @@ describe('RewriteFrames', () => { filename: 'app:///index.android.bundle', function: 'k', lineno: 1, - colno: 74094, + colno: 74095, in_app: true, }, { filename: 'native', function: 'apply', in_app: true }, @@ -824,7 +827,7 @@ describe('RewriteFrames', () => { filename: 'app:///index.android.bundle', function: 'b', lineno: 1, - colno: 74037, + colno: 74038, in_app: true, }, { filename: 'native', function: 'apply', in_app: true }, @@ -835,21 +838,21 @@ describe('RewriteFrames', () => { filename: 'app:///index.android.bundle', function: '_performSideEffectsForTransition', lineno: 1, - colno: 230843, + colno: 230844, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'anonymous', lineno: 1, - colno: 224280, + colno: 224281, in_app: true, }, { filename: 'app:///index.android.bundle', function: 'onPress', lineno: 1, - colno: 452701, + colno: 452702, in_app: true, }, ], From 6ed427630f44206cfed387d80af4e2d7e98ec79f Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Mon, 18 Sep 2023 17:11:41 +0200 Subject: [PATCH 3/7] Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b4dc16307..469bc06438 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixes - Create profiles for start up transactions ([#3281](https://github.com/getsentry/sentry-react-native/pull/3281)) +- Fix Hermes Bytecode Symbolication one line off ([#3283](https://github.com/getsentry/sentry-react-native/pull/3283)) ### Dependencies From 867d4d04047f09cc61ff9d10c6dbdeaefee34306 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Mon, 18 Sep 2023 17:15:12 +0200 Subject: [PATCH 4/7] feat(sdk): Add Hermes Debug Info flag to React Native Context --- CHANGELOG.md | 5 ++ src/js/integrations/reactnativeinfo.ts | 14 +++- test/integrations/reactnativeinfo.test.ts | 85 +++++++++++++++++++++++ 3 files changed, 103 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 469bc06438..930807b864 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +### Features + +- Add Hermes Debug Info flag to React Native Context ([#3290](https://github.com/getsentry/sentry-react-native/pull/3290)) + - This flag equals `true` when Hermes Bundle contains Debug Info (Hermes Source Map was not emitted) + ### Fixes - Create profiles for start up transactions ([#3281](https://github.com/getsentry/sentry-react-native/pull/3281)) diff --git a/src/js/integrations/reactnativeinfo.ts b/src/js/integrations/reactnativeinfo.ts index e1aa23a052..33f301d181 100644 --- a/src/js/integrations/reactnativeinfo.ts +++ b/src/js/integrations/reactnativeinfo.ts @@ -18,6 +18,7 @@ export interface ReactNativeContext extends Context { hermes_version?: string; react_native_version: string; component_stack?: string; + hermes_debug_info?: boolean; } /** Loads React Native context at runtime */ @@ -50,7 +51,18 @@ export class ReactNativeInfo implements Integration { reactNativeContext.js_engine = 'hermes'; const hermesVersion = getHermesVersion(); if (hermesVersion) { - reactNativeContext.hermes_version = getHermesVersion(); + reactNativeContext.hermes_version = hermesVersion; + } + + // Check if Hermes Bundle has debug info + for(const value of event.exception?.values || event.threads?.values || []) { + for (const frame of value.stacktrace?.frames || []) { + // platform === undefined we assume it's javascript (only native frames use the platform attribute) + if (frame.platform === undefined && frame.lineno === 1) { + reactNativeContext.hermes_debug_info = true; + break; + } + } } } else if (reactNativeError?.jsEngine) { reactNativeContext.js_engine = reactNativeError.jsEngine; diff --git a/test/integrations/reactnativeinfo.test.ts b/test/integrations/reactnativeinfo.test.ts index 52c9c5a823..9e2adaa006 100644 --- a/test/integrations/reactnativeinfo.test.ts +++ b/test/integrations/reactnativeinfo.test.ts @@ -148,6 +148,91 @@ describe('React Native Info', () => { test: 'context', }); }); + + it('add hermes_debug_info to react_native_context based on exception frames', async () => { + mockedIsHermesEnabled = jest.fn().mockReturnValue(true); + + const mockedEvent: Event = { + exception: { + values: [ + { + stacktrace: { + frames: [ + { + platform: 'java', + lineno: 2, + }, + { + lineno: 1, + }, + ], + }, + }, + ], + }, + }; + const actualEvent = await executeIntegrationFor(mockedEvent, {}); + + expectMocksToBeCalledOnce(); + expect(actualEvent?.contexts?.react_native_context?.hermes_debug_info).toEqual(true); + }); + + it('add hermes_debug_info to react_native_context based on threads frames', async () => { + mockedIsHermesEnabled = jest.fn().mockReturnValue(true); + + const mockedEvent: Event = { + threads: { + values: [ + { + stacktrace: { + frames: [ + { + platform: 'java', + lineno: 2, + }, + { + lineno: 1, + }, + ], + }, + }, + ], + }, + }; + const actualEvent = await executeIntegrationFor(mockedEvent, {}); + + expectMocksToBeCalledOnce(); + expect(actualEvent?.contexts?.react_native_context?.hermes_debug_info).toEqual(true); + }); + + + it('does not add hermes_debug_info to react_native_context (no hermes bytecode frames)', async () => { + mockedIsHermesEnabled = jest.fn().mockReturnValue(true); + + const mockedEvent: Event = { + threads: { + values: [ + { + stacktrace: { + frames: [ + { + platform: 'java', + lineno: 2, + }, + { + lineno: 2, + }, + ], + }, + }, + ], + }, + }; + const actualEvent = await executeIntegrationFor(mockedEvent, {}); + + expectMocksToBeCalledOnce(); + expect(actualEvent?.contexts?.react_native_context?.hermes_debug_info).toEqual(undefined); + }); }); function expectMocksToBeCalledOnce() { From 6227986d8932f1a16fe3a68675f2895a5792d1b0 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Mon, 18 Sep 2023 17:21:58 +0200 Subject: [PATCH 5/7] fix lint --- src/js/integrations/reactnativeinfo.ts | 2 +- test/integrations/reactnativeinfo.test.ts | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/js/integrations/reactnativeinfo.ts b/src/js/integrations/reactnativeinfo.ts index 33f301d181..c517d87f90 100644 --- a/src/js/integrations/reactnativeinfo.ts +++ b/src/js/integrations/reactnativeinfo.ts @@ -55,7 +55,7 @@ export class ReactNativeInfo implements Integration { } // Check if Hermes Bundle has debug info - for(const value of event.exception?.values || event.threads?.values || []) { + for (const value of event.exception?.values || event.threads?.values || []) { for (const frame of value.stacktrace?.frames || []) { // platform === undefined we assume it's javascript (only native frames use the platform attribute) if (frame.platform === undefined && frame.lineno === 1) { diff --git a/test/integrations/reactnativeinfo.test.ts b/test/integrations/reactnativeinfo.test.ts index 9e2adaa006..7675710c94 100644 --- a/test/integrations/reactnativeinfo.test.ts +++ b/test/integrations/reactnativeinfo.test.ts @@ -205,7 +205,6 @@ describe('React Native Info', () => { expect(actualEvent?.contexts?.react_native_context?.hermes_debug_info).toEqual(true); }); - it('does not add hermes_debug_info to react_native_context (no hermes bytecode frames)', async () => { mockedIsHermesEnabled = jest.fn().mockReturnValue(true); From 25d3697e66af66dd3bec41369ba5f0ecae7689ec Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Tue, 19 Sep 2023 15:09:54 +0200 Subject: [PATCH 6/7] Export hermes debug info logic to function, fix tests --- src/js/integrations/reactnativeinfo.ts | 41 +++++++++++++++++------ test/integrations/reactnativeinfo.test.ts | 13 +++---- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/src/js/integrations/reactnativeinfo.ts b/src/js/integrations/reactnativeinfo.ts index c517d87f90..c7b52cebbe 100644 --- a/src/js/integrations/reactnativeinfo.ts +++ b/src/js/integrations/reactnativeinfo.ts @@ -53,17 +53,7 @@ export class ReactNativeInfo implements Integration { if (hermesVersion) { reactNativeContext.hermes_version = hermesVersion; } - - // Check if Hermes Bundle has debug info - for (const value of event.exception?.values || event.threads?.values || []) { - for (const frame of value.stacktrace?.frames || []) { - // platform === undefined we assume it's javascript (only native frames use the platform attribute) - if (frame.platform === undefined && frame.lineno === 1) { - reactNativeContext.hermes_debug_info = true; - break; - } - } - } + reactNativeContext.hermes_debug_info = !isEventWithHermesBytecodeFrames(event); } else if (reactNativeError?.jsEngine) { reactNativeContext.js_engine = reactNativeError.jsEngine; } @@ -88,3 +78,32 @@ export class ReactNativeInfo implements Integration { }); } } + +/** + * Guess if the event contains frames with Hermes bytecode + * (thus Hermes bundle doesn't contain debug info) + * based on the event exception/threads frames. + * + * This function can be relied on only if Hermes is enabled! + * + * Hermes bytecode position is always line 1 and column 0-based number. + * If Hermes bundle has debug info, the bytecode frames pos are calculated + * back to the plain bundle source code positions and line will be > 1. + * + * Line 1 contains start time var, it's safe to assume it won't crash. + * The above only applies when Hermes is enabled. + * + * Javascript/Hermes bytecode frames have platform === undefined. + * Native (Java, ObjC, C++) frames have platform === 'android'/'ios'/'native'. + */ +function isEventWithHermesBytecodeFrames(event: Event): boolean { + for (const value of event.exception?.values || event.threads?.values || []) { + for (const frame of value.stacktrace?.frames || []) { + // platform === undefined we assume it's javascript (only native frames use the platform attribute) + if (frame.platform === undefined && frame.lineno === 1) { + return true; + } + } + } + return false; +} diff --git a/test/integrations/reactnativeinfo.test.ts b/test/integrations/reactnativeinfo.test.ts index 7675710c94..62784f4dc4 100644 --- a/test/integrations/reactnativeinfo.test.ts +++ b/test/integrations/reactnativeinfo.test.ts @@ -49,6 +49,7 @@ describe('React Native Info', () => { turbo_module: false, fabric: false, js_engine: 'hermes', + hermes_debug_info: true, react_native_version: '1000.0.0-test', expo: false, }, @@ -149,7 +150,7 @@ describe('React Native Info', () => { }); }); - it('add hermes_debug_info to react_native_context based on exception frames', async () => { + it('add hermes_debug_info to react_native_context based on exception frames (hermes bytecode frames present -> no debug info)', async () => { mockedIsHermesEnabled = jest.fn().mockReturnValue(true); const mockedEvent: Event = { @@ -174,10 +175,10 @@ describe('React Native Info', () => { const actualEvent = await executeIntegrationFor(mockedEvent, {}); expectMocksToBeCalledOnce(); - expect(actualEvent?.contexts?.react_native_context?.hermes_debug_info).toEqual(true); + expect(actualEvent?.contexts?.react_native_context?.hermes_debug_info).toEqual(false); }); - it('add hermes_debug_info to react_native_context based on threads frames', async () => { + it('does not hermes_debug_info to react_native_context based on threads frames (hermes bytecode frames present -> no debug info)', async () => { mockedIsHermesEnabled = jest.fn().mockReturnValue(true); const mockedEvent: Event = { @@ -202,10 +203,10 @@ describe('React Native Info', () => { const actualEvent = await executeIntegrationFor(mockedEvent, {}); expectMocksToBeCalledOnce(); - expect(actualEvent?.contexts?.react_native_context?.hermes_debug_info).toEqual(true); + expect(actualEvent?.contexts?.react_native_context?.hermes_debug_info).toEqual(false); }); - it('does not add hermes_debug_info to react_native_context (no hermes bytecode frames)', async () => { + it('adds hermes_debug_info to react_native_context (no hermes bytecode frames found -> debug info present)', async () => { mockedIsHermesEnabled = jest.fn().mockReturnValue(true); const mockedEvent: Event = { @@ -230,7 +231,7 @@ describe('React Native Info', () => { const actualEvent = await executeIntegrationFor(mockedEvent, {}); expectMocksToBeCalledOnce(); - expect(actualEvent?.contexts?.react_native_context?.hermes_debug_info).toEqual(undefined); + expect(actualEvent?.contexts?.react_native_context?.hermes_debug_info).toEqual(true); }); }); From 078eb286710c1675ae64b2d5edc5bc0b4443420a Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Tue, 19 Sep 2023 15:29:35 +0200 Subject: [PATCH 7/7] Fix changelog --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe2e1a515c..c369ef789e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,12 +1,14 @@ # Changelog -## 5.9.2 +## Unreleased ### Features - Add Hermes Debug Info flag to React Native Context ([#3290](https://github.com/getsentry/sentry-react-native/pull/3290)) - This flag equals `true` when Hermes Bundle contains Debug Info (Hermes Source Map was not emitted) +## 5.9.2 + ### Fixes - Create profiles for start up transactions ([#3281](https://github.com/getsentry/sentry-react-native/pull/3281))