-
Notifications
You must be signed in to change notification settings - Fork 47.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix: "Captured" updates on legacy queue #18265
Conversation
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.
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) |
e8a6a37
to
4bcc6c5
Compare
4bcc6c5
to
287932c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not very familiar with this code, but I don't see anything that stands out as wrong.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit "last first" is a super confusing name. Maybe something like "newLast" or "nextLast" would be clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
bbb7134
to
62de1b3
Compare
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
baseQueue
structure is a circular linked list. The motivation for this was to save memory; instead of separatefirst
andlast
pointers, 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
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.