Bugfix: "Captured" updates on legacy queue #18265
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
This fixes a bug with error boundaries. Error boundaries have a notion of "captured" updates that represent errors that are thrown in its subtree during the render phase. These updates are meant to be dropped if the render is aborted. The bug happens when there's a concurrent update (an update from an interleaved event) in between when the error is thrown and when the error boundary does its second pass. The concurrent update is transferred from the pending queue onto the base queue. Usually, at this point the base queue is the same as the current queue. So when we append the pending updates to the work-in-progress queue, it also appends to the current queue. However, in the case of an error boundary's second pass, the base queue has already forked from the current queue; it includes both the "captured" updates and any concurrent updates. In that case, what we need to do is append separately to both queues. Which we weren't doing. That isn't the full story, though. You would expect that this mistake would manifest as dropping the interleaved updates. But instead what was happening is that the "captured" updates, the ones that are meant to be dropped if the render is aborted, were being added to the current queue. The reason is that the `baseQueue` structure is a circular linked list. The motivation for this was to save memory; instead of separate `first` and `last` pointers, you only need to point to `last`. But this approach does not work with structural sharing. So what was happening is that the captured updates were accidentally being added to the current queue because of the circular link. To fix this, I changed the `baseQueue` from a circular linked list to a singly-linked list so that we can take advantage of structural sharing. The "pending" queue, however, remains a circular list because it doesn't need to be persistent. This bug also affects the root fiber, which uses the same update queue implementation and also acts like an error boundary. It does not affect the hook update queue because they do not have any notion of "captured" updates. So I've left it alone for now. However, when we implement resuming, we will have to account for the same issue.
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 62de1b3:
|
Details of bundled changes.Comparing: bdc5cc4...62de1b3 react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (experimental) |
Details of bundled changes.Comparing: bdc5cc4...62de1b3 react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
Size changes (stable) |
Merged
acdlite
force-pushed
the
bugfix-captured-updates
branch
from
e8a6a37
to
4bcc6c5
Compare
acdlite
force-pushed
the
bugfix-captured-updates
branch
from
4bcc6c5
to
287932c
Compare
bvaughn
approved these changes
Mar 11, 2020
| @@ -984,19 +984,19 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { | |||
|
|
|||
| function logUpdateQueue(updateQueue: UpdateQueue<mixed>, depth) { | |||
| log(' '.repeat(depth + 1) + 'QUEUED UPDATES'); | |||
| const last = updateQueue.baseQueue; | |||
| const first = updateQueue.firstBaseUpdate; | |||
| const last = updateQueue.lastBaseUpdate; | |||
| if (last === null) { | |||
There was a problem hiding this comment.
Just asking out of curiosity, not really suggesting a change, but couldn't you just check if first === null here? Doesn't seem like the extra last pointer is needed.
There was a problem hiding this comment.
Ah yeah that's left over from before. You're right, don't need it.
| // processUpdateQueue, but that didn't happen in this case because we | ||
| // skipped over the parent when we bailed out. | ||
| let newFirst = null; | ||
| let lastFirst = null; |
There was a problem hiding this comment.
nit "last first" is a super confusing name. Maybe something like "newLast" or "nextLast" would be clearer?
There was a problem hiding this comment.
Lol I meant for that to be newLast
When an error boundary captures an error, we append the error update to the work-in-progress queue only so that if the render is aborted, the error update is dropped. Before appending to the queue, we need to make sure the queue is a work-in-progress copy. Usually we clone the queue during `processUpdateQueue`; however, if the base queue has lower priority than the current render, we may have bailed out on the boundary fiber without ever entering `processUpdateQueue`. So we need to lazily clone the queue.
The hook queue does not have resuming or "captured" updates, but if we ever add them in the future, we'll need to make sure we check if the queue is forked before transfering the pending updates to them.
acdlite
force-pushed
the
bugfix-captured-updates
branch
from
bbb7134
to
62de1b3
Compare
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.
I suggest hiding whitespace changes when reviewing
This fixes a bug with error boundaries. Error boundaries have a notion of "captured" updates that represent errors that are thrown in its subtree during the render phase. These updates are meant to be dropped if the render is aborted.
The bug happens when there's a concurrent update (an update from an interleaved event) in between when the error is thrown and when the error boundary does its second pass. The concurrent update is transferred from the pending queue onto the base queue. Usually, at this point the base queue is the same as the current queue. So when we append the pending updates to the work-in-progress queue, it also appends to the current queue.
However, in the case of an error boundary's second pass, the base queue has already forked from the current queue; it includes both the "captured" updates and any concurrent updates. In that case, what we need to do is append separately to both queues. Which we weren't doing.
That isn't the full story, though. You would expect that this mistake would manifest as dropping the interleaved updates. But instead what was happening is that the "captured" updates, the ones that are meant to be dropped if the render is aborted, were being added to the current queue.
The reason is that the
baseQueuestructure is a circular linked list. The motivation for this was to save memory; instead of separatefirstandlastpointers, you only need to point tolast.But this approach does not work with structural sharing. So what was happening is that the captured updates were accidentally being added to the current queue because of the circular link.
To fix this, I changed the
baseQueuefrom a circular linked list to a singly-linked list so that we can take advantage of structural sharing.The "pending" queue, however, remains a circular list because it doesn't need to be persistent.
This bug also affects the root fiber, which uses the same update queue implementation and also acts like an error boundary.
It does not affect the hook update queue because they do not have any notion of "captured" updates. So I've left it alone for now. However, when we implement resuming, we will have to account for the same issue.