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

Fixing uncatchable, crashing, exception on Android #355

Merged
merged 3 commits into from
Feb 16, 2018

Conversation

cworsley4
Copy link
Contributor

NOTE: This does not seem to occur on iOS. We believe that it fails silently which is also an error. We have not done extensive testing there.


We found an issue on Android (8.1 was used) where if we used the public DSN URL we would get an exception citing that we were missing the Sentry secret. In react native, this gave us a red box with the error but it crashed our app. We tried to wrap in a try/catch but no luck.

The error was happening asynchronously in the Sentry Java SDK and not being broadcasted over the RN bridge like you would expect (note this line errorCallback.invoke(e.getMessage());, where errorCallback is of type Callback from the RN Callback class), causing the error to get lost and our app unable to recover.

To fix this we wanted to await on the asynchronous operation. So we created a new method async customInit() that returns a Promise that we await on.

In that Promise, we resolve/reject based on if the error/success callback is called.

awaiting on the install() method allows us to wrap that call in a try/catch in case something does throw either Native-ly or in JavaScript.

This was a very concerning bug to us, that our Error Reporting tool would throw an uncatchable error on initialization.

If your SDK is throwing exceptions upon startup, how will it behave now that this PR fixes the uncaught exception? If we catch this error and then another part of our code tries to capture an exception with sentry what happens? Have you thought about that?

If we had properly set the secret we never would have caught this! And in the future, if we improperly updated the secret or fat fingered the DSN URL then we would have users with essentially bricked apps that would have lost as customers.

While we have forked the repo we would hope that this would be either merged or otherwise patched in the official react-native-sentry repository.

…ve part of the sentry module doesn't crash in a way that prevents us from catching the error in javascript'
@cworsley4 cworsley4 requested a review from HazAT as a code owner February 15, 2018 02:29
@HazAT HazAT self-assigned this Feb 16, 2018
@HazAT
Copy link
Member

HazAT commented Feb 16, 2018

Hey @cworsley4
Thanks for this PR, this makes absolute sense.
I will merge this to master and create a new PR for iOS, before that I cannot release it.

@HazAT HazAT merged commit 453fe1c into getsentry:master Feb 16, 2018
HazAT added a commit that referenced this pull request Feb 16, 2018
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.

2 participants