Summary:
I spent the last several days thinking about state reconciliation issues, some crashes (T65586949) that suspiciously happen somewhere inside, and a bunch of issues that might be connected to that (possibly, some of T65516263 sub-task).
I cannot see some obvious problems in the current state reconciliation algorithm that might cause the crash (because of some use-after-free or other pure C++ issues), but I suspect some of the problems we experience might be caused by some details of how we reconcile states.
In the current approach, we rank all states based on the "hierarchical" history of their creation (state version is being calculated based on the version of the base tree). That's usually fine but in some cases when trees are being constructed concurrently, a logical version of a based tree does not correspond to the local version of a committed tree. In other words, the linear history of commits does not always correspond to the "hierarchical" history of trees generation that was done by different parties (e.g. React vs native state update pipeline).
In this diff, I tried to change the approach to change the algorithm to follow this logic: If some state is `obsolete` (already been committed and then replaced with newer one), we replace that with the most recent one. This change does not introduce the `obsolete` flag; is already used by State infra to avoid cloning nodes with an outdated state.
Interestingly, it fixes the issue with an empty BottomSheet on Android (T66177144). See the attached video. The hope is that it's also will
This change theoretically might affect all things that use State, so it hard to predict what can break and how. So, if we don't see obvious problems here, I would set up a GK/QE and run the experiment in prod.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross
Differential Revision: D21295137
fbshipit-source-id: e5613218d3e11a56623cab9bbf2540495b2b24e8
Summary:
This change removes the concept of `StateTarget`, replacing its role with `ShadowNodeFamily`.
When `StateTarget` was built, we didn't have a concept of `Family`, and when we added it we introduced a retain-cycle: ShadowNode -> Family -> StateTarget -> ShadowNode. This diff fixes that.
This change does not change conceptually how the state behaves, it just adjusts internal machinery.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D19799013
fbshipit-source-id: c1360bfbf6b8ac34e2a856a40047eafeb50ed070
Summary:
Before this change, the concrete component-specific data/payload object associated with a State was stored inside a templated subclass as a normal instance variable; after the change, it's stored as a shared pointer inside the base class. The original motivation was that storing that inside subclass saves us one shared pointer and one heap allocation.
This approach overcomplicated a lot of things and all possible savings are probably compensated with additional complexity (we have to have templated state-update lamdas in subclasses and so on). And to update the data in the previous approach we need to create a shared pointer to data anyway.
This change will allow future improvements in the coming diff.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross
Differential Revision: D19799014
fbshipit-source-id: 287ed939353ba58d9e434d1502ecfbb208c6daa5
Summary:
Changelog: [internal]
Merges all of responsibilities of `StateCoordinator` into `ShadowNodeFamily`.
Reviewed By: shergin
Differential Revision: D19500104
fbshipit-source-id: f31ffded5a840e722fd898eef6a9f52cd2186df7
Summary:
Changelog: [internal]
1. Moves `ShadowNode::getAncestors` to `ShadowNodeFamily`.
2. Exposes shadowNode's family through `ShadowNode::getFamily()`.
# Why?
This is a first step in order to merge `StateCoordinator` into `ShadowNodeFamily` and use it as target for state updates.
Reviewed By: shergin
Differential Revision: D19465188
fbshipit-source-id: b5a3625aa21c040301259de02beedbf97e11f20e
Summary:
This diff introduces a revision concept to a State object. Every newly created State object gets a revision number which equals a source state object revision number plus one.
This functionality is useful to detect which state object is newer.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross
Differential Revision: D19467162
fbshipit-source-id: fc5e165193009c9818bf124bd4df7021288b2389
Summary:
Why crash in production if we can make potential bug not compile?
Changelog: [INTERNAL]
Reviewed By: sammy-SC
Differential Revision: D18499813
fbshipit-source-id: dc2d7ac12b8a2ae27d1e6b832fccc1555cab3b75
Summary:
`StateTarget` no longer uses `shared_from_this`, this allows us to remove need for `enable_shared_from_this`
I decided to put `state->commit` call inside `ShadowTree.cpp` because I needed to have access to `shared_ptr` of shadow node from outside of the class itself.
`state->commit` was originally designed to be only called by `ShadowNode` but this does not seem to be the case anymore since it is called from `UIManager` as well.
changelog: [internal]
Reviewed By: shergin
Differential Revision: D18032532
fbshipit-source-id: 75c874fd04f86adc07bfddbef3a0384e17c2067b
Summary: That was always part of design, that's okay to get a newer state from any other state. We will need it in the future diffs.
Reviewed By: sammy-SC
Differential Revision: D17053429
fbshipit-source-id: 2174e7d6e3a1ed231f7f6e238d216d0b09ec8797
Summary: `Target` inside the Stage can be empty; in this case, we should not try to extract `ShadowNode` from it.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D15982479
fbshipit-source-id: 83a4bebadc88b59d7fe77acbdf07e8ce9f2f6be1
Summary: Returning a shared pointer by const reference in this context is not correct/safe because the object (the ShadowNode) doesn't own the object, so the caller cannot reason about the lifetime (esp. in a multithreaded environment).
Reviewed By: mdvacca
Differential Revision: D15958737
fbshipit-source-id: 8f03e6530d07d63ece5f955055c5c67c204b8223
Summary: A couple of new methods in ConcreteShadowNode allows us to deal with State in more LocalData-like manner.
Reviewed By: mdvacca
Differential Revision: D15323686
fbshipit-source-id: ede4aa1f1d0ad6f876bd963e57a00a0ad470c1c0
Summary: Small changes to State objects to support Android. See following diffs.
Reviewed By: mdvacca
Differential Revision: D14663470
fbshipit-source-id: 878f4dc39265991a7b8ff54ca80bdb862f1dd3de
Summary:
In React Native there are several use cases where React State is not the only input that affects the component tree. E.g., in case of a <Modal> component, the screen size directly affects the layout of components inside modal; in the case of uncontrolled <TextInput> component, the text inside the input affects the layout of the surrounding components. `State` is a special (legit!) workaround for all those similar cases. Native part of React Native maintains a special shared object between all nodes of the same family and use that during cloning and commits.
See coming commits to know how to use that.
In the near future State will fully replace LocalData concept but for simplicity they both exist for now.
Reviewed By: mdvacca
Differential Revision: D14217184
fbshipit-source-id: 6e018c5b68208d662462013bce0f4e2733d2f673
Valentin Shergin
Samuel Susla
Andres Suarez
Joshua Gross