Summary:
We need it to stop repeating to commit new shadow tree in `ShadowTree::commit` when a transaction cancels the commit.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross, sammy-SC
Differential Revision: D23603959
fbshipit-source-id: d279fb3bf4190e860740a6450595d6f2fc3117f7
Summary:
This pull request adds a call to `std::move` on the lambda capture in `Element::stateData`.
On Windows/Visual Studio 2017, this fixes a failure in the test `LayoutableShadowNodeTest.contentOriginOffset` where the error `std::bad_function_call` was being thrown. This was narrowed down to the callback being empty when called in `Element::stateData`.
https://github.com/facebook/react-native/blob/7e899348c74238a4a042380f86a8fe0d7e05511b/ReactCommon/react/renderer/element/Element.h#L98
Making sure the callback survives with `std::move` allows that test to pass under Windows.
## Changelog
Changelog: [Internal][Changed] - Fabric: Use std::move on callback in Element::stateData
Pull Request resolved: https://github.com/facebook/react-native/pull/29897
Test Plan: The Fabric test suite passes on Windows after this change is made. I also tested it under macOS and Linux built with Clang and they both pass with this change made.
Reviewed By: sammy-SC
Differential Revision: D23591969
Pulled By: shergin
fbshipit-source-id: e5c88bb0e94641e5128c4d49dd2f9dbfa49e9cfa
Summary:
In `StateReconciliationTest`, the way initializer lists are used to create null `ShadowNode`s causes this error on Visual Studio 2017 on Windows:
```cpp
auto result = (ShadowNode const *){nullptr};
```
---
```
StateReconciliationTest.cpp(35): error C4576: a parenthesized type followed by an init
ializer list is a non-standard explicit type conversion syntax
```
This change allows this test to compile in Visual Studio 2017, and the effected tests successfully compile and pass on Windows. They also compile and pass on Linux and macOS (both built with Clang)
## Changelog
Changelog: [Internal][Changed] - Fabric Tests: Change null ShadowNode creation in StateReconciliationTest
Pull Request resolved: https://github.com/facebook/react-native/pull/29899
Test Plan: The Fabric test suite passes on Windows after this change is made. I also tested it under macOS and Linux built with Clang and they both pass with this change made.
Reviewed By: sammy-SC
Differential Revision: D23592007
Pulled By: shergin
fbshipit-source-id: 7c6131736d478a0bf29d6c9475ef9149b7602dd6
Summary:
This pull request changes the default constructor in the `TestProps` class in `react/renderer/core/tests/TestComponent.h`
`using ViewProps::ViewProps;` was causing problems in Visual Studio 2017 - changing it to ` TestProps() = default;` allowed dependent CPP files to compile on MSVC and caused no regressions in the Fabric test suite.
## Changelog
Changelog: [Internal] [Changed] - Fabric Tests: Change default constructor in TestComponents' TestProps
Pull Request resolved: https://github.com/facebook/react-native/pull/29898
Test Plan: The Fabric test suite passes on Windows after this change is made. I also tested it under macOS and Linux built with Clang and they both pass with this change made.
Reviewed By: sammy-SC
Differential Revision: D23591986
Pulled By: shergin
fbshipit-source-id: 132e1c2e38fa74aa4f2c8746054d6152f30035e9
Summary:
This pull request adds an include statement including the header `<algorithm>` to `react/renderer/graphics/Rect.h`.
It is needed for building with MSVC because of the use of the `std::max` function. It seems that this header is included as a side effect of some other header that's being used when building with Clang, and for some reason it is not included when being built with MSVC.
## Changelog
Changelog: [Internal][Added] - Fabric: Explicitly include algorithm in Rect.h
Pull Request resolved: https://github.com/facebook/react-native/pull/29884
Test Plan: Files that include this header now successfully compile on MSVC.
Reviewed By: sammy-SC
Differential Revision: D23591910
Pulled By: shergin
fbshipit-source-id: d8367bbdd94bc66c05d8fa308ed46c7aca24a68a
Summary:
The standard says that zIndex should only be defined for non-`static` positioned views. This diff implements it.
For now, it actually enables zIndex for all views in RN because there is no way to specify `position: static` but we will give that ability by changing Flow definitions in future diffs in a couple of weeks (to ensure OTA safety).
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D23559447
fbshipit-source-id: 20ea10c9349de2c5b1adea5735324a8f57150695
Summary:
Fabric Core does not support exception safety, so technically exceptions are unrecoverable and binaries should be built without exception support.
While it's not the case, some exceptions might bubble to JavaScript realm and be caught as JavaScript exceptions. In this case, we lose the stack trace. To prevent that and to investigate one particular error we have we add this try-catch block in this diff.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D23529233
fbshipit-source-id: 7ac7fb26ac08ad26af8790172de471ac178c3a37
Summary:
Shadow tree introspection was disabled for a while, now we need it back working. This diff also restructures the logic of `MountingCoordinator::pullTransaction()` splitting it into two sections, first one for the base case and the second for the overriding case.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D23374948
fbshipit-source-id: 0b5f1c598975bceb3dcb6a0eaee67ff58ef9dda1
Summary:
Just renaming, nothing more.
The idea of MountingTelemetry already grown to something bigger than just mounting telemetry, so we are renaming it.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D23374947
fbshipit-source-id: f60ce38b75d1ce77498b84688e59598314c69a78
Summary:
This diff fixes the initial render of RN Android TextInput. The problem was that "attributedString" was not serialized until an event was sent from native side.
Changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D23383969
fbshipit-source-id: 86601434b1fbaa9f712bdb79b013a1d004bc55a4
Summary:
My goal in this diff is to make LayoutAnimations more stable, and more resilient to challenging situations. Namely, LayoutAnimations already works fine with:
1) Reorders
2) Flattening/unflattening
3) Deletion and recreation of the same hierarchy
4) Updates conflicting with an existing animation
However, what if /all/ of those things are combined? Handling update conflicts with multiple ongoing animations, repeatedly flattening/unflattening the same layer of hierarchy and reordering both parents and children, etc .
This diff does not make LayoutAnimations perfect, but it does make LayoutAnimations much more resilient to situations it was not able to handle before.
My primary method of testing was to use two Playground examples: one just repeatedly queues up mutations (some animated, some not) that create, update, and delete in the same hierarchy layer. The second, more complex one, mutates between random view hierarchies that involve a lot of flattening and unflattening as well as reordering, over a depth of 5. It also exercises animations over TextInlineViews, which is more challenging.
LayoutAnimations works best with the new "Flattening Differ" for now, because the Flattening Differ produces a much smaller, nearly minimal set of instructions in cases of flattening-unflattening. I would like that to not be a hard requirement for using LayoutAnimations, but it's a good starting-point for now.
As part of this work, I also developed a lot of debugging and logging mechanisms that are handy for detecting inconsistencies and debugging crashes. Some are included in this diff behind `#define` statements that are disabled by default, and the rest will be published separately and likely cannot be landed permanently, as they're more invasive changes that are only helpful in debugging.
# Followups:
- Automate testing: write a suite of C++ tests that mutates between random diffs and guarantees that all mutations in a StubViewTree are sensible
- Construct a set of minimal repros that catalogues all remaining crashes and inconsistency issues (these seem to be extremely marginal cases and are very hard to repro - so I think it's fine to run this in prod for now, but I will follow-up as soon as I'm back to catalogue and fix all remaining issues)
- This diff focuses on *not crashing*, but it is still possible to construct a sequence of complex mutations that results in (for example) views having some opacity between 0 and 1 if animations are interrupted repeatedly. Although this is easy enough to prevent in product code - the types of scenarios I'm running in tests are very unlikely to ever happen in production - it would be nice to be *sure* that LayoutAnimations will always converge to a sensible View hierarchy with up-to-date props.
- In general, the index adjustment logic is complicated. I don't know if there's a great way around it, so I need to at least catalogue and test all edge-cases as mentioned above.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D23382975
fbshipit-source-id: f379d9aa2a4b9c33fa2ba8fa07870c9e31fad5e7
Summary:
Now we store a revision number of a Shadow Tree that leads to a transaction for which the concrete instance of MountingTelemetry corresponds. This is useful to understand how many actual transactions were skipped during a mounting phase (a mounting transaction does not directly correspond to a commit operation).
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D23364663
fbshipit-source-id: 32b86bcdfc1ae97d8fff3b97a8615cc5a5b4d4a9
Summary:
With this change, we now collect the number of text measurements that we perform during the layout phase of the commit. Text measurements are the most expensive layout operations which pretty much responsible for the vast majority of time spent in the layout phase.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D23364664
fbshipit-source-id: 19514b93166b4053c2f3be37e79507f2c5248000
Summary:
Microsoft’s RN for macOS fork supports the Hermes engine nowadays https://github.com/microsoft/react-native-macos/pull/473. As a longer term work item, we’ve started moving bits that are not invasive for iOS but _are_ a maintenance burden on us—mostly when merging—upstream. Seeing as this one is a recent addition, it seemed like a good candidate to start with.
As to the actual changes, these include:
* Sharing Android’s Hermes executor with the objc side of the codebase.
* Adding a CocoaPods subspec to build the Hermes inspector source and its dependencies (`Folly/Futures`, `libevent`).
* Adding the bits to the Xcode build phase script that creates the JS bundle for release builds to compile Hermes bytecode and source-maps…
* …coincidentally it turns out that the Xcode build phase script did _not_ by default output source-maps for iOS, which is now fixed too.
All of the Hermes bits are automatically enabled, on macOS, when providing the `hermes-engine-darwin` [npm package](https://www.npmjs.com/package/hermes-engine-darwin) and enabling the Hermes pods.
## Changelog
[General] [Added] - Upstream RN macOS Hermes integration bits
Pull Request resolved: https://github.com/facebook/react-native/pull/29748
Test Plan:
Building RNTester for iOS and Android still works as before.
To test the actual changes themselves, you’ll have to use the macOS target in RNTester in the macOS fork, or create a new application from `master`:
<img width="812" alt="Screenshot 2020-08-18 at 16 55 06" src="https://user-images.githubusercontent.com/2320/90547606-160f6480-e18c-11ea-9a98-edbbaa755800.png">
Reviewed By: TheSavior
Differential Revision: D23304618
Pulled By: fkgozali
fbshipit-source-id: 4ef0e0f60d909f3c59f9cfc87c667189df656a3b
Summary:
I noticed when porting my iOS app to macOS via Catalyst that the text rendering was somewhat different on the two platforms. Text looked blurry and over-weight on macOS, even when disabling the Catalyst scaling transform.
I hazily remembered that I'd seen this problem before in my old Cocoa development days: this kind of blurring occurs when rendering text with sub-pixel anti-aliasing into an offscreen buffer which will then be traditionally composited, because when the SPAA algorithm attempts to blend with the underlying content (i.e. in the offscreen buffer), there isn't any. SPAA is disabled on iOS, so the issue wouldn't appear there. On macOS, typical approachs to displaying text (e.g. `CATextLayer`) normally disable SPAA, since it's been incompatible with the platform's compositing strategy since the transition to layer-backed views some years ago. But React Native uses `NSLayoutManager` to rasterize text (rather than relying on the system render server via `CATextLayer`), and that class doesn't touch the context's font smoothing bit before drawing.
This change makes macOS/Catalyst text rendering consistent with iOS text rendering by disabling SPAA.
It appears that the code I've modified is in the process of being refactored (for Fabric?). It looks like [this](https://github.com/facebook/react-native/blob/8d6b41e9bcede07fb627d57cf6c11050ae590d57/ReactCommon/react/renderer/textlayoutmanager/platform/ios/RCTTextLayoutManager.mm#L111) is the corresponding place in the new code (sammy-SC, is that right?). I'm happy to include a change to the new renderer in this patch if someone can point me at how to test that change.
## Changelog
[iOS] [Fixed] - Improved text rendering on macOS Catalyst
Pull Request resolved: https://github.com/facebook/react-native/pull/29609
Test Plan:
1. Prepare RNTester for running on macOS (or apply [this patch](https://gist.github.com/andymatuschak/d0f5b4fc1a28efc4f860801aa1deddcd) to handle parts 1 and 2, but you'll still need to do part 3):
1. Open the workspace, navigate to the `RNTester` target's configuration, and check the "Mac" checkbox under "Deployment Info.
2. Flipper doesn't yet compile for Catalyst (https://github.com/facebook/react-native/issues/27845), so you must disable it by: a) commenting out `use_flipper!` and `flipper_post_install` in the Podfile, then running `pod install`; and b) removing the `FB_SONARKIT_ENABLED` preprocessor flags in the Xcode project.
3. macOS has different signing rules from iOS; you must set a development team in the "Signing & Capabilities" tab of the `RNTester` target configuration pane. Unfortunately, you must also do this in the `Pods` project for the `React-Core-AccessibilityResources` target ([this is an issue which CocoaPods must fix](https://github.com/CocoaPods/CocoaPods/issues/8891)).
2. Run RNTester with and without the patch. You'll see that the font hinting is overweight without the patch; see screenshots below (incorrect rendering above, correct rendering below; note that fonts still remain slightly blurred because of Catalyst's window scaling transform, but that's removed on Big Sur).
Reviewed By: PeteTheHeat
Differential Revision: D23344751
Pulled By: sammy-SC
fbshipit-source-id: 1bbf682b681e381a8a90e152245d9b0df8ec7697
Summary:
Changelog: [internal]
According to React Native docs, the default value for `autoCapitalize` is `sentences`.
Fabric's TextInput default value does not align with this.
[Source](https://reactnative.dev/docs/textinput#autocapitalize)
Reviewed By: JoshuaGross
Differential Revision: D23344479
fbshipit-source-id: f9e6f2aa6e1fbba2b08cb4aff23b842e49fa8c21
Summary:
This diff deletes the activityindicator buck module because these files are created by the code gen.
changelog: [internal]
Reviewed By: PeteTheHeat
Differential Revision: D23227860
fbshipit-source-id: 133315a44c9181be3263e0c6227884ed15487c1f
Summary:
# What is this?
For a very long time, we've discussed the possibility of detecting Node Reparenting in the Fabric Differ. Practically, from the developer perspective, ReactJS and React Native do not allow reparenting: nodes cannot be reparented, only deleted and then recreated with entirely new tags.
However, Fabric introduced the idea of View Flattening where views deemed unnecessary would be removed from the View hierarchy entirely. This is great and improves memory usage, except for one issue: if a View becomes unflattened, or becomes flattened, the entire tree underneath it must be rebuilt.
In a past diff we introduced a mechanism to detect sibling reordering cleverly, and produce a minimal instruction set. This diff is very similar: we know the invariants around flattening and unflattening of views and we take advantage of them to produce an optimal set of instructions efficiently.
# What's different from previous attempts?
No global maps! Those are slow!
This seems to work and (hopefully) might even improve performance, since way less work is being done on the UI thread in cases when views are (un)flattened.
This *only* does extra work when flattening/unflattening happens, which gives product engineers a little more control over perf.
# So, how's it work?
This algorithm is intuitively simple (I think) but tricky to pull off, because there are lots of edge-cases.
In short: In the past, that information was hidden from the Differ: the differ didn't know if views were being reparented, it would see them
as entirely new views or as views being deleted if a View was flattened or unflattened. We very subtly change the information given to the differ:
all nodes are visible to the differ, but marked as Flattened or Unflattened. Thus, when the differ compares two nodes in the "old" and "new" tree,
it can tell not just if there are updates to the node but if it has been unflattened or flattened as well.
For example, take this tree, where * indicates that a View is flattened:
```
A
+
+----+---+
B* X
+ +
| |
+---+--+ +
E F Y
```
When the Differ asks for the children of A, in the past it would get a list `[E, F, X]`. That is, B* and X are both its children, but since B is flattened, it is omitted entirely from the list and
its children are substituted.
Now, when the Differ asks for the children of A, we give it this list instead: `[B*, E, F, X]`. That is: we give it a list which includes B, but B is marked as flattened.
Another wrinkle: A node `X` could have its children flattened, but still be a concrete view: so flattening/unflattening is a different operation from making a view "concrete" or "unconcrete", which can change independently of flattening.
There is one additional wrinkle: because of zIndex/stacking order, the children of `B` might not actually appear after `B` in the list. Depending on zIndex, a tree that looks like this:
```
A
+
+------+------+
B* C*
+ +
| |
+--+--+ +--+--+
D E F G
```
Could actually be linearized as: `[D G B* F C* E]` (as an extreme example; but basically all permutations as possible).
This is the reason, and the *only* reason that the inner Flattener/Unflattener
## The cases we need to handle
There are 7 cases/edge-cases of flattening and unflattening that we need to handle. Practically, all cases of reordering + flattening/unflattening, and taking recursive cases into account:
1. View A and A' (A in the old tree, A' in the new tree) are matched in the differ, and A* has been flattened or unflattened. These two cases are the easiest to handle.
2. View A' has been reordered with its siblings, and has been flattened or unflattened. These cases are slightly trickier to handle.
3. While flattening or unflattening, we encounter a child that has also been unflattened or flattened. So we need to handle four cases here in total: Flatten-Flatten, Flatten-Unflatten, Unflatten-Flatten, and Unflatten-Unflatten.
Other things to think about, also covered above:
1. Ordering. Views can be reordered and flattened/unflattened at the same time.
2. zIndex ordering: children in a certain order from the ShadowNode perspective may be stacked differently from a View perspective. We use the zIndex ordering for everything in the differ, and this prevents us from performing certain optimizations (see above: we cannot assume that children come after their parent in a list; they may come before, may be interwoven with children from other parents, etc).
# Perf Implications?
Practically, there should be very little negative overhead. There is some overhead in actually performing a flattening/unflattening operation, but... not much more than before. We don't use global maps, so the cost of flattening/unflattening is basically `O(number of nodes reparented)` - note that that's direct nodes reparented, *not* descendants.
tl;dr the perf hit should be similar to reordering, which is non-zero, but close to zero, and zero-cost for any diff operations on parts of the tree that don't involve flattening/unflattening. AFAICT this is very close to an ideal solution for that reason (but I wish it was simpler overall).
# In Summary?
I hope this works out and I think it could improve a number of things downstream: perf, LayoutAnimations, Bindings, certain crashes because of platform assumptions about mutations, etc.
Is it worth it? This new implementation is substantially harder to reason about, harder to read, and harder to understand. This is an important consideration. All I can say there is that I trust the test suite I've been using, but
the decreased readability is a big negative. Hopefully we can improve this in the future.
The rest is fiddly implementation details that I sincerely hope can be improved and simplified in the future.
# Followups?
The part that makes this algorithm the most expensive is that because of zIndex ordering, we cannot assume that children are linearized after their parents and so we rely more heavily on maps for the flattening/unflattening. Our TinyMap implementation should make these `find` operations fast enough unless trees' children are constantly being reordered, but it's still worth thinking of ways to make this even faster.
Changelog: [Internal]
Reviewed By: shergin, mdvacca
Differential Revision: D23259341
fbshipit-source-id: 35d9b90caf262d601a31996ea2cb37e329c61ffc
Summary:
1. When testing major changes to the differ, it can be useful to have more verbose logging.
2. On Android, since asserts don't fire yet, I log which asserts are failing.
Should have no impact on any builds unless you manually set the macro here, and it will have no impact on production builds regardless.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D23257859
fbshipit-source-id: 94a8e74ece8023064de0f2203db6074975f8f1f0
Summary:
Simplify the TextInput measurement mechanism.
Now, data only flows from JS->C++->Java and from Java->JS. C++ passes along AttributedStrings from JS if JS updates, and otherwise Java maintains the only source of truth.
Previously we tried to keep all three in sync. This was complicated, slow, and even lead to some crashes.
This feels a bit hacky but I believe it's the simplest way to achieve this short-term. Ideally, we would use something like `AttributedStringBox` and pass that to State from Java,
but currently everything passed through the State system from Java must be serializable as `folly::dynamic`. So, instead, we just cache one Spannable per TextInput component and
use ReactTag as the cache identifier for lookup.
An interesting side-effect is that `measure` could race with TextInput updates, but the race condition favors measuring the latest text, not outdated values.
Followups:
- Can we do this without copying the EditText Spannable on every keystroke? Maybe this approach is too aggressive, but I don't want a background thread measuring a Spannable as it's being mutated.
- Do we need to support measuring Attachments?
- How can we clean up this API? It should work for now, but feels a little hacky.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D23290230
fbshipit-source-id: 832d2f397d30dfb17b77958af970d9c52a37e88b
Summary:
Changelog: [internal]
# What is Teller?
Teller is a bank's employee who deals with the customer on behalf of the bank. In Fabric's scenario it is a class that on behalf of the view deals with State.
# Why do we need it?
Dealing with `ConcreteState` can be complicated and patterns are often repeated among different component views. `ConcreteStateTeller` aims to resolve these issues. Examples:
- You can call teller's methods without checking for nullptr (we have had crashes because of this before).
- Methods are save to be called on any thread.
- Mechanism to retry state update if it fails is built in.
It is designed to be used from ComponentView so views don't have to talk directly to `ConcreteState`.
Reviewed By: JoshuaGross, shergin
Differential Revision: D23216865
fbshipit-source-id: 90a50702e036eac084f89743ebab687a67182dc0
Summary:
Changelog: [Internal]
If `ConcreteShadowNode::setStateData` is called and the node is cloned before it is mounted, the cloned node will have old state before `setStateData` was called.
To solve this, simply call `setMostRecentState` on the family inside `ConcreteShadowNode::setStateData`.
Reviewed By: JoshuaGross
Differential Revision: D23283560
fbshipit-source-id: f9822fb69e4234f776d512fc02fe13ea7de64897
Summary:
This diff moves AndroidSwitch C++ files to make them compatible with RN OSS Build
Changelog: [Internal] internal
Reviewed By: PeteTheHeat
Differential Revision: D23227861
fbshipit-source-id: 8f23c2eb266a47cb9af82f4159f64b987c14141b
Summary:
This diff moves Android Slider C++ files to make them compatible with RN Tester OSS build
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D23227862
fbshipit-source-id: 7a5ed1bdc03cbe715467eddd4aad9af82761d4f0
Summary:
This diff moves AndroidDialogPicker C++ files to make it compatible with RN OSS build system.
changelog: [internal]
Reviewed By: fkgozali
Differential Revision: D23198857
fbshipit-source-id: 8fd4b7081f5f573946a6b9cd3fdc408488f91e13
Summary:
This diff updates the directory hierarchy of AndroidTextInput C++ files to be compatible with Android OSS build system
changelog: [internal] Internal
Reviewed By: PeteTheHeat
Differential Revision: D23179390
fbshipit-source-id: 1c52e4f882853799a58d44876cadd392b4a35050
Summary:
`accessibilityElementsHidden` property hides a subtree starting from the node with this prop from VoiceOver and TalkBack, so the node with this prop should form the stacking context (otherwise the children will mount as siblings and the property will not have an effect).
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross
Differential Revision: D23202220
fbshipit-source-id: d87dfab5f791219551c1eb90cdf3b3fa86f9c51f
Summary:
This diff flattens the hierarchy of Text C++ files.
This is required to integrate Text into RN OSS
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D23173556
fbshipit-source-id: ce90000cae147460aa4ddad55b7c90369fa734a2
Summary:
Partial backout of D23123575 (https://github.com/facebook/react-native/commit/1e4d8d902daca8e524ba67fc3c1f4b77698c4d08). It's causing some crashes and there is a more efficient way of doing it, which I will land in a future diff.
Leaving unused feature-flags in place for now, they'll be used shortly.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D23198625
fbshipit-source-id: 6e9cbc6b39898a604b8f4dfccf5a6dd238511a68
Summary:
Logbox inside bridgeless surfaces was crashing and not able to open, this diff should fix it.
Changelog: [Internal]
Reviewed By: PeteTheHeat
Differential Revision: D23043773
fbshipit-source-id: 6e584f97e53e626b9bfedd6dc997ba6ba8c08b8f
Valentin Shergin
empyrical
David Vacca
Joshua Gross
Eloy Durán
Andy Matuschak
Samuel Susla
generatedunixname89002005287564@sandcastle995.atn2.facebook.com
Stephy Ma
Lulu Wu
generatedunixname89002005287564
