Fix: Stylesheet in error UI suspends indefinitely

This fixes the regression test added in the previous commit. The "Suspensey
commit" implementation relies on the `shouldRemainOnPreviousScreen` function to
determine whether to 1) suspend the commit 2) activate a parent fallback and
schedule a retry. The issue was that we were sometimes attempting option 2 even
when there was no parent fallback.

Part of the reason this bug landed is due to how `throwException` is structured.
In the case of Suspensey commits, we pass a special "noop" thenable to
`throwException` as a way to trigger the Suspense path. This special thenable
must never have a listener attached to it. This is not a great way to structure
the logic, it's just a consequence of how the code evolved over time. We should
refactor it into multiple functions so we can trigger a fallback directly
without having to check the type. In the meantime, I added an internal warning
to help detect similar mistakes in the future.

Author: acdlite

packages/react-dom/src/__tests__/ReactDOMFloat-test.js

@@ -3385,7 +3385,6 @@ body {
     );
   });
 
-  // @gate FIXME
   it('loading a stylesheet as part of an error boundary UI, during initial render', async () => {
     class ErrorBoundary extends React.Component {
       state = {error: null};

packages/react-reconciler/src/ReactFiberThenable.js

@@ -42,7 +42,16 @@ export const SuspenseyCommitException: mixed = new Error(
 // TODO: It would be better to refactor throwException into multiple functions
 // so we can trigger a fallback directly without having to check the type. But
 // for now this will do.
-export const noopSuspenseyCommitThenable = {then() {}};
+export const noopSuspenseyCommitThenable = {
+  then() {
+    if (__DEV__) {
+      console.error(
+        'Internal React error: A listener was unexpectedly attached to a ' +
+          '"noop" thenable. This is a bug in React. Please file an issue.',
+      );
+    }
+  },
+};
 
 export function createThenableState(): ThenableState {
   // The ThenableState is created the first time a component suspends. If it

packages/react-reconciler/src/ReactFiberThrow.js

@@ -438,8 +438,15 @@ function throwException(
               } else {
                 retryQueue.add(wakeable);
               }
+
+              // We only attach ping listeners in concurrent mode. Legacy
+              // Suspense always commits fallbacks synchronously, so there are
+              // no pings.
+              if (suspenseBoundary.mode & ConcurrentMode) {
+                attachPingListener(root, wakeable, rootRenderLanes);
+              }
             }
-            break;
+            return;
           }
           case OffscreenComponent: {
             if (suspenseBoundary.mode & ConcurrentMode) {
@@ -466,24 +473,17 @@ function throwException(
                     retryQueue.add(wakeable);
                   }
                 }
+
+                attachPingListener(root, wakeable, rootRenderLanes);
               }
-              break;
+              return;
             }
-            // Fall through
-          }
-          default: {
-            throw new Error(
-              `Unexpected Suspense handler tag (${suspenseBoundary.tag}). This ` +
-                'is a bug in React.',
-            );
           }
         }
-        // We only attach ping listeners in concurrent mode. Legacy Suspense always
-        // commits fallbacks synchronously, so there are no pings.
-        if (suspenseBoundary.mode & ConcurrentMode) {
-          attachPingListener(root, wakeable, rootRenderLanes);
-        }
-        return;
+        throw new Error(
+          `Unexpected Suspense handler tag (${suspenseBoundary.tag}). This ` +
+            'is a bug in React.',
+        );
       } else {
         // No boundary was found. Unless this is a sync update, this is OK.
         // We can suspend and wait for more data to arrive.

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -1687,6 +1687,16 @@ export function shouldRemainOnPreviousScreen(): boolean {
   // takes into account both the priority of render and also whether showing a
   // fallback would produce a desirable user experience.
 
+  const handler = getSuspenseHandler();
+  if (handler === null) {
+    // There's no Suspense boundary that can provide a fallback. We have no
+    // choice but to remain on the previous screen.
+    // NOTE: We do this even for sync updates, for lack of any better option. In
+    // the future, we may change how we handle this, like by putting the whole
+    // root into a "detached" mode.
+    return true;
+  }
+
   // TODO: Once `use` has fully replaced the `throw promise` pattern, we should
   // be able to remove the equivalent check in finishConcurrentRender, and rely
   // just on this one.
@@ -1705,29 +1715,22 @@ export function shouldRemainOnPreviousScreen(): boolean {
     }
   }
 
-  const handler = getSuspenseHandler();
-  if (handler === null) {
-    // TODO: We should support suspending in the case where there's no
-    // parent Suspense boundary, even outside a transition. Somehow. Otherwise,
-    // an uncached promise can fall into an infinite loop.
-  } else {
-    if (
-      includesOnlyRetries(workInProgressRootRenderLanes) ||
-      // In this context, an OffscreenLane counts as a Retry
-      // TODO: It's become increasingly clear that Retries and Offscreen are
-      // deeply connected. They probably can be unified further.
-      includesSomeLane(workInProgressRootRenderLanes, OffscreenLane)
-    ) {
-      // During a retry, we can suspend rendering if the nearest Suspense boundary
-      // is the boundary of the "shell", because we're guaranteed not to block
-      // any new content from appearing.
-      //
-      // The reason we must check if this is a retry is because it guarantees
-      // that suspending the work loop won't block an actual update, because
-      // retries don't "update" anything; they fill in fallbacks that were left
-      // behind by a previous transition.
-      return handler === getShellBoundary();
-    }
+  if (
+    includesOnlyRetries(workInProgressRootRenderLanes) ||
+    // In this context, an OffscreenLane counts as a Retry
+    // TODO: It's become increasingly clear that Retries and Offscreen are
+    // deeply connected. They probably can be unified further.
+    includesSomeLane(workInProgressRootRenderLanes, OffscreenLane)
+  ) {
+    // During a retry, we can suspend rendering if the nearest Suspense boundary
+    // is the boundary of the "shell", because we're guaranteed not to block
+    // any new content from appearing.
+    //
+    // The reason we must check if this is a retry is because it guarantees
+    // that suspending the work loop won't block an actual update, because
+    // retries don't "update" anything; they fill in fallbacks that were left
+    // behind by a previous transition.
+    return handler === getShellBoundary();
   }
 
   // For all other Lanes besides Transitions and Retries, we should not wait