Summary:
The new C++ Differ has been validated on Android and iOS. Delete the old code path.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D28904330
fbshipit-source-id: 2e0d8682f6b2a79f9758ed8b7b92809060835815
Summary:
Root nodes doesn't have a ComponentDescriptor that can be looked up via this mechanism, and we probably shouldn't be animating Root nodes anyway (?). This is a debug-only assert but could be causing issues in production.
The fix is simple - just don't animate any changes to a root node.
Changelog: [Internal]
Reviewed By: Nick177
Differential Revision: D28856396
fbshipit-source-id: 43fa0aa723b03b031fee22e0563eb63cc86239b3
Summary:
Builds on previous diff. Instead of running a single animation to completion, queues up an animation and then spams mutations that could conflict with the animation.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D28357892
fbshipit-source-id: 6ca83a56dded629d8a1444360f1d02037661a5ef
Summary:
Add unit tests for Layout Animations.
This first batch generates a random mutation, then animates it to completion.
I found one issue with UPDATE+REMOVE+INSERT animation consistency. That shouldn't cause any crashes in production, but is a chance to improve consistency of mutations overall - and could in theory point to memory corruption, though it's somewhat unlikely.
I ran with randomized seeds, found issues, fixed them, re-ran to ensure issues were fixed, rinsed and repeated. At the end I was able to run dozens of times (with random seeds) and found nothing.
The next step is to repeatedly generate mutations that conflict with ongoing animations.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D28343750
fbshipit-source-id: c1c60d89a31be3ac05d57482f0af3c482b866abe
Summary:
When REMOVE animations finish, they are hitting asserts in StubViewTree because of UPDATEs and REMOVEs being out-of-order. By simply not forcing REMOVEs to happen before UPDATEs, this problem seems to go away.
Long-term (in a couple weeks, likely) I want to add unit tests to catch all cases like this, but I'm comfortable with this change since it does fix a known assert failure.
Changelog: [Internal]
Differential Revision: D28086223
fbshipit-source-id: eb77ea94d76e996d7b2cb37038ce6f2a9799f8b4
Summary:
Make these logs more readable/useful and add debug print of ShadowView hashes in one place.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27585135
fbshipit-source-id: 5f526856d893c32015d8b480522580732fda0cc6
Summary:
Temporarily disable due to build failures.
This is debug-only and I've never hit these asserts, so this should be safe; though it should be reenabled during active development of LA.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27505224
fbshipit-source-id: b3d6d2184d8f226d6f82d3fe0ae6303813c8356a
Summary:
Changelog: [Internal] enable support for C++ 17.
C++ 17 in React Native targets.
Short and comprehensive list of C++ features:
https://github.com/AnthonyCalandra/modern-cpp-features#c17-language-features
Reviewed By: JoshuaGross
Differential Revision: D27431145
fbshipit-source-id: e8da6fe9d70e9b7343a8caec21cdbeb043478575
Summary:
To ensure that we're eventually deleting all ShadowViews from the StubViewTree and from the mounting layer, make sure that we always queue conflicts if they're DELETE instructions, including for virtual nodes.
I was able to hit this by running some extremely complex animations. It is extremely unlikely that even a single user is hitting this in prod. Therefore, while this is the right change to make, I don't expect (for example) OOMs to go down at all.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27492935
fbshipit-source-id: bce332feb15229af271cc6e14b8367ebcb36536b
Summary:
This is more insurance against future changes than fixing any existing bugs. In a very small number of cases it looks like this sorting isn't working, for reasons that are not easily reproducible (way less than 1 in 10000 times for me, if that) and not obvious, since we're sorting vectors in a canonical and straightforward way.
Again, this insures that logic won't change in the future.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27492936
fbshipit-source-id: 7f47e44ac1101915e1a0433c8f0a50a3c6a0c7b3
Summary:
When displaying instructions in debug logs, also print ShadowView hash to make it easier to reconstruct a series of events.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27492938
fbshipit-source-id: 276a080a4afcfb4aa0aafc215e2ccd56ef849fcf
Summary:
We have no evidence of this happening, but it matches other bail-out cases that already exist in this function. If we
fail something that would have been an assert in debug mode, bail out so we don't return garbage values in production.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27449775
fbshipit-source-id: 5b85016c9611484b615debec514d12a984e6b1ff
Summary:
This chunk of code is repeated 3x in the codebase with minor variations. Consolidate as `queueFinalMutationsForCompletedKeyFrame`.
Should be no changes in behavior.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407766
fbshipit-source-id: 196f0d4c7868007f0d103b024feb4450640a3f62
Summary:
This probably won't compile outside of debug mode, because variables are unused outside of asserts. Don't do any of this outside of debug mode.
This path is also a pessimisation, we shouldn't need to run it unless we're running in ultra-conservative debug mode.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407767
fbshipit-source-id: 71a8d025463c85d65cd2bc193a19953b97669d84
Summary:
Log final when synthetic final mutations are generated at the end of an animation.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407768
fbshipit-source-id: 0e9aface54bcca73dfa5ea263a8474db7c47c078
Summary:
Call getAndEraseConflictingAnimations recursively, to clean up any conflicting animations.
With a specific sequence of mutation instructions, it is possible that an animation on a tree is set up, and the entire tree needs to be cleaned up if a parent is deleted, say.
This can happen especially if rapidly mutating trees in such a way that invokes view flattening and unflattening (which is not recommended, but is certainly possible).
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407776
fbshipit-source-id: 5125250f051c58870692f8fdc43ed23da5058a7f
Summary:
When reconstructing a second animation based on a conflict with a first, we want to ensure we set 'viewPrev' of the second animation properly.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407777
fbshipit-source-id: 8fcc72c4c5fadaab4287824a944ad21230f2f97d
Summary:
In general, when an animation is interrupted or completed, we want to forcibly "flush" an update that will make the StubViewTree/mounting layer consistent with the ShadowTree.
However, in cases where a conflict creates a second animation and stops the first, we simply updated the "viewPrev" of the second animation and smoothly transition to it.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407774
fbshipit-source-id: 928363867d8994c9d69c53154a07bda94f954afa
Summary:
Detect conflicts not just when a node is updated, but when its parent is DELETEd or CREATEd.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407781
fbshipit-source-id: 719d9a8822bf691d9059073a30d8b5ccb50eece1
Summary:
pull out tag so it's easier to read
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407770
fbshipit-source-id: e7d2a3ce8e4ac55b6446cddc8c39e73a18fa7170
Summary:
One of the struggles with the LA engine is to make sure that LA doesn't break the StubViewTree. In particular, we have strict requirements that the "oldShadowView" with every mutation matches exactly what is in the shadow tree, so it appears that there is a linear progression of ShadowNodes for every mutation instruction.
This is a struggle to do with the current setup, requires some wacky code, and doesn't work properly. Instead of spreading REMOVE/DELETE (especially) or INSERT/UPDATE instructions across multiple keyframes, we now allow keyframes to have multiple "final" instructions to execute,
which makes it much easier to keep state consistent.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407769
fbshipit-source-id: 3a709503dc30be69efc345690cb1920eb9591e9a
Summary:
Just makes it easier to log and inspect these values.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27407778
fbshipit-source-id: 68e93d100b56bc9065f0ff1370260412d729312d
Summary:
On Android we have the notion of "virtual views", which are defined consistently but the logic is scattered and duplicated throughout the codebase.
The logic exists to mark nodes that exist in the ShadowTree, but not the View tree. We want to CREATE, UPDATE, and DELETE them on the platform, but not INSERT or REMOVE
them. They basically exist as EventEmitter objects.
The only issue with this is (1) duplicated code, which opened the possibility for inconsistent definition (2) StubViewTree did not account for virtualized views, which caused
assert crashes in debug mode for certain LayoutAnimations on Android.
By moving the definition to ShadowViewMutation and accounting for it in StubViewTree, asserts are correct and consistent on all platforms.
This was not caught until recently, because, until recently, no asserts actually ran on Android.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27001199
fbshipit-source-id: eb29085317037ba8a286d7813bdd57095ad4746f
Summary:
Changelog: [Internal]
Adds react_debug dependency in Android.mk where it was missing
Reviewed By: mdvacca
Differential Revision: D26617400
fbshipit-source-id: 5ac799269b106eadd881d30490ac34bd2134a9b7
Summary:
See react_native_assert.{h,cpp}. Because of the BUCK+Android issue where NDEBUG is always defined, we use react_native_assert instead of assert to enable xplat asserts in debug/dev mode.
This migrates most of the codebase, but probably not 100%. The goal is to increase assertion coverage on Android, not to get to 100% (yet).
Changelog: [Internal]
Reviewed By: RSNara
Differential Revision: D26562866
fbshipit-source-id: a7bf2055b973e1d3650ed8d68a6d02d556604af9
Summary:
Use react_native_assert in LayoutAnimations to enable asserts to fire on Android.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26517096
fbshipit-source-id: f000c4848f29c8779170625d357f547f2e9e6365
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/30988
We have a bunch of flags scattered throughout the codebase with poor hygiene and commenting. Consolidate.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26392518
fbshipit-source-id: 2823de123a5009d6b8c358e8a3f451b9fa0e05b7
Summary:
Building ReactAndroid from source on Windows has recently hit the limitation of maximum path lengths.
At build time, during the `:ReactAndroid:buildReactNdkLib` task, the linker tries to access several of the intermediate binaries located deep in the tmp folder hierarchy, eg.
```
D:\r\ReactAndroid\build\tmp\buildReactNdkLib/local/armeabi-v7a/objs/react_render_components_progressbar/D_/r/ReactAndroid/__/ReactCommon/react/renderer/components/progressbar/android/react/renderer/components/progressbar/AndroidProgressBarMeasurementsManager.o
```
**Suggested fix:** for modules such as `react_render_components_progressbar` and `react_render_components_picker`, rename them to `rrc_progressbar` etc.
**NOTE**: this assumes that the fix from https://github.com/facebook/react-native/issues/30535 is in place. This regression happened while https://github.com/facebook/react-native/issues/30535 has been pending checkin.
**Other mitigations I've tried:**
- setting [`LOCAL_SHORT_COMMANDS`](https://developer.android.com/ndk/guides/android_mk#local_short_commands) for the problematic modules or `APP_SHORT_COMMANDS` for the root project. Turns out those commands don't work on the NDK version RN requires, but even after manually applying a [patch ](https://android-review.googlesource.com/c/platform/ndk/+/1126440) to my local copy of the NDK, these flags had no effect.
- moving the repo directory higher in the file system tree, and using short directory names `D:\r\...` was not enough
- creating virtual drive letters for specific long paths with the [`sust`](https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/subst#examples) command is not workable, since they depend on the source folder structure, and get partly generated by the build system, which I can't plug into
- just enabling long path support on Windows is not enough, since the compiler toolchain doesn't support them.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[Android] [Fixed] - Fix source build on Windows machines vol. 2
Pull Request resolved: https://github.com/facebook/react-native/pull/30776
Test Plan:
Run `.\gradlew installArchives`
Before:
Now:
Differential Revision: D26194286
Pulled By: mdvacca
fbshipit-source-id: 778b5a0515148e2ace19e7902f74301831ebed94
Summary:
remove dead code; this flag is always defaulted to false, and not used anywhere
Changelog: [Internal]
Reviewed By: sammy-SC, mdvacca
Differential Revision: D26271507
fbshipit-source-id: e2277cc24f164c53f2e8a0aa72456ac400834d70
Summary:
Add LA_ASSERT macro, this just makes debugging easier on Android since these asserts are compiled out for us even in debug.
Changelog: [internal]
Reviewed By: mdvacca
Differential Revision: D26271508
fbshipit-source-id: 9be8c71e273d762a4f31ff1fcc629ce48218b98d
Summary:
These asserts don't run in prod; only set starting/final view props if the result props object is non-null.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D26271512
fbshipit-source-id: b495c014a062cf255fd4b5cb8609582f23edcec8
Summary:
In this case where there's an ongoing UPDATE animation and an INSERT of the same node, make sure the ongoing animation type is what we expect, and that the `newChildShadowView` is valid.
We were already guarding against these, but we should (1) crash more in debug and (2) fail more elegantly in prod when the asserts don't run.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D26271514
fbshipit-source-id: 48e7d37a2493241f16099d9fe5ecb0d247707ca7
Summary:
We still have usages of "fbsource//tools/build_defs/apple:flag_defs.bzl" in react-native-github. But this should get us closer towards not using the fbsource cell. Hopefully, this is enough to unbreak the test_docker CircleCI build.
Changelog: [Internal]
Reviewed By: fkgozali
Differential Revision: D26289304
fbshipit-source-id: 1c6464bb84df4f82f8a797321a73a1ed324e319a
Summary:
LayoutAnimations: assert that tag >0 instead of != 0. It's possible that a corrupt tag value would be below zero which is not valid.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26271515
fbshipit-source-id: a62445ad29d60e5180e62ec4c6d5b08784655808
Summary:
We should always be able to reference a ComponentDescriptor. If there are any valid cases where the ComponentDescriptor doesn't exist (?) we need to document those and investigate more.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26271511
fbshipit-source-id: 659d160f82f1e78025d3dbe16efa0fa2d072d15f
Summary:
Make sure there's no corruption happening to these ShadowViews.
Changelog: [internal]
Reviewed By: mdvacca
Differential Revision: D26271509
fbshipit-source-id: 01bfa1bfce56e72b48304fe15b6f6426d3b5247e
Summary:
To ensure that we're not sending old eventEmitters or State objects to the mounting layer, or potentially out-of-date Props objects, base
animated interpolations on the final ShadowNode instead of the initial.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D25727481
fbshipit-source-id: 560ae8d25c7cec4c2137e70b4571b762f461edff
Summary:
The `index` parameter for UpdateMutation is optional, and is normally just -1. It's not useful, so remove it. `parentShadowView` is also not relevant and is not used; in some existing use-cases the actual parent view of the updated view is available, and in some contexts the parent view is not set.
The function now will always set the index to -1 for UpdateMutations, and `{}` for ParentShadowView.
This should have no impact on iOS or Android, as this parameter is not used. It could theoretically have an impact on lifetimes of objects retained (now not retained) by not passing parentShadowView into the mutation. For example, any shared props or state associated with the parent will not be retained in the Update mutation now.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D25342943
fbshipit-source-id: 0ddbef76a6e2eefc2629c9729f721d8674d7737e
Summary:
If a ShadowNode is being animated from opacity 0 to 1, and that animation is interrupted by another update to the same ShadowNode, we stop the animation and send the original "final" ShadowView
to the mounting layer. On iOS, this is enough to make the Mounting layer consistent with the Shadow layer. However, on Android, since only prop deltas are passed to the mounting layer and not the
entire props bag, this will NOT be enough because the opacity will not be updated in that final step.
Therefore, in those cases where we've detected a conflict and we're cleaning up after an interrupted animation, we must send two updates to the mounting layer: one to force the opacity to 1,
and another to make the ShadowTree consistent with the Mounting layer.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D25324942
fbshipit-source-id: 5d9666128feaae87d7530c394ef05db580aa5a75
Summary:
In very marginal cases, it was possible to set up an animation of the following diffing instructions:
```
REMOVE X from parent Y
DELETE X
```
If your LayoutAnimation configuration had no "delete" config, the DELETE would be executed immediately; the REMOVE was erroneously being categorized as an "update" (now fixed)
which caused the REMOVE to be delayed, but then executed very shortly thereafter. So the order of instructions would become:
```
DELETE X
REMOVE X from parent Y
```
which would crash (or at least fail an assertion) when the REMOVE instruction was processed.
This fixes the issue by ensuring that REMOVEs have a corresponding "delete" config, or they are also executed immediately; unless followed by an INSERT (and any combination of `REMOVE, DELETE, INSERT` in the same frame is not possible).
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D25292560
fbshipit-source-id: 7ffdd6cbcb43126de07a70c197dfaf1ebff83555
Summary:
Changelog: [internal]
Return value from `LayoutAnimationKeyFrameManager::getAndEraseConflictingAnimations` was a tuple with 3 elements. Two of them are not being used so let's get rid of them.
Reviewed By: JoshuaGross
Differential Revision: D25220601
fbshipit-source-id: 35781e735b6a2e518337fdeaf956c18bb370993b
Summary:
These configs are never actually empty, so they shouldn't be optionals.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D25129254
fbshipit-source-id: 626119fefad0440732541c680286ebbbfab6aeba
Summary:
Changelog: [internal]
`currentAnimation_` is accessed on multiple threads and has dedicated mutex, but it was not acquiring the mutex in `LayoutAnimationKeyFrameManager::shouldAnimateFrame`
Reviewed By: JoshuaGross
Differential Revision: D25121654
fbshipit-source-id: 38b1c82eaabab283beab18dc210ea21379edbe93
Summary:
Changelog: [Internal]
Contents of `callComplete_` are accessed from JavaScript thread and main thread and there is possible race condition. This diff changes `bool` to `atomic_bool` to prevent the race.
Reviewed By: JoshuaGross
Differential Revision: D25094907
fbshipit-source-id: 6a2c6e33ab5ba0c6ab728e175f2e5c11fdd0a579
Summary:
Imagine the scenario in which there's an ongoing UPDATE animation; a REMOVE+INSERT (move) is queued up for the same tag, but there's no
new corresponding UPDATE - so maybe the indices of the view have changed, but the layout stays the same. Under the old model, the previous animation would be canceled and the node would jump to the final position. In theory, if there's no new UPDATE, we should continue animating the node to its final position.
I'm much happier with this - conflicting animations transition into each other super seamlessly now, and I think the logic is more straightforward as well.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D25071664
fbshipit-source-id: fcefc4619dc34cdafdc4d8e8e730b935e5528290
Joshua Gross
Samuel Susla
Andrei Shikov
Igor Klemenski
Ramanpreet Nara
Andres Suarez
