Skip to content

Commit

Permalink
Crash reporting heaven (#23691)
Browse files Browse the repository at this point in the history
Summary:
<!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? -->
I have used RN for a long time, and for all this time, crash reporting has been less great than native development crash reporting. At some point, companies like sentry, bugsnag and a bunch of others started supporting sourcemaps for js crashes in RN, which helped a lot.
But native crashes were (and still are) much harder to diagnose.

..Until now :D

I have make a repo of a sample RN app, included this PR in it, and some code and screenshots to help.
The repo is [here](https://github.com/pvinis/react-native-project-with-crash-heaven-pr).

I was trying to get good crash reports from native crashes in iOS for a looong time. I spoke with people in sentry, in bugsnag and more, and I could not get this solved. There was no clear way to get the **native** crashed to display correctly.
I made two repos here, one for [sentry](https://github.com/pvinis/SentryBadStack) and one for [bugsnag](https://github.com/pvinis/BugsnagBadStack), demonstrating the correct js handling and the bad native handling.

After all this, and talks with their support, twitter etc, I investigated further, on **why** this was happening. I thought there must be some reason that native crashes look bad in all the tools, and in the same way. Maybe it's not their fault, or up to them to fix it, or maybe they didn't have the experience to fix it.

In a test project I created, I checked what's up with the `RCTFatalException`, and I found out that the React Native code is catching the `NSException`s that come from any native modules of a RN app and converting it into an string and sending it to `RCTFatal` that created an `NSError` out of that string. Then it checks if the app has set a fatal error handler and if not, goes ahead and throws that `NSError`.

The problem here is that `NSException` has a bunch more info that the resulting `NSError` is missing or is altering. Turning the callstack into a string renders crash reporting tools useless as they are missing the original place the exception was thrown, symbols, return addresses etc. In both repos above it can be seen that both tools were thinking that the error happened somewhere in the `RCTFatal` function, and it did, since we create it there, losing all the previous useful info of the original exception. That leaves us with just a very long name including a callstack, but very hard to actually map this to the code and dsym.

I added a fatal exception handler, that mirrors the fatal error handler, as the error handler is used around React Native internal code.

Then I stopped making a string out of the original `NSException` and calling `RCTFatal`, and I simply throw the exception. This way no info is lost!

Finally, I added some code examples of native and js crashes and added a part in the `RNTester` app, so people can see how a js and a native error look like while debugging, as well as try to compile the app in release mode and see how the crash report would look like if they connect it to bugsnag or sentry or their tool of choice.

I have attached some images at the bottom of this PR, and you can find some in the 3 repos I linked above.

[iOS] [Fixed] - Changed the way iOS native module exceptions get handled. Instead of making them into an `NSError` and lose the context and callstack, we keep them as `NSException`s and propagate them.
[General] [Added] - Example code for native crashes in iOS and Android, with buttons on RNTester, so developers can see how these look when debugging, as well as the crash reports in release mode.
Pull Request resolved: #23691

Reviewed By: fkgozali

Differential Revision: D14276366

Pulled By: cpojer

fbshipit-source-id: b308d5608e1432d7676447347ae77c0721094e62
  • Loading branch information
pvinis authored and facebook-github-bot committed Mar 13, 2019
1 parent c991e1c commit 629708b
Show file tree
Hide file tree
Showing 9 changed files with 161 additions and 4 deletions.
15 changes: 15 additions & 0 deletions RNTester/RNTester.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@
83636F8F1B53F22C009F943E /* RCTUIManagerScenarioTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 83636F8E1B53F22C009F943E /* RCTUIManagerScenarioTests.m */; };
8385CEF51B873B5C00C6273E /* RCTImageLoaderTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 8385CEF41B873B5C00C6273E /* RCTImageLoaderTests.m */; };
8385CF041B87479200C6273E /* RCTImageLoaderHelpers.m in Sources */ = {isa = PBXBuildFile; fileRef = 8385CF031B87479200C6273E /* RCTImageLoaderHelpers.m */; };
AFEACA842223EB05004E5198 /* CrashyCrash.m in Sources */ = {isa = PBXBuildFile; fileRef = AFEACA832223EB05004E5198 /* CrashyCrash.m */; };
BC9C03401DC9F1D600B1C635 /* RCTDevMenuTests.m in Sources */ = {isa = PBXBuildFile; fileRef = BC9C033F1DC9F1D600B1C635 /* RCTDevMenuTests.m */; };
C60A228221C9726800B820FE /* RCTFormatErrorTests.m in Sources */ = {isa = PBXBuildFile; fileRef = C60A228121C9726800B820FE /* RCTFormatErrorTests.m */; };
C60A228321C9726800B820FE /* RCTFormatErrorTests.m in Sources */ = {isa = PBXBuildFile; fileRef = C60A228121C9726800B820FE /* RCTFormatErrorTests.m */; };
Expand Down Expand Up @@ -563,6 +564,8 @@
8385CEF41B873B5C00C6273E /* RCTImageLoaderTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTImageLoaderTests.m; sourceTree = "<group>"; };
8385CF031B87479200C6273E /* RCTImageLoaderHelpers.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTImageLoaderHelpers.m; sourceTree = "<group>"; };
8385CF051B8747A000C6273E /* RCTImageLoaderHelpers.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = RCTImageLoaderHelpers.h; sourceTree = "<group>"; };
AFEACA822223EB05004E5198 /* CrashyCrash.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CrashyCrash.h; sourceTree = "<group>"; };
AFEACA832223EB05004E5198 /* CrashyCrash.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = CrashyCrash.m; sourceTree = "<group>"; };
BC9C033F1DC9F1D600B1C635 /* RCTDevMenuTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTDevMenuTests.m; sourceTree = "<group>"; };
C60A228121C9726800B820FE /* RCTFormatErrorTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = RCTFormatErrorTests.m; sourceTree = "<group>"; };
C654F0B21EB34A73000B7A9A /* RNTesterTestModule.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RNTesterTestModule.m; sourceTree = "<group>"; };
Expand Down Expand Up @@ -774,6 +777,7 @@
272E6B3A1BEA846C001FCF37 /* NativeExampleViews */,
13B07FAF1A68108700A75B9A /* AppDelegate.h */,
13B07FB01A68108700A75B9A /* AppDelegate.m */,
AFEACAB12223EB2C004E5198 /* NativeExampleModules */,
13B07FB11A68108700A75B9A /* LaunchScreen.xib */,
13B07FB71A68108700A75B9A /* main.m */,
1323F18D1C04ABAC0091BED0 /* Supporting Files */,
Expand Down Expand Up @@ -1021,6 +1025,16 @@
name = Products;
sourceTree = "<group>";
};
AFEACAB12223EB2C004E5198 /* NativeExampleModules */ = {
isa = PBXGroup;
children = (
AFEACA822223EB05004E5198 /* CrashyCrash.h */,
AFEACA832223EB05004E5198 /* CrashyCrash.m */,
);
name = NativeExampleModules;
path = RNTester/NativeExampleModules;
sourceTree = "<group>";
};
D85B82921AB6D5CE003F4FE2 /* Products */ = {
isa = PBXGroup;
children = (
Expand Down Expand Up @@ -1724,6 +1738,7 @@
13B07FBC1A68108700A75B9A /* AppDelegate.m in Sources */,
27F441EC1BEBE5030039B79C /* FlexibleSizeExampleView.m in Sources */,
13B07FC11A68108700A75B9A /* main.m in Sources */,
AFEACA842223EB05004E5198 /* CrashyCrash.m in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
};
Expand Down
18 changes: 18 additions & 0 deletions RNTester/RNTester/NativeExampleModules/CrashyCrash.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
*/

#import <Foundation/Foundation.h>
#import <React/RCTBridgeModule.h>


NS_ASSUME_NONNULL_BEGIN

@interface CrashyCrash : NSObject <RCTBridgeModule>
@end

NS_ASSUME_NONNULL_END
23 changes: 23 additions & 0 deletions RNTester/RNTester/NativeExampleModules/CrashyCrash.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
*/

#import "CrashyCrash.h"


@implementation CrashyCrash

RCT_EXPORT_MODULE();

RCT_EXPORT_METHOD(letsCrash)
{
NSArray *a = @[@"wow"];
NSString *s = [a objectAtIndex:42]; // native crash here
NSLog(@"%@", s);
}

@end
52 changes: 52 additions & 0 deletions RNTester/js/CrashExample.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @format
* @flow strict-local
*/

'use strict';

import React from 'react';
import {NativeModules, Button} from 'react-native';

const {CrashyCrash} = NativeModules;

exports.displayName = (undefined: ?string);
exports.framework = 'React';
exports.title = 'Crash';
exports.description = 'Crash examples.';

exports.examples = [
{
title: 'JS crash',
render() {
return (
<Button
title="JS crash"
onPress={() => {
const a = {};
const b = a.w.q; // js crash here
console.log(b);
}}
/>
);
},
},
{
title: 'Native crash',
render() {
return (
<Button
title="Native crash"
onPress={() => {
CrashyCrash.letsCrash();
}}
/>
);
},
},
];
4 changes: 4 additions & 0 deletions RNTester/js/RNTesterList.android.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@ const APIExamples: Array<RNTesterExample> = [
key: 'ClipboardExample',
module: require('./ClipboardExample'),
},
{
key: 'CrashExample',
module: require('./CrashExample'),
},
{
key: 'DatePickerAndroidExample',
module: require('./DatePickerAndroidExample'),
Expand Down
5 changes: 5 additions & 0 deletions RNTester/js/RNTesterList.ios.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,11 @@ const APIExamples: Array<RNTesterExample> = [
module: require('./ClipboardExample'),
supportsTVOS: false,
},
{
key: 'CrashExample',
module: require('./CrashExample'),
supportsTVOS: false,
},
{
key: 'Dimensions',
module: require('./DimensionsExample'),
Expand Down
10 changes: 8 additions & 2 deletions React/Base/RCTAssert.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,11 @@ RCT_EXTERN void _RCTAssertFormat(
/**
* Report a fatal condition when executing. These calls will _NOT_ be compiled out
* in production, and crash the app by default. You can customize the fatal behaviour
* by setting a custom fatal handler through `RCTSetFatalHandler`.
* by setting a custom fatal handler through `RCTSetFatalHandler` and
* `RCTSetFatalExceptionHandler`.
*/
RCT_EXTERN void RCTFatal(NSError *error);
RCT_EXTERN void RCTFatalException(NSException *exception);

/**
* The default error domain to be used for React errors.
Expand Down Expand Up @@ -73,6 +75,7 @@ typedef void (^RCTAssertFunction)(NSString *condition,
NSString *message);

typedef void (^RCTFatalHandler)(NSError *error);
typedef void (^RCTFatalExceptionHandler)(NSException *exception);

/**
* Convenience macro for asserting that a parameter is non-nil/non-zero.
Expand Down Expand Up @@ -114,10 +117,13 @@ RCT_EXTERN void RCTAddAssertFunction(RCTAssertFunction assertFunction);
RCT_EXTERN void RCTPerformBlockWithAssertFunction(void (^block)(void), RCTAssertFunction assertFunction);

/**
These methods get and set the current fatal handler called by the RCTFatal method.
* These methods get and set the current fatal handler called by the `RCTFatal`
* and `RCTFatalException` methods.
*/
RCT_EXTERN void RCTSetFatalHandler(RCTFatalHandler fatalHandler);
RCT_EXTERN RCTFatalHandler RCTGetFatalHandler(void);
RCT_EXTERN void RCTSetFatalExceptionHandler(RCTFatalExceptionHandler fatalExceptionHandler);
RCT_EXTERN RCTFatalExceptionHandler RCTGetFatalExceptionHandler(void);

/**
* Get the current thread's name (or the current queue, if in debug mode)
Expand Down
34 changes: 32 additions & 2 deletions React/Base/RCTAssert.m
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

RCTAssertFunction RCTCurrentAssertFunction = nil;
RCTFatalHandler RCTCurrentFatalHandler = nil;
RCTFatalExceptionHandler RCTCurrentFatalExceptionHandler = nil;

NSException *_RCTNotImplementedException(SEL, Class);
NSException *_RCTNotImplementedException(SEL cmd, Class cls)
Expand Down Expand Up @@ -149,9 +150,9 @@ void RCTFatal(NSError *error)
}
}

void RCTSetFatalHandler(RCTFatalHandler fatalhandler)
void RCTSetFatalHandler(RCTFatalHandler fatalHandler)
{
RCTCurrentFatalHandler = fatalhandler;
RCTCurrentFatalHandler = fatalHandler;
}

RCTFatalHandler RCTGetFatalHandler(void)
Expand Down Expand Up @@ -187,3 +188,32 @@ RCTFatalHandler RCTGetFatalHandler(void)

return [NSString stringWithFormat:@"%@%@", message, prettyStack];
}

void RCTFatalException(NSException *exception)
{
_RCTLogNativeInternal(RCTLogLevelFatal, NULL, 0, @"%@: %@", exception.name, exception.reason);

RCTFatalExceptionHandler fatalExceptionHandler = RCTGetFatalExceptionHandler();
if (fatalExceptionHandler) {
fatalExceptionHandler(exception);
} else {
#if DEBUG
@try {
#endif
@throw exception;
#if DEBUG
} @catch (NSException *e) {}
#endif
}
}

void RCTSetFatalExceptionHandler(RCTFatalExceptionHandler fatalExceptionHandler)
{
RCTCurrentFatalExceptionHandler = fatalExceptionHandler;
}

RCTFatalExceptionHandler RCTGetFatalExceptionHandler(void)
{
return RCTCurrentFatalExceptionHandler;
}

4 changes: 4 additions & 0 deletions React/CxxModule/RCTNativeModule.mm
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,14 @@ static MethodCallResult invokeInner(RCTBridge *bridge, RCTModuleData *moduleData
@throw exception;
}

#if RCT_DEBUG
NSString *message = [NSString stringWithFormat:
@"Exception '%@' was thrown while invoking %s on target %@ with params %@\ncallstack: %@",
exception, method.JSMethodName, moduleData.name, objcParams, exception.callStackSymbols];
RCTFatal(RCTErrorWithMessage(message));
#else
RCTFatalException(exception);
#endif
}

return folly::none;
Expand Down

0 comments on commit 629708b

Please sign in to comment.