Prevent infinite re-renders in StrictMode + Offscreen #25203

sammy-SC · 2022-09-07T16:12:03Z

@gnoff reported an infinite render caused by recent changes to StrictMode.

The root cause is the special handling of Offscreen in doubleInvokeEffectsInDEV. To fix this, we add a check to see if Visibility flag for Offscreen is set before double invoking effects.

sizebot · 2022-09-07T16:16:54Z

Comparing: a9dc73c...3b01085

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	=	134.97 kB	134.97 kB	=	43.23 kB	43.23 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	141.64 kB	141.64 kB	=	45.20 kB	45.20 kB
facebook-www/ReactDOM-prod.classic.js	=	486.06 kB	486.06 kB	=	86.51 kB	86.51 kB
facebook-www/ReactDOM-prod.modern.js	=	471.34 kB	471.34 kB	=	84.29 kB	84.29 kB
facebook-www/ReactDOMForked-prod.classic.js	=	486.06 kB	486.06 kB	=	86.51 kB	86.51 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 3b01085

acdlite · 2022-09-07T16:23:13Z

Nice! Do you mind adding a regression test?

Edit: I guess we can just use @gnoff's PR #25179

acdlite · 2022-09-07T16:23:50Z

@gnoff Could you confirm whether this fixes the issue you found?

gnoff · 2022-09-07T17:23:24Z

@acdlite @sammy-SC that does fix the failing test from #25179 (though it looks like it breaks other tests).

Once this is fixed, I'm not sure entirely where this test best belongs. I put it in ReactDOMHooks-test.js but that doesn't actually seem right since it's more of a reconciler thing. any suggestions on a better home?

sammy-SC · 2022-09-07T18:23:24Z

I need to understand why some tests started failing now. I think the check for Offscreen component is needed to avoid double invoking Offscreen's subtree if it is hidden.
But only experimental tests started failing, I need to spend some more time on this.
I'll make sure to add regression test.

Thank you @gnoff for great repro.

dai-shi · 2022-09-08T11:00:52Z

Is this somehow related with #25180?

sammy-SC · 2022-09-08T12:18:11Z

Is this somehow related with #25180?

Yes, this PR should fix that as well.

acdlite · 2022-09-09T14:43:24Z

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

-    if (isInStrictMode) {
+    const hasOffscreenVisibilityFlag =
+      fiber.tag !== OffscreenComponent || fiber.flags & Visibility;
+    if (isInStrictMode && hasOffscreenVisibilityFlag) {


Can you explain why this fixes it? Is it because we shouldn't fire effects in a hidden offscreen tree?

As far as I understand, Visibility flag is set on Offscreen when its visibility changes.

We only want to fire effects on Offscreen when it is visible and only when its visibility changes. This prevents the infinite loop. Without it, it kept firing effects unconditionally if any effect causes synchronous re-render.

We only want to fire effects on Offscreen when it is visible and only when its visibility changes.

I don't think that's true, the strict mode double invoke logic happens for all newly mounted trees, not just Offscreen ones. (Consider that this behavior is already in open source and offscreen doesn't exist.)

In fact, I don't think we need any Offscreen specific logic at all. Do you remember why you added the fiber.tag === OffscreenComponent check originally? None of the tests seem to fail if you remove it. And it also fixes the regression test.

~~I tried running the regression test on main and it passes even without any changes, so we need to fix that~~ Nvm was running in the wrong release channel

Do you remember why you added the fiber.tag === OffscreenComponent check originally?

It is needed to prevent double invoking on components that are hidden by OffscreenComponent.

This is the test specifically for that behaviour: https://github.com/facebook/react/blob/main/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js#L70

Without the special condition for Offscreen, <Component label="A" /> will have its effects invoked.

I think it would be unexpected for our users to see mount/unmount on a component that is hidden by Offscreen.

What if you change code in a parent that would break an offscreen subtree, but you don't know because you don't navigate to that offscreen component in development (because you're not working on that tab/modal, for example). It seems like these type of bugs are something you eagerly want to know about with StrictMode.

Curious what @gaearon thinks as well.

What if you change code in a parent that would break an offscreen subtree, but you don't know because you don't navigate to that offscreen component in development (because you're not working on that tab/modal, for example).

I agree, there is definitely a value in double invoking effects even if Offscreen is hidden. On the other hand, it will cause confusion. Suddenly you would see effects triggering for something you wouldn't expect. This will be even more confusing with pre rendering where a screen that engineer might not even know about has its effects executed. I think there is already expectation with StrictMode that component needs to be used to trigger double invoking. It aligns well with Offscreen suppressing when hidden.

Suddenly you would see effects triggering for something you wouldn't expect.

Isn't this also the case for double rendering? Is it confusing that we double render those hidden trees but don't double fire the effects?

I don't think so. Take pre-rendering as an example. User expects render functions to be executed even if the subtree is hidden. This is true for pre-rendering example or for possible virtualised list implementations.
I do see value in double invoking effects in hidden subtree as well. I don't hold strong opinions on this. I'm just explaining how I was thinking about this when I implemented it.

Discussed offline, the other concern here is that to do this right, you also need to hide the currently visible content, as though you fully switch to the hidden tab. And when you switch to the hidden tab, you may not have the props you need to do a proper render. For example, if you're getting the userId from the route, then you don't have an ID to test the offscreen tree with, so you end up testing behavior that doesn't occur in the wild.

So it's easier to test making visible content hidden than it is testing hidden/pre-rendered content visible without more information. The best practice is to test navigating to the offscreen trees.

acdlite

Looks great!

Do you want to cherry-pick the test in #25180 too?

packages/react-reconciler/src/ReactFiberWorkLoop.old.js

sammy-SC · 2022-09-09T17:49:43Z

Looks great!

Do you want to cherry-pick the test in #25180 too?

It shows exactly the same problem. Having two tests for the same behaviour seems unnecessary. I'll exclude it.

* Prevent infinite re-render in StrictMode + Offscreen * Only fire effects for Offscreen when it is revealed * Move setting debug fiber into if branch * Move settings of debug fiber out of if branch

Summary: This sync includes the following changes: - **[0cac4d54c](facebook/react@0cac4d54c )**: Double invoked effects on suspended children ([#25307](facebook/react#25307)) //<Samuel Susla>// - **[3d615fc14](facebook/react@3d615fc14 )**: Grammar. Removed doubles of the word "the". ([#25295](facebook/react#25295)) //<Victoria Graf>// - **[6e3bc8a](facebook/react@6e3bc8a2e )**: [DevTools] Check if Proxy exists before creating DispatcherProxy ([#25278](facebook/react#25278)) //<Tianyu Yao>// - **[e7fc04b29](facebook/react@e7fc04b29 )**: [react-dom] Reorganize react-dom internals to match react ([#25277](facebook/react#25277)) //<Josh Story>// - **[0b54e0047](facebook/react@0b54e0047 )**: Handle rejections to avoid uncaught rejections ([#25272](facebook/react#25272)) //<Sebastian Markbåge>// - **[c5d06fdc5](facebook/react@c5d06fdc5 )**: [Flight] Fix Webpack Chunk Loading ([#25271](facebook/react#25271)) //<Sebastian Markbåge>// - **[975b64464](facebook/react@975b64464 )**: [Flight] response.readRoot() -> use(response) ([#25267](facebook/react#25267)) //<Sebastian Markbåge>// - **[60fbb7b14](facebook/react@60fbb7b14 )**: [Flight] Implement FlightClient in terms of Thenable/Promises instead of throwing Promises ([#25260](facebook/react#25260)) //<Sebastian Markbåge>// - **[c91a1e03b](facebook/react@c91a1e03b )**: experimental_useEvent ([#25229](facebook/react#25229)) //<Lauren Tan>// - **[346c7d4c4](facebook/react@346c7d4c4 )**: straightford explicit types ([#25253](facebook/react#25253)) //<Jan Kassens>// - **[3401e9200](facebook/react@3401e9200 )**: useMemoCache implementation ([#25143](facebook/react#25143)) //<Joseph Savona>// - **[0556bab32](facebook/react@0556bab32 )**: [Transition Tracing] More Accurate End Time ([#25105](facebook/react#25105)) //<Luna Ruan>// - **[5fdcd23aa](facebook/react@5fdcd23aa )**: Flow: upgrade to 0.140 ([#25252](facebook/react#25252)) //<Jan Kassens>// - **[5c43c6f02](facebook/react@5c43c6f02 )**: Unwind the current workInProgress if it's suspended ([#25247](facebook/react#25247)) //<Sebastian Markbåge>// - **[e52fa4c57](facebook/react@e52fa4c57 )**: Add early exit to strict mode ([#25235](facebook/react#25235)) //<Samuel Susla>// - **[6aa38e74c](facebook/react@6aa38e74c )**: Flow: enable unsafe-addition error ([#25242](facebook/react#25242)) //<Jan Kassens>// - **[ba7b6f418](facebook/react@ba7b6f418 )**: Flow: upgrade to 0.132 ([#25244](facebook/react#25244)) //<Jan Kassens>// - **[9328988c0](facebook/react@9328988c0 )**: Flow: fix Fiber typed as any ([#25241](facebook/react#25241)) //<Jan Kassens>// - **[c739cef2f](facebook/react@c739cef2f )**: Flow: ReactFiberHotReloading recursive type ([#25225](facebook/react#25225)) //<Jan Kassens>// - **[c156ecd48](facebook/react@c156ecd48 )**: Add some test coverage for some error cases ([#25240](facebook/react#25240)) //<Sebastian Markbåge>// - **[3613284dc](facebook/react@3613284dc )**: experimental_use(context) for server components and ssr ([#25226](facebook/react#25226)) //<mofeiZ>// - **[269c4e975](facebook/react@269c4e975 )**: Prevent infinite re-renders in StrictMode + Offscreen ([#25203](facebook/react#25203)) //<Samuel Susla>// - **[8003ab9cf](facebook/react@8003ab9cf )**: Flow: remove explicit object syntax ([#25223](facebook/react#25223)) //<Jan Kassens>// - **[492c6e29e](facebook/react@492c6e29e )**: Flow: upgrade to 0.127 ([#25221](facebook/react#25221)) //<Jan Kassens>// - **[8a9e7b6ce](facebook/react@8a9e7b6ce )**: Flow: implicit-inexact-object=error ([#25210](facebook/react#25210)) //<Jan Kassens>// - **[37cc6bf12](facebook/react@37cc6bf12 )**: Remove useDeferredValue and useTransition from Flight subset ([#25215](facebook/react#25215)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions c28f313...0cac4d5 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D39696377 fbshipit-source-id: 113878d22d6244b8555b5fb86db1da5d43f7cfd9

* Prevent infinite re-render in StrictMode + Offscreen * Only fire effects for Offscreen when it is revealed * Move setting debug fiber into if branch * Move settings of debug fiber out of if branch

* 'main' of ssh://github.com/GrinZero/react: (51 commits) Flow: add simple explicit export types to Devtools (facebook#25251) [react devtools][easy] Centralize calls to patchConsoleUsingWindowValues (facebook#25222) Unwind the current workInProgress if it's suspended (facebook#25247) Add early exit to strict mode (facebook#25235) fix: prettier ignore removed and fixed (facebook#24811) Flow: enable unsafe-addition error (facebook#25242) Flow: upgrade to 0.132 (facebook#25244) Flow: fix Fiber typed as any (facebook#25241) Flow: ReactFiberHotReloading recursive type (facebook#25225) Add some test coverage for some error cases (facebook#25240) experimental_use(context) for server components and ssr (facebook#25226) Flow: upgrade to 0.131 (facebook#25224) Prevent infinite re-renders in StrictMode + Offscreen (facebook#25203) Flow: remove explicit object syntax (facebook#25223) Flow: upgrade to 0.127 (facebook#25221) Flow: enable exact_by_default (facebook#25220) [react devtools] Don't check for NODE_ENV==='test' because it never is (facebook#25186) [react devtools][easy] Change variable names, etc. (facebook#25211) Bump async from 2.6.3 to 2.6.4 in /fixtures/concurrent/time-slicing (facebook#24443) Flow: implicit-inexact-object=error (facebook#25210) ...

Summary: This sync includes the following changes: - **[0cac4d54c](facebook/react@0cac4d54c )**: Double invoked effects on suspended children ([facebook#25307](facebook/react#25307)) //<Samuel Susla>// - **[3d615fc14](facebook/react@3d615fc14 )**: Grammar. Removed doubles of the word "the". ([facebook#25295](facebook/react#25295)) //<Victoria Graf>// - **[6e3bc8a](facebook/react@6e3bc8a2e )**: [DevTools] Check if Proxy exists before creating DispatcherProxy ([facebook#25278](facebook/react#25278)) //<Tianyu Yao>// - **[e7fc04b29](facebook/react@e7fc04b29 )**: [react-dom] Reorganize react-dom internals to match react ([facebook#25277](facebook/react#25277)) //<Josh Story>// - **[0b54e0047](facebook/react@0b54e0047 )**: Handle rejections to avoid uncaught rejections ([facebook#25272](facebook/react#25272)) //<Sebastian Markbåge>// - **[c5d06fdc5](facebook/react@c5d06fdc5 )**: [Flight] Fix Webpack Chunk Loading ([facebook#25271](facebook/react#25271)) //<Sebastian Markbåge>// - **[975b64464](facebook/react@975b64464 )**: [Flight] response.readRoot() -> use(response) ([facebook#25267](facebook/react#25267)) //<Sebastian Markbåge>// - **[60fbb7b14](facebook/react@60fbb7b14 )**: [Flight] Implement FlightClient in terms of Thenable/Promises instead of throwing Promises ([facebook#25260](facebook/react#25260)) //<Sebastian Markbåge>// - **[c91a1e03b](facebook/react@c91a1e03b )**: experimental_useEvent ([facebook#25229](facebook/react#25229)) //<Lauren Tan>// - **[346c7d4c4](facebook/react@346c7d4c4 )**: straightford explicit types ([facebook#25253](facebook/react#25253)) //<Jan Kassens>// - **[3401e9200](facebook/react@3401e9200 )**: useMemoCache implementation ([facebook#25143](facebook/react#25143)) //<Joseph Savona>// - **[0556bab32](facebook/react@0556bab32 )**: [Transition Tracing] More Accurate End Time ([facebook#25105](facebook/react#25105)) //<Luna Ruan>// - **[5fdcd23aa](facebook/react@5fdcd23aa )**: Flow: upgrade to 0.140 ([facebook#25252](facebook/react#25252)) //<Jan Kassens>// - **[5c43c6f02](facebook/react@5c43c6f02 )**: Unwind the current workInProgress if it's suspended ([facebook#25247](facebook/react#25247)) //<Sebastian Markbåge>// - **[e52fa4c57](facebook/react@e52fa4c57 )**: Add early exit to strict mode ([facebook#25235](facebook/react#25235)) //<Samuel Susla>// - **[6aa38e74c](facebook/react@6aa38e74c )**: Flow: enable unsafe-addition error ([facebook#25242](facebook/react#25242)) //<Jan Kassens>// - **[ba7b6f418](facebook/react@ba7b6f418 )**: Flow: upgrade to 0.132 ([facebook#25244](facebook/react#25244)) //<Jan Kassens>// - **[9328988c0](facebook/react@9328988c0 )**: Flow: fix Fiber typed as any ([facebook#25241](facebook/react#25241)) //<Jan Kassens>// - **[c739cef2f](facebook/react@c739cef2f )**: Flow: ReactFiberHotReloading recursive type ([facebook#25225](facebook/react#25225)) //<Jan Kassens>// - **[c156ecd48](facebook/react@c156ecd48 )**: Add some test coverage for some error cases ([facebook#25240](facebook/react#25240)) //<Sebastian Markbåge>// - **[3613284dc](facebook/react@3613284dc )**: experimental_use(context) for server components and ssr ([facebook#25226](facebook/react#25226)) //<mofeiZ>// - **[269c4e975](facebook/react@269c4e975 )**: Prevent infinite re-renders in StrictMode + Offscreen ([facebook#25203](facebook/react#25203)) //<Samuel Susla>// - **[8003ab9cf](facebook/react@8003ab9cf )**: Flow: remove explicit object syntax ([facebook#25223](facebook/react#25223)) //<Jan Kassens>// - **[492c6e29e](facebook/react@492c6e29e )**: Flow: upgrade to 0.127 ([facebook#25221](facebook/react#25221)) //<Jan Kassens>// - **[8a9e7b6ce](facebook/react@8a9e7b6ce )**: Flow: implicit-inexact-object=error ([facebook#25210](facebook/react#25210)) //<Jan Kassens>// - **[37cc6bf12](facebook/react@37cc6bf12 )**: Remove useDeferredValue and useTransition from Flight subset ([facebook#25215](facebook/react#25215)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions c28f313...0cac4d5 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D39696377 fbshipit-source-id: 113878d22d6244b8555b5fb86db1da5d43f7cfd9

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 7, 2022

sammy-SC force-pushed the fix-infinite-rerender-in-strict-mode branch from 24f56a0 to 94da1f6 Compare September 7, 2022 16:16

sammy-SC force-pushed the fix-infinite-rerender-in-strict-mode branch from 94da1f6 to dca46a2 Compare September 7, 2022 16:19

sammy-SC force-pushed the fix-infinite-rerender-in-strict-mode branch 3 times, most recently from a598a71 to 041a359 Compare September 8, 2022 10:49

sammy-SC changed the title ~~Remove special case for OffscreenComponent in StrictMode~~ Prevent infinite re-renders in StrictMode + Offscreen Sep 8, 2022

Prevent infinite re-render in StrictMode + Offscreen

3523be2

sammy-SC force-pushed the fix-infinite-rerender-in-strict-mode branch from 041a359 to 3523be2 Compare September 8, 2022 10:53

sammy-SC marked this pull request as ready for review September 8, 2022 11:02

sammy-SC requested review from acdlite and rickhanlonii September 8, 2022 12:36

acdlite reviewed Sep 9, 2022

View reviewed changes

Only fire effects for Offscreen when it is revealed

fc298f5

sammy-SC requested a review from acdlite September 9, 2022 16:04

acdlite approved these changes Sep 9, 2022

View reviewed changes

packages/react-reconciler/src/ReactFiberWorkLoop.old.js Show resolved Hide resolved

sammy-SC force-pushed the fix-infinite-rerender-in-strict-mode branch from 9c2f26b to 536ae09 Compare September 9, 2022 18:01

Move setting debug fiber into if branch

618b38c

sammy-SC force-pushed the fix-infinite-rerender-in-strict-mode branch from 536ae09 to 618b38c Compare September 9, 2022 18:03

Move settings of debug fiber out of if branch

3b01085

acdlite approved these changes Sep 9, 2022

View reviewed changes

sammy-SC merged commit 269c4e9 into main Sep 9, 2022

sammy-SC deleted the fix-infinite-rerender-in-strict-mode branch September 9, 2022 20:10

sammy-SC mentioned this pull request Sep 9, 2022

Bug: useReducer bail-out w/ useEffect + Suspense causes infinite loop (StrictMode error with next/experimental builds) #25180

Closed

sammy-SC mentioned this pull request Sep 12, 2022

failing test demonstrating infinite updates #25179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent infinite re-renders in StrictMode + Offscreen #25203

Prevent infinite re-renders in StrictMode + Offscreen #25203

sammy-SC commented Sep 7, 2022 •

edited

Loading

sizebot commented Sep 7, 2022 •

edited

Loading

acdlite commented Sep 7, 2022 •

edited

Loading

acdlite commented Sep 7, 2022

gnoff commented Sep 7, 2022

sammy-SC commented Sep 7, 2022 •

edited

Loading

dai-shi commented Sep 8, 2022

sammy-SC commented Sep 8, 2022

acdlite Sep 9, 2022

sammy-SC Sep 9, 2022

acdlite Sep 9, 2022

acdlite Sep 9, 2022 •

edited

Loading

sammy-SC Sep 9, 2022

rickhanlonii Sep 11, 2022

sammy-SC Sep 12, 2022

rickhanlonii Sep 12, 2022

sammy-SC Sep 12, 2022

rickhanlonii Sep 12, 2022

acdlite left a comment

sammy-SC commented Sep 9, 2022

Prevent infinite re-renders in StrictMode + Offscreen #25203

Prevent infinite re-renders in StrictMode + Offscreen #25203

Conversation

sammy-SC commented Sep 7, 2022 • edited Loading

sizebot commented Sep 7, 2022 • edited Loading

Critical size changes

Significant size changes

acdlite commented Sep 7, 2022 • edited Loading

acdlite commented Sep 7, 2022

gnoff commented Sep 7, 2022

sammy-SC commented Sep 7, 2022 • edited Loading

dai-shi commented Sep 8, 2022

sammy-SC commented Sep 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite left a comment

Choose a reason for hiding this comment

sammy-SC commented Sep 9, 2022

sammy-SC commented Sep 7, 2022 •

edited

Loading

sizebot commented Sep 7, 2022 •

edited

Loading

acdlite commented Sep 7, 2022 •

edited

Loading

sammy-SC commented Sep 7, 2022 •

edited

Loading

acdlite Sep 9, 2022 •

edited

Loading