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: Revealing a hidden update #24685

Merged
merged 3 commits into from
Jun 8, 2022
Merged

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jun 7, 2022

This fixes a bug I discovered related to revealing a hidden Offscreen tree. When this happens, we include in that render all the updates that had previously been deferred — that is, all the updates that would have already committed if the tree weren't hidden. This is necessary to avoid tearing with the surrounding contents. (This was the "flickering" Suspense bug we found a few years ago: #18411.)

The way we do this is by tracking the lanes of the updates that were deferred by a hidden tree. These are the "base" lanes. Then, in order to reveal the hidden tree, we process any update that matches one of those base lanes.

The bug I discovered is that some of these base lanes may include updates that were not present at the time the tree was hidden. We cannot flush those updates earlier that the surrounding contents — that, too, could cause tearing.

The crux of the problem is that we sometimes reuse the same lane for base updates and for non-base updates. So the lane alone isn't sufficient to distinguish between these cases. We must track this in some other way.

The solution I landed upon was to add an extra OffscreenLane bit to any update that is made to a hidden tree. Then later when we reveal the tree, we'll know not to treat them as base updates.

The extra OffscreenLane bit is removed as soon as that lane is committed by the root (markRootFinished) — at that point, it gets "upgraded" to a base update.

The trickiest part of this algorithm is reliably detecting when an update is made to a hidden tree. What makes this challenging is when the update is received during a concurrent event, while a render is already in progress — it's possible the work-in-progress render is about to flip the visibility of the tree that's being updated, leading to a race condition.

To avoid a race condition, we will wait to read the visibility of the tree until the current render has finished. In other words, this makes it an atomic operation. Most of this logic was already implemented in #24663.

Because this bugfix depends on a moderately risky refactor to the update queue (#24663), it only works in the "new" reconciler fork. We will roll it out gradually to www before landing in the main fork.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 7, 2022
@sizebot
Copy link

sizebot commented Jun 7, 2022

Comparing: 7e8a020...5f66498

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.05% 131.83 kB 131.89 kB +0.06% 42.33 kB 42.36 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.05% 137.10 kB 137.16 kB +0.07% 43.92 kB 43.95 kB
facebook-www/ReactDOM-prod.classic.js +0.04% 441.04 kB 441.21 kB +0.05% 80.69 kB 80.73 kB
facebook-www/ReactDOM-prod.modern.js +0.04% 426.35 kB 426.52 kB +0.05% 78.49 kB 78.53 kB
facebook-www/ReactDOMForked-prod.classic.js +0.31% 440.57 kB 441.92 kB +0.42% 80.61 kB 80.95 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactDOMForked-prod.modern.js +0.32% 425.89 kB 427.24 kB +0.43% 78.39 kB 78.73 kB
facebook-www/ReactDOMForked-dev.modern.js +0.31% 1,131.90 kB 1,135.38 kB +0.36% 250.34 kB 251.24 kB
facebook-www/ReactDOMForked-prod.classic.js +0.31% 440.57 kB 441.92 kB +0.42% 80.61 kB 80.95 kB
facebook-www/ReactDOMForked-dev.classic.js +0.30% 1,155.41 kB 1,158.89 kB +0.36% 254.76 kB 255.69 kB
facebook-www/ReactDOMForked-profiling.modern.js +0.30% 456.65 kB 458.00 kB +0.40% 82.90 kB 83.23 kB
facebook-www/ReactDOMForked-profiling.classic.js +0.29% 471.39 kB 472.74 kB +0.39% 85.19 kB 85.52 kB

Generated by 🚫 dangerJS against 5f66498

@acdlite acdlite force-pushed the bugfix-hidden-updates branch 2 times, most recently from 8b13e88 to 1ee663d Compare June 7, 2022 22:46
Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

We need to be able to read whether an offscreen tree is hidden from
an imperative event. We can store this on its OffscreenInstance.

We were already scheduling a commit effect whenever the visibility
changes, in order to toggle the inner effects. So we can reuse that.
This fixes a bug I discovered related to revealing a hidden Offscreen
tree. When this happens, we include in that render all the updates that
had previously been deferred — that is, all the updates that would have
already committed if the tree weren't hidden. This is necessary to avoid
tearing with the surrounding contents. (This was the "flickering"
Suspense bug we found a few years ago: facebook#18411.)

The way we do this is by tracking the lanes of the updates that were
deferred by a hidden tree. These are the "base" lanes. Then, in order
to reveal the hidden tree, we process any update that matches one of
those base lanes.

The bug I discovered is that some of these base lanes may include
updates that were not present at the time the tree was hidden. We cannot
flush those updates earlier that the surrounding contents — that, too,
could cause tearing.

The crux of the problem is that we sometimes reuse the same lane for
base updates and for non-base updates. So the lane alone isn't
sufficient to distinguish between these cases. We must track this in
some other way.

The solution I landed upon was to add an extra OffscreenLane bit to any
update that is made to a hidden tree. Then later when we reveal the
tree, we'll know not to treat them as base updates.

The extra OffscreenLane bit is removed as soon as that lane is committed
by the root (markRootFinished) — at that point, it gets
"upgraded" to a base update.

The trickiest part of this algorithm is reliably detecting when an
update is made to a hidden tree. What makes this challenging is when the
update is received during a concurrent event, while a render is already
in progress — it's possible the work-in-progress render is about to
flip the visibility of the tree that's being updated, leading to a race
condition.

To avoid a race condition, we will wait to read the visibility of the
tree until the current render has finished. In other words, this makes
it an atomic operation. Most of this logic was already implemented
in facebook#24663.

Because this bugfix depends on a moderately risky refactor to the update
queue (facebook#24663), it only works in the "new" reconciler fork. We will roll
it out gradually to www before landing in the main fork.
@acdlite acdlite merged commit 79f54c1 into facebook:main Jun 8, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jun 15, 2022
Summary:
This sync includes the following changes:
- **[5cc2487e0](facebook/react@5cc2487e0 )**: bump versions for next release ([#24725](facebook/react#24725)) //<Josh Story>//
- **[54f17e490](facebook/react@54f17e490 )**: [Transition Tracing] Fix Cache and Transitions Pop Order ([#24719](facebook/react#24719)) //<Luna Ruan>//
- **[7cf8dfd94](facebook/react@7cf8dfd94 )**: [Transition Tracing] Create/Process Marker Complete Callback ([#24700](facebook/react#24700)) //<Luna Ruan>//
- **[327e4a1f9](facebook/react@327e4a1f9 )**: [Follow-up] Land enableClientRenderFallbackOnTextMismatch //<Andrew Clark>//
- **[a8c9cb18b](facebook/react@a8c9cb18b )**: Land enableSuspenseLayoutEffectSemantics flag ([#24713](facebook/react#24713)) //<Andrew Clark>//
- **[a8555c308](facebook/react@a8555c308 )**: [Transition Tracing] Add Tracing Marker Stack ([#24661](facebook/react#24661)) //<Luna Ruan>//
- **[8186b1937](facebook/react@8186b1937 )**: Check for infinite update loops even if unmounted ([#24697](facebook/react#24697)) //<Andrew Clark>//
- **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([#24688](facebook/react#24688)) //<Josh Story>//
- **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([#24689](facebook/react#24689)) //<Mathieu Dutour>//
- **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([#24680](facebook/react#24680)) //<Josh Story>//
- **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([#24685](facebook/react#24685)) //<Andrew Clark>//
- **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([#24683](facebook/react#24683)) //<Sebastian Markbåge>//
- **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([#24591](facebook/react#24591)) //<Josh Story>//
- **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([#24663](facebook/react#24663)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions d300ceb...256aefb

jest_e2e[run_all_tests]

Reviewed By: cortinico

Differential Revision: D37155957

fbshipit-source-id: 4c0afc95abe8fa13c3803584922c8dc0059ff562
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Aug 8, 2022
Summary:
Sync goes to v18.2 release.

I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days.

This sync includes the following changes:
- **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([#24688](facebook/react#24688)) //<Josh Story>//
- **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([#24689](facebook/react#24689)) //<Mathieu Dutour>//
- **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([#24680](facebook/react#24680)) //<Josh Story>//
- **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([#24685](facebook/react#24685)) //<Andrew Clark>//
- **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([#24683](facebook/react#24683)) //<Sebastian Markbåge>//
- **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([#24591](facebook/react#24591)) //<Josh Story>//
- **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([#24663](facebook/react#24663)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions d300ceb...9e3b772

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross

Differential Revision: D38496392

fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
Summary:
Sync goes to v18.2 release.

I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days.

This sync includes the following changes:
- **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([facebook#24688](facebook/react#24688)) //<Josh Story>//
- **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([facebook#24689](facebook/react#24689)) //<Mathieu Dutour>//
- **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([facebook#24680](facebook/react#24680)) //<Josh Story>//
- **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([facebook#24685](facebook/react#24685)) //<Andrew Clark>//
- **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([facebook#24683](facebook/react#24683)) //<Sebastian Markbåge>//
- **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([facebook#24591](facebook/react#24591)) //<Josh Story>//
- **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([facebook#24663](facebook/react#24663)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions d300ceb...9e3b772

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross

Differential Revision: D38496392

fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
Summary:
Sync goes to v18.2 release.

I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days.

This sync includes the following changes:
- **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([facebook#24688](facebook/react#24688)) //<Josh Story>//
- **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([facebook#24689](facebook/react#24689)) //<Mathieu Dutour>//
- **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([facebook#24680](facebook/react#24680)) //<Josh Story>//
- **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([facebook#24685](facebook/react#24685)) //<Andrew Clark>//
- **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([facebook#24683](facebook/react#24683)) //<Sebastian Markbåge>//
- **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([facebook#24591](facebook/react#24591)) //<Josh Story>//
- **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([facebook#24663](facebook/react#24663)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions d300ceb...9e3b772

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross

Differential Revision: D38496392

fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
acdlite added a commit to acdlite/react that referenced this pull request Dec 9, 2022
This code was originally added in the old ExpirationTime implementation
of Suspense. The idea is that if multiple updates suspend inside the
same Suspense boundary, and both of them resolve, we should render both
results in the same batch, to reduce jank.

This was an incomplete idea, though. We later discovered a stronger
requirement — once we show a fallback, we cannot fill in that fallback
without completing _all_ the updates that were previously skipped over.
Otherwise you get tearing. This was fixed by facebook#18411, then we
discovered additional related flaws that were addressed in facebook#24685. See 
those PR descriptions for additional context.

So I believe this older code is no longer necessary.
acdlite added a commit to acdlite/react that referenced this pull request Dec 22, 2022
This code was originally added in the old ExpirationTime implementation
of Suspense. The idea is that if multiple updates suspend inside the
same Suspense boundary, and both of them resolve, we should render both
results in the same batch, to reduce jank.

This was an incomplete idea, though. We later discovered a stronger
requirement — once we show a fallback, we cannot fill in that fallback
without completing _all_ the updates that were previously skipped over.
Otherwise you get tearing. This was fixed by facebook#18411, then we
discovered additional related flaws that were addressed in facebook#24685. See 
those PR descriptions for additional context.

So I believe this older code is no longer necessary.
acdlite added a commit to acdlite/react that referenced this pull request Jan 4, 2023
This code was originally added in the old ExpirationTime implementation
of Suspense. The idea is that if multiple updates suspend inside the
same Suspense boundary, and both of them resolve, we should render both
results in the same batch, to reduce jank.

This was an incomplete idea, though. We later discovered a stronger
requirement — once we show a fallback, we cannot fill in that fallback
without completing _all_ the updates that were previously skipped over.
Otherwise you get tearing. This was fixed by facebook#18411, then we
discovered additional related flaws that were addressed in facebook#24685. See 
those PR descriptions for additional context.

So I believe this older code is no longer necessary.
acdlite added a commit that referenced this pull request Jan 4, 2023
This code was originally added in the old ExpirationTime implementation
of Suspense. The idea is that if multiple updates suspend inside the
same Suspense boundary, and both of them resolve, we should render both
results in the same batch, to reduce jank.

This was an incomplete idea, though. We later discovered a stronger
requirement — once we show a fallback, we cannot fill in that fallback
without completing _all_ the updates that were previously skipped over.
Otherwise you get tearing. This was fixed by #18411, then we discovered
additional related flaws that were addressed in #24685. See those PR
descriptions for additional context.

So I believe this older code is no longer necessary.
github-actions bot pushed a commit that referenced this pull request Jan 4, 2023
This code was originally added in the old ExpirationTime implementation
of Suspense. The idea is that if multiple updates suspend inside the
same Suspense boundary, and both of them resolve, we should render both
results in the same batch, to reduce jank.

This was an incomplete idea, though. We later discovered a stronger
requirement — once we show a fallback, we cannot fill in that fallback
without completing _all_ the updates that were previously skipped over.
Otherwise you get tearing. This was fixed by #18411, then we discovered
additional related flaws that were addressed in #24685. See those PR
descriptions for additional context.

So I believe this older code is no longer necessary.

DiffTrain build for [48274a4](48274a4)
[View git log for this commit](https://github.com/facebook/react/commits/48274a43aa708f63a7580142a4c1c1a47f31c1ac)
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