fix: Linked errors processed before other integrations (#3535)

krystofwoldrich · web-flow · commit 58b326188d41 · 2024-01-25T00:08:53.000-05:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -34,6 +34,8 @@
 - Performance Tracing should be disabled by default ([#3533](https://github.com/getsentry/sentry-react-native/pull/3533))
 - Use `$NODE_BINARY` to execute Sentry CLI in Xcode scripts ([#3493](https://github.com/getsentry/sentry-react-native/pull/3493))
 - Return auto Release and Dist to source maps auto upload ([#3540](https://github.com/getsentry/sentry-react-native/pull/3540))
+- Linked errors processed before other integrations ([#3535](https://github.com/getsentry/sentry-react-native/pull/3535))
+  - This ensure their frames are correctly symbolicated
 
 ### Dependencies
 
diff --git a/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java b/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java
@@ -705,8 +705,8 @@ public void fetchNativeSdkInfo(Promise promise) {
         }
     }
 
-    public void fetchNativePackageName(Promise promise) {
-        promise.resolve(packageInfo.packageName);
+    public String fetchNativePackageName() {
+        return packageInfo.packageName;
     }
 
     private void setEventOriginTag(SentryEvent event) {
diff --git a/android/src/newarch/java/io/sentry/react/RNSentryModule.java b/android/src/newarch/java/io/sentry/react/RNSentryModule.java
@@ -135,12 +135,13 @@ public WritableMap stopProfiling() {
     }
 
     @Override
-    public void fetchNativePackageName(Promise promise) {
-        this.impl.fetchNativePackageName(promise);
+    public String fetchNativePackageName() {
+        return this.impl.fetchNativePackageName();
     }
 
     @Override
-    public void fetchNativeStackFramesBy(ReadableArray instructionsAddr, Promise promise) {
+    public WritableMap fetchNativeStackFramesBy(ReadableArray instructionsAddr) {
         // Not used on Android
+        return null;
     }
 }
diff --git a/android/src/oldarch/java/io/sentry/react/RNSentryModule.java b/android/src/oldarch/java/io/sentry/react/RNSentryModule.java
@@ -133,13 +133,14 @@ public WritableMap stopProfiling() {
         return this.impl.stopProfiling();
     }
 
-    @ReactMethod
-    public void fetchNativePackageName(Promise promise) {
-        this.impl.fetchNativePackageName(promise);
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    public String fetchNativePackageName() {
+        return this.impl.fetchNativePackageName();
     }
 
-    @ReactMethod
-    public void fetchNativeStackFramesBy(ReadableArray instructionsAddr, Promise promise) {
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    public WritableMap fetchNativeStackFramesBy(ReadableArray instructionsAddr) {
         // Not used on Android
+        return null;
     }
 }
diff --git a/ios/RNSentry.mm b/ios/RNSentry.mm
@@ -201,11 +201,10 @@ - (void)setEventEnvironmentTag:(SentryEvent *)event
     resolve(modulesString);
 }
 
-RCT_EXPORT_METHOD(fetchNativePackageName:(RCTPromiseResolveBlock)resolve
-                  rejecter:(RCTPromiseRejectBlock)reject)
+RCT_EXPORT_SYNCHRONOUS_TYPED_METHOD(NSString *, fetchNativePackageName)
 {
     NSString *packageName = [[NSBundle mainBundle] executablePath];
-    resolve(packageName);
+    return packageName;
 }
 
 - (NSDictionary*) fetchNativeStackFramesBy: (NSArray<NSNumber*>*)instructionsAddr
@@ -273,12 +272,10 @@ - (NSDictionary*) fetchNativeStackFramesBy: (NSArray<NSNumber*>*)instructionsAdd
   }
 }
 
-RCT_EXPORT_METHOD(fetchNativeStackFramesBy:(NSArray *)instructionsAddr
-                  resolve:(RCTPromiseResolveBlock)resolve
-                  reject:(RCTPromiseRejectBlock)reject)
+RCT_EXPORT_SYNCHRONOUS_TYPED_METHOD(NSDictionary *, fetchNativeStackFramesBy:(NSArray *)instructionsAddr)
 {
-  resolve([self fetchNativeStackFramesBy:instructionsAddr
-                             symbolicate:dladdr]);
+  return [self fetchNativeStackFramesBy:instructionsAddr
+                             symbolicate:dladdr];
 }
 
 RCT_EXPORT_METHOD(fetchNativeDeviceContexts:(RCTPromiseResolveBlock)resolve
diff --git a/jest.config.js b/jest.config.js
@@ -1,7 +1,7 @@
 module.exports = {
   collectCoverage: true,
   preset: 'react-native',
-  setupFilesAfterEnv: ['<rootDir>/test/mockConsole.ts'],
+  setupFilesAfterEnv: ['jest-extended/all', '<rootDir>/test/mockConsole.ts'],
   globals: {
     __DEV__: true,
     'ts-jest': {
diff --git a/package.json b/package.json
@@ -96,6 +96,7 @@
     "expo-module-scripts": "^3.1.0",
     "jest": "^29.6.2",
     "jest-environment-jsdom": "^29.6.2",
+    "jest-extended": "^4.0.2",
     "madge": "^6.1.0",
     "metro": "0.76",
     "prettier": "^2.0.5",
diff --git a/src/js/NativeRNSentry.ts b/src/js/NativeRNSentry.ts
@@ -34,8 +34,8 @@ export interface Spec extends TurboModule {
   fetchViewHierarchy(): Promise<number[] | undefined | null>;
   startProfiling(): { started?: boolean; error?: string };
   stopProfiling(): { profile?: string; nativeProfile?: UnsafeObject; error?: string };
-  fetchNativePackageName(): Promise<string | undefined | null>;
-  fetchNativeStackFramesBy(instructionsAddr: number[]): Promise<NativeStackFrames | undefined | null>;
+  fetchNativePackageName(): string | undefined | null;
+  fetchNativeStackFramesBy(instructionsAddr: number[]): NativeStackFrames | undefined | null;
 }
 
 export type NativeStackFrame = {
diff --git a/src/js/integrations/nativelinkederrors.ts b/src/js/integrations/nativelinkederrors.ts
@@ -1,5 +1,6 @@
 import { exceptionFromError } from '@sentry/browser';
 import type {
+  Client,
   DebugImage,
   Event,
   EventHint,
@@ -53,35 +54,29 @@ export class NativeLinkedErrors implements Integration {
   /**
    * @inheritDoc
    */
-  public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
-    const client = getCurrentHub().getClient();
-    if (!client) {
-      return;
+  public setupOnce(_addGlobalEventProcessor: (callback: EventProcessor) => void, _getCurrentHub: () => Hub): void {
+    /* noop */
+  }
+
+  /**
+   * @inheritDoc
+   */
+  public preprocessEvent(event: Event, hint: EventHint | undefined, client: Client): void {
+    if (this._nativePackage === null) {
+      this._nativePackage = this._fetchNativePackage();
     }
 
-    addGlobalEventProcessor(async (event: Event, hint?: EventHint) => {
-      if (this._nativePackage === null) {
-        this._nativePackage = await this._fetchNativePackage();
-      }
-      const self = getCurrentHub().getIntegration(NativeLinkedErrors);
-      return self ? this._handler(client.getOptions().stackParser, self._key, self._limit, event, hint) : event;
-    });
+    this._handler(client.getOptions().stackParser, this._key, this._limit, event, hint);
   }
 
   /**
    * Enriches passed event with linked exceptions and native debug meta images.
    */
-  private async _handler(
-    parser: StackParser,
-    key: string,
-    limit: number,
-    event: Event,
-    hint?: EventHint,
-  ): Promise<Event | null> {
+  private _handler(parser: StackParser, key: string, limit: number, event: Event, hint?: EventHint): void {
     if (!event.exception || !event.exception.values || !hint || !isInstanceOf(hint.originalException, Error)) {
-      return event;
+      return;
     }
-    const { exceptions: linkedErrors, debugImages } = await this._walkErrorTree(
+    const { exceptions: linkedErrors, debugImages } = this._walkErrorTree(
       parser,
       limit,
       hint.originalException as ExtendedError,
@@ -92,25 +87,23 @@ export class NativeLinkedErrors implements Integration {
     event.debug_meta = event.debug_meta || {};
     event.debug_meta.images = event.debug_meta.images || [];
     event.debug_meta.images.push(...(debugImages || []));
-
-    return event;
   }
 
   /**
    * Walks linked errors and created Sentry exceptions chain.
    * Collects debug images from native errors stack frames.
    */
-  private async _walkErrorTree(
+  private _walkErrorTree(
     parser: StackParser,
     limit: number,
     error: ExtendedError,
     key: string,
     exceptions: Exception[] = [],
     debugImages: DebugImage[] = [],
-  ): Promise<{
+  ): {
     exceptions: Exception[];
     debugImages?: DebugImage[];
-  }> {
+  } {
     const linkedError = error[key];
     if (!linkedError || exceptions.length + 1 >= limit) {
       return {
@@ -126,7 +119,7 @@ export class NativeLinkedErrors implements Integration {
       exception = this._exceptionFromJavaStackElements(linkedError);
     } else if ('stackReturnAddresses' in linkedError) {
       // isObjCException
-      const { appleException, appleDebugImages } = await this._exceptionFromAppleStackReturnAddresses(linkedError);
+      const { appleException, appleDebugImages } = this._exceptionFromAppleStackReturnAddresses(linkedError);
       exception = appleException;
       exceptionDebugImages = appleDebugImages;
     } else if (isInstanceOf(linkedError, Error)) {
@@ -193,15 +186,15 @@ export class NativeLinkedErrors implements Integration {
   /**
    * Converts StackAddresses to a SentryException with DebugMetaImages
    */
-  private async _exceptionFromAppleStackReturnAddresses(objCException: {
+  private _exceptionFromAppleStackReturnAddresses(objCException: {
     name: string;
     message: string;
     stackReturnAddresses: number[];
-  }): Promise<{
+  }): {
     appleException: Exception;
     appleDebugImages: DebugImage[];
-  }> {
-    const nativeStackFrames = await this._fetchNativeStackFrames(objCException.stackReturnAddresses);
+  } {
+    const nativeStackFrames = this._fetchNativeStackFrames(objCException.stackReturnAddresses);
 
     return {
       appleException: {
@@ -218,14 +211,14 @@ export class NativeLinkedErrors implements Integration {
   /**
    * Fetches the native package/image name from the native layer
    */
-  private _fetchNativePackage(): Promise<string | null> {
+  private _fetchNativePackage(): string | null {
     return NATIVE.fetchNativePackageName();
   }
 
   /**
    * Fetches native debug image information on iOS
    */
-  private _fetchNativeStackFrames(instructionsAddr: number[]): Promise<NativeStackFrames | null> {
+  private _fetchNativeStackFrames(instructionsAddr: number[]): NativeStackFrames | null {
     return NATIVE.fetchNativeStackFramesBy(instructionsAddr);
   }
 }
diff --git a/src/js/wrapper.ts b/src/js/wrapper.ts
@@ -85,12 +85,12 @@ interface SentryNativeWrapper {
   startProfiling(): boolean;
   stopProfiling(): { hermesProfile: Hermes.Profile; nativeProfile?: NativeProfileEvent } | null;
 
-  fetchNativePackageName(): Promise<string | null>;
+  fetchNativePackageName(): string | null;
 
   /**
    * Fetches native stack frames and debug images for the instructions addresses.
    */
-  fetchNativeStackFramesBy(instructionsAddr: number[]): Promise<NativeStackFrames | null>;
+  fetchNativeStackFramesBy(instructionsAddr: number[]): NativeStackFrames | null;
 }
 
 const EOL = utf8ToBytes('\n');
@@ -554,26 +554,26 @@ export const NATIVE: SentryNativeWrapper = {
     }
   },
 
-  async fetchNativePackageName(): Promise<string | null> {
+  fetchNativePackageName(): string | null {
     if (!this.enableNative) {
       return null;
     }
     if (!this._isModuleLoaded(RNSentry)) {
       return null;
     }
 
-    return (await RNSentry.fetchNativePackageName()) || null;
+    return RNSentry.fetchNativePackageName() || null;
   },
 
-  async fetchNativeStackFramesBy(instructionsAddr: number[]): Promise<NativeStackFrames | null> {
+  fetchNativeStackFramesBy(instructionsAddr: number[]): NativeStackFrames | null {
     if (!this.enableNative) {
       return null;
     }
     if (!this._isModuleLoaded(RNSentry)) {
       return null;
     }
 
-    return (await RNSentry.fetchNativeStackFramesBy(instructionsAddr)) || null;
+    return RNSentry.fetchNativeStackFramesBy(instructionsAddr) || null;
   },
 
   /**
diff --git a/test/integrations/integrationsexecutionorder.test.ts b/test/integrations/integrationsexecutionorder.test.ts
@@ -0,0 +1,98 @@
+import * as mockWrapper from '../mockWrapper';
+jest.mock('../../src/js/wrapper', () => mockWrapper);
+jest.mock('../../src/js/utils/environment');
+
+import { defaultStackParser } from '@sentry/browser';
+import type { Integration } from '@sentry/types';
+
+import { ReactNativeClient } from '../../src/js/client';
+import { getDefaultIntegrations } from '../../src/js/integrations/default';
+import type { ReactNativeClientOptions } from '../../src/js/options';
+import { isHermesEnabled, notWeb } from '../../src/js/utils/environment';
+import { MOCK_DSN } from '../mockDsn';
+
+describe('Integration execution order', () => {
+  describe('mobile hermes', () => {
+    beforeEach(() => {
+      (notWeb as jest.Mock).mockReturnValue(true);
+      (isHermesEnabled as jest.Mock).mockReturnValue(true);
+    });
+
+    it('NativeLinkedErrors is before RewriteFrames', async () => {
+      // NativeLinkedErrors has to process event before RewriteFrames
+      // otherwise the linked errors stack trace frames won't be rewritten
+
+      const client = createTestClient();
+      const { integrations } = client.getOptions();
+
+      const nativeLinkedErrors = spyOnIntegrationById('NativeLinkedErrors', integrations);
+      const rewriteFrames = spyOnIntegrationById('RewriteFrames', integrations);
+
+      client.setupIntegrations();
+
+      client.captureException(new Error('test'));
+      jest.runAllTimers();
+
+      expect(nativeLinkedErrors.preprocessEvent).toHaveBeenCalledBefore(rewriteFrames.processEvent!);
+    });
+  });
+
+  describe('web', () => {
+    beforeEach(() => {
+      (notWeb as jest.Mock).mockReturnValue(false);
+      (isHermesEnabled as jest.Mock).mockReturnValue(false);
+    });
+
+    it('LinkedErrors is before RewriteFrames', async () => {
+      // LinkedErrors has to process event before RewriteFrames
+      // otherwise the linked errors stack trace frames won't be rewritten
+
+      const client = createTestClient();
+      const { integrations } = client.getOptions();
+
+      const linkedErrors = spyOnIntegrationById('LinkedErrors', integrations);
+      const rewriteFrames = spyOnIntegrationById('RewriteFrames', integrations);
+
+      client.setupIntegrations();
+
+      client.captureException(new Error('test'));
+      jest.runAllTimers();
+
+      expect(linkedErrors.preprocessEvent).toHaveBeenCalledBefore(rewriteFrames.processEvent!);
+    });
+  });
+});
+
+interface IntegrationSpy {
+  name: string;
+  setupOnce?: Integration['setupOnce'] & jest.Mock;
+  preprocessEvent?: Integration['preprocessEvent'] & jest.Mock;
+  processEvent?: Integration['processEvent'] & jest.Mock;
+}
+
+function spyOnIntegrationById(id: string, integrations: Integration[]): IntegrationSpy {
+  const candidate = integrations?.find(integration => integration.name === id);
+  if (!candidate) {
+    throw new Error(`Integration ${id} not found`);
+  }
+
+  jest.spyOn(candidate, 'setupOnce');
+  candidate.preprocessEvent && jest.spyOn(candidate, 'preprocessEvent');
+  candidate.processEvent && jest.spyOn(candidate, 'processEvent');
+  return candidate as IntegrationSpy;
+}
+
+function createTestClient(): ReactNativeClient {
+  const clientOptions: ReactNativeClientOptions = {
+    dsn: MOCK_DSN,
+    transport: () => ({
+      send: jest.fn().mockResolvedValue(undefined),
+      flush: jest.fn().mockResolvedValue(true),
+    }),
+    stackParser: defaultStackParser,
+    integrations: [],
+  };
+  clientOptions.integrations = getDefaultIntegrations(clientOptions);
+
+  return new ReactNativeClient(clientOptions);
+}
diff --git a/test/integrations/nativelinkederrors.test.ts b/test/integrations/nativelinkederrors.test.ts
diff --git a/typings/jest-extended.d.ts b/typings/jest-extended.d.ts
diff --git a/yarn.lock b/yarn.lock

Original file line number	Diff line number	Diff line change
`@@ -705,8 +705,8 @@ public void fetchNativeSdkInfo(Promise promise) {`
`705`	`705`	`}`
`706`	`706`	`}`
`707`	`707`
`708`		`- public void fetchNativePackageName(Promise promise) {`
`709`		`- promise.resolve(packageInfo.packageName);`
	`708`	`+ public String fetchNativePackageName() {`
	`709`	`+ return packageInfo.packageName;`
`710`	`710`	`}`
`711`	`711`
`712`	`712`	`private void setEventOriginTag(SentryEvent event) {`
Original file line number	Diff line number	Diff line change
`@@ -135,12 +135,13 @@ public WritableMap stopProfiling() {`
`135`	`135`	`}`
`136`	`136`
`137`	`137`	`@Override`
`138`		`- public void fetchNativePackageName(Promise promise) {`
`139`		`- this.impl.fetchNativePackageName(promise);`
	`138`	`+ public String fetchNativePackageName() {`
	`139`	`+ return this.impl.fetchNativePackageName();`
`140`	`140`	`}`
`141`	`141`
`142`	`142`	`@Override`
`143`		`- public void fetchNativeStackFramesBy(ReadableArray instructionsAddr, Promise promise) {`
	`143`	`+ public WritableMap fetchNativeStackFramesBy(ReadableArray instructionsAddr) {`
`144`	`144`	`// Not used on Android`
	`145`	`+ return null;`
`145`	`146`	`}`
`146`	`147`	`}`
Original file line number	Diff line number	Diff line change
`@@ -133,13 +133,14 @@ public WritableMap stopProfiling() {`
`133`	`133`	`return this.impl.stopProfiling();`
`134`	`134`	`}`
`135`	`135`
`136`		`- @ReactMethod`
`137`		`- public void fetchNativePackageName(Promise promise) {`
`138`		`- this.impl.fetchNativePackageName(promise);`
	`136`	`+ @ReactMethod(isBlockingSynchronousMethod = true)`
	`137`	`+ public String fetchNativePackageName() {`
	`138`	`+ return this.impl.fetchNativePackageName();`
`139`	`139`	`}`
`140`	`140`
`141`		`- @ReactMethod`
`142`		`- public void fetchNativeStackFramesBy(ReadableArray instructionsAddr, Promise promise) {`
	`141`	`+ @ReactMethod(isBlockingSynchronousMethod = true)`
	`142`	`+ public WritableMap fetchNativeStackFramesBy(ReadableArray instructionsAddr) {`
`143`	`143`	`// Not used on Android`
	`144`	`+ return null;`
`144`	`145`	`}`
`145`	`146`	`}`
Original file line number	Diff line number	Diff line change
`@@ -201,11 +201,10 @@ - (void)setEventEnvironmentTag:(SentryEvent *)event`
`201`	`201`	`resolve(modulesString);`
`202`	`202`	`}`
`203`	`203`
`204`		`-RCT_EXPORT_METHOD(fetchNativePackageName:(RCTPromiseResolveBlock)resolve`
`205`		`- rejecter:(RCTPromiseRejectBlock)reject)`
	`204`	`+RCT_EXPORT_SYNCHRONOUS_TYPED_METHOD(NSString *, fetchNativePackageName)`
`206`	`205`	`{`
`207`	`206`	`NSString *packageName = [[NSBundle mainBundle] executablePath];`
`208`		`- resolve(packageName);`
	`207`	`+ return packageName;`
`209`	`208`	`}`
`210`	`209`
`211`	`210`	`- (NSDictionary) fetchNativeStackFramesBy: (NSArray<NSNumber>*)instructionsAddr`
`@@ -273,12 +272,10 @@ - (NSDictionary) fetchNativeStackFramesBy: (NSArray<NSNumber>*)instructionsAdd`
`273`	`272`	`}`
`274`	`273`	`}`
`275`	`274`
`276`		`-RCT_EXPORT_METHOD(fetchNativeStackFramesBy:(NSArray *)instructionsAddr`
`277`		`- resolve:(RCTPromiseResolveBlock)resolve`
`278`		`- reject:(RCTPromiseRejectBlock)reject)`
	`275`	`+RCT_EXPORT_SYNCHRONOUS_TYPED_METHOD(NSDictionary , fetchNativeStackFramesBy:(NSArray )instructionsAddr)`
`279`	`276`	`{`
`280`		`- resolve([self fetchNativeStackFramesBy:instructionsAddr`
`281`		`- symbolicate:dladdr]);`
	`277`	`+ return [self fetchNativeStackFramesBy:instructionsAddr`
	`278`	`+ symbolicate:dladdr];`
`282`	`279`	`}`
`283`	`280`
`284`	`281`	`RCT_EXPORT_METHOD(fetchNativeDeviceContexts:(RCTPromiseResolveBlock)resolve`