Summary:
This intentionally leaks the static map, since it still might be accessed after static destructors are run. This is a common approach to this problem, see https://github.com/facebook/react-native/pull/22607 and https://github.com/facebook/componentkit/pull/906 as examples. It also sets up an autorelease pool from `RCTNativeModule::invoke` as a precaution since there's no strict guarantee one exists when it is called.
Changelog:
[iOS][Fixed] - Fix crash in RCTCoreModulesClassProvider during quit
Reviewed By: RSNara
Differential Revision: D27932062
fbshipit-source-id: fa75da4b78290027a762440ac6943c81b8594a57
Summary:
## Motivation
We got this crash T67304907, which shows a `EXC_BAD_ACCESS / KERN_INVALID_ADDRESS` when calling this line:
```
NativeModulePerfLogger::getInstance().asyncMethodCallBatchPreprocessStart();
```
There are no arguments in that call, so I figured the only error could be when we try to invoke `getInstance()` or `asyncMethodCallBatchPreprocessStart()`.
This diff:
1. Removes the `NativeModulePerfLogger::getInstance()` bit. Now NativeModulePerfLogger is used via regular static C functions. So, there's no way that simply invoking one of the logging functions crashes the application: there's no vtable lookup.
2. Inside each logging function, when perf-logging is disabled, the global perflogger should be `nullptr`. This diff makes it so that in that case, we won't execute any code in the control group of the perf-logging experiment.
## Changes
**How do we enable NativeModule perf-logging?**
- Previously:
- `NativeModulePerfLogger::setInstance(std::make_shared<FBReactNativeModulePerfLogger>(...))`
- `TurboModulePerfLogger::setInstance(std::make_shared<FBReactNativeModulePerfLogger>(...))`.
- Now:
- `BridgeNativeModulePerfLogger::enableLogging(std::make_unique<FBReactNativeModulePerfLogger>(...))`
- `TurboModulePerfLogger::enableLogging(std::make_unique<FBReactNativeModulePerfLogger>(...))`
**How do we do NativeModule perf-logging now?**
- Previously:
- `NativeModulePerfLogger::getInstance().command(...args)`
- `TurboModulePerfLogger::getInstance().command(...args)`.
- Now:
- `BridgeNativeModulePerfLogger::command(...args)`
- `TurboModulePerfLogger::command(...args)`.
The benefit of this approach is that each method in `BridgeNativeModulePerfLogger` is guarded with an if check. Example:
```
void moduleCreateConstructStart(const char *moduleName, int32_t id) {
NativeModulePerfLogger *logger = g_perfLogger.get();
if (logger != nullptr) {
logger->moduleCreateConstructStart(moduleName, id);
}
}
```
Therefore, we don't actually execute any code when perf-logging is disabled.
Changelog:
[Internal]
Reviewed By: fkgozali
Differential Revision: D21669888
fbshipit-source-id: 80c73754c430ce787404b563878bad146295e01f
Summary:
## Motivation
This rename will fix the following CircleCI build failures:
- [test_ios_unit_frameworks](https://circleci.com/gh/facebook/react-native/150473?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link)
- [test_ios_detox_frameworks](https://circleci.com/gh/facebook/react-native/150474?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link)
## Investigation
We have 4 podspec targets that map to the same header namespace (i.e: `header_dir`) `ReactCommon`:
- **New:** `React-perflogger`: Directory is `ReactCommon/preflogger`, and contains `NativeModulePerfLogger.{h,cpp}`.
- `React-runtimeexecutor`: Directory is `ReactCommon/runtimeexecutor`, and contains only `RuntimeExecutor.h`
- `React-callinvoker`: Directory is `ReactCommon/callinvoker`, and contains only `CallInvoker.h`
- `ReactCommon/turbomodule/core`: Directory is `ReactCommon/turbomodule`, and contains C++ files, as well has header files.
**The problem:**
We couldn't import headers from `React-perflogger` in `ReactCommon/turbomodule/core` files.
**The cause:**
I'm not entirely sure why, but I was able to discern the following two rules by playing around with the podspecs:
1. If your podspec target has a cpp file, it'll generate a framework when `USE_FRAMEWORKS=1`.
2. Two different frameworks cannot map to the same `module_name` or `header_dir`. (Why? No clue. But something breaks silently when this is the case).
So, this is what happened when I landed `React-perflogger` (D21443610):
1. The TurboModules code generates the `ReactCommon` framework that uses the `ReactCommon` header namespace.
2. `React-runtimeexecutor` and `React-callinvoker` also used the `ReactCommon` header namespace. However, neither generate a framework because of Rule 1.
3. When I comitted `React-perflogger`, I introduced a second framework that competed with the `ReactCommon` framework (i.e: TurboModules code) for the `ReactCommon` header namespace. Rule 2 violation.
## Thoughts on renaming
- `<perflogger/NativeModulePerfLogger.h>` is too generic, and the `perflogger` namepsace is used internally within FB.
- `<react/perflogger/NativeModulePerfLogger.h>` matches our fabric header format, but I'm pretty sure that slashes aren't allowed in `header_dir`: I tested this and it didn't work. IIRC, only alphanumeric and underscore are valid characters for `header_dir` or `module_name`. So, I opted to just use `reactperflogger`.
Changelog: [Internal]
Reviewed By: fkgozali
Differential Revision: D21598852
fbshipit-source-id: 60da5d0f7758eaf13907a080b7d8756688f40723
Summary:
## Motivation
This rename will fix the following CircleCI build failures:
- [test_ios_unit_frameworks](https://circleci.com/gh/facebook/react-native/150473?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link)
- [test_ios_detox_frameworks](https://circleci.com/gh/facebook/react-native/150474?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link)
## Investigation
We have 4 podspec targets that map to the same header namespace (i.e: `header_dir`) `ReactCommon`:
- **New:** `React-perflogger`: Directory is `ReactCommon/preflogger`, and contains `NativeModulePerfLogger.{h,cpp}`.
- `React-runtimeexecutor`: Directory is `ReactCommon/runtimeexecutor`, and contains only `RuntimeExecutor.h`
- `React-callinvoker`: Directory is `ReactCommon/callinvoker`, and contains only `CallInvoker.h`
- `ReactCommon/turbomodule/core`: Directory is `ReactCommon/turbomodule`, and contains C++ files, as well has header files.
**The problem:**
We couldn't import headers from `React-perflogger` in `ReactCommon/turbomodule/core` files.
**The cause:**
I'm not entirely sure why, but I was able to discern the following two rules by playing around with the podspecs:
1. If your podspec target has a cpp file, it'll generate a framework when `USE_FRAMEWORKS=1`.
2. Two different frameworks cannot map to the same `module_name` or `header_dir`. (Why? No clue. But something breaks silently when this is the case).
So, this is what happened when I landed `React-perflogger` (D21443610):
1. The TurboModules code generates the `ReactCommon` framework that uses the `ReactCommon` header namespace.
2. `React-runtimeexecutor` and `React-callinvoker` also used the `ReactCommon` header namespace. However, neither generate a framework because of Rule 1.
3. When I comitted `React-perflogger`, I introduced a second framework that competed with the `ReactCommon` framework (i.e: TurboModules code) for the `ReactCommon` header namespace. Rule 2 violation.
## Thoughts on renaming
- `<perflogger/NativeModulePerfLogger.h>` is too generic, and the `perflogger` namepsace is used internally within FB.
- `<react/perflogger/NativeModulePerfLogger.h>` matches our fabric header format, but I'm pretty sure that slashes aren't allowed in `header_dir`: I tested this and it didn't work. IIRC, only alphanumeric and underscore are valid characters for `header_dir` or `module_name`. So, I opted to just use `reactperflogger`.
Changelog: [Internal]
Reviewed By: fkgozali
Differential Revision: D21585006
fbshipit-source-id: e3339273af5dfd65a1454d87213d1221de6a4651
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/28893
`JSIExecutor::callSerializableNativeHook` converts the arguments from `JSI::Value` to `folly::dynamic`. Then, `RCTNativeModule` converts the arguments from `folly::dynamic` to ObjC data structures in its `static invokeInner` function.
Therefore, I decided to start the sync markers inside `JSIExecutor::callSerializableNativeHook`, which required me to expose these two methode `ModuleRegistry::getModuleName` and `ModuleRegistry::getModuleSyncMethodName`. This shouldn't modify performance because we eagerly generate a NativeModule's methods when it's first required. So, at worst, this is doing a cache lookup.
Changelog: [Internal]
Reviewed By: PeteTheHeat
Differential Revision: D21443610
fbshipit-source-id: 67cf563b0b06153e56e63ba7e186eea31eafc853
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 https://github.com/facebook/react-native/commit/f94521244798f859648d1769f397102c06d763f0#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:
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: 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
Scott Kyle
Ramanpreet Nara
Keegan Mendonca
Valentin Shergin
Andres Suarez
ericlewis
Pavlos Vinieratos
aleclarson
Agustin Collazo
Héctor Ramos
Kevin Gozali
Sophie Alpert
Alex Dvornikov
Pieter De Baets