Fix sloppy factoring in `performSyncWorkOnRoot` (#21246)
* Warn if `finishedLanes` is empty in commit phase See #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.
Andrew Clark committed
Apr 20, 2021 at 11:14 UTC
bd7f4a013be3ef272a01874532bee71ad861e617
4 files changed
+82
-50
packages/react-reconciler/src/ReactFiberLane.new.js
-6
@@ -614,12 +614,6 @@ export function markRootExpired(root: FiberRoot, expiredLanes: Lanes) {
614
root.pendingLanes |= SyncLane;
615
}
616
617
-export function areLanesExpired(root: FiberRoot, lanes: Lanes) {
618
- const SyncLaneIndex = 0;
619
- const entanglements = root.entanglements;
620
- return (entanglements[SyncLaneIndex] & lanes) !== NoLanes;
621
-}
622
-
617
export function markRootMutableRead(root: FiberRoot, updateLane: Lane) {
618
root.mutableReadLanes |= updateLane & root.pendingLanes;
619
}
packages/react-reconciler/src/ReactFiberLane.old.js
-6
@@ -614,12 +614,6 @@ export function markRootExpired(root: FiberRoot, expiredLanes: Lanes) {
614
root.pendingLanes |= SyncLane;
615
}
616
617
-export function areLanesExpired(root: FiberRoot, lanes: Lanes) {
618
- const SyncLaneIndex = 0;
619
- const entanglements = root.entanglements;
620
- return (entanglements[SyncLaneIndex] & lanes) !== NoLanes;
621
-}
622
-
617
export function markRootMutableRead(root: FiberRoot, updateLane: Lane) {
618
root.mutableReadLanes |= updateLane & root.pendingLanes;
619
}
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+41
-19
@@ -163,7 +163,6 @@ import {
163
markRootPinged,
164
markRootExpired,
165
markRootFinished,
166
- areLanesExpired,
166
getHighestPriorityLane,
167
addFiberToLanesMap,
168
movePendingFibersToMemoized,
@@ -840,9 +839,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
839
// synchronously to block concurrent data mutations, and we'll includes
840
// all pending updates are included. If it still fails after the second
841
// attempt, we'll give up and commit the resulting tree.
843
- lanes = getLanesToRetrySynchronouslyOnError(root);
844
- if (lanes !== NoLanes) {
845
- exitStatus = renderRootSync(root, lanes);
842
+ const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root);
843
+ if (errorRetryLanes !== NoLanes) {
844
+ lanes = errorRetryLanes;
845
+ exitStatus = renderRootSync(root, errorRetryLanes);
846
}
847
}
848
@@ -1007,21 +1007,29 @@ function performSyncWorkOnRoot(root) {
1007
1008
flushPassiveEffects();
1009
1010
- let lanes;
1011
- let exitStatus;
1012
- if (
1013
- root === workInProgressRoot &&
1014
- areLanesExpired(root, workInProgressRootRenderLanes)
1010
+ let lanes = getNextLanes(root, NoLanes);
1011
+ if (includesSomeLane(lanes, SyncLane)) {
1012
+ if (
1013
+ root === workInProgressRoot &&
1014
+ includesSomeLane(lanes, workInProgressRootRenderLanes)
1015
+ ) {
1016
+ // There's a partial tree, and at least one of its lanes has expired. Finish
1017
+ // rendering it before rendering the rest of the expired work.
1018
+ lanes = workInProgressRootRenderLanes;
1019
+ }
1020
+ } else if (
1021
+ !(
1022
+ enableSyncDefaultUpdates &&
1023
+ (includesSomeLane(lanes, DefaultLane) ||
1024
+ includesSomeLane(lanes, DefaultHydrationLane))
1025
+ )
1026
) {
1016
- // There's a partial tree, and at least one of its lanes has expired. Finish
1017
- // rendering it before rendering the rest of the expired work.
1018
- lanes = workInProgressRootRenderLanes;
1019
- exitStatus = renderRootSync(root, lanes);
1020
- } else {
1021
- lanes = getNextLanes(root, NoLanes);
1022
- exitStatus = renderRootSync(root, lanes);
1027
+ // There's no remaining sync work left.
1028
+ ensureRootIsScheduled(root, now());
1029
+ return null;
1030
}
1031
1032
+ let exitStatus = renderRootSync(root, lanes);
1033
if (root.tag !== LegacyRoot && exitStatus === RootErrored) {
1034
executionContext |= RetryAfterError;
1035
@@ -1039,8 +1047,9 @@ function performSyncWorkOnRoot(root) {
1047
// synchronously to block concurrent data mutations, and we'll includes
1048
// all pending updates are included. If it still fails after the second
1049
// attempt, we'll give up and commit the resulting tree.
1042
- lanes = getLanesToRetrySynchronouslyOnError(root);
1043
- if (lanes !== NoLanes) {
1050
+ const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root);
1051
+ if (errorRetryLanes !== NoLanes) {
1052
+ lanes = errorRetryLanes;
1053
exitStatus = renderRootSync(root, lanes);
1054
}
1055
}
@@ -1058,7 +1067,11 @@ function performSyncWorkOnRoot(root) {
1067
const finishedWork: Fiber = (root.current.alternate: any);
1068
root.finishedWork = finishedWork;
1069
root.finishedLanes = lanes;
1061
- if (enableSyncDefaultUpdates && !includesSomeLane(lanes, SyncLane)) {
1070
+ if (
1071
+ enableSyncDefaultUpdates &&
1072
+ (includesSomeLane(lanes, DefaultLane) ||
1073
+ includesSomeLane(lanes, DefaultHydrationLane))
1074
+ ) {
1075
finishConcurrentRender(root, exitStatus, lanes);
1076
} else {
1077
commitRoot(root);
@@ -1826,6 +1839,15 @@ function commitRootImpl(root, renderPriorityLevel) {
1839
}
1840
1841
return null;
1842
+ } else {
1843
+ if (__DEV__) {
1844
+ if (lanes === NoLanes) {
1845
+ console.error(
1846
+ 'root.finishedLanes should not be empty during a commit. This is a ' +
1847
+ 'bug in React.',
1848
+ );
1849
+ }
1850
+ }
1851
}
1852
root.finishedWork = null;
1853
root.finishedLanes = NoLanes;
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+41
-19
@@ -163,7 +163,6 @@ import {
163
markRootPinged,
164
markRootExpired,
165
markRootFinished,
166
- areLanesExpired,
166
getHighestPriorityLane,
167
addFiberToLanesMap,
168
movePendingFibersToMemoized,
@@ -840,9 +839,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
839
// synchronously to block concurrent data mutations, and we'll includes
840
// all pending updates are included. If it still fails after the second
841
// attempt, we'll give up and commit the resulting tree.
843
- lanes = getLanesToRetrySynchronouslyOnError(root);
844
- if (lanes !== NoLanes) {
845
- exitStatus = renderRootSync(root, lanes);
842
+ const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root);
843
+ if (errorRetryLanes !== NoLanes) {
844
+ lanes = errorRetryLanes;
845
+ exitStatus = renderRootSync(root, errorRetryLanes);
846
}
847
}
848
@@ -1007,21 +1007,29 @@ function performSyncWorkOnRoot(root) {
1007
1008
flushPassiveEffects();
1009
1010
- let lanes;
1011
- let exitStatus;
1012
- if (
1013
- root === workInProgressRoot &&
1014
- areLanesExpired(root, workInProgressRootRenderLanes)
1010
+ let lanes = getNextLanes(root, NoLanes);
1011
+ if (includesSomeLane(lanes, SyncLane)) {
1012
+ if (
1013
+ root === workInProgressRoot &&
1014
+ includesSomeLane(lanes, workInProgressRootRenderLanes)
1015
+ ) {
1016
+ // There's a partial tree, and at least one of its lanes has expired. Finish
1017
+ // rendering it before rendering the rest of the expired work.
1018
+ lanes = workInProgressRootRenderLanes;
1019
+ }
1020
+ } else if (
1021
+ !(
1022
+ enableSyncDefaultUpdates &&
1023
+ (includesSomeLane(lanes, DefaultLane) ||
1024
+ includesSomeLane(lanes, DefaultHydrationLane))
1025
+ )
1026
) {
1016
- // There's a partial tree, and at least one of its lanes has expired. Finish
1017
- // rendering it before rendering the rest of the expired work.
1018
- lanes = workInProgressRootRenderLanes;
1019
- exitStatus = renderRootSync(root, lanes);
1020
- } else {
1021
- lanes = getNextLanes(root, NoLanes);
1022
- exitStatus = renderRootSync(root, lanes);
1027
+ // There's no remaining sync work left.
1028
+ ensureRootIsScheduled(root, now());
1029
+ return null;
1030
}
1031
1032
+ let exitStatus = renderRootSync(root, lanes);
1033
if (root.tag !== LegacyRoot && exitStatus === RootErrored) {
1034
executionContext |= RetryAfterError;
1035
@@ -1039,8 +1047,9 @@ function performSyncWorkOnRoot(root) {
1047
// synchronously to block concurrent data mutations, and we'll includes
1048
// all pending updates are included. If it still fails after the second
1049
// attempt, we'll give up and commit the resulting tree.
1042
- lanes = getLanesToRetrySynchronouslyOnError(root);
1043
- if (lanes !== NoLanes) {
1050
+ const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root);
1051
+ if (errorRetryLanes !== NoLanes) {
1052
+ lanes = errorRetryLanes;
1053
exitStatus = renderRootSync(root, lanes);
1054
}
1055
}
@@ -1058,7 +1067,11 @@ function performSyncWorkOnRoot(root) {
1067
const finishedWork: Fiber = (root.current.alternate: any);
1068
root.finishedWork = finishedWork;
1069
root.finishedLanes = lanes;
1061
- if (enableSyncDefaultUpdates && !includesSomeLane(lanes, SyncLane)) {
1070
+ if (
1071
+ enableSyncDefaultUpdates &&
1072
+ (includesSomeLane(lanes, DefaultLane) ||
1073
+ includesSomeLane(lanes, DefaultHydrationLane))
1074
+ ) {
1075
finishConcurrentRender(root, exitStatus, lanes);
1076
} else {
1077
commitRoot(root);
@@ -1826,6 +1839,15 @@ function commitRootImpl(root, renderPriorityLevel) {
1839
}
1840
1841
return null;
1842
+ } else {
1843
+ if (__DEV__) {
1844
+ if (lanes === NoLanes) {
1845
+ console.error(
1846
+ 'root.finishedLanes should not be empty during a commit. This is a ' +
1847
+ 'bug in React.',
1848
+ );
1849
+ }
1850
+ }
1851
}
1852
root.finishedWork = null;
1853
root.finishedLanes = NoLanes;