Implement potential fix for mounting errors during synchronous state updates #44015
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D55942125 |
…updates (facebook#44015) Summary: Changelog: [internal] ## Context When we introduced synchronous state updates in Fabric, we saw some crashes on coming from the mounting layer on Android. It seems some of these crashes are caused by nested mount operations. When we're mounting some views, like [scroll views](https://github.com/facebook/react-native/blob/881c0bc8970b9e402df6b4f87e1759b238b24735/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java#L383), we dispatch state updates that end up doing more mutations. When we were doing these state updates asynchronously, all the original mutations were processed before these updates, but now that we do them synchronously, the mutations are interleaved causing errors. ## Changes This introduces a new flag that will force all the mutations going through `MountItemExecutor` in `FabricUIManager` to be batched instead of executed synchronously. This fixes the issues I saw locally and I'm expecting this will unblock synchronous state updates in production. Potentially, this might fix other crashes we've been seeing with a low frequency. Differential Revision: D55942125
rubennorte
force-pushed
the
export-D55942125
branch
from
a291d0d to
a5cb39a
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55942125 |
Base commit: 27ef6cd |
|
This pull request has been merged in b9a2249. |
|
This pull request was successfully merged by @rubennorte in b9a2249. When will my fix make it into a release? | How to file a pick request? |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 8, 2024
…updates (v2) Summary: This is a new attempt at fixing mounting errors during synchronous state updates after what we tried in facebook#44015. That fix didn't work because `dispatchMountItems` actually makes a copy of the mount items that it's going to process, so when we added the mount items to the list they were actually not being picked up by the current processing. This changes the fix to expose a new list of mount items being processed, and if that's defined then we add it to the list. Otherwise we process as usual. Differential Revision: D57107212
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 8, 2024
…updates (v2) (facebook#44492) Summary: Changelog: [internal] This is a new attempt at fixing mounting errors during synchronous state updates after what we tried in facebook#44015. That fix didn't work because `dispatchMountItems` actually makes a copy of the mount items that it's going to process, so when we added the mount items to the list they were actually not being picked up by the current processing. This changes the fix to expose a new list of mount items being processed, and if that's defined then we add it to the list. Otherwise we process as usual. Reviewed By: sammy-SC Differential Revision: D57107212
facebook-github-bot
pushed a commit
that referenced
this pull request
May 8, 2024
…updates (v2) Summary: Changelog: [internal] This is a new attempt at fixing mounting errors during synchronous state updates after what we tried in #44015. That fix didn't work because `dispatchMountItems` actually makes a copy of the mount items that it's going to process, so when we added the mount items to the list they were actually not being picked up by the current processing. This changes the fix to call `dispatchMountItems` as many times as needed, while there are mount items to process in the list. Reviewed By: sammy-SC Differential Revision: D57107212 fbshipit-source-id: 46988a71daae15d70399258f850653046d0790ff
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 30, 2024
Summary: Changelog: [internal] ## Context We ran an experiment to test synchronous state updates in Fabric and we saw some crashes on Android. Those crashes were caused by mounting operations not being applied in the correct order. There were 2 root causes for that problem: 1. State updates triggered during mount would be committed and mounted synchronously during that specific mount operation. That caused problems like trying to clip views that weren't created already (as we were processing the state update for the content offset before we actually created the child views). 2. Same problem as before, but with mount operations that were processed when the root view wasn't available yet (this is a separate queue). We tried to fix the problem in facebook#44015, but the solution for 2) was incorrect, as we didn't account for those operations being in a different queue (it was reverted in facebook#44724). ## Changes I think the right solution for point 2) is that, instead of marking the root view as available and then process all pending operations, we flip those operations. That was, if there are any mount operations as a side-effect of processing that queue, those will also be added to the same queue, instead of being processed immediately in `MountItemDispatcher`. Differential Revision: D57968937
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 30, 2024
…cebook#44726) Summary: Changelog: [internal] ## Context We ran an experiment to test synchronous state updates in Fabric and we saw some crashes on Android. Those crashes were caused by mounting operations not being applied in the correct order. There were 2 root causes for that problem: 1. State updates triggered during mount would be committed and mounted synchronously during that specific mount operation. That caused problems like trying to clip views that weren't created already (as we were processing the state update for the content offset before we actually created the child views). 2. Same problem as before, but with mount operations that were processed when the root view wasn't available yet (this is a separate queue). We tried to fix the problem in facebook#44015, but the solution for 2) was incorrect, as we didn't account for those operations being in a different queue (it was reverted in facebook#44724). ## Changes I think the right solution for point 2) is that, instead of marking the root view as available and then process all pending operations, we flip those operations. That was, if there are any mount operations as a side-effect of processing that queue, those will also be added to the same queue, instead of being processed immediately in `MountItemDispatcher`. Differential Revision: D57968937
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 30, 2024
…cebook#44726) Summary: Changelog: [internal] ## Context We ran an experiment to test synchronous state updates in Fabric and we saw some crashes on Android. Those crashes were caused by mounting operations not being applied in the correct order. There were 2 root causes for that problem: 1. State updates triggered during mount would be committed and mounted synchronously during that specific mount operation. That caused problems like trying to clip views that weren't created already (as we were processing the state update for the content offset before we actually created the child views). 2. Same problem as before, but with mount operations that were processed when the root view wasn't available yet (this is a separate queue). We tried to fix the problem in facebook#44015, but the solution for 2) was incorrect, as we didn't account for those operations being in a different queue (it was reverted in facebook#44724). ## Changes I think the right solution for point 2) is that, instead of marking the root view as available and then process all pending operations, we flip those operations. That was, if there are any mount operations as a side-effect of processing that queue, those will also be added to the same queue, instead of being processed immediately in `MountItemDispatcher`. Reviewed By: sammy-SC Differential Revision: D57968937
facebook-github-bot
pushed a commit
that referenced
this pull request
May 30, 2024
…4726) Summary: Pull Request resolved: #44726 Changelog: [internal] ## Context We ran an experiment to test synchronous state updates in Fabric and we saw some crashes on Android. Those crashes were caused by mounting operations not being applied in the correct order. There were 2 root causes for that problem: 1. State updates triggered during mount would be committed and mounted synchronously during that specific mount operation. That caused problems like trying to clip views that weren't created already (as we were processing the state update for the content offset before we actually created the child views). 2. Same problem as before, but with mount operations that were processed when the root view wasn't available yet (this is a separate queue). We tried to fix the problem in #44015, but the solution for 2) was incorrect, as we didn't account for those operations being in a different queue (it was reverted in #44724). ## Changes I think the right solution for point 2) is that, instead of marking the root view as available and then process all pending operations, we flip those operations. That was, if there are any mount operations as a side-effect of processing that queue, those will also be added to the same queue, instead of being processed immediately in `MountItemDispatcher`. Reviewed By: sammy-SC Differential Revision: D57968937 fbshipit-source-id: 93d10cdeced0c837d4301768aee8575d2c940b10
kosmydel
pushed a commit
to kosmydel/react-native
that referenced
this pull request
Jun 11, 2024
…updates (v2) Summary: Changelog: [internal] This is a new attempt at fixing mounting errors during synchronous state updates after what we tried in facebook#44015. That fix didn't work because `dispatchMountItems` actually makes a copy of the mount items that it's going to process, so when we added the mount items to the list they were actually not being picked up by the current processing. This changes the fix to call `dispatchMountItems` as many times as needed, while there are mount items to process in the list. Reviewed By: sammy-SC Differential Revision: D57107212 fbshipit-source-id: 46988a71daae15d70399258f850653046d0790ff
kosmydel
pushed a commit
to kosmydel/react-native
that referenced
this pull request
Jun 11, 2024
…cebook#44726) Summary: Pull Request resolved: facebook#44726 Changelog: [internal] ## Context We ran an experiment to test synchronous state updates in Fabric and we saw some crashes on Android. Those crashes were caused by mounting operations not being applied in the correct order. There were 2 root causes for that problem: 1. State updates triggered during mount would be committed and mounted synchronously during that specific mount operation. That caused problems like trying to clip views that weren't created already (as we were processing the state update for the content offset before we actually created the child views). 2. Same problem as before, but with mount operations that were processed when the root view wasn't available yet (this is a separate queue). We tried to fix the problem in facebook#44015, but the solution for 2) was incorrect, as we didn't account for those operations being in a different queue (it was reverted in facebook#44724). ## Changes I think the right solution for point 2) is that, instead of marking the root view as available and then process all pending operations, we flip those operations. That was, if there are any mount operations as a side-effect of processing that queue, those will also be added to the same queue, instead of being processed immediately in `MountItemDispatcher`. Reviewed By: sammy-SC Differential Revision: D57968937 fbshipit-source-id: 93d10cdeced0c837d4301768aee8575d2c940b10
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
Jul 3, 2024
…ng of mount items on Android Summary: Changelog: [internal] We added a flag to fix some issues when committing state updates synchronously from the main thread in facebook#44015 but that implementation was incorrectly not invoking item dispatch listeners after mount. This adds the missing logic so we can unblock shipping sync state updates. Differential Revision: D59319230
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
Jul 3, 2024
…ng of mount items on Android (facebook#45264) Summary: Pull Request resolved: facebook#45264 Changelog: [internal] We added a flag to fix some issues when committing state updates synchronously from the main thread in facebook#44015 but that implementation was incorrectly not invoking item dispatch listeners after mount. This adds the missing logic so we can unblock shipping sync state updates. Reviewed By: javache Differential Revision: D59319230
facebook-github-bot
pushed a commit
that referenced
this pull request
Jul 3, 2024
…ng of mount items on Android (#45264) Summary: Pull Request resolved: #45264 Changelog: [internal] We added a flag to fix some issues when committing state updates synchronously from the main thread in #44015 but that implementation was incorrectly not invoking item dispatch listeners after mount. This adds the missing logic so we can unblock shipping sync state updates. Reviewed By: javache Differential Revision: D59319230 fbshipit-source-id: b0ab7e7c79a3315ef29dbb024e62c10444192509
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Changelog: [internal]
Context
When we introduced synchronous state updates in Fabric, we saw some crashes on coming from the mounting layer on Android.
It seems some of these crashes are caused by nested mount operations. When we're mounting some views, like scroll views, we dispatch state updates that end up doing more mutations. When we were doing these state updates asynchronously, all the original mutations were processed before these updates, but now that we do them synchronously, the mutations are interleaved causing errors.
Changes
This introduces a new flag that will force all the mutations going through
MountItemExecutorinFabricUIManagerto be batched instead of executed synchronously. This fixes the issues I saw locally and I'm expecting this will unblock synchronous state updates in production.Potentially, this might fix other crashes we've been seeing with a low frequency.
Differential Revision: D55942125