bugsnag / bugsnag-react-native

Error monitoring and reporting tool for native exceptions and JS errors in React Native apps
https://docs.bugsnag.com/platforms/react-native
MIT License
370 stars 121 forks source link

bugsnag doesn't report crash until app is restarted #129

Closed grigored closed 7 years ago

grigored commented 7 years ago

I saw there were a few old tickets about this, mostly for Android, but I'm having this problem also on iOS with bugsnag-react-native 2.3.0.

It's very important to get the crash when it happens, as the user might not open the app again if it crashed.

How come there still is time to log the crash to disk, but there isn't time to sent it to bugsnag?

kattrali commented 7 years ago

Thanks for the report. The problem isn't as much time, as that when there is a fatal crash on iOS, the system is in a fundamentally unsafe state. In this case, only async-safe functions can be used by a crash reporter to do some final cleanup, like write to a file. Here is a list of all async-safe functions in C. So while it is possible that delivering a crash report might succeed, it also might just crash again and corrupt the report or other app information. This is why a report is sent on next launch.

For other platforms like Android, this limitation is not as much of a problem, though still something to consider if you are interfacing with C. For the JS layer, fatal errors are not really fatal to the integrity of the system, and thus can be sent immediately.

grigored commented 7 years ago

For the JS layer, fatal errors are not really fatal to the integrity of the system, and thus can be sent immediately.

Do you mean that this is happening only on Android? Because I'm having this problem with JS crashes on ios as well. This is probably a dumb question, but can't we just surround the entry point of the app with a try catch, so that there are no more fatals?

kattrali commented 7 years ago

Oh gotcha!

Do you mean that this is happening only on Android? Because I'm having this problem with JS crashes on ios as well.

Well, we might have to unravel two different kinds of errors. In React Native, there can be JS which calls a native function and later crashes, which would fall into the category of reports which need to be written to disk and sent on next app launch. This sort of crash may have a native stack trace rather than a JS one, depending on the context. There are also errors which exist solely at the JS level, like syntax issues or thrown errors, which are caught by React Native's global error handler. Bugsnag captures these and sends them immediately.

This is probably a dumb question, but can't we just surround the entry point of the app with a try catch, so that there are no more fatals?

I think that's a reasonable question! In cocoa applications, exceptions are used for unrecoverable conditions rather than flow control, so recovering from them may still have consequences. There is more info about this design decision in the Exception Programming Topics for Cocoa introduction. Things are a little different for swift, but not much if we are talking about Apple operating systems.

All that said, are you seeing exceptions caused by and caught at the JS level not being sent until next launch? That would definitely be a bug.

grigored commented 7 years ago

I'm writing only JS code, which, afaik, shouldn't cause a native exception. I tested putting in the onPress of the same Touchable: a) throw 'Error2'; -> no crash report is generated in bugsnag

screen shot 2017-08-09 at 4 19 33 am

b) x; -> crash report is generated only when I restart the app.

screen shot 2017-08-09 at 4 20 22 am

Could you explain what is happening?

kattrali commented 7 years ago

That does seem odd. What versions of react-native and bugsnag-react-native are you using? Did you configure it like in the integration guide?

grigored commented 7 years ago

I created a sample repo to easily reproduce this problem: https://github.com/grigored/testbugsnag

My questions are:

  1. why the throw error crash is not captured? What other crashes are not captured?
  2. why the x; crash is only captured when we restart the app? If this is not considered a js crash, what is?
  3. how come the example app doesn't have the native configuration from the integration guide and it's still working?
kattrali commented 7 years ago

Thanks for the example, this is really helpful!

  1. When clicking throw "Error2", the following is printed in the console:

    Bugsnag could not notify: error must be of type Error

    It is also shown in a yellow banner onscreen, but the banner is hidden under the red box in the latest versions of RN, so you don't see it unless you dismiss the red box.

    Throwing objects which aren't errors do not provide a stack trace, which is why Error instances are required. If the button action is changed to throw new Error("A problem occurred") then error notifications are sent as normal.

  2. I see the crash from x; right after opening/refreshing app.bugsnag.com. No app restart required.

  3. The example app just needs to be updated. The flaw in not following the documentation is that any exception which occurs before bugsnag is loaded will not be reported. I'm spending some time today to get the example up to date again (some of the dependencies seem to have issues on RN 0.47), but the integration guide has the final word here.

grigored commented 7 years ago

Great, thanks for your help on this. re1. I just used the first example from the throw docs. I understand that's a problem with js, hopefully all the libs i'm using throw Errors

re2. I just retested, waited for 3 minutes without any report. I restarted the app and I could see the report immediately. Are you sure you tested on a device, and used release, not debug? Because in debug it works fine indeed. It doesn't make any difference, but I just figured out the code in my sample repo didn't include the native changes. Just did another push with that.

re3. cool, thanks.

kattrali commented 7 years ago

re 1. That is a bit confusing. I think we can do something nicer here at minimum when the object is a string, and coerce it into a RuntimeError using the string as a message, for example. Other objects will need more thought.

re 2. Ah, thanks for the clarification, I missed previously that it was in release mode. When in debug mode, React Native logs JavaScript exceptions without crashing, and instead shows the red box. however in release mode, it terminates the app by raising an exception, which is why the report is not sent until the next launch. Here is the relevant code from RCTAssert.m. I'm still digging a little to follow this behavior, but it looks like in release mode the crash is captured at the native level instead of JS. Does this help?

grigored commented 7 years ago

It definitely does help to better understand what's going on. So basically no crash will be reported until the app is restarted. So basically no crash is ever going to be reported until the user restarts the app? I tested Sentry a few days ago, and if I remember right it (sometimes) reported crashes without having to restart the app (but sometimes it wouldn't report anything even after restarting the app).

Do you think both 1. and 2. will be fixed in a future release, or do you see any hard constraint preventing a fix?

kattrali commented 7 years ago

I think we can and should do both! For 2 we can override RCTFatal and add our own behavior. If we synchronously deliver crash reports in that block, all JS exceptions should be delivered before the app crashes. I'll draft some new issues as this conversation was really helpful.

cbrevik commented 7 years ago

We've also experienced this error in our app on Android. Unhandled JS exceptions (throw new Error()) are not logged in Bugsnag on Android release builds. But they also do not seem to be logged when the app is re-opened.

It works fine on iOS though, it is reported right away.

Edit: This is on React Native 0.47.2 and bugsnag-react-native 2.3.2