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: https://github.com/facebook/react-native/pull/23691
Reviewed By: fkgozali
Differential Revision: D14276366
Pulled By: cpojer
fbshipit-source-id: b308d5608e1432d7676447347ae77c0721094e62
Summary:
Between invalidating a bridge and suspending its JS thread, native modules may have their methods called.
Only warn when a native module has been invalidated, which happens right before its JS thread is suspended.
Avoid initializing a native module's instance if its bridge is invalidated.
/cc fkgozali f945212447 (commitcomment-32467567)
[iOS] [Fixed] - Properly validate JS->native method calls
Pull Request resolved: https://github.com/facebook/react-native/pull/23658
Differential Revision: D14287594
Pulled By: fkgozali
fbshipit-source-id: 89dd1906a0c55f3f48ba4ff220aac0cddf2eb822
Summary: Add assorted missing includes in `xplat` that would be exposed by future changes.
Reviewed By: ispeters, nlutsenko
Differential Revision: D14213660
fbshipit-source-id: 329f133784015fe20ee99feaec8ef05e117fe3a6
Summary:
I downgrade the invalid bridge warning because I believe that it is a pain that every time that the JS gets refreshed this warnings are being thrown. If the project increase size and more and more NativeModules are added this warnings just spam the emulator or the device.
I understand the reason of validating if the bridge is valid. However in case of invalidness nothing is done, just the warning is thrown. Hence, the reason of downgrading it to improve the development process.
The error message still exist and it will be in the logs. But it will not spam the development screen
[iOS] [Changed] - Downgrade the Invalid bridge warning message to a log
Pull Request resolved: https://github.com/facebook/react-native/pull/23557
Differential Revision: D14161290
Pulled By: cpojer
fbshipit-source-id: e5608a9b2db5625309fd18d133fe69a9013043f3
Summary:
TurboModules depend on a getConstants method. Existing ObjectiveC modules do not have this method. Therefore, I moved the contents of `constantsToExport` to `getConstants` and then had `constantsToExports` call `getConstants`.
facebook
Since all NativeModules will eventually need to be migrated to the TurboModule system, I didn't restrict this to just the NativeModules in Marketplace.
```
const fs = require('fs');
if (process.argv.length < 3) {
throw new Error('Expected a file containing a list of native modules as the third param');
}
function read(filename) {
return fs.readFileSync(filename, 'utf8');
}
const nativeModuleFilenames = read(process.argv[2]).split('\n').filter(Boolean);
nativeModuleFilenames.forEach((fileName) => {
if (fileName.endsWith('.h')) {
return;
}
const absPath = `${process.env.HOME}/${fileName}`;
const fileSource = read(absPath);
if (/(\n|^)-\s*\((.+)\)getConstants/.test(fileSource)) {
return;
}
const constantsToExportRegex = /(\n|^)-\s*\((.+)\)constantsToExport/;
const result = constantsToExportRegex.exec(fileSource);
if (result == null) {
throw new Error(`Didn't find a constantsToExport function inside NativeModule ${fileName}`);
}
const returnType = result[2];
const newFileSource = fileSource.replace(
constantsToExportRegex,
'$1- ($2)constantsToExport\n' +
'{\n' +
` return ${returnType.includes('ModuleConstants') ? '($2)' : ''}[self getConstants];\n` +
'}\n' +
'\n' +
'- ($2)getConstants'
);
fs.writeFileSync(absPath, newFileSource);
});
```
```
> xbgs -l ')constantsToExport'
```
Reviewed By: fkgozali
Differential Revision: D13951197
fbshipit-source-id: 394a319d42aff466c56a3d748e17c335307a8f47
Summary:
change RCTCxxBridge to use JSIExecutorFactory+JSCRuntime
instead of JSCExecutorFactory. Also remove JSC usage from RN in other
files. This allows deleting files, too, which is done further down
the stack.
Reviewed By: RSNara
Differential Revision: D9369111
fbshipit-source-id: 67ef3146e3abe5244b6cf3249a0268598f91b277
Summary: This change drops the year from the copyright headers and the LICENSE file.
Reviewed By: yungsters
Differential Revision: D9727774
fbshipit-source-id: df4fc1e4390733fe774b1a160dd41b4a3d83302a
Summary: A bunch of flows including JS reload and e2e tests seem to hit the race condition, causing redbox. For now, make it a warning to unblock.
Differential Revision: D9327418
fbshipit-source-id: a72b378d88f7566268fd9415fbd34225c8b931e7
Summary: This helps prevent race condition where JS calls to NativeModules got queued and executed while the bridge is invalidating itself, causing assertion failures in test setup (for example). It won't prevent it 100% of the time, due to threading (and adding lock is expensive for each nativemodule call).
Reviewed By: yungsters
Differential Revision: D9231636
fbshipit-source-id: 298eaf52ffa4b84108184124e75b206b9ca7a41d
Summary:
Includes React Native and its dependencies Fresco, Metro, and Yoga. Excludes samples/examples/docs.
find: ^(?:( *)|( *(?:[\*~#]|::))( )? *)?Copyright (?:\(c\) )?(\d{4})\b.+Facebook[\s\S]+?BSD[\s\S]+?(?:this source tree|the same directory)\.$
replace: $1$2$3Copyright (c) $4-present, Facebook, Inc.\n$2\n$1$2$3This source code is licensed under the MIT license found in the\n$1$2$3LICENSE file in the root directory of this source tree.
Reviewed By: TheSavior, yungsters
Differential Revision: D7007050
fbshipit-source-id: 37dd6bf0ffec0923bfc99c260bb330683f35553e
Summary: As RCTNativeModule can be destructed at any time, it's unsafe to capture "this" in a callback.
Reviewed By: javache
Differential Revision: D5963728
fbshipit-source-id: c80a01c851d97813e4fead2b31c442eaeb8ae204
Summary:
[`CxxModule::Method::callbacks`](2161f92aaf/ReactCommon/cxxreact/CxxModule.h (L66)) is `size_t` which uses `%zu` as the format specifier. Newer versions of clang can warn or error on this. This change prevents that from happening.
Local builds with clang 5.
Closes https://github.com/facebook/react-native/pull/15402
Differential Revision: D5583710
Pulled By: javache
fbshipit-source-id: 1d1e740c275fddfea177cf9476cd1d03b7e27330
Summary:
We've simplified a lot of the conditions for eager of the module init so now we can introduce a final switch to allow modules to opt-out (and in the future opt-in if they still require the behaviour).
We now require you to be explicit about the intended behaviour and implement the `+ (BOOL)requiresMainQueueSetup` method on your module. When you return YES from this method, it tells the bridge the module needs to be created on the main thread (and to avoid deadlocks, we do so eagerly during bridge startup). When you return NO, the native module will be initialised when it's first accessed from JS.
The current behaviour is maintained but a warning is emitted until the new API is adopted.
Reviewed By: fkgozali
Differential Revision: D5527788
fbshipit-source-id: 56d38f81e58cf950547b9780e89bfac4667eeaaa
ericlewis
Pavlos Vinieratos
aleclarson
Yedidya Feldblum
Agustin Collazo
Ramanpreet Nara
Marc Horowitz
Héctor Ramos
Kevin Gozali
Sophie Alpert
Alex Dvornikov
Pieter De Baets
Dave Lee
Viacheslav Radchenko
Ashok Menon