Fix sloppy factoring in performSyncWorkOnRoot (facebook#21246)

* Warn if `finishedLanes` is empty in commit phase

See facebook#21233 for context.

* Fix sloppy factoring when assigning finishedLanes

`finishedLanes` is assigned in `performSyncWorkOnRoot` and
`performSyncWorkOnRoot`. It's meant to represent whichever lanes we
used to render, but because of some sloppy factoring, it can sometimes
equal `NoLanes`.

The fixes are:
- Always check if the lanes are not `NoLanes` before entering the work
loop. There was a branch where this wasn't always true.
- In `performSyncWorkOnRoot`, don't assume the next lanes are sync; the
priority may have changed, or they may have been flushed by a
previous task.
- Don't re-assign the `lanes` variable (the one that gets assigned to
`finishedLanes` until right before we enter the work loop, so that it
is always corresponds to the newest complete root.

Author: acdlite

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

@@ -614,12 +614,6 @@ export function markRootExpired(root: FiberRoot, expiredLanes: Lanes) {
   root.pendingLanes |= SyncLane;
 }
 
-export function areLanesExpired(root: FiberRoot, lanes: Lanes) {
-  const SyncLaneIndex = 0;
-  const entanglements = root.entanglements;
-  return (entanglements[SyncLaneIndex] & lanes) !== NoLanes;
-}
-
 export function markRootMutableRead(root: FiberRoot, updateLane: Lane) {
   root.mutableReadLanes |= updateLane & root.pendingLanes;
 }

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

@@ -614,12 +614,6 @@ export function markRootExpired(root: FiberRoot, expiredLanes: Lanes) {
   root.pendingLanes |= SyncLane;
 }
 
-export function areLanesExpired(root: FiberRoot, lanes: Lanes) {
-  const SyncLaneIndex = 0;
-  const entanglements = root.entanglements;
-  return (entanglements[SyncLaneIndex] & lanes) !== NoLanes;
-}
-
 export function markRootMutableRead(root: FiberRoot, updateLane: Lane) {
   root.mutableReadLanes |= updateLane & root.pendingLanes;
 }

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

@@ -163,7 +163,6 @@ import {
   markRootPinged,
   markRootExpired,
   markRootFinished,
-  areLanesExpired,
   getHighestPriorityLane,
   addFiberToLanesMap,
   movePendingFibersToMemoized,
@@ -840,9 +839,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
       // synchronously to block concurrent data mutations, and we'll includes
       // all pending updates are included. If it still fails after the second
       // attempt, we'll give up and commit the resulting tree.
-      lanes = getLanesToRetrySynchronouslyOnError(root);
-      if (lanes !== NoLanes) {
-        exitStatus = renderRootSync(root, lanes);
+      const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root);
+      if (errorRetryLanes !== NoLanes) {
+        lanes = errorRetryLanes;
+        exitStatus = renderRootSync(root, errorRetryLanes);
       }
     }
 
@@ -1007,21 +1007,29 @@ function performSyncWorkOnRoot(root) {
 
   flushPassiveEffects();
 
-  let lanes;
-  let exitStatus;
-  if (
-    root === workInProgressRoot &&
-    areLanesExpired(root, workInProgressRootRenderLanes)
+  let lanes = getNextLanes(root, NoLanes);
+  if (includesSomeLane(lanes, SyncLane)) {
+    if (
+      root === workInProgressRoot &&
+      includesSomeLane(lanes, workInProgressRootRenderLanes)
+    ) {
+      // There's a partial tree, and at least one of its lanes has expired. Finish
+      // rendering it before rendering the rest of the expired work.
+      lanes = workInProgressRootRenderLanes;
+    }
+  } else if (
+    !(
+      enableSyncDefaultUpdates &&
+      (includesSomeLane(lanes, DefaultLane) ||
+        includesSomeLane(lanes, DefaultHydrationLane))
+    )
   ) {
-    // There's a partial tree, and at least one of its lanes has expired. Finish
-    // rendering it before rendering the rest of the expired work.
-    lanes = workInProgressRootRenderLanes;
-    exitStatus = renderRootSync(root, lanes);
-  } else {
-    lanes = getNextLanes(root, NoLanes);
-    exitStatus = renderRootSync(root, lanes);
+    // There's no remaining sync work left.
+    ensureRootIsScheduled(root, now());
+    return null;
   }
 
+  let exitStatus = renderRootSync(root, lanes);
   if (root.tag !== LegacyRoot && exitStatus === RootErrored) {
     executionContext |= RetryAfterError;
 
@@ -1039,8 +1047,9 @@ function performSyncWorkOnRoot(root) {
     // synchronously to block concurrent data mutations, and we'll includes
     // all pending updates are included. If it still fails after the second
     // attempt, we'll give up and commit the resulting tree.
-    lanes = getLanesToRetrySynchronouslyOnError(root);
-    if (lanes !== NoLanes) {
+    const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root);
+    if (errorRetryLanes !== NoLanes) {
+      lanes = errorRetryLanes;
       exitStatus = renderRootSync(root, lanes);
     }
   }
@@ -1058,7 +1067,11 @@ function performSyncWorkOnRoot(root) {
   const finishedWork: Fiber = (root.current.alternate: any);
   root.finishedWork = finishedWork;
   root.finishedLanes = lanes;
-  if (enableSyncDefaultUpdates && !includesSomeLane(lanes, SyncLane)) {
+  if (
+    enableSyncDefaultUpdates &&
+    (includesSomeLane(lanes, DefaultLane) ||
+      includesSomeLane(lanes, DefaultHydrationLane))
+  ) {
     finishConcurrentRender(root, exitStatus, lanes);
   } else {
     commitRoot(root);
@@ -1826,6 +1839,15 @@ function commitRootImpl(root, renderPriorityLevel) {
     }
 
     return null;
+  } else {
+    if (__DEV__) {
+      if (lanes === NoLanes) {
+        console.error(
+          'root.finishedLanes should not be empty during a commit. This is a ' +
+            'bug in React.',
+        );
+      }
+    }
   }
   root.finishedWork = null;
   root.finishedLanes = NoLanes;

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

@@ -163,7 +163,6 @@ import {
   markRootPinged,
   markRootExpired,
   markRootFinished,
-  areLanesExpired,
   getHighestPriorityLane,
   addFiberToLanesMap,
   movePendingFibersToMemoized,
@@ -840,9 +839,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
       // synchronously to block concurrent data mutations, and we'll includes
       // all pending updates are included. If it still fails after the second
       // attempt, we'll give up and commit the resulting tree.
-      lanes = getLanesToRetrySynchronouslyOnError(root);
-      if (lanes !== NoLanes) {
-        exitStatus = renderRootSync(root, lanes);
+      const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root);
+      if (errorRetryLanes !== NoLanes) {
+        lanes = errorRetryLanes;
+        exitStatus = renderRootSync(root, errorRetryLanes);
       }
     }
 
@@ -1007,21 +1007,29 @@ function performSyncWorkOnRoot(root) {
 
   flushPassiveEffects();
 
-  let lanes;
-  let exitStatus;
-  if (
-    root === workInProgressRoot &&
-    areLanesExpired(root, workInProgressRootRenderLanes)
+  let lanes = getNextLanes(root, NoLanes);
+  if (includesSomeLane(lanes, SyncLane)) {
+    if (
+      root === workInProgressRoot &&
+      includesSomeLane(lanes, workInProgressRootRenderLanes)
+    ) {
+      // There's a partial tree, and at least one of its lanes has expired. Finish
+      // rendering it before rendering the rest of the expired work.
+      lanes = workInProgressRootRenderLanes;
+    }
+  } else if (
+    !(
+      enableSyncDefaultUpdates &&
+      (includesSomeLane(lanes, DefaultLane) ||
+        includesSomeLane(lanes, DefaultHydrationLane))
+    )
   ) {
-    // There's a partial tree, and at least one of its lanes has expired. Finish
-    // rendering it before rendering the rest of the expired work.
-    lanes = workInProgressRootRenderLanes;
-    exitStatus = renderRootSync(root, lanes);
-  } else {
-    lanes = getNextLanes(root, NoLanes);
-    exitStatus = renderRootSync(root, lanes);
+    // There's no remaining sync work left.
+    ensureRootIsScheduled(root, now());
+    return null;
   }
 
+  let exitStatus = renderRootSync(root, lanes);
   if (root.tag !== LegacyRoot && exitStatus === RootErrored) {
     executionContext |= RetryAfterError;
 
@@ -1039,8 +1047,9 @@ function performSyncWorkOnRoot(root) {
     // synchronously to block concurrent data mutations, and we'll includes
     // all pending updates are included. If it still fails after the second
     // attempt, we'll give up and commit the resulting tree.
-    lanes = getLanesToRetrySynchronouslyOnError(root);
-    if (lanes !== NoLanes) {
+    const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root);
+    if (errorRetryLanes !== NoLanes) {
+      lanes = errorRetryLanes;
       exitStatus = renderRootSync(root, lanes);
     }
   }
@@ -1058,7 +1067,11 @@ function performSyncWorkOnRoot(root) {
   const finishedWork: Fiber = (root.current.alternate: any);
   root.finishedWork = finishedWork;
   root.finishedLanes = lanes;
-  if (enableSyncDefaultUpdates && !includesSomeLane(lanes, SyncLane)) {
+  if (
+    enableSyncDefaultUpdates &&
+    (includesSomeLane(lanes, DefaultLane) ||
+      includesSomeLane(lanes, DefaultHydrationLane))
+  ) {
     finishConcurrentRender(root, exitStatus, lanes);
   } else {
     commitRoot(root);
@@ -1826,6 +1839,15 @@ function commitRootImpl(root, renderPriorityLevel) {
     }
 
     return null;
+  } else {
+    if (__DEV__) {
+      if (lanes === NoLanes) {
+        console.error(
+          'root.finishedLanes should not be empty during a commit. This is a ' +
+            'bug in React.',
+        );
+      }
+    }
   }
   root.finishedWork = null;
   root.finishedLanes = NoLanes;