Summary:
`[RCTBridge setUp]` and `[RCTBridge invalidate]` execute asynchronously and concurrently. Therefore, it's not safe to call one method after the other, as we do in `[RCTBridge reload]`.
In this test, we create a bridge, and immediately reload it. Initializing the bridge causes the JS bundle to execute. Invalidating the bridge causes the jsThread to be terminated. If circumstances are correct, we could end up trying to executing the JS bundle after the jsThread has been terminated, which can lead to these assertions being triggered:
1. `RCTAssert(_jsThread, @"This method must not be called before the JS thread is created");` in `ensureOnJavaScriptThread:`.
2. `RCTAssert(_jsMessageThread != nullptr, @"Cannot invoke completion without jsMessageThread");` in `enqueueApplicationScript:url:onComplete:`.
```
- (void)testUnderlyingBridgeIsDeallocated
{
RCTBridge *bridge;
__weak id batchedBridge;
autoreleasepool {
bridge = [[RCTBridge alloc] initWithBundleURL:_bundleURL moduleProvider:nil launchOptions:nil];
batchedBridge = bridge.batchedBridge;
XCTAssertTrue([batchedBridge isValid], @"RCTBridge impl should be valid");
[bridge reload];
}
RCT_RUN_RUNLOOP_WHILE(batchedBridge != nil)
XCTAssertNotNil(bridge, @"RCTBridge should not have been deallocated");
XCTAssertNil(batchedBridge, @"RCTBridge impl should have been deallocated");
// Wait to complete the test until the new bridge impl is also deallocated
autoreleasepool {
batchedBridge = bridge.batchedBridge;
[bridge invalidate];
bridge = nil;
}
RCT_RUN_RUNLOOP_WHILE(batchedBridge != nil);
XCTAssertNil(batchedBridge);
}
```
To verify that this race is real, patch: P62410422. This adds an artificial delay in the `[RCTCxxBridge start]` method, which makes it so that the bridge is invalidated and the js thread is destroyed before we start executing the jsBundle.
I think a proper solution to this problem would require some bit of restructuring of `[RCTCxxBridge invalidate]` and `[RCTCxxBridge start]` to either:
1. Force `[RCTCxxBridge invalidate]` to wait for `[RCTCxxBridge start]` to complete and vice versa.
2. Make it safe to interleave execution of `[RCTCxxBridge start]` and `[RCTCxxBridge invalidate]`.
I tried the first approach using two semaphores: `_startSem(1)` and `_invalidateSem(0)`. When you start executing the code inside `[RCTCxxBridge start]`, you `semWait(_startSem)`. When you stop executing the code inside `[RCTCxxBridge start]` (which could happen in another thread at some later point in time), you `semSignal(_invalidateSem)`. Likewise, when you start executing `[RCTCxxBridge invalidate]`, you `semWait(_invalidateSem)` and when you stop executing the code inside `[RCTCxxBridge invalidate]` you `semSignal(_startSem)`. This way, invalidates always wait for starts to finish, and starts always wait for invalidates to finish. But considering all the concurrency involved in these methods, this is hard to get right.
The second approach seems possible. You could keep locks for the shared data, and create critical sections whever you want to access that data. I didn't actually try to implement this approach though.
Given that we're going to elminate the Bridge anyway, and that this race condition practically only occurs when you reload imediately after initializing the bridge (which can only really be done programmatically), I think it's fine to just disable the test for now. One other thing I considered was making the current thread sleep for some time after we created the bridge in the test. The reason why I'm hesitant to implement this approach is that it would slow down the execution of the test suite and still wouldn't guarantee that we don't hit this race condition. Ultimately, our infra might end up disabling these tests again.
Reviewed By: shergin
Differential Revision: D14909121
fbshipit-source-id: d7d441c3e2f0ad59182c8c7e23740be4ac4cf83c
Summary:
There are cases where native modules will try to reference other native modules during startup, which can causes a deadlock on `RCTModuleData::_instanceLock`.
My system was in a state where the deadlock would repro 100% so I could actually debug it a bit, but really this whole system is very fragile and needs to die in a fire.
I tried a recursive lock and it was not sufficient - there are definitely issues with multiple threads invoking at the same time and then calling `RCTUnsafeExecuteOnMainQueueSync`...
Reviewed By: fkgozali
Differential Revision: D14863583
fbshipit-source-id: 8c0d062353595a4ed3871aa9135950bc57983907
Summary:
Consolidate JavaScript tests from open source into a single script that can be executed by both Circle CI and Sandcastle, the internal Facebook CI.
[General] [Changed] - Switch internal and external CI to use the same script when running JavaScript tests
Pull Request resolved: https://github.com/facebook/react-native/pull/24422
Reviewed By: cpojer
Differential Revision: D14895773
fbshipit-source-id: d428929cc4e5219e02f5920259e08e0b3d24874c
Summary:
This diff replaces all MountItem classes with a bunch of static C functions that do the same job as classes did.
Seems, originally we overestimated the complexity of MountItem classes and that they ended up being notably trivial. Now, maintaining that even longer would mean paying for abstractions and allocations that we don't really need and writing a lot of tedious code.
Besides that, the one particular change that will be introduced in the coming diffs is not particularly fit very well in the existing class-based model.
This change also should save us many hundreds of allocations and atomic counters bumps, so maybe we can get a millisecond-or-two win.
This diff does not introduce any practical behavioral/logical changes in the mounting layer.
Reviewed By: mdvacca
Differential Revision: D14893764
fbshipit-source-id: 6f1247923ae36f29c12a7d358e2d496cf6c3e298
Summary:
`RCTImageView` is now a subclass of `RCTView` and includes `UIImageView` as it's subview.
This enables the use of `borderRadius`, `borderWidth`, `borderColor` properties and all of their derivatives.
Possible problem:
Now `RCTImageView` is backed by two views (`RCTView` + `UIImageView`), not a single one. That's 4 `CALayers`. Possible workaround would be to insert the image directly into `self.layer.contents`.
Reviewed By: RSNara
Differential Revision: D14875673
fbshipit-source-id: 594b2cd1ddffc6627566e07983c6d8f0b37dc2bb
Summary: This fixes a minor bug where the original props (like for styling) got dropped when the system falls back to UnimplementedView.
Reviewed By: mdvacca
Differential Revision: D14898906
fbshipit-source-id: 4a07952ceac66e491a5c0bc1ffd99f21438cda31
Summary: Similar to iOS, here we provide the basic impl of unimplemented view.
Reviewed By: mdvacca
Differential Revision: D14895706
fbshipit-source-id: 9053edfb2175b370d9070b6921794dbcafa1f37a
Summary: The changes made in D14811733 introduced a regression in React Native's open source iOS tests.
Reviewed By: fkgozali
Differential Revision: D14895096
fbshipit-source-id: 4307b094e3bc94e9f703b0271a95a9b318bcd49a
Summary:
Currently, `RCTLocalAssetImageLoader` only support directory of `Library` and `App bundle`, actually, we need to support other directories like `tmp` or `Documents`. Otherwise, the local image load in `tmp` or `Documents` would be handled by `NSURLSession`, we don't need that.
<img width="405" alt="image" src="https://user-images.githubusercontent.com/5061845/54188126-0ba66100-44ea-11e9-9a7b-0f721100e9be.png">
[iOS] [Fixed] - Enhance search of directories when load local asset image
Pull Request resolved: https://github.com/facebook/react-native/pull/23857
Differential Revision: D14894136
Pulled By: shergin
fbshipit-source-id: 26361cd952a423467be9af9a84a80100d868776b
Summary:
Motivation is following - I'm sure many people encountered this because it has been like this for a long time.
1 . you're developing something on android, HMR and dev mode is enabled
2 . you go to dev settings, you disable dev mode because you want to see how something behaves
3 . you reload the app because that's what is required for the change to take effect
4 . you wait for the bundle to be compiled and served, and when that is done, you get an error message about HMR not being a registered callable module - because HMR is not available when `__DEV__ === false` (todo screenshot)
this fixes the described case by checking if HMR is enabled and dev mode disabled when reloading (step 3) and disables HMR in that case.
this also fixes the case when dev mode is disabled and without knowing it, you try to enable HRM (will enable both dev hmr and dev mode).
[Android] [Changed] - improve developer experience around Dev mode and HMR interop
Pull Request resolved: https://github.com/facebook/react-native/pull/24377
Differential Revision: D14890695
Pulled By: cpojer
fbshipit-source-id: 95b6ff4131c6d05a32aadd09a9d5ed11f602122c
Summary:
There is a problem rendering text shadows on iOS. If the offset of the text shadow is `{width:0,height:0}`, the shadow does not display. This prevents you from representing a light directly above the text. This occurs because a text shadow only renders if the offset is a non-zero CGRect `{width:0,height:0}`.
My change checks `textShadowRadius` instead. If `textShadowRadius` is not nan then the user is rendering a text shadow. There are no situations to render a shadow without `textShadowRadius` making it a good variable to check.
This PR fixes this stale issue: https://github.com/facebook/react-native/issues/17277
[iOS] [Fixed] - Text shadow now displays when the textShadowOffset is {width:0,height:0}
Pull Request resolved: https://github.com/facebook/react-native/pull/24398
Differential Revision: D14890768
Pulled By: cpojer
fbshipit-source-id: a43b96a4a04a5603eede466abacd95c010d053e5
Summary: Running animations sometimes fail in Android. we are ignoring those failures temporarly
Reviewed By: fkgozali
Differential Revision: D14884510
fbshipit-source-id: 66d6113e12b1bd67e8bcc564943b423825b4cea6
Summary: This diff adds extra debug information in the mounting manager
Reviewed By: shergin
Differential Revision: D14817456
fbshipit-source-id: 5619c94eb76cdc20f5d7767f1aa4263e63f8d021
Summary:
This diff changes the way views are inserted by the diffing algorithm.
Previously the diffing algorithm inserted views top-down, now it insert views bottom-up (same order as previous version of RN).
Let say we need to create the following tree:
```
A --> B --> C
|
| --> D
```
Before, the diffing algorithm created the following list of instructions:
```
insert(A, B, 0)
insert(B, C, 0)
insert(B, D, 1)
```
After this diff, the insert instructions are going to be:
```
insert(B, C, 0)
insert(B, D, 1)
insert(A, B, 0)
```
Reviewed By: shergin
Differential Revision: D14817454
fbshipit-source-id: 7aac1a1e1784c53bca2747aee80a5bc8ee788e7a
Summary: This diff ensure that only views created by RN are taken into consideration for recycling.
Reviewed By: fkgozali
Differential Revision: D14874678
fbshipit-source-id: ea7dd5a0f29f6acf0dce8573fc77b012395476bd
Summary: AppRegistry always hides the small fabric indicator. This allows it to be enabled.
Reviewed By: mdvacca
Differential Revision: D14877831
fbshipit-source-id: 4fe3aac13074641049a9298f5a616aea3138ce93
Summary:
Android API 26 and Android Support Library 26 added support for font resource type and native/downloadable fonts. It allows apps to easily download fonts from online providers, but also use of various font weights other than normal and bold, like medium. So it deprecated APIs for asset fonts, and should be removed in the future.
Advantages:
- Just copy font files in res/font and use it specifying filename (without extension) in fontFamily
- Define custom font-family using XML file (in res/font) and font files, it may have many weights and styles. See PR for example.
- Define configuration to download fonts from online font providers, and use it.
See https://developer.android.com/guide/topics/ui/look-and-feel/fonts-in-xml and https://developer.android.com/guide/topics/ui/look-and-feel/downloadable-fonts
[Android] [Changed] - add support for custom/downloadable fonts
Pull Request resolved: https://github.com/facebook/react-native/pull/23865
Differential Revision: D14506542
Pulled By: hramos
fbshipit-source-id: 67ba3148fb4b548cdbc779213cf6c1b2c3baffd2
Summary:
This diff removes the copyToClipBoardMiddleware. This was exposing the /copy-to-clipboard endpoint that allowed unauthenticated access to writing to a developers clipboard.
This was only used in a single place, the Android red box dialog, so I've removed it from there.
Reviewed By: cpojer
Differential Revision: D14790813
fbshipit-source-id: 80e044eefd9658b8c5885643cdadf7947c00d34a
Summary: Remove dependency on ReactRootView in ReactInstanceManager by creating a rough interface (ReactRoot) so that either a Surface or ReactRootView can be attached to ReactInstanceManager.
Reviewed By: ejanzer, mdvacca
Differential Revision: D14158890
fbshipit-source-id: b7ab4654b1e0ef8343230a3c15023653a7f23a4b
Summary:
This allows an unsupported component to be rendered as a "unimplemented view" for better visualization of which component is missing. It is off by default, but configurable in the component factory.
For now, the layout simply follows regular <View />, which means the width/height etc is based on the react component styling. The side effect is that components with 0 height/width won't show up at all.
Reviewed By: mdvacca
Differential Revision: D14869656
fbshipit-source-id: f31e012fb7dc1c64fcc431ea5aa45079a23a618e
Summary:
For props that expects a struct/object value, like scrollView's contentInset, not all keys may be present. For example:
```
<ScrollView ... contentInset={{top: 10}} />
```
In this example, `left`, `bottom`, and `right` should just default to 0 (or whatever the default is in the platform). Before this fix, an exception occured when calling `fromRawValue()` because it is assuming all 4 keys are present in the prop bag. However, only `top` key was present in this example.
To fix this, we have to loop through the available keys in the prop bag, then assign the values accordingly.
Reviewed By: JoshuaGross
Differential Revision: D14868549
fbshipit-source-id: e25208eb31f6d4061338e9cac48a93fe71859859
Summary:
In the case where the first rootView we load enables Fabric, the UIManagerBinding ended up calling AppRegistry (JS) too early, before the bridge was fully ready. This means AppRegistry wasn't set up yet, causing redbox.
To fix this, RuntimeExecutor impl should ask the bridge to queue the callback instead of asking the MessageQueueThread directly. MessageQueueThread only works well IFF the bridge is alive, but it has no knowledge about whether the bridge is alive or not. To support this, we add `invokeAsync` impl to the RCTCxxBridge.
Reviewed By: JoshuaGross
Differential Revision: D14868547
fbshipit-source-id: 3b3f2b9150e930b4a2c71012242305241fc6dbed
Summary:
See https://github.com/facebook/react-native/issues/24316 for the motivation. This commit rewrites the imports in the RNTester project.
[General] [Changed] - Replaced Haste-style imports with standard path-style imports for RNTester
Pull Request resolved: https://github.com/facebook/react-native/pull/24317
Differential Revision: D14870504
Pulled By: cpojer
fbshipit-source-id: b14f22e7ce559efc332ced032617ca581196d90f
Summary:
Fixes#22511
I understand the motivation of adding extraRadiusForOutline to corner radius: without it there can be an empty space between border and view's background color.
So I add simple check: if corner radius is more than zero we still add extraRadiusForOutline. And if not, we don't add it to prevent all the corners to become slightly curved.
[GENERAL] [Fixed] - fix of Android's bug that causes all the corners to become slightly curved if only specific corners' radius should be more than zero.
Pull Request resolved: https://github.com/facebook/react-native/pull/24390
Differential Revision: D14870476
Pulled By: cpojer
fbshipit-source-id: df40fc584a2f8badc499413cb3c4e4d96b9e18cf
Summary:
The root cause comes down to the fact that historically `-DFOO=0` and `-DFOO=1` patterns have been used when it comes to preprocessor definitions and various engineers use `#ifdef FOO` and `#if FOO` interchangeably. That results in an incorrect pattern because the real evaluation is following:
- `FOO` not defined: `#if FOO` = `FALSE`, `#ifdef FOO` = `FALSE`.
- `-DFOO=0`: `#if FOO` = `FALSE`, `#ifdef FOO` = `TRUE`.
- `-DFOO=1`: `#if FOO` = `TRUE`, `#ifdef FOO` = `TRUE`.
Reviewed By: aditya7fb
Differential Revision: D14811733
fbshipit-source-id: a7fedc56ab0ce8eedcb48bda783739a4c1eecf81
Summary:
Following 6df2edeb2a, the template for a new app no longer refers to a specific version of React Native, as the React Native CLI will set the correct version when `react-native init` is run. As it happens, the CLI cannot find a local react-native@1000 version when run as part of the JavaScript e2e tests. Therefore, I am working around using the CLI directly, and instead we're copying the contents of `template/` into a temporary directory for e2e tests. This is not dissimilar to what we already test internally at Facebook.
[General] [Fixed] - Fixed JavaScript e2e tests
Pull Request resolved: https://github.com/facebook/react-native/pull/24381
Differential Revision: D14865627
Pulled By: hramos
fbshipit-source-id: b036da7479200f3efe41b81b6b336333d5558182
Summary: This change synchronizes the JSI API with its upstream, aiming to make this easier to automate in the future.
Reviewed By: mhorowitz
Differential Revision: D14783311
fbshipit-source-id: c180d0f728afbeb87a3e8e7331a39c06e73b907e
Summary: This will simplify updating the JSI API from upstream in the future.
Reviewed By: mhorowitz
Differential Revision: D14762674
fbshipit-source-id: fa4a86f08425943e301da4ef3df9893ebaa1493e
Summary:
Changelog:
----------
[Changed][General] Move callback-related logic to `AnimatedNode` class in order to make it possible to add the listener for other animated nodes than `AnimatedValue`.
I observed that native code appears to be fully prepared for listening not only to animated value but animated nodes generally. Therefore I managed to modify js code for exposing `addListener` method from `AnimatedNode` class instead of `AnimatedValue`. It called for some minor changes, which are not breaking.
If you're fine with these changes, I could add proper docs if needed.
Pull Request resolved: https://github.com/facebook/react-native/pull/22883
Differential Revision: D14041747
Pulled By: cpojer
fbshipit-source-id: 94c68024ceaa259d9bb145bf4b3107af0b15db88
Summary:
@public
compiler flags were pushed to C++14, but we don’t currently have any code that requires it. Setting to `-std=c++11` in order to fix the OSS build.
Reviewed By: SidharthGuglani
Differential Revision: D14833737
fbshipit-source-id: df2cd7da8c7124e89863c90d7b77bcf86c495618
Summary: Need to force the double thing in more places.
Reviewed By: cpojer
Differential Revision: D14835792
fbshipit-source-id: fb7a5435675b322d5fbbe9858e08804e9abe65db
Summary:
[Fixes an issue where the Docker Android tests container cannot be built.](https://github.com/facebook/react-native/pull/24276#issuecomment-480915232) The JSC is now pulled from the npm registry, so we need to run `yarn` prior to pulling the Gradle dependencies.
[Android] [Fixed] - Fixed React Native Android tests Docker container issue related to the JSC
Pull Request resolved: https://github.com/facebook/react-native/pull/24360
Differential Revision: D14842534
Pulled By: hramos
fbshipit-source-id: 3a1a714879e9c52a812b1077dce449470c30bddd
Summary: Adding JNativeRunnable to exported headers in react/jni/BUCK so I can use it outside of CatalystInstance.
Reviewed By: axe-fb
Differential Revision: D14817655
fbshipit-source-id: 15aa794e50f273778da337956c887c235a5aec3d
zhongwuzw
Valentin Shergin
Ramanpreet Nara
Alexander Vasyuk
Spencer Ahrens
Héctor Ramos
Samuel Susla
Kevin Gozali
Dulmandakh
Mike Diarmid
Vojtech Novak
Woodpav
David Vacca
Ryan Donnelly
Sebastian McKenzie
Luna Wei
James Ide
Kacper Wiszczuk
Пётр Потапов
michalchudziak
Gunnar Kudrjavets
Héctor Ramos
gengjiawen
Chad Smith
Will Holen
osdnk
David Aurelio
Emily Janzer
