Bugfix: SuspenseList incorrectly forces a fallback (#26453)

Fixes a bug in SuspenseList that @kassens found when deploying React to Meta. In some scenarios, SuspenseList would force the fallback of a deeply nested Suspense boundary into fallback mode, which should never happen under any circumstances — SuspenseList should only affect the nearest descendent Suspense boundaries, without going deeper. The cause was that the internal ForceSuspenseFallback context flag was not being properly reset when it reached the nearest Suspense boundary. It should only be propagated shallowly. We didn't discover this earlier because the scenario where it happens is not that common. To trigger the bug, you need to insert a new Suspense boundary into an already-mounted row of the list. But often when a new Suspense boundary is rendered, it suspends and shows a fallback, anyway, because its content hasn't loaded yet. Another reason we didn't discover this earlier is because there was another bug that was accidentally masking it, which was fixed by #25922. When that fix landed, it revealed this bug. The SuspenseList implementation is complicated but I'm not too concerned with the current messiness. It's an experimental API, and we intend to release it soon, but there are some known flaws and missing features that we need to address first regardless. We'll likely end up rewriting most of it. Co-authored-by: Jan Kassens <jkassens@meta.com> DiffTrain build for commit 51a7c45.
facebook · Mar 23, 2023 · 2a91152 · 2a91152
1 parent 7ec3959
commit 2a91152
Show file tree

Hide file tree

Showing 13 changed files with 232 additions and 135 deletions.
diff --git a/...ook-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-dev.js b/...ook-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-dev.js
@@ -5886,6 +5886,13 @@ function getShellBoundary() {
 function pushPrimaryTreeSuspenseHandler(handler) {
   // TODO: Pass as argument
   var current = handler.alternate;
+  // propagated a single level. For example, when ForceSuspenseFallback is set,
+  // it should only force the nearest Suspense boundary into fallback mode.
+
+  pushSuspenseListContext(
+    handler,
+    setDefaultShallowSuspenseListContext(suspenseStackCursor.current)
+  ); // Experimental feature: Some Suspense boundaries are marked as having an
   // to push a nested Suspense handler, because it will get replaced by the
   // outer fallback, anyway. Consider this as a future optimization.
 
@@ -5913,6 +5920,11 @@ function pushFallbackTreeSuspenseHandler(fiber) {
 }
 function pushOffscreenSuspenseHandler(fiber) {
   if (fiber.tag === OffscreenComponent) {
+    // A SuspenseList context is only pushed here to avoid a push/pop mismatch.
+    // Reuse the current value on the stack.
+    // TODO: We can avoid needing to push here by by forking popSuspenseHandler
+    // into separate functions for Suspense and Offscreen.
+    pushSuspenseListContext(fiber, suspenseStackCursor.current);
     push(suspenseHandlerStackCursor, fiber, fiber);
 
     if (shellBoundary !== null);
@@ -5935,6 +5947,7 @@ function pushOffscreenSuspenseHandler(fiber) {
   }
 }
 function reuseSuspenseHandlerOnStack(fiber) {
+  pushSuspenseListContext(fiber, suspenseStackCursor.current);
   push(suspenseHandlerStackCursor, getSuspenseHandler(), fiber);
 }
 function getSuspenseHandler() {
@@ -5947,6 +5960,8 @@ function popSuspenseHandler(fiber) {
     // Popping back into the shell.
     shellBoundary = null;
   }
+
+  popSuspenseListContext(fiber);
 } // SuspenseList context
 // TODO: Move to a separate module? We may change the SuspenseList
 // implementation to hide/show in the commit phase, anyway.
@@ -12176,6 +12191,8 @@ function shouldRemainOnFallback(current, workInProgress, renderLanes) {
   // If we're already showing a fallback, there are cases where we need to
   // remain on that fallback regardless of whether the content has resolved.
   // For example, SuspenseList coordinates when nested content appears.
+  // TODO: For compatibility with offscreen prerendering, this should also check
+  // whether the current fiber (if it exists) was visible in the previous tree.
   if (current !== null) {
     var suspenseState = current.memoizedState;
 
@@ -23632,7 +23649,7 @@ function createFiberRoot(
   return root;
 }
 
-var ReactVersion = "18.3.0-next-afb3d51dc-20230322";
+var ReactVersion = "18.3.0-next-51a7c45f8-20230322";
 
 // Might add PROFILE later.
 

diff --git a/...ok-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-prod.js b/...ok-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-prod.js
@@ -1950,6 +1950,7 @@ var suspenseHandlerStackCursor = createCursor(null),
   shellBoundary = null;
 function pushPrimaryTreeSuspenseHandler(handler) {
   var current = handler.alternate;
+  push(suspenseStackCursor, suspenseStackCursor.current & 1);
   push(suspenseHandlerStackCursor, handler);
   null === shellBoundary &&
     (null === current || null !== currentTreeHiddenStackCursor.current
@@ -1958,20 +1959,26 @@ function pushPrimaryTreeSuspenseHandler(handler) {
 }
 function pushOffscreenSuspenseHandler(fiber) {
   if (22 === fiber.tag) {
-    if ((push(suspenseHandlerStackCursor, fiber), null === shellBoundary)) {
+    if (
+      (push(suspenseStackCursor, suspenseStackCursor.current),
+      push(suspenseHandlerStackCursor, fiber),
+      null === shellBoundary)
+    ) {
       var current = fiber.alternate;
       null !== current &&
         null !== current.memoizedState &&
         (shellBoundary = fiber);
     }
-  } else reuseSuspenseHandlerOnStack();
+  } else reuseSuspenseHandlerOnStack(fiber);
 }
 function reuseSuspenseHandlerOnStack() {
+  push(suspenseStackCursor, suspenseStackCursor.current);
   push(suspenseHandlerStackCursor, suspenseHandlerStackCursor.current);
 }
 function popSuspenseHandler(fiber) {
   pop(suspenseHandlerStackCursor);
   shellBoundary === fiber && (shellBoundary = null);
+  pop(suspenseStackCursor);
 }
 var suspenseStackCursor = createCursor(0);
 function findFirstSuspended(row) {
@@ -3220,11 +3227,11 @@ function updateOffscreenComponent(current, workInProgress, renderLanes) {
     null !== prevState
       ? (pushTransition(workInProgress, prevState.cachePool),
         pushHiddenContext(workInProgress, prevState),
-        reuseSuspenseHandlerOnStack(),
+        reuseSuspenseHandlerOnStack(workInProgress),
         (workInProgress.memoizedState = null))
       : (null !== current && pushTransition(workInProgress, null),
         reuseHiddenContextOnStack(),
-        reuseSuspenseHandlerOnStack());
+        reuseSuspenseHandlerOnStack(workInProgress));
   reconcileChildren(current, workInProgress, nextChildren, renderLanes);
   return workInProgress.child;
 }
@@ -3579,7 +3586,7 @@ function updateSuspenseComponent(current, workInProgress, renderLanes) {
     current = nextProps.fallback;
     if (showFallback)
       return (
-        reuseSuspenseHandlerOnStack(),
+        reuseSuspenseHandlerOnStack(workInProgress),
         (nextProps = workInProgress.mode),
         (showFallback = workInProgress.child),
         (didSuspend = { mode: "hidden", children: didSuspend }),
@@ -3625,7 +3632,7 @@ function updateSuspenseComponent(current, workInProgress, renderLanes) {
       );
   }
   if (showFallback) {
-    reuseSuspenseHandlerOnStack();
+    reuseSuspenseHandlerOnStack(workInProgress);
     showFallback = nextProps.fallback;
     didSuspend = workInProgress.mode;
     JSCompiler_temp = current.child;
@@ -3751,12 +3758,12 @@ function updateDehydratedSuspenseComponent(
       );
     if (null !== workInProgress.memoizedState)
       return (
-        reuseSuspenseHandlerOnStack(),
+        reuseSuspenseHandlerOnStack(workInProgress),
         (workInProgress.child = current.child),
         (workInProgress.flags |= 128),
         null
       );
-    reuseSuspenseHandlerOnStack();
+    reuseSuspenseHandlerOnStack(workInProgress);
     suspenseState = nextProps.fallback;
     didSuspend = workInProgress.mode;
     nextProps = createFiberFromOffscreen(
@@ -8558,19 +8565,19 @@ function wrapFiber(fiber) {
     fiberToWrapper.set(fiber, wrapper));
   return wrapper;
 }
-var devToolsConfig$jscomp$inline_1018 = {
+var devToolsConfig$jscomp$inline_1029 = {
   findFiberByHostInstance: function () {
     throw Error("TestRenderer does not support findFiberByHostInstance()");
   },
   bundleType: 0,
-  version: "18.3.0-next-afb3d51dc-20230322",
+  version: "18.3.0-next-51a7c45f8-20230322",
   rendererPackageName: "react-test-renderer"
 };
-var internals$jscomp$inline_1206 = {
-  bundleType: devToolsConfig$jscomp$inline_1018.bundleType,
-  version: devToolsConfig$jscomp$inline_1018.version,
-  rendererPackageName: devToolsConfig$jscomp$inline_1018.rendererPackageName,
-  rendererConfig: devToolsConfig$jscomp$inline_1018.rendererConfig,
+var internals$jscomp$inline_1217 = {
+  bundleType: devToolsConfig$jscomp$inline_1029.bundleType,
+  version: devToolsConfig$jscomp$inline_1029.version,
+  rendererPackageName: devToolsConfig$jscomp$inline_1029.rendererPackageName,
+  rendererConfig: devToolsConfig$jscomp$inline_1029.rendererConfig,
   overrideHookState: null,
   overrideHookStateDeletePath: null,
   overrideHookStateRenamePath: null,
@@ -8587,26 +8594,26 @@ var internals$jscomp$inline_1206 = {
     return null === fiber ? null : fiber.stateNode;
   },
   findFiberByHostInstance:
-    devToolsConfig$jscomp$inline_1018.findFiberByHostInstance ||
+    devToolsConfig$jscomp$inline_1029.findFiberByHostInstance ||
     emptyFindFiberByHostInstance,
   findHostInstancesForRefresh: null,
   scheduleRefresh: null,
   scheduleRoot: null,
   setRefreshHandler: null,
   getCurrentFiber: null,
-  reconcilerVersion: "18.3.0-next-afb3d51dc-20230322"
+  reconcilerVersion: "18.3.0-next-51a7c45f8-20230322"
 };
 if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
-  var hook$jscomp$inline_1207 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
+  var hook$jscomp$inline_1218 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
   if (
-    !hook$jscomp$inline_1207.isDisabled &&
-    hook$jscomp$inline_1207.supportsFiber
+    !hook$jscomp$inline_1218.isDisabled &&
+    hook$jscomp$inline_1218.supportsFiber
   )
     try {
-      (rendererID = hook$jscomp$inline_1207.inject(
-        internals$jscomp$inline_1206
+      (rendererID = hook$jscomp$inline_1218.inject(
+        internals$jscomp$inline_1217
       )),
-        (injectedHook = hook$jscomp$inline_1207);
+        (injectedHook = hook$jscomp$inline_1218);
     } catch (err) {}
 }
 exports._Scheduler = Scheduler;

diff --git a/...source/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-profiling.js b/...source/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-profiling.js
@@ -1968,6 +1968,7 @@ var suspenseHandlerStackCursor = createCursor(null),
   shellBoundary = null;
 function pushPrimaryTreeSuspenseHandler(handler) {
   var current = handler.alternate;
+  push(suspenseStackCursor, suspenseStackCursor.current & 1);
   push(suspenseHandlerStackCursor, handler);
   null === shellBoundary &&
     (null === current || null !== currentTreeHiddenStackCursor.current
@@ -1976,20 +1977,26 @@ function pushPrimaryTreeSuspenseHandler(handler) {
 }
 function pushOffscreenSuspenseHandler(fiber) {
   if (22 === fiber.tag) {
-    if ((push(suspenseHandlerStackCursor, fiber), null === shellBoundary)) {
+    if (
+      (push(suspenseStackCursor, suspenseStackCursor.current),
+      push(suspenseHandlerStackCursor, fiber),
+      null === shellBoundary)
+    ) {
       var current = fiber.alternate;
       null !== current &&
         null !== current.memoizedState &&
         (shellBoundary = fiber);
     }
-  } else reuseSuspenseHandlerOnStack();
+  } else reuseSuspenseHandlerOnStack(fiber);
 }
 function reuseSuspenseHandlerOnStack() {
+  push(suspenseStackCursor, suspenseStackCursor.current);
   push(suspenseHandlerStackCursor, suspenseHandlerStackCursor.current);
 }
 function popSuspenseHandler(fiber) {
   pop(suspenseHandlerStackCursor);
   shellBoundary === fiber && (shellBoundary = null);
+  pop(suspenseStackCursor);
 }
 var suspenseStackCursor = createCursor(0);
 function findFirstSuspended(row) {
@@ -3300,11 +3307,11 @@ function updateOffscreenComponent(current, workInProgress, renderLanes) {
     null !== prevState
       ? (pushTransition(workInProgress, prevState.cachePool),
         pushHiddenContext(workInProgress, prevState),
-        reuseSuspenseHandlerOnStack(),
+        reuseSuspenseHandlerOnStack(workInProgress),
         (workInProgress.memoizedState = null))
       : (null !== current && pushTransition(workInProgress, null),
         reuseHiddenContextOnStack(),
-        reuseSuspenseHandlerOnStack());
+        reuseSuspenseHandlerOnStack(workInProgress));
   reconcileChildren(current, workInProgress, nextChildren, renderLanes);
   return workInProgress.child;
 }
@@ -3663,7 +3670,7 @@ function updateSuspenseComponent(current, workInProgress, renderLanes) {
     current = nextProps.fallback;
     if (showFallback)
       return (
-        reuseSuspenseHandlerOnStack(),
+        reuseSuspenseHandlerOnStack(workInProgress),
         (nextProps = workInProgress.mode),
         (showFallback = workInProgress.child),
         (didSuspend = { mode: "hidden", children: didSuspend }),
@@ -3714,7 +3721,7 @@ function updateSuspenseComponent(current, workInProgress, renderLanes) {
       );
   }
   if (showFallback) {
-    reuseSuspenseHandlerOnStack();
+    reuseSuspenseHandlerOnStack(workInProgress);
     showFallback = nextProps.fallback;
     didSuspend = workInProgress.mode;
     JSCompiler_temp = current.child;
@@ -3845,12 +3852,12 @@ function updateDehydratedSuspenseComponent(
       );
     if (null !== workInProgress.memoizedState)
       return (
-        reuseSuspenseHandlerOnStack(),
+        reuseSuspenseHandlerOnStack(workInProgress),
         (workInProgress.child = current.child),
         (workInProgress.flags |= 128),
         null
       );
-    reuseSuspenseHandlerOnStack();
+    reuseSuspenseHandlerOnStack(workInProgress);
     suspenseState = nextProps.fallback;
     didSuspend = workInProgress.mode;
     nextProps = createFiberFromOffscreen(
@@ -8983,19 +8990,19 @@ function wrapFiber(fiber) {
     fiberToWrapper.set(fiber, wrapper));
   return wrapper;
 }
-var devToolsConfig$jscomp$inline_1061 = {
+var devToolsConfig$jscomp$inline_1072 = {
   findFiberByHostInstance: function () {
     throw Error("TestRenderer does not support findFiberByHostInstance()");
   },
   bundleType: 0,
-  version: "18.3.0-next-afb3d51dc-20230322",
+  version: "18.3.0-next-51a7c45f8-20230322",
   rendererPackageName: "react-test-renderer"
 };
-var internals$jscomp$inline_1247 = {
-  bundleType: devToolsConfig$jscomp$inline_1061.bundleType,
-  version: devToolsConfig$jscomp$inline_1061.version,
-  rendererPackageName: devToolsConfig$jscomp$inline_1061.rendererPackageName,
-  rendererConfig: devToolsConfig$jscomp$inline_1061.rendererConfig,
+var internals$jscomp$inline_1258 = {
+  bundleType: devToolsConfig$jscomp$inline_1072.bundleType,
+  version: devToolsConfig$jscomp$inline_1072.version,
+  rendererPackageName: devToolsConfig$jscomp$inline_1072.rendererPackageName,
+  rendererConfig: devToolsConfig$jscomp$inline_1072.rendererConfig,
   overrideHookState: null,
   overrideHookStateDeletePath: null,
   overrideHookStateRenamePath: null,
@@ -9012,26 +9019,26 @@ var internals$jscomp$inline_1247 = {
     return null === fiber ? null : fiber.stateNode;
   },
   findFiberByHostInstance:
-    devToolsConfig$jscomp$inline_1061.findFiberByHostInstance ||
+    devToolsConfig$jscomp$inline_1072.findFiberByHostInstance ||
     emptyFindFiberByHostInstance,
   findHostInstancesForRefresh: null,
   scheduleRefresh: null,
   scheduleRoot: null,
   setRefreshHandler: null,
   getCurrentFiber: null,
-  reconcilerVersion: "18.3.0-next-afb3d51dc-20230322"
+  reconcilerVersion: "18.3.0-next-51a7c45f8-20230322"
 };
 if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
-  var hook$jscomp$inline_1248 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
+  var hook$jscomp$inline_1259 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
   if (
-    !hook$jscomp$inline_1248.isDisabled &&
-    hook$jscomp$inline_1248.supportsFiber
+    !hook$jscomp$inline_1259.isDisabled &&
+    hook$jscomp$inline_1259.supportsFiber
   )
     try {
-      (rendererID = hook$jscomp$inline_1248.inject(
-        internals$jscomp$inline_1247
+      (rendererID = hook$jscomp$inline_1259.inject(
+        internals$jscomp$inline_1258
       )),
-        (injectedHook = hook$jscomp$inline_1248);
+        (injectedHook = hook$jscomp$inline_1259);
     } catch (err) {}
 }
 exports._Scheduler = Scheduler;

diff --git a/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-dev.js b/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-dev.js
@@ -27,7 +27,7 @@ if (
 }
           "use strict";
 
-var ReactVersion = "18.3.0-next-afb3d51dc-20230322";
+var ReactVersion = "18.3.0-next-51a7c45f8-20230322";
 
 // ATTENTION
 // When adding new symbols to this file,

diff --git a/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-prod.js b/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-prod.js
@@ -639,4 +639,4 @@ exports.useSyncExternalStore = function (
   );
 };
 exports.useTransition = useTransition;
-exports.version = "18.3.0-next-afb3d51dc-20230322";
+exports.version = "18.3.0-next-51a7c45f8-20230322";
diff --git a/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-profiling.js b/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-profiling.js
@@ -642,7 +642,7 @@ exports.useSyncExternalStore = function (
   );
 };
 exports.useTransition = useTransition;
-exports.version = "18.3.0-next-afb3d51dc-20230322";
+exports.version = "18.3.0-next-51a7c45f8-20230322";
 
           /* global __REACT_DEVTOOLS_GLOBAL_HOOK__ */
 if (

diff --git a/compiled-rn/facebook-fbsource/xplat/js/react-native-github/Libraries/Renderer/REVISION b/compiled-rn/facebook-fbsource/xplat/js/react-native-github/Libraries/Renderer/REVISION
@@ -1 +1 @@
-afb3d51dc6310f0dbeffdd303eb3c6895e6f7db0
+51a7c45f8799cab903693fcfdd305ce84ba15273
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		afb3d51dc6310f0dbeffdd303eb3c6895e6f7db0
		51a7c45f8799cab903693fcfdd305ce84ba15273