Skip to content

Commit 628d185

Browse files
committed
Re-arrange slightly to prevent refactor hazard
It should not be possible to perform any work on a root without calling `ensureRootIsScheduled` before exiting. Otherwise, we could fail to schedule a callback for pending work and the app could freeze. To help prevent a future refactor from introducing such a bug, this change makes it so that `renderRoot` is always wrapped in try-finally, and the `finally` block calls `ensureRootIsScheduled`.
1 parent 09461dc commit 628d185

File tree

1 file changed

+51
-37
lines changed

1 file changed

+51
-37
lines changed

packages/react-reconciler/src/ReactFiberWorkLoop.js

Lines changed: 51 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,7 @@ export function scheduleUpdateOnFiber(
405405
// This is a legacy edge case. The initial mount of a ReactDOM.render-ed
406406
// root inside of batchedUpdates should be synchronous, but layout updates
407407
// should be deferred until the end of the batch.
408-
renderRoot(root, Sync, true);
408+
performSyncWorkOnRoot(root, Sync);
409409
} else {
410410
ensureRootIsScheduled(root);
411411
schedulePendingInteractions(root, expirationTime);
@@ -593,16 +593,18 @@ function ensureRootIsScheduled(root: FiberRoot) {
593593
let callbackNode;
594594
if (expirationTime === Sync) {
595595
// Sync React callbacks are scheduled on a special internal queue
596-
callbackNode = scheduleSyncCallback(performWorkOnRoot.bind(null, root));
596+
callbackNode = scheduleSyncCallback(
597+
performSyncWorkOnRoot.bind(null, root, Sync),
598+
);
597599
} else if (disableSchedulerTimeoutBasedOnReactExpirationTime) {
598600
callbackNode = scheduleCallback(
599601
priorityLevel,
600-
performWorkOnRoot.bind(null, root),
602+
performConcurrentWorkOnRoot.bind(null, root),
601603
);
602604
} else {
603605
callbackNode = scheduleCallback(
604606
priorityLevel,
605-
performWorkOnRoot.bind(null, root),
607+
performConcurrentWorkOnRoot.bind(null, root),
606608
// Compute a task timeout based on the expiration time. This also affects
607609
// ordering because tasks are processed in timeout order.
608610
{timeout: expirationTimeToMs(expirationTime) - now()},
@@ -612,45 +614,57 @@ function ensureRootIsScheduled(root: FiberRoot) {
612614
root.callbackNode = callbackNode;
613615
}
614616

615-
// This is the entry point for every concurrent task.
616-
function performWorkOnRoot(root, isSync) {
617+
// This is the entry point for every concurrent task, i.e. anything that
618+
// goes through Scheduler.
619+
function performConcurrentWorkOnRoot(root, didTimeout) {
620+
// Since we know we're in a React event, we can clear the current
621+
// event time. The next update will compute a new event time.
622+
currentEventTime = NoWork;
623+
617624
// Determine the next expiration time to work on, using the fields stored
618625
// on the root.
619626
let expirationTime = getNextRootExpirationTimeToWorkOn(root);
620627
if (expirationTime !== NoWork) {
621-
if (expirationTime !== Sync) {
622-
// Since we know we're in a React event, we can clear the current
623-
// event time. The next update will compute a new event time.
624-
currentEventTime = NoWork;
625-
628+
if (didTimeout) {
626629
// An async update expired. There may be other expired updates on
627630
// this root.
628-
if (isSync) {
629-
const currentTime = requestCurrentTime();
630-
if (currentTime < expirationTime) {
631-
// Render all the expired work in a single batch.
632-
expirationTime = currentTime;
633-
}
631+
const currentTime = requestCurrentTime();
632+
if (currentTime < expirationTime) {
633+
// Render all the expired work in a single batch.
634+
expirationTime = currentTime;
634635
}
635636
}
636637

637638
const originalCallbackNode = root.callbackNode;
638639
try {
639-
renderRoot(root, expirationTime, isSync);
640-
} finally {
641-
// Before exiting, make sure there's a callback scheduled for the
642-
// pending level.
643-
ensureRootIsScheduled(root);
640+
renderRoot(root, expirationTime, didTimeout);
644641
if (root.callbackNode === originalCallbackNode) {
645642
// The task node scheduled for this root is the same one that's
646643
// currently executed. Need to return a continuation.
647-
return performWorkOnRoot.bind(null, root);
644+
return performConcurrentWorkOnRoot.bind(null, root);
648645
}
646+
} finally {
647+
// Before exiting, make sure there's a callback scheduled for the
648+
// pending level.
649+
ensureRootIsScheduled(root);
649650
}
650651
}
651652
return null;
652653
}
653654

655+
// This is the entry point for synchronous tasks that don't go
656+
// through Scheduler
657+
function performSyncWorkOnRoot(root, expirationTime) {
658+
try {
659+
renderRoot(root, expirationTime, true);
660+
} finally {
661+
// Before exiting, make sure there's a callback scheduled for the
662+
// pending level.
663+
ensureRootIsScheduled(root);
664+
}
665+
return null;
666+
}
667+
654668
export function flushRoot(root: FiberRoot, expirationTime: ExpirationTime) {
655669
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
656670
invariant(
@@ -659,11 +673,7 @@ export function flushRoot(root: FiberRoot, expirationTime: ExpirationTime) {
659673
'means you attempted to commit from inside a lifecycle method.',
660674
);
661675
}
662-
scheduleSyncCallback(() => {
663-
renderRoot(root, expirationTime, true);
664-
return null;
665-
});
666-
flushSyncCallbackQueue();
676+
performSyncWorkOnRoot(root, expirationTime);
667677
}
668678

669679
export function flushDiscreteUpdates() {
@@ -731,10 +741,9 @@ function flushPendingDiscreteUpdates() {
731741
const roots = rootsWithPendingDiscreteUpdates;
732742
rootsWithPendingDiscreteUpdates = null;
733743
roots.forEach((expirationTime, root) => {
734-
scheduleSyncCallback(() => {
735-
renderRoot(root, expirationTime, true);
736-
return null;
737-
});
744+
scheduleSyncCallback(
745+
performSyncWorkOnRoot.bind(null, root, expirationTime),
746+
);
738747
});
739748
// Now flush the immediate queue.
740749
flushSyncCallbackQueue();
@@ -879,6 +888,8 @@ function prepareFreshStack(root, expirationTime) {
879888
}
880889
}
881890

891+
// renderRoot should only be called from inside either
892+
// `performConcurrentWorkOnRoot` or `performSyncWorkOnRoot`.
882893
function renderRoot(
883894
root: FiberRoot,
884895
expirationTime: ExpirationTime,
@@ -1021,7 +1032,7 @@ function renderRoot(
10211032
// synchronously, to see if the error goes away. If there are lower
10221033
// priority updates, let's include those, too, in case they fix the
10231034
// inconsistency. Render at Idle to include all updates.
1024-
renderRoot(root, Idle, true);
1035+
performSyncWorkOnRoot(root, Idle);
10251036
return;
10261037
}
10271038
// Commit the root in its errored state.
@@ -1863,9 +1874,8 @@ function commitRootImpl(root, renderPriorityLevel) {
18631874
);
18641875
}
18651876
}
1877+
schedulePendingInteractions(root, remainingExpirationTime);
18661878
}
1867-
ensureRootIsScheduled(root);
1868-
schedulePendingInteractions(root, expirationTime);
18691879
} else {
18701880
// If there's no remaining work, we can clear the set of already failed
18711881
// error boundaries.
@@ -1882,8 +1892,6 @@ function commitRootImpl(root, renderPriorityLevel) {
18821892
}
18831893
}
18841894

1885-
onCommitRoot(finishedWork.stateNode, expirationTime);
1886-
18871895
if (remainingExpirationTime === Sync) {
18881896
// Count the number of times the root synchronously re-renders without
18891897
// finishing. If there are too many, it indicates an infinite update loop.
@@ -1897,6 +1905,12 @@ function commitRootImpl(root, renderPriorityLevel) {
18971905
nestedUpdateCount = 0;
18981906
}
18991907

1908+
onCommitRoot(finishedWork.stateNode, expirationTime);
1909+
1910+
// Always call this before exiting `commitRoot`, to ensure that any
1911+
// additional work on this root is scheduled.
1912+
ensureRootIsScheduled(root);
1913+
19001914
if (hasUncaughtError) {
19011915
hasUncaughtError = false;
19021916
const error = firstUncaughtError;

0 commit comments

Comments
 (0)