Bugfix: memo drops lower pri updates on bail out
#18091
Merged
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
Fixes a bug where lower priority updates on a components wrapped with `memo` are sometimes left dangling in the queue without ever being processed, if they are preceded by a higher priority bailout. Cause ----- The pending update priority field is cleared at the beginning of `beginWork`. If there is remaining work at a lower priority level, it's expected that it will be accumulated on the work-in-progress fiber during the begin phase. There's an exception where this assumption doesn't hold: SimpleMemoComponent contains a bailout that occurs *before* the component is evaluated and the update queues are processed, which means we don't accumulate the next priority level. When we complete the fiber, the work loop is left to believe that there's no remaining work. Mitigation ---------- Since this only happens in a single case, a late bailout in SimpleMemoComponent, I've mitigated the bug in that code path by restoring the original update priority from the current fiber. This same case does not apply to MemoComponent, because MemoComponent fibers do not contain hooks or update queues; rather, they wrap around an inner fiber that may contain those. However, I've added a test case for MemoComponent to protect against a possible future regression. Possible next steps ------------------- We should consider moving the update priority assignment in `beginWork` out of the common path and into each branch, to avoid similar bugs in the future.
facebook-github-bot
added
CLA Signed
React Core Team
Details of bundled changes.Comparing: ea6ed3d...0da64c2 react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 🔺+0.1% Size changes (experimental) |
Details of bundled changes.Comparing: ea6ed3d...0da64c2 react-dom
react-native-renderer
react-reconciler
react-art
react-test-renderer
ReactDOM: size: 0.0%, gzip: 🔺+0.1% Size changes (stable) |
sebmarkbage
approved these changes
Feb 20, 2020
|
@dai-shi Yes probably. I'll confirm in a sec. |
This was referenced Feb 21, 2020
Closed
Merged
This was referenced Mar 6, 2020
This was referenced Sep 25, 2024
This was referenced Sep 28, 2024
This was referenced Sep 29, 2024
This was referenced Oct 4, 2024
This was referenced Oct 5, 2024
This was referenced Oct 12, 2024
This was referenced Oct 25, 2024
This was referenced Oct 26, 2024
This was referenced Oct 27, 2024
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.
Fixes a bug where lower priority updates on a components wrapped with
memoare sometimes left dangling in the queue without ever being processed, if they are preceded by a higher priority bailout.Cause
The pending update priority field is cleared at the beginning of
beginWork. If there is remaining work at a lower priority level, it's expected that it will be accumulated on the work-in-progress fiber during the begin phase.There's an exception where this assumption doesn't hold: SimpleMemoComponent contains a bailout that occurs before the component is evaluated and the update queues are processed, which means we don't accumulate the next priority level. When we complete the fiber, the work loop is left to believe that there's no remaining work.
Mitigation
Since this only happens in a single case, a late bailout in SimpleMemoComponent, I've mitigated the bug in that code path by restoring the original update priority from the current fiber.
This same case does not apply to MemoComponent, because MemoComponent fibers do not contain hooks or update queues; rather, they wrap around an inner fiber that may contain those. However, I've added a test case for MemoComponent to protect against a possible future regression.
Possible next steps
We should consider moving the update priority assignment in
beginWorkout of the common path and into each branch, to avoid similar bugs in the future.