From cc24d0ea56b0538d1ac61dc09faedd70ced5bb47 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 13 May 2019 16:15:50 -0700 Subject: [PATCH] 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. --- .../react-reconciler/src/ReactFiberRoot.js | 4 +- .../src/ReactFiberScheduler.js | 52 ++++++++++++------- scripts/error-codes/codes.json | 2 +- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index 426a55c81f..93a90f87d0 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -46,7 +46,7 @@ type BaseFiberRootProperties = {| | Map> | 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; diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 334b8322b2..3de722bf11 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/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,7 +930,7 @@ 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; @@ -937,11 +938,11 @@ function renderRoot( } } // 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, diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 11db5352b0..95c0bd384a 100644 --- a/scripts/error-codes/codes.json +++ b/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.",