Invariant that throws when committing wrong tree (#15517)

If React finishes rendering a tree, delays committing it (e.g.
Suspense), then subsequently starts over or renders a new tree, the
pending tree is no longer valid. That's because rendering a new work-in
progress mutates the old one in place.

The current structure of the work loop makes this hard to reason about
because, although `renderRoot` and `commitRoot` are separate functions,
they can't be interleaved. If they are interleaved by accident, it
either results in inconsistent render output or invariant violations
that are hard to debug.

This commit adds an invariant that throws if the new tree is the same as
the old one. This won't prevent all bugs of this class, but it should
catch the most common kind.

To implement the invariant, I store the finished tree on a field on the
root. We already had a field for this, but it was only being used for
the unstable `createBatch` feature.

A more rigorous way to address this type of problem could be to unify
`renderRoot` and `commitRoot` into a single function, so that it's
harder to accidentally interleave the two phases. I plan to do something
like this in a follow-up.

Author: acdlite

packages/react-reconciler/src/ReactFiberRoot.js

@@ -46,7 +46,7 @@ type BaseFiberRootProperties = {|
     | Map<Thenable, Set<ExpirationTime>>
     | null,
 
-  pendingCommitExpirationTime: ExpirationTime,
+  finishedExpirationTime: ExpirationTime,
   // A finished work-in-progress HostRoot that's ready to be committed.
   finishedWork: Fiber | null,
   // Timeout handle returned by setTimeout. Used to cancel a pending timeout, if
@@ -99,7 +99,7 @@ function FiberRootNode(containerInfo, tag, hydrate) {
   this.containerInfo = containerInfo;
   this.pendingChildren = null;
   this.pingCache = null;
-  this.pendingCommitExpirationTime = NoWork;
+  this.finishedExpirationTime = NoWork;
   this.finishedWork = null;
   this.timeoutHandle = noTimeout;
   this.context = null;

packages/react-reconciler/src/ReactFiberScheduler.js

@@ -569,8 +569,6 @@ function resolveLocksOnRoot(root: FiberRoot, expirationTime: ExpirationTime) {
     firstBatch._defer &&
     firstBatch._expirationTime >= expirationTime
   ) {
-    root.finishedWork = root.current.alternate;
-    root.pendingCommitExpirationTime = expirationTime;
     scheduleCallback(NormalPriority, () => {
       firstBatch._onComplete();
       return null;
@@ -689,7 +687,8 @@ export function flushControlled(fn: () => mixed): void {
 }
 
 function prepareFreshStack(root, expirationTime) {
-  root.pendingCommitExpirationTime = NoWork;
+  root.finishedWork = null;
+  root.finishedExpirationTime = NoWork;
 
   const timeoutHandle = root.timeoutHandle;
   if (timeoutHandle !== noTimeout) {
@@ -741,10 +740,9 @@ function renderRoot(
     return null;
   }
 
-  if (root.pendingCommitExpirationTime === expirationTime) {
+  if (root.finishedExpirationTime === expirationTime) {
     // There's already a pending commit at this expiration time.
-    root.pendingCommitExpirationTime = NoWork;
-    return commitRoot.bind(null, root, expirationTime);
+    return commitRoot.bind(null, root);
   }
 
   flushPassiveEffects();
@@ -867,6 +865,9 @@ function renderRoot(
   // something suspended, wait to commit it after a timeout.
   stopFinishedWorkLoopTimer();
 
+  root.finishedWork = root.current.alternate;
+  root.finishedExpirationTime = expirationTime;
+
   const isLocked = resolveLocksOnRoot(root, expirationTime);
   if (isLocked) {
     // This root has a lock that prevents it from committing. Exit. If we begin
@@ -905,7 +906,7 @@ function renderRoot(
       }
       // If we're already rendering synchronously, commit the root in its
       // errored state.
-      return commitRoot.bind(null, root, expirationTime);
+      return commitRoot.bind(null, root);
     }
     case RootSuspended: {
       if (!isSync) {
@@ -929,19 +930,19 @@ function renderRoot(
             // priority work to do. Instead of committing the fallback
             // immediately, wait for more data to arrive.
             root.timeoutHandle = scheduleTimeout(
-              commitRoot.bind(null, root, expirationTime),
+              commitRoot.bind(null, root),
               msUntilTimeout,
             );
             return null;
           }
         }
       }
       // The work expired. Commit immediately.
-      return commitRoot.bind(null, root, expirationTime);
+      return commitRoot.bind(null, root);
     }
     case RootCompleted: {
       // The work completed. Ready to commit.
-      return commitRoot.bind(null, root, expirationTime);
+      return commitRoot.bind(null, root);
     }
     default: {
       invariant(false, 'Unknown root exit status.');
@@ -1223,11 +1224,8 @@ function resetChildExpirationTime(completedWork: Fiber) {
   completedWork.childExpirationTime = newChildExpirationTime;
 }
 
-function commitRoot(root, expirationTime) {
-  runWithPriority(
-    ImmediatePriority,
-    commitRootImpl.bind(null, root, expirationTime),
-  );
+function commitRoot(root) {
+  runWithPriority(ImmediatePriority, commitRootImpl.bind(null, root));
   // If there are passive effects, schedule a callback to flush them. This goes
   // outside commitRootImpl so that it inherits the priority of the render.
   if (rootWithPendingPassiveEffects !== null) {
@@ -1240,7 +1238,7 @@ function commitRoot(root, expirationTime) {
   return null;
 }
 
-function commitRootImpl(root, expirationTime) {
+function commitRootImpl(root) {
   flushPassiveEffects();
   flushRenderPhaseStrictModeWarningsInDEV();
   flushSuspensePriorityWarningInDEV();
@@ -1249,8 +1247,20 @@ function commitRootImpl(root, expirationTime) {
     workPhase !== RenderPhase && workPhase !== CommitPhase,
     'Should not already be working.',
   );
-  const finishedWork = root.current.alternate;
-  invariant(finishedWork !== null, 'Should have a work-in-progress root.');
+
+  const finishedWork = root.finishedWork;
+  const expirationTime = root.finishedExpirationTime;
+  if (finishedWork === null) {
+    return null;
+  }
+  root.finishedWork = null;
+  root.finishedExpirationTime = NoWork;
+
+  invariant(
+    finishedWork !== root.current,
+    'Cannot commit the same tree as before. This error is likely caused by ' +
+      'a bug in React. Please file an issue.',
+  );
 
   // commitRoot never returns a continuation; it always finishes synchronously.
   // So we can clear these now to allow a new callback to be scheduled.
@@ -1794,6 +1804,12 @@ export function pingSuspendedRoot(
   // Mark the time at which this ping was scheduled.
   root.pingTime = suspendedTime;
 
+  if (root.finishedExpirationTime === suspendedTime) {
+    // If there's a pending fallback waiting to commit, throw it away.
+    root.finishedExpirationTime = NoWork;
+    root.finishedWork = null;
+  }
+
   const currentTime = requestCurrentTime();
   const priorityLevel = inferPriorityFromExpirationTime(
     currentTime,

scripts/error-codes/codes.json

@@ -176,7 +176,7 @@
   "174": "Expected host context to exist. This error is likely caused by a bug in React. Please file an issue.",
   "175": "Expected prepareToHydrateHostInstance() to never be called. This error is likely caused by a bug in React. Please file an issue.",
   "176": "Expected prepareToHydrateHostTextInstance() to never be called. This error is likely caused by a bug in React. Please file an issue.",
-  "177": "Cannot commit the same tree as before. This is probably a bug related to the return field. This error is likely caused by a bug in React. Please file an issue.",
+  "177": "Cannot commit the same tree as before. This error is likely caused by a bug in React. Please file an issue.",
   "178": "Should have next effect. This error is likely caused by a bug in React. Please file an issue.",
   "179": "Should have a pending commit. This error is likely caused by a bug in React. Please file an issue.",
   "180": "Commit phase errors should be scheduled to recover with task priority. This error is likely caused by a bug in React. Please file an issue.",