Skip to content

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Apr 5, 2025

Found a bug that occurs during a specific combination of very subtle implementation details.

It occurs sometimes (not always) when 1) a transition is scheduled during a popstate event, and 2) as a result, a new value is passed to an already-mounted useDeferredValue hook.

The fix is relatively straightforward, and I found it almost immediately; it took a bit longer to figure out exactly how the scenario occurred in production and create a test case to simulate it.

Rather than couple the test to the implementation details, I've chosen to keep it as high-level as possible so that it doesn't break if the details change. In the future, it might not be trigger the exact set of internal circumstances anymore, but it could be useful for catching similar bugs because it represents a realistic real world situation — namely, switching tabs repeatedly in an app that uses useDeferredValue.

Found a bug that occurs during a specific combination of very subtle
implementation details.

It occurs sometimes (not always) when 1) a transition is scheduled
during a popstate event, and 2) as a result, a new value is passed to an
already-mounted useDeferredValue hook.

The fix is relatively straightforward, and I found it almost
immediately; it took a bit longer to figure out exactly how the scenario
occurred in production and create a test case to simulate it.

Rather than couple the test to the implementation details, I've chosen
to keep it as high-level as possible so that it doesn't break if the
details change. In the future, it might not be trigger the exact set of
internal circumstances anymore, but it could be useful for catching
similar bugs because it represents a realistic real world situation —
namely, switching tabs repeatedly in an app that uses useDeferredValue.
@acdlite acdlite requested a review from sebmarkbage April 5, 2025 04:42
@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Apr 5, 2025
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.

Oh snap nice

@react-sizebot
Copy link

Comparing: efb22d8...64acfd5

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.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 516.12 kB 516.15 kB = 91.87 kB 91.87 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 619.49 kB 619.52 kB = 109.51 kB 109.51 kB
facebook-www/ReactDOM-prod.classic.js = 650.72 kB 650.76 kB = 114.93 kB 114.94 kB
facebook-www/ReactDOM-prod.modern.js = 641.00 kB 641.04 kB = 113.38 kB 113.38 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 64acfd5

@acdlite acdlite merged commit 6a7650c into facebook:main Apr 5, 2025
243 checks passed
github-actions bot pushed a commit that referenced this pull request Apr 5, 2025
Found a bug that occurs during a specific combination of very subtle
implementation details.

It occurs sometimes (not always) when 1) a transition is scheduled
during a popstate event, and 2) as a result, a new value is passed to an
already-mounted useDeferredValue hook.

The fix is relatively straightforward, and I found it almost
immediately; it took a bit longer to figure out exactly how the scenario
occurred in production and create a test case to simulate it.

Rather than couple the test to the implementation details, I've chosen
to keep it as high-level as possible so that it doesn't break if the
details change. In the future, it might not be trigger the exact set of
internal circumstances anymore, but it could be useful for catching
similar bugs because it represents a realistic real world situation —
namely, switching tabs repeatedly in an app that uses useDeferredValue.

DiffTrain build for [6a7650c](6a7650c)
github-actions bot pushed a commit that referenced this pull request Apr 5, 2025
Found a bug that occurs during a specific combination of very subtle
implementation details.

It occurs sometimes (not always) when 1) a transition is scheduled
during a popstate event, and 2) as a result, a new value is passed to an
already-mounted useDeferredValue hook.

The fix is relatively straightforward, and I found it almost
immediately; it took a bit longer to figure out exactly how the scenario
occurred in production and create a test case to simulate it.

Rather than couple the test to the implementation details, I've chosen
to keep it as high-level as possible so that it doesn't break if the
details change. In the future, it might not be trigger the exact set of
internal circumstances anymore, but it could be useful for catching
similar bugs because it represents a realistic real world situation —
namely, switching tabs repeatedly in an app that uses useDeferredValue.

DiffTrain build for [6a7650c](6a7650c)
@venpisey
Copy link

venpisey commented Apr 6, 2025

5167170015919920

@brhx
Copy link
Contributor

brhx commented Apr 6, 2025

This is such a good catch! I've been noticing this in next v15.3.0-canary.38. I only noticed it when the React DevTools extension was enabled. My suspicion was that it slowed down fiber enough for this to be more likely to occur.

acdlite added a commit to acdlite/prefetch-test that referenced this pull request Apr 7, 2025
This includes a React bugfix to popstate transitions. (See:
facebook/react#32821.) Seems to fix the crashes
on back/forward we were seeing.
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.

7 participants