Skip to content
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

Merged
merged 3 commits into from
Mar 11, 2020

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Mar 10, 2020

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 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 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 facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 10, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 10, 2020

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:

Sandbox Source
jovial-waterfall-bxv90 Configuration

@sizebot
Copy link

sizebot commented Mar 10, 2020

Details of bundled changes.

Comparing: bdc5cc4...62de1b3

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js +0.4% +0.4% 124.38 KB 124.89 KB 38.79 KB 38.94 KB NODE_PROFILING
ReactDOM-dev.js +0.3% +0.3% 910.99 KB 914.16 KB 203.61 KB 204.29 KB FB_WWW_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10.27 KB 10.27 KB 3.49 KB 3.49 KB UMD_PROD
react-dom-test-utils.production.min.js 0.0% -0.0% 10.96 KB 10.96 KB 4.17 KB 4.17 KB UMD_PROD
ReactDOMTesting-dev.js +0.4% +0.3% 875.09 KB 878.25 KB 195.94 KB 196.63 KB FB_WWW_DEV
ReactDOMTesting-prod.js 🔺+0.4% 🔺+0.2% 369.92 KB 371.41 KB 67.69 KB 67.85 KB FB_WWW_PROD
ReactDOMTesting-profiling.js +0.4% +0.2% 369.92 KB 371.41 KB 67.69 KB 67.85 KB FB_WWW_PROFILING
react-dom-server.node.development.js 0.0% -0.0% 131.08 KB 131.08 KB 34.83 KB 34.83 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 10.81 KB 10.81 KB 4.1 KB 4.1 KB NODE_PROD
react-dom.development.js +0.4% +0.3% 916.24 KB 919.54 KB 200.49 KB 201.17 KB UMD_DEV
react-dom-server.browser.development.js 0.0% -0.0% 136.87 KB 136.87 KB 34.99 KB 34.99 KB UMD_DEV
react-dom.production.min.js 🔺+0.4% 🔺+0.3% 120.26 KB 120.77 KB 38.4 KB 38.51 KB UMD_PROD
react-dom.profiling.min.js +0.4% +0.3% 124.08 KB 124.6 KB 39.62 KB 39.75 KB UMD_PROFILING
react-dom.development.js +0.4% +0.3% 872.13 KB 875.29 KB 198.05 KB 198.7 KB NODE_DEV
react-dom-server.browser.development.js 0.0% -0.0% 129.87 KB 129.87 KB 34.58 KB 34.58 KB NODE_DEV
react-dom.production.min.js 🔺+0.4% 🔺+0.2% 120.41 KB 120.92 KB 37.6 KB 37.69 KB NODE_PROD
ReactDOM-prod.js 🔺+0.4% 🔺+0.2% 375.84 KB 377.33 KB 68.41 KB 68.58 KB FB_WWW_PROD
ReactDOM-profiling.js +0.4% +0.2% 387.53 KB 389.02 KB 70.57 KB 70.72 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 55.87 KB 55.87 KB 14.47 KB 14.47 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.69 KB 3.69 KB 1.34 KB 1.34 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.1% 10 KB 10 KB 3.37 KB 3.37 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 2.95 KB 2.95 KB 1.18 KB 1.18 KB NODE_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFabric-prod.js 🔺+0.6% 🔺+0.4% 251.89 KB 253.38 KB 43.74 KB 43.89 KB RN_OSS_PROD
ReactFabric-profiling.js +0.6% +0.3% 263.69 KB 265.18 KB 46 KB 46.16 KB RN_OSS_PROFILING
ReactNativeRenderer-prod.js 🔺+0.6% 🔺+0.4% 259.93 KB 261.42 KB 45.22 KB 45.38 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.5% +0.3% 271.73 KB 273.22 KB 47.47 KB 47.63 KB RN_OSS_PROFILING
ReactNativeRenderer-dev.js +0.5% +0.5% 636.77 KB 639.93 KB 137.92 KB 138.6 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.6% 🔺+0.4% 260.08 KB 261.56 KB 45.25 KB 45.41 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.5% +0.3% 271.87 KB 273.36 KB 47.5 KB 47.66 KB RN_FB_PROFILING
ReactFabric-dev.js +0.5% +0.5% 616.01 KB 619.17 KB 133.25 KB 133.92 KB RN_OSS_DEV
ReactFabric-dev.js +0.5% +0.5% 618.72 KB 621.89 KB 133.58 KB 134.25 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.6% 🔺+0.4% 252.05 KB 253.54 KB 43.77 KB 43.92 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.5% +0.5% 634.06 KB 637.22 KB 137.58 KB 138.25 KB RN_OSS_DEV
ReactFabric-profiling.js +0.6% +0.3% 263.84 KB 265.33 KB 46.03 KB 46.19 KB RN_FB_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.5% +0.5% 637.41 KB 640.71 KB 133.87 KB 134.55 KB UMD_DEV
react-art.production.min.js 🔺+0.5% 🔺+0.5% 107.01 KB 107.53 KB 32.35 KB 32.52 KB UMD_PROD
react-art.development.js +0.6% +0.6% 541.46 KB 544.62 KB 116.29 KB 116.96 KB NODE_DEV
react-art.production.min.js 🔺+0.7% 🔺+0.5% 71.99 KB 72.51 KB 21.54 KB 21.65 KB NODE_PROD
ReactART-dev.js +0.6% +0.6% 547.12 KB 550.29 KB 115.23 KB 115.9 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.7% 🔺+0.4% 226.21 KB 227.7 KB 38.55 KB 38.7 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer-shallow.development.js 0.0% -0.0% 38.4 KB 38.4 KB 9.3 KB 9.3 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 12.92 KB 12.92 KB 3.94 KB 3.94 KB UMD_PROD
ReactTestRenderer-dev.js +0.6% +0.6% 555.01 KB 558.18 KB 117.25 KB 117.93 KB FB_WWW_DEV
react-test-renderer.development.js +0.6% +0.6% 553.96 KB 557.25 KB 115.38 KB 116.07 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.7% 🔺+0.5% 71.3 KB 71.82 KB 21.72 KB 21.84 KB UMD_PROD
react-test-renderer.development.js +0.6% +0.6% 527.93 KB 531.09 KB 114.07 KB 114.72 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.7% 🔺+0.5% 71.11 KB 71.63 KB 21.37 KB 21.49 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.5% +0.5% 579.94 KB 583.1 KB 122.22 KB 122.89 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% -0.0% 16.25 KB 16.25 KB 4.97 KB 4.96 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.7% 🔺+0.5% 76.85 KB 77.37 KB 22.55 KB 22.67 KB NODE_PROD

ReactDOM: size: 0.0%, gzip: -0.0%

Size changes (experimental)

Generated by 🚫 dangerJS against 62de1b3

@sizebot
Copy link

sizebot commented Mar 10, 2020

Details of bundled changes.

Comparing: bdc5cc4...62de1b3

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactDOM-profiling.js +0.4% +0.2% 414.21 KB 415.7 KB 75.34 KB 75.5 KB FB_WWW_PROFILING
react-dom-server.browser.development.js 0.0% -0.0% 135.28 KB 135.28 KB 34.78 KB 34.78 KB UMD_DEV
react-dom.development.js +0.4% +0.3% 885.77 KB 889.07 KB 195.25 KB 195.91 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 9.99 KB 9.99 KB 3.36 KB 3.36 KB NODE_PROD
react-dom.production.min.js 🔺+0.4% 🔺+0.4% 116.06 KB 116.58 KB 37.19 KB 37.35 KB UMD_PROD
react-dom.profiling.min.js +0.4% +0.4% 119.77 KB 120.29 KB 38.41 KB 38.57 KB UMD_PROFILING
react-dom.development.js +0.4% +0.3% 842.95 KB 846.1 KB 192.81 KB 193.48 KB NODE_DEV
react-dom.production.min.js 🔺+0.4% 🔺+0.2% 116.14 KB 116.65 KB 36.51 KB 36.6 KB NODE_PROD
react-dom.profiling.min.js +0.4% +0.4% 120 KB 120.51 KB 37.64 KB 37.78 KB NODE_PROFILING
ReactDOM-dev.js +0.3% +0.3% 956.92 KB 960.09 KB 213.58 KB 214.26 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.4% 🔺+0.2% 402.44 KB 403.93 KB 73.08 KB 73.24 KB FB_WWW_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.68 KB 3.68 KB 1.33 KB 1.33 KB NODE_DEV
ReactDOMTesting-dev.js +0.4% +0.3% 901.73 KB 904.9 KB 201.51 KB 202.17 KB FB_WWW_DEV
ReactDOMTesting-prod.js 🔺+0.4% 🔺+0.2% 383.47 KB 384.95 KB 69.85 KB 70.01 KB FB_WWW_PROD
react-dom-server.browser.development.js 0.0% -0.0% 128.35 KB 128.35 KB 34.36 KB 34.36 KB NODE_DEV
react-dom-test-utils.development.js 0.0% -0.0% 53.77 KB 53.77 KB 14 KB 14 KB UMD_DEV
ReactDOMTesting-profiling.js +0.4% +0.2% 383.47 KB 384.95 KB 69.85 KB 70.01 KB FB_WWW_PROFILING
react-dom-test-utils.production.min.js 0.0% -0.0% 10.94 KB 10.94 KB 4.16 KB 4.16 KB UMD_PROD
ReactDOMServer-dev.js 0.0% -0.0% 138.5 KB 138.5 KB 35.3 KB 35.3 KB FB_WWW_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 10.79 KB 10.79 KB 4.09 KB 4.09 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 58.9 KB 58.9 KB 14.7 KB 14.7 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10.26 KB 10.26 KB 3.49 KB 3.49 KB UMD_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.5% +0.5% 634.04 KB 637.21 KB 137.58 KB 138.25 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.6% 🔺+0.4% 259.92 KB 261.41 KB 45.21 KB 45.38 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.5% +0.3% 271.71 KB 273.2 KB 47.47 KB 47.62 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.5% +0.5% 615.99 KB 619.16 KB 133.24 KB 133.92 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.6% 🔺+0.4% 251.88 KB 253.37 KB 43.73 KB 43.89 KB RN_OSS_PROD
ReactFabric-profiling.js +0.6% +0.3% 263.68 KB 265.17 KB 45.99 KB 46.15 KB RN_OSS_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.6% +0.6% 521.35 KB 524.5 KB 112.55 KB 113.22 KB NODE_DEV
react-art.production.min.js 🔺+0.8% 🔺+0.6% 69.29 KB 69.82 KB 20.8 KB 20.93 KB NODE_PROD
ReactART-dev.js +0.6% +0.6% 575.2 KB 578.36 KB 120.83 KB 121.5 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.6% 🔺+0.4% 235.06 KB 236.55 KB 39.96 KB 40.14 KB FB_WWW_PROD
react-art.development.js +0.5% +0.5% 616.46 KB 619.75 KB 130.18 KB 130.87 KB UMD_DEV
react-art.production.min.js 🔺+0.5% 🔺+0.5% 104.26 KB 104.77 KB 31.62 KB 31.78 KB UMD_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer-shallow.development.js 0.0% -0.0% 38.39 KB 38.39 KB 9.29 KB 9.29 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 12.91 KB 12.91 KB 3.93 KB 3.93 KB UMD_PROD
react-test-renderer.development.js +0.6% +0.6% 527.91 KB 531.06 KB 114.06 KB 114.71 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.7% 🔺+0.5% 71.09 KB 71.61 KB 21.35 KB 21.47 KB NODE_PROD
ReactTestRenderer-dev.js +0.6% +0.6% 555 KB 558.17 KB 117.24 KB 117.93 KB FB_WWW_DEV
react-test-renderer.development.js +0.6% +0.6% 553.93 KB 557.22 KB 115.37 KB 116.06 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.7% 🔺+0.5% 71.28 KB 71.79 KB 21.71 KB 21.83 KB UMD_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.6% +0.6% 557.5 KB 560.65 KB 118.02 KB 118.69 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.7% 🔺+0.6% 73.76 KB 74.28 KB 21.78 KB 21.91 KB NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 62de1b3

@bvaughn
Copy link
Contributor

bvaughn commented Mar 10, 2020

Just FYI I pulled this change into my PR to see if it fixed the two failing tests I added recently (a6d4eed). It fixes this one 👍

It does not fix this one, but the reason for that is expected and I'll leave a note on my other PR.

@bvaughn bvaughn mentioned this pull request Mar 10, 2020
@acdlite acdlite marked this pull request as ready for review March 11, 2020 03:11
Copy link
Contributor

@bvaughn bvaughn left a 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) {
Copy link
Contributor

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.

Copy link
Collaborator Author

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;
Copy link
Contributor

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?

Copy link
Collaborator Author

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants