Summary:
I had intended to make this change as part of the stack I landed earlier, but I had some poorly resolved merge conflicts that left this path disabled.
I verified that T76057501 no longer repros and ran unit tests.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27788467
fbshipit-source-id: 42148b887c6b3c0e815f1805e6bfb3ee58503e48
Summary:
Add mounting layer test that stress-tests differ on (un)flattening.
This fails before D27759380 and D27730952, and passes after.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27767219
fbshipit-source-id: a7e186e510f95792da6f98f80fcae5ff8ac74775
Summary:
While I think this was a very marginal bug with no known issues in the wild, incorrect layout values were sometimes being propagated to certain nodes. This would only occur during complex nested (un)flattening operations and may only impact node consistency, specifically with setting the "previous" ShadowView of mutation instructions, specifically REMOVE and DELETE. Even in rigorous testing I had trouble hitting this case and it didn't seem to impact the "next" values in CREATE, INSERT, or UPDATE.
The issue: previously `sliceChildShadowNodeViewPairsV2` assumed that the node it's operating on is a child of a non-flattened view, and the baseline origin is `{0,0}`. You can see when `sliceChildShadowNodeViewPairsRecursivelyV2` is called, a `layoutOffset` is passed in. If we ever got a list of a node that was in a flattened parent by calling `sliceChildShadowNodeViewPairsV2`, we would incorrectly assume that baseline layoutOffset for the node is `0,0`.
Now, we store the layoutOffset in the ShadowViewNodePair and can retrieve it when getting child pairs of a node.
Changelog: [internal]
Reviewed By: sammy-SC
Differential Revision: D27759380
fbshipit-source-id: a89756190a1cb377bcc55ff31799c2afbaecdaa9
Summary:
Previously, `ShadowViewNodePair::List` owned each `ShadowViewNodePair` but whenever we put `ShadowViewNodePair` into a TinyMap, those were unowned pointer references. This worked... 99% of the time. But in some marginal cases, it would cause dangling pointers, leading to difficult-to-track-down issues. So, I'm moving both of these to be unowned pointers and keeping a `std::deque` that owns all `ShadowViewNodePair`s and is itself owned by the main differ function. See comments for more implementation details. I'm moderately concerned about memory usage regressions, but practically speaking this will contain many items when a tree is created for the first time, and then very few items after that (space complexity should be similar to `O(n)` where `n` is the number of changed nodes after the last diff).
See comments as to why I believe `std::deque` is the right choice. Long-term there might be data-structures that are even more optimal, but std::deque has the right tradeoffs compared to other built-in STL structures like std::list and std::vector, and is probably better than std::forward_list too. Long-term we may want a custom data-structure that fits our needs exactly, but std::deque comes close and is possibly optimal.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27730952
fbshipit-source-id: 2194b535439bd309803a221188da5db75242005a
Summary:
I am fixing an extremely marginal case that probably impacts nothing in production, but in theory, could - see next diff in stack for the assert that is being hit.
TL;DR in marginal, complex cases with a lot of un/flattening, we can generate the following sequence of mutations:
```
UPDATE node V1 -> V2
REMOVE node V1
```
That is incorrect, and what we actually want is:
```
UPDATE node V1 -> V2
REMOVE node V2
```
While this, again, impacts /nothing/ in prod that we know of, it would be good to get this correct so that we can enable stricter asserts (see next diff).
This will also help with debugging LayoutAnimations.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27697788
fbshipit-source-id: 47f34fe3e8107167b3df4db841d2cc14c58cb31d
Summary:
Changes in following diffs will be gated by this feature flag.
The differ in the new file is copied from the current stable implementation and will not be modified until it's deleted.
Changelog: [Internal]
Reviewed By: sammy-SC, mdvacca
Differential Revision: D27775698
fbshipit-source-id: 03d9518ffd2b1f25712386c56a38bd2b4d839fc2
Summary:
First, I make the breadcrumbs mechanism (landed just this week) more readable - I forgot to add separators between the breadcrumbs.
Second, there is a path that I am 99% sure we never hit. I've had comments to that effect for a ~year, but now I'm adding a falsey assert. If we don't hit it in prod after a few months I'll be more comfortable just deleting the branch entirely (while probably keeping the assert).
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27697786
fbshipit-source-id: 6d74d1703b2212d069fbed510f2655ec17294458
Summary:
Move this assert so the debug logging executes first; in the case where this assert fires, we'll get a little more information (for Android, where attaching a Cxx debugger is a bit harder vs iOS).
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27585132
fbshipit-source-id: e3f4cc3d78587744b9e73db685eda1fd6c36ca9d
Summary:
With all known remaining issues (outside of LayoutAnimations, potentially) resolved, enable this assert.
Note for future readers: this is the first time this particular assert has EVER been enabled widely, so if we hit this assert in dev, it's great signal for debugging BUT does not necessarily indicate any wide-spread problems in prod, or with any subsystems. This will simply help us become "more correct" over time.
It is possible, but not extremely likely, that cleaning up things that cause this assert will improve stability/crashes.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27697787
fbshipit-source-id: 28fa34eba70548f5001bbd47f41dbe3c6ff3b4c1
Summary:
Allow conversion of LayoutMetrics to DebuggableString.
We also skipped a field in comparison. It probably isn't impactful in terms of production issues, but still wasn't correct.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27709451
fbshipit-source-id: 987fc2de0a4562a295d6cbeffdd922cbf056b811
Summary:
Changelog: [Internal]
Calls to `surfaceHandler.start()` and `setDisplayMode(DisplayMode::Visible)` in quick succession on different threads can cause a race condition between mount and commit operations.
The mountingCoordinator will try to mount an empty revision without any commits causing it to fail with:
```
TransactionTelemetry.cpp:108: function getCommitStartTime: assertion failed (commitStartTime_ != kTelemetryUndefinedTimePoint)
```
which is called from [Binding.cpp](https://www.internalfb.com/intern/diffusion/FBS/browse/master/xplat/js/react-native-github/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp?lines=791-791&blame=1).
This change avoids this initial commit by verifying we had at least 1 revision commited before mounting it.
Reviewed By: sammy-SC
Differential Revision: D27430174
fbshipit-source-id: d208d55f02cd218a316d6fea62d0106c2bcb4be8
Summary:
Introduce a new debugging mechanism for the debugger. Outside of debug mode (you must defined `DEBUG_LOGS_BREADCRUMBS` manually to enable this feature) it will have no cost or binary size.
When the debug mode is enabled, it allows you to trace the call stack to trace what the differ is doing, making logs more useful.
Motivation: tracking down a tricky bug caught by D27585136 which originates in the differ.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27667885
fbshipit-source-id: ef75a9a1c8890f9bbe3e5b2e8a8ffcde92fb22c2
Summary:
Turns out that ShadowViews that have different LayoutMetrics will have the same hash. Fix that.
This helps for debugging LayoutAnimations.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27585133
fbshipit-source-id: f0ac50619115150339089276e34fee5ddd0270bc
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:
Currently we compare StubViewTrees only in contexts where equality is expected. With additional debug flags turned on, we print a verbose summary of differences between trees.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27492937
fbshipit-source-id: 6851c2a4056cdbc9ae790070a3d86bb3f2b462fe
Summary:
Improve debuggability of ShadowViewTree failures by logging ShadowView hashes. This allows us to track down exactly where a change to the tree was introduced.
If necessary, if specific repros are found, logging can also be added to LayoutAnimations or other places to find out where a specific ShadowView version (identified by hash) was introduced.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27488766
fbshipit-source-id: f4866f771fc6494435fb46f09953ea69fb280a5b
Summary:
When playing around with issues in debug mode but without having a debugger attached, these logs are helpful.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407772
fbshipit-source-id: 2083901094d3bd2b0b6c6799baffcf5a60cf57ae
Summary:
This impacts Android only. It's not high-impact to check the consistency of these views and they can become "inconsistent" easily in a way that is not interesting or useful to us,
since we handle "virtual" views differently on the platform.
The need for these hacks should go away in the future if/when we account for virtual views in the differ, and stop sending redundant instructions entirely.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27407779
fbshipit-source-id: 01a7ea5106ed5d94b908de03a6a710cc6275018d
Summary:
We need to do this to break a dependency cycle that would happen if we try to have `view` depend on `mounting` just to add some telemetry to `view`.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D26827446
fbshipit-source-id: 4c415ebf5be3a02c18c80ea8a4a77068cae0f0fe
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:
As part of stoping a Surface, we have to commit an empty shadow node tree. And to avoid a deadlock we have to do it not under a `linkMutex_`. To do so, we need to move the part that commits an empty tree from UIManager to SurfaceHandler. And this diff does exactly this.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D27010535
fbshipit-source-id: 60cc79b1c81d7340b550e78653737d2cc5bec24d
Summary:
Now cumulative time spent on text measuring is available as as part of SurfaceTelemetry.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D26827445
fbshipit-source-id: 8ad3178557ea71170557fcf19b6f62f7072d6da2
Summary:
Now we not only measure how many times we measured text but also measure how much time it takes. This way we can see which portion of the layout process is spent by layout itself (and measuring embedded components).
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D26827447
fbshipit-source-id: e0b09fcacc86aed50dd94b48458215adbb0a60ef
Summary:
Fix warnings about implicit type truncation.
## Changelog
[Internal] [Fixed] - Fix various C++ warnings
Pull Request resolved: https://github.com/facebook/react-native/pull/31002
Test Plan:
Almost all the changes here are simply making explicit conversions which are already occurring. With the exception of a couple of constants being changed from doubles to floats.
With these changes I am able to remove a bunch of warning suppressions in react-native-windows.
Reviewed By: shergin
Differential Revision: D26900502
Pulled By: rozele
fbshipit-source-id: d5e415282815c2212a840a863713287bbf118c10
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:
We have a custom STUB_VIEW_ASSERT that helps debugging stub view issues on platforms (like iOS) where flushing logs around assert-time isn't 100% guaranteed.
Move that logic into react_native_assert since it's generally useful.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26567218
fbshipit-source-id: 79f5ae66fc65a0af48dbcf4c7204ac8245911cb0
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:
This will allow these asserts to crash on Android debug builds.
We will migrate more sites as we confirm this is stable through testing.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26409354
fbshipit-source-id: fb35cd8de29890f7c2b761435eaa02de377bdd1e
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:
iOS and Android platform code already explicitly check this invariant: nodes cannot have parents when they're inserted into the View hierarchy.
Check this in the core so we get these checks in unit tests, and earlier in the core before platform code runs.
Changelog: [Internal]
Reviewed By: shergin, mdvacca
Differential Revision: D26331842
fbshipit-source-id: c12bc9066d280cb85ccc9e754c9fa475927e6080
Summary:
During earlier testing I didn't fully realize that Android disables a core bit of View Flattening: Views can be concrete or non-concrete; and their children can be flattened or not. None of these properties are mutually exclusive with each other.
Except on Android - that functionality is currently disabled. A View can be either flattened and non-concrete, or non-flat and concrete. So there are some flattening edge-cases hit on iOS but not Android, due to the larger state-space on iOS.
To test, I forced Android to align with iOS and tested; and then tested on iOS; and ensured no mounting errors, assertions, or crashes were hit during some specific tests.
Changelog: [Internal]
Differential Revision: D26298872
fbshipit-source-id: 2f0f78127a7bf057c7cf109005f1dae74f0ff6ba
Summary:
On iOS, log lines are not entirely flushed when an assert is hit right after (or during) this mutate function. Make sure to flush log lines regularly during this function so that debugging is easier.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D26291789
fbshipit-source-id: 47af109cdc3dcfc6bf08cbb41db06e9260bfaa08
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:
Changelog: [internal]
Name `shouldCancel` is misleading. It implies the commit is cancelled and doesn't happen.
`shouldYield` expresses the intent better, because the commit's changes do eventually reach mounting layer but the current commit is rather yielding to the next one.
Reviewed By: mdvacca
Differential Revision: D26126121
fbshipit-source-id: 18988f217cbc651f0010f6e2381682bdbaed8bd4
Summary:
Changelog: [internal]
`updateMountedFlag` needs to be called only when new revision will be mounted. In case a commit is throttled, this wasn't the case. Therefore, moving cancellation of commit so it happens before `updateMountedFlag`. We already cancel commits on that place so it should be safe.
Reviewed By: shergin
Differential Revision: D26049262
fbshipit-source-id: e0ecdd2d8f0cdb09d0c0a07ad3931ce77bcf03cf
Summary:
Before this change, `mountingOverrideDelegate` was proxied via `Scheduler::startSurface` and `ShadowTree::ShadowTree` constructor down to `MountingCordinator`. Now we set it on the `MountingCoordinator` directly.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D26049076
fbshipit-source-id: 7f1ecf2c8b6f264a7e59d19881464fe529c53d30
Summary:
This diff simplifies `ShadowTree` constructor by removing `rootComponentDescriptor` argument. It was previously stored and supplied by `Scheduler`; now `ShadowTree` class allocates one instance of it for all instances of `ShadowTree`. The `RootComponentDescriptor` instance of it is only needed to clone a `RootShadowNode` instance; it cannot issue events, state updates, or anything like that because it does not have React counterpart.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross, sammy-SC
Differential Revision: D26048466
fbshipit-source-id: ec02b1b4bcc917efe17cef58112fa870b341c85f
Summary:
See title. Not used in this diff; see next diff in stack.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26001014
fbshipit-source-id: 475eb265f1c27bd9aff978a49652764a50287e47
Summary:
Original commit changeset: 3ed8e78e31b0
Backing-out D25938851 (https://github.com/facebook/react-native/commit/69b3016171bb2f994dd4a62c34c2c4645b5a7d56) and D25935785 (https://github.com/facebook/react-native/commit/bdea479a1faa0f1f7d7c9d9162212cce94bc9720). Based on analysis documented in T83141606, I believe this issue should be fixed in JS.
Additionally, this crash actually has nothing to do with (un)flattening or the differ; it is a side-effect of stale ShadowNodes being cloned, which I believe is either UB or a contract violation. Either way, it should probably be fixed either in JS, or in node cloning. So this isn't the right solution for this issue and should be reverted.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25949569
fbshipit-source-id: 8cf1094a767da98fff4430da60d223412e029545
Summary:
Android has some optimizations around view allocation and pre-allocation that, in the case of View Unflattening, can cause "Create" mutations to be skipped.
To make sure that doesn't happen, we add a flag to ShadowViewMutation (in the core) that any platform can consume, that indicates if the mutation is a "recreation" mutation.
It is still a bit unclear why this is needed, in the sense that I would expect props revision to increment if a view is unflattened. However, there is at least one documented reproduction where that is *not* the case. So for now, we'll have a hack pending further investigation.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25935785
fbshipit-source-id: 6fb4f0a6dedba0fe46ba3cd558ac1daa70f671f5
Summary:
CommitMode allows customizing the side-effects of commit operations. In `Suspended` mode, the results of commit operations will not be passed down to a MountingCoordinator which will result in skipping the mounting phase completely.
This is one of the core elements of the Pre-rendering infrastructure.
Changelog: [Internal] Fabric-specific internal change.
Differential Revision: D24290773
fbshipit-source-id: c10ec20d13f3131fc632352ef22f4465c9dfb3c2
Joshua Gross
CodemodService FBSourceClangFormatLinterBot
Andrei Shikov
Samuel Susla
Valentin Shergin
Andrew Coates
Igor Klemenski
Ramanpreet Nara
