fix: preserve hook chains when unwinding interrupted renders#36174
Open
arunanshub wants to merge 2 commits intofacebook:mainfrom
Open
fix: preserve hook chains when unwinding interrupted renders#36174arunanshub wants to merge 2 commits intofacebook:mainfrom
arunanshub wants to merge 2 commits intofacebook:mainfrom
Conversation
This file contains hidden or 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
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.
Summary
Fixes #33580.
This PR fixes a hook-chain corruption bug in
resetHooksOnUnwindthat can surface asRendered more hooks than during the previous render.after hydration when a component conditionally callsuse(thenable)and a later update retries the render.The failure mode is:
updateWorkInProgressHookruns out of current hook entries and throwsRendered more hooks than during the previous render.What Changed
This change preserves hook-list integrity in both ways the work-in-progress list can be incomplete on unwind:
workInProgress.memoizedStateis stillnull, clone the entire current hook list into the work-in-progress fiber.The fix intentionally completes the hook chain only after React finishes cleaning up render phase updates. Doing this earlier is incorrect because unprocessed hooks share queues with the current fiber, and cloning them before cleanup can cause legitimate pending updates on later hooks to be cleared.
Prior Art
This PR differs from #35717 in two important ways:
[Fiber] Complete partial hook chain on unwind to prevent corruption #35717 appends the remaining current hooks before render phase update cleanup.
useState, schedules a render phase update on that first hook, and then receives a normal pending update on a lateruseStatehook that was never processed during the aborted render.resetHooksOnUnwindto walk the cloned hook list and clearqueue.pending, which drops that legitimate later update.ReactHooksWithNoopRenderer-testregression covers this case. Without the fix, the test regresses from the expectedB:0toA:0.[Fiber] Complete partial hook chain on unwind to prevent corruption #35717 only handles the partial-suffix case where both
currentHookandworkInProgressHookare already non-null.It does not handle the case where render unwinds before the first tracked hook. In that case, the full current hook list must be cloned to avoid remounting hooks on the next render.
Relevant prior work that helped narrow this down:
How did you test this change?
Tests
Added regression coverage for the original bug and the two additional edge cases found while validating the unwind fix:
ReactDOMFizzShellHydration-testuse(thenable), a cascading update,Suspense, and anErrorBoundaryReactHooksWithNoopRenderer-testFormatting, lint, and type checks:
node ./scripts/prettier/index.js --check packages/react-reconciler/src/ReactFiberHooks.js packages/react-dom/src/__tests__/ReactDOMFizzShellHydration-test.js packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js node ./scripts/tasks/eslint.js packages/react-reconciler/src/ReactFiberHooks.js packages/react-dom/src/__tests__/ReactDOMFizzShellHydration-test.js packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js node ./scripts/tasks/eslint.js yarn flow dom-node yarn flow-ci # for every Flow-typed host config in scripts/shared/inlinedHostConfigsFocused and adjacent test coverage:
yarn test ReactDOMFizzShellHydration-test ReactHooks-test.internal ReactSuspenseWithNoopRenderer-test ReactUse-test ReactHooksWithNoopRenderer-test --runInBand --ciStable release-channel coverage:
Targeted bundle build: