Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(apple): Disable enableNativeCrashHandling and enableAutoPerformanceTracing #2936

Merged
merged 5 commits into from
Mar 31, 2023

Conversation

mwerder
Copy link
Contributor

@mwerder mwerder commented Mar 28, 2023

fix cast to BOOL for sentry init options 'enableNativeCrashHandling' and 'enableAutoPerformanceTracing'

๐Ÿ“ข Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

๐Ÿ“œ Description

Environment:
XCode: 14.2
sentry/react-native: ^5.2.0
platform: MacOS Catalyst & iOS

I integrated sentry into our react-native catalyst app for MacOS.
While building I ran into following errors:

"cast from pointer to smaller type 'bool' (aka 'signed char') loses information"
on following lines inside RNSentry.mm:

ln:111 BOOL enableNativeCrashHandling = (BOOL)[mutableOptions valueForKey:@"enableNativeCrashHandling"];

ln:122BOOL enableAutoPerformanceTracing = (BOOL)[mutableOptions valueForKey:@"enableAutoPerformanceTracing"];

I'm not familiar with objective-c and its types. As the type BOOL behaves differently on 32 and 64-bit machines, I thought that was the problem. But both, iOS and MacOS are 64-bit systems.
So I cannot figure out why it works on iOS but it throws an error on MacOS.

What I found out after testing though, is that with the existing cast, the options enableNativeCrashHandling and enableAutoPerformanceTracing are always true even if I set them to false inside the Sentry.init() options (test cases 5 and 6 below failed with the existing code using the (BOOL) cast).

By debugging I can confirm that following if statement is never entered when setting enableNativeCrashHandling to false:

BOOL enableNativeCrashHandling = (BOOL)[mutableOptions valueForKey:@"enableNativeCrashHandling"];

if (!enableNativeCrashHandling) {

  NSMutableArray *integrations = sentryOptions.integrations.mutableCopy;
  [integrations removeObject:@"SentryCrashIntegration"];
  sentryOptions.integrations = integrations;
}

๐Ÿ’ก Motivation and Context

Fixes catalyst build and allows options enableNativeCrashHandling & enableAutoPerformanceTracing to be disabled on iOS

The solution is based on following answer on Stack Overflow

๐Ÿ’š How did you test it?

I ran tests locally while running the app on iOS and macOS. Tested all options using console.logs to verify the options are set correctly.
As mentioned above, using the existing code I always get true. So test case 5 and 6 are failing.
This would mean that it is not possible to disable AutoPerformanceTracing and NativeCrashHandling.
I tried to look into the test cases but honestly, I'm not sure where to add them as the options seems to be mocked.
Would this need an e2e test to verify the iOS code?

# Sentry.init options expected Result actual Result iOS actual Result MacOS Pass/Fail
1 enableAutoPerformanceTracing: not set (default) 1 1 1 โœ…
2 enableNativeCrashHandling: not set (default) 1 1 1 โœ…
3 enableAutoPerformanceTracing: true 1 1 1 โœ…
4 enableNativeCrashHandling: true 1 1 1 โœ…
5 enableAutoPerformanceTracing: false 0 0 0 โœ…
6 enableNativeCrashHandling: false 0 0 0 โœ…

closes: #2944

๐Ÿ“ Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

๐Ÿ”ฎ Next steps

Maybe add test cases for options.

@krystofwoldrich
Copy link
Member

Thank you for the PR,
the changes look good, also thank you for the detailed description, that is awesome.

We have a few tests for the method createOptionsWithDictionary would you be to add few to the changes you made to avoid future regression. (enable/disable enableNativeCrashHandling and check if the the integrations are present or not, enable/disable enableAutoPerformanceTracing and check if the private flags are set correctly)

@mwerder
Copy link
Contributor Author

mwerder commented Mar 29, 2023

Hey @krystofwoldrich thanks for your message.

I added some tests and can confirm that without my changes, enableAutoPerformanceTracing and enableNativeCrashHandling were enabled even if set to false within the options:

old_cast_behaviour

With my fix all tests pass:
fixed

I had to add following line inside RNSentry.mm to check the state of AutoPerformanceTracing:

sentryOptions.enableAutoPerformanceTracing = enableAutoPerformanceTracing;

ios/RNSentry.mm Outdated Show resolved Hide resolved
Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thank you @mwerder and @birdofpreyru ๐Ÿš€

@krystofwoldrich krystofwoldrich changed the title fix cast to BOOL for sentry init options Fix enableNativeCrashHandling and enableAutoPerformanceTracing on Apple Mar 31, 2023
@krystofwoldrich krystofwoldrich changed the title Fix enableNativeCrashHandling and enableAutoPerformanceTracing on Apple fix(apple): Disable enableNativeCrashHandling and enableAutoPerformanceTracing Mar 31, 2023
@krystofwoldrich krystofwoldrich merged commit c940e37 into getsentry:main Mar 31, 2023
@mwerder
Copy link
Contributor Author

mwerder commented Mar 31, 2023

Glad I could contribute. Thanks for your help @krystofwoldrich ๐Ÿš€

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Mac Catalyst] Build issues in a React Native project with Sentry
2 participants