[Reliability] Making Android robust during development and production

[ide]

There are a couple of things that are fragile about the RN Android environment right now. There has been some work put into RN iOS to be more forgiving about errors and bad inputs while still telling developers about the problems. A few things I've noticed that RN iOS does well that Android currently doesn't do:

• When there is a redbox, the app continues to function. On Android, the Java React instance is destroyed.
• You can provide custom handlers for soft and fatal JS exceptions. So if you wanted to ignore errors you could, or you could reconstruct the native React instance and do a soft reboot of the app.
• When arguments to native methods are of the wrong type, iOS doesn't invoke the method and skips over it: e9c7ebf
• On Android there's a great opportunity for reliability by catching all Java exceptions instead of crashing

cc @mkonicek

[mkonicek]

Thanks for putting this together!

1. As @kmagiera said somewhere, not sure this is actually a good thing. Why do you want to keep using the app instead of being forced to fix the error that causes the redbox?
2. Didn't know about soft and fatal JS exceptions - interesting. Do you have some examples?
3. Looking at the commit I understand iOS does show a redbox?
4. I believe we catch Java exceptions and show redboxes. Do you know of cases where we hard-crash the app?

[satya164]

Subscribing.

[ide]

@mkonicek some replies:

1. If you are doing QA testing or showing a demo, it's useful to be able to keep using the app even if one method call or prop setter failed. For example POST requests on Android trigger a redbox if you don't specify a method body -- but the rest of the app mostly keeps working. It's also helpful for debugging to be able to keep using the app to figure out what causes the redbox (ex: perhaps the component hierarchy or data stores are in a state that triggers an unhandled edge case -- in which case I want to keep using the current React instance so I can debug it).

   To be clear -- I am not saying that errors should be hidden from programmers or not fixed, but that being lenient and robust until the bugs are fixed is probably the right policy for the native bridge.

2. JS ErrorUtils has the methods reportError and reportFatalError, that call reportSoftException and reportFatalException on the native side. Additionally on iOS you can override what happens when each kind of these errors occurs.

3. Yeah, there's a redbox in dev (logging a message in production by default, since it's one level of severity below "mustfix") and the app continues to function. On iOS it used to crash by passing null into native methods that don't take nullable values (I believe Android never crashed in a similar scenario), so that commit makes things a lot better.

   For RN Android, I've seen quite a few issues showing redboxes that say a method wanted an int64 but got a double, for example. Most of the time the RN code has been fixed, when it was indeed correct that the method should have taken a double, so that's good. Two other things I think we could do better here are to improve the error message (print the name of the native component or module, and the name of the method / view prop that is ) and as I mentioned above, try to be graceful and keep the Catalyst/React instance alive.

4. I'm not sure off the top of my head but I have encountered several crashes. I'll get the stack traces and crash logs if it happens again if they're interesting to you.

[mangogogos]

+1 to #3 @ide
Especially without the chrome React tools not knowing which component caused a crash can sometimes be a pain!

[mkonicek]

Thanks for the awesome writeup @ide! I'm just cleaning up the IntentAndroid API in order to open source it and noticed the following:

@ReactMethod
public void openURI(String uri) {
  if (uri == null || uri.isEmpty()) {
    throw new JSApplicationIllegalArgumentException("Invalid URI: " + uri);
  }
  ...

I'm wondering whether we should be handling errors like this. It causes redbox in dev but hard crashes in production without the developer having any chance to handle this. They can't do for example:

try {
  IntentAndroid.openURI(inputFromUser);
} catch (err) {
   ...
}

When writing an app in Obj-C or Java the developer always has the option to handle the error.

@kmagiera, @nicklockwood what do you think? Should we always be returning a Promise to pass any error to back to JS and logging it?

I like the idea of specifying what errors should redbox / hard crash vs just log on a case-by-case basis by distinguishing between soft and fatal errors (empty POST request body could be a soft error for example).

[mkonicek]

But then if we handle errors like this:

@ReactMethod
public void openURI(String uri, Promise promise) {
  if (uri == null || uri.isEmpty()) {
    promise.reject("Invalid URI: " + uri);
  }
  ...

It is really easy to call IntentAndroid.openURI(bogusURI) and ignore the error :/

Making it easy to ignore errors and just logging them to the console (like browsers do) is a bit strange to me but could be just a matter of preference.

If we want to allow the above and still have redboxes we'd have to find a way to detect all unhandled errors somehow, e.g.:
http://stackoverflow.com/questions/28001722/how-to-catch-uncaught-exception-in-promise

[nicklockwood]

Runtime errors due to bad data, user input, network failures, hardware malfunctions, etc. should always be passed back to JS to be handled by the developer.

Programming errors, such as bad arguments sent to a function should red box and/or crash in production.

The tricky part is cases where it's not clear which it is. For example, is a bad url sent to a JS function a programmer error or a user input error? It depends on whether the value eas hard coded or provided by the user, but we can't know that.

In the case of a URI, I'd tend to assume that it's been provided by the programmer and that it's their responsibility to validate it before sending it over the bridge.

If we were going to validate URIs in release mode and return an error, it would probably make more sense to do that in JS anyway, so the behavior is consistent across platform.

[nicklockwood]

cc: @javache, who's overhauling the error handling logic on iOS at the moment.

[nicklockwood]

FWIW, I'm not a fan of the soft exceptions / hard exceptions system.

Our error handling at the moment is a bit of a mashup of code written by people who subscribe to the "fail soft" approach and those who subscribe to "fail hard", so that a programmer error might call the js error handler in one function, even though there's no programmatic way to handle it, and corrupt data in an http response might hard crash the app in another function even though there's no programmatic way to sanitise the data beforehand

There are also some differences between platforms. For example red box errors are recoverable/closable on iOS, but on Android you only have the option to reload.

I'd prefer we just stick to:

1. programmer errors always show a (recoverable) red box at dev time, and either crash or fail silently at release time, optionally logging to somewhere useful to aid debugging either on the JS or native side (or both).

2. runtime/input errors call a callback in JS, and don't log anything.

[ide]

@mkonicek I like the async function API. It is consistent with ES7 and async functions in other languages.

I'm not too concerned (but still slightly concerned :P) about people ignoring promise errors because (a) it's a problem that the entire JavaScript language faces and needs a solution and (b) with async functions it becomes more natural to catch these errors. I've certainly forgotten to catch errors from promises before and do wish there a better solution existed, but tooling around promises/async functions will likely improve next year.

@nicklockwood

Programming errors, such as bad arguments sent to a function should red box and/or crash in production.

Just my perspective on this - I would like for the severity of programmer errors to be configurable since I generally believe a policy of forgiveness works well for frameworks / platforms. As an example, if you make a bad DOM API call the browser window doesn't crash (hopefully!) but you'll still get an exception that you can handle however you want.

[nicklockwood]

@ide the problem is that checking for all possible error conditions has a runtime penalty, which we don't want to be paying in release mode, so we disable these checks for release.

Doing the checks in dev mode allows us to produce helpful, specific errors, but those errors need to be fatal (red box) so that developers see and fix them. If we leave it up to the developer how to handle them in dev mode, those soft errors may turn into hard crashes in production when we disable the checks.

From a philosophical PoV, while I agree that APIs should be friendly to n00bs, I don't think that quietly allowing errors to slide is particularly friendly. Each error we detect is an opportunity to teach the developer about a misconception they may have, and steer them down the right path. If we encourage them to ignore or suppress errors, that's a missed opportunity.

[mkonicek]

I don't think that quietly allowing errors to slide is particularly friendly. Each error we detect is an opportunity to teach the developer about a misconception they may have, and steer them down the right path.

I'm with Nick on this.

1. programmer errors always show a (recoverable) red box at dev time, and either crash or fail silently at release time, optionally logging to somewhere useful

Right, we just need a way to allow the developer to configure what should happen in prod. @javache told me iOS has a way to do this. For example, in the main fb iOS app, we don't want to crash on exceptions from RN.

2. runtime/input errors call a callback in JS, and don't log anything.

I like this too. Don't think we do this consistently (yet). Also, let's expose native methods using promises everywhere and let's stop using callbacks. Promises are a really nice API.

[mkonicek]

but tooling around promises/async functions will likely improve next year.

That would be awesome 👍 Do you have any links to what's being worked on?

[mkonicek]

Created separate issue for error handling with promises/async functions: #4045

[ide]

@mkonicek - regarding devtool improvements, Chrome has an "Async" checkbox (http://www.html5rocks.com/en/tutorials/developertools/async-call-stack/). Node.js + Google were looking into a tracing API for debugging/instrumentation and async APIs (what Node's AsyncWrap native abstraction was doing, I believe): https://github.com/nodejs/tracing-wg. Generally I'm happy that some major JS stakeholders are thinking about asychrony more.