From 79f54c16dc3d5298e6037df75db2beb3552896e9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 7 Jun 2022 20:04:02 -0400 Subject: [PATCH] Bugfix: Revealing a hidden update (#24685) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add `isHidden` to OffscreenInstance We need to be able to read whether an offscreen tree is hidden from an imperative event. We can store this on its OffscreenInstance. We were already scheduling a commit effect whenever the visibility changes, in order to toggle the inner effects. So we can reuse that. * [FORKED] Bugfix: Revealing a hidden update This fixes a bug I discovered related to revealing a hidden Offscreen tree. When this happens, we include in that render all the updates that had previously been deferred — that is, all the updates that would have already committed if the tree weren't hidden. This is necessary to avoid tearing with the surrounding contents. (This was the "flickering" Suspense bug we found a few years ago: #18411.) The way we do this is by tracking the lanes of the updates that were deferred by a hidden tree. These are the "base" lanes. Then, in order to reveal the hidden tree, we process any update that matches one of those base lanes. The bug I discovered is that some of these base lanes may include updates that were not present at the time the tree was hidden. We cannot flush those updates earlier that the surrounding contents — that, too, could cause tearing. The crux of the problem is that we sometimes reuse the same lane for base updates and for non-base updates. So the lane alone isn't sufficient to distinguish between these cases. We must track this in some other way. The solution I landed upon was to add an extra OffscreenLane bit to any update that is made to a hidden tree. Then later when we reveal the tree, we'll know not to treat them as base updates. The extra OffscreenLane bit is removed as soon as that lane is committed by the root (markRootFinished) — at that point, it gets "upgraded" to a base update. The trickiest part of this algorithm is reliably detecting when an update is made to a hidden tree. What makes this challenging is when the update is received during a concurrent event, while a render is already in progress — it's possible the work-in-progress render is about to flip the visibility of the tree that's being updated, leading to a race condition. To avoid a race condition, we will wait to read the visibility of the tree until the current render has finished. In other words, this makes it an atomic operation. Most of this logic was already implemented in #24663. Because this bugfix depends on a moderately risky refactor to the update queue (#24663), it only works in the "new" reconciler fork. We will roll it out gradually to www before landing in the main fork. * Add previous commit to list of forked revisions --- .../react-reconciler/src/ReactFiber.new.js | 4 +- .../react-reconciler/src/ReactFiber.old.js | 4 +- .../src/ReactFiberClassUpdateQueue.new.js | 21 +- .../src/ReactFiberCommitWork.new.js | 11 + .../src/ReactFiberCommitWork.old.js | 11 + .../src/ReactFiberConcurrentUpdates.new.js | 52 +++- .../src/ReactFiberHooks.new.js | 18 +- .../src/ReactFiberLane.new.js | 33 +++ .../src/ReactFiberOffscreenComponent.js | 4 +- .../src/ReactFiberRoot.new.js | 2 + .../src/ReactFiberWorkLoop.new.js | 7 +- .../src/ReactInternalTypes.js | 2 + .../src/__tests__/ReactOffscreen-test.js | 122 +++++++++ .../__tests__/ReactOffscreenSuspense-test.js | 246 ++++++++++++++++++ scripts/merge-fork/forked-revisions | 1 + 15 files changed, 520 insertions(+), 18 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index 33fc93ef9361..58aabf70afa5 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -715,7 +715,9 @@ export function createFiberFromOffscreen( const fiber = createFiber(OffscreenComponent, pendingProps, key, mode); fiber.elementType = REACT_OFFSCREEN_TYPE; fiber.lanes = lanes; - const primaryChildInstance: OffscreenInstance = {}; + const primaryChildInstance: OffscreenInstance = { + isHidden: false, + }; fiber.stateNode = primaryChildInstance; return fiber; } diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/packages/react-reconciler/src/ReactFiber.old.js index a65f460c6b27..afa8f26503cc 100644 --- a/packages/react-reconciler/src/ReactFiber.old.js +++ b/packages/react-reconciler/src/ReactFiber.old.js @@ -715,7 +715,9 @@ export function createFiberFromOffscreen( const fiber = createFiber(OffscreenComponent, pendingProps, key, mode); fiber.elementType = REACT_OFFSCREEN_TYPE; fiber.lanes = lanes; - const primaryChildInstance: OffscreenInstance = {}; + const primaryChildInstance: OffscreenInstance = { + isHidden: false, + }; fiber.stateNode = primaryChildInstance; return fiber; } diff --git a/packages/react-reconciler/src/ReactFiberClassUpdateQueue.new.js b/packages/react-reconciler/src/ReactFiberClassUpdateQueue.new.js index 0311920ba299..ae9254add8d8 100644 --- a/packages/react-reconciler/src/ReactFiberClassUpdateQueue.new.js +++ b/packages/react-reconciler/src/ReactFiberClassUpdateQueue.new.js @@ -90,8 +90,10 @@ import type {Lanes, Lane} from './ReactFiberLane.new'; import { NoLane, NoLanes, + OffscreenLane, isSubsetOfLanes, mergeLanes, + removeLanes, isTransitionLane, intersectLanes, markRootEntangled, @@ -108,6 +110,7 @@ import {StrictLegacyMode} from './ReactTypeOfMode'; import { markSkippedUpdateLanes, isUnsafeClassRenderPhaseUpdate, + getWorkInProgressRootRenderLanes, } from './ReactFiberWorkLoop.new'; import { enqueueConcurrentClassUpdate, @@ -523,9 +526,23 @@ export function processUpdateQueue( let update = firstBaseUpdate; do { - const updateLane = update.lane; + // TODO: Don't need this field anymore const updateEventTime = update.eventTime; - if (!isSubsetOfLanes(renderLanes, updateLane)) { + + // An extra OffscreenLane bit is added to updates that were made to + // a hidden tree, so that we can distinguish them from updates that were + // already there when the tree was hidden. + const updateLane = removeLanes(update.lane, OffscreenLane); + const isHiddenUpdate = updateLane !== update.lane; + + // Check if this update was made while the tree was hidden. If so, then + // it's not a "base" update and we should disregard the extra base lanes + // that were added to renderLanes when we entered the Offscreen tree. + const shouldSkipUpdate = isHiddenUpdate + ? !isSubsetOfLanes(getWorkInProgressRootRenderLanes(), updateLane) + : !isSubsetOfLanes(renderLanes, updateLane); + + if (shouldSkipUpdate) { // Priority is insufficient. Skip this update. If this is the first // skipped update, the previous update/state is the new base // update/state. diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index d023c1f776b3..8448f8c25eb2 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -2309,8 +2309,14 @@ function commitMutationEffectsOnFiber( const offscreenFiber: Fiber = (finishedWork.child: any); if (offscreenFiber.flags & Visibility) { + const offscreenInstance: OffscreenInstance = offscreenFiber.stateNode; const newState: OffscreenState | null = offscreenFiber.memoizedState; const isHidden = newState !== null; + + // Track the current state on the Offscreen instance so we can + // read it during an event + offscreenInstance.isHidden = isHidden; + if (isHidden) { const wasHidden = offscreenFiber.alternate !== null && @@ -2354,10 +2360,15 @@ function commitMutationEffectsOnFiber( commitReconciliationEffects(finishedWork); if (flags & Visibility) { + const offscreenInstance: OffscreenInstance = finishedWork.stateNode; const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; const offscreenBoundary: Fiber = finishedWork; + // Track the current state on the Offscreen instance so we can + // read it during an event + offscreenInstance.isHidden = isHidden; + if (enableSuspenseLayoutEffectSemantics) { if (isHidden) { if (!wasHidden) { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index a9eae21939df..ab273999764b 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -2309,8 +2309,14 @@ function commitMutationEffectsOnFiber( const offscreenFiber: Fiber = (finishedWork.child: any); if (offscreenFiber.flags & Visibility) { + const offscreenInstance: OffscreenInstance = offscreenFiber.stateNode; const newState: OffscreenState | null = offscreenFiber.memoizedState; const isHidden = newState !== null; + + // Track the current state on the Offscreen instance so we can + // read it during an event + offscreenInstance.isHidden = isHidden; + if (isHidden) { const wasHidden = offscreenFiber.alternate !== null && @@ -2354,10 +2360,15 @@ function commitMutationEffectsOnFiber( commitReconciliationEffects(finishedWork); if (flags & Visibility) { + const offscreenInstance: OffscreenInstance = finishedWork.stateNode; const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; const offscreenBoundary: Fiber = finishedWork; + // Track the current state on the Offscreen instance so we can + // read it during an event + offscreenInstance.isHidden = isHidden; + if (enableSuspenseLayoutEffectSemantics) { if (isHidden) { if (!wasHidden) { diff --git a/packages/react-reconciler/src/ReactFiberConcurrentUpdates.new.js b/packages/react-reconciler/src/ReactFiberConcurrentUpdates.new.js index ed5a02cfea9e..ee35a11640e2 100644 --- a/packages/react-reconciler/src/ReactFiberConcurrentUpdates.new.js +++ b/packages/react-reconciler/src/ReactFiberConcurrentUpdates.new.js @@ -17,14 +17,21 @@ import type { Update as ClassUpdate, } from './ReactFiberClassUpdateQueue.new'; import type {Lane, Lanes} from './ReactFiberLane.new'; +import type {OffscreenInstance} from './ReactFiberOffscreenComponent'; import {warnAboutUpdateOnNotYetMountedFiberInDEV} from './ReactFiberWorkLoop.new'; -import {NoLane, NoLanes, mergeLanes} from './ReactFiberLane.new'; +import { + NoLane, + NoLanes, + mergeLanes, + markHiddenUpdate, +} from './ReactFiberLane.new'; import {NoFlags, Placement, Hydrating} from './ReactFiberFlags'; -import {HostRoot} from './ReactWorkTags'; +import {HostRoot, OffscreenComponent} from './ReactWorkTags'; -type ConcurrentUpdate = { +export type ConcurrentUpdate = { next: ConcurrentUpdate, + lane: Lane, }; type ConcurrentQueue = { @@ -38,11 +45,13 @@ type ConcurrentQueue = { const concurrentQueues: Array = []; let concurrentQueuesIndex = 0; -export function finishQueueingConcurrentUpdates(): Lanes { +let concurrentlyUpdatedLanes: Lanes = NoLanes; + +export function finishQueueingConcurrentUpdates(): void { const endIndex = concurrentQueuesIndex; concurrentQueuesIndex = 0; - let lanes = NoLanes; + concurrentlyUpdatedLanes = NoLanes; let i = 0; while (i < endIndex) { @@ -68,12 +77,13 @@ export function finishQueueingConcurrentUpdates(): Lanes { } if (lane !== NoLane) { - lanes = mergeLanes(lanes, lane); - markUpdateLaneFromFiberToRoot(fiber, lane); + markUpdateLaneFromFiberToRoot(fiber, update, lane); } } +} - return lanes; +export function getConcurrentlyUpdatedLanes(): Lanes { + return concurrentlyUpdatedLanes; } function enqueueUpdate( @@ -89,6 +99,8 @@ function enqueueUpdate( concurrentQueues[concurrentQueuesIndex++] = update; concurrentQueues[concurrentQueuesIndex++] = lane; + concurrentlyUpdatedLanes = mergeLanes(concurrentlyUpdatedLanes, lane); + // The fiber's `lane` field is used in some places to check if any work is // scheduled, to perform an eager bailout, so we need to update it immediately. // TODO: We should probably move this to the "shared" queue instead. @@ -151,11 +163,15 @@ export function unsafe_markUpdateLaneFromFiberToRoot( sourceFiber: Fiber, lane: Lane, ): FiberRoot | null { - markUpdateLaneFromFiberToRoot(sourceFiber, lane); + markUpdateLaneFromFiberToRoot(sourceFiber, null, lane); return getRootForUpdatedFiber(sourceFiber); } -function markUpdateLaneFromFiberToRoot(sourceFiber: Fiber, lane: Lane): void { +function markUpdateLaneFromFiberToRoot( + sourceFiber: Fiber, + update: ConcurrentUpdate | null, + lane: Lane, +): void { // Update the source fiber's lanes sourceFiber.lanes = mergeLanes(sourceFiber.lanes, lane); let alternate = sourceFiber.alternate; @@ -163,15 +179,31 @@ function markUpdateLaneFromFiberToRoot(sourceFiber: Fiber, lane: Lane): void { alternate.lanes = mergeLanes(alternate.lanes, lane); } // Walk the parent path to the root and update the child lanes. + let isHidden = false; let parent = sourceFiber.return; + let node = sourceFiber; while (parent !== null) { parent.childLanes = mergeLanes(parent.childLanes, lane); alternate = parent.alternate; if (alternate !== null) { alternate.childLanes = mergeLanes(alternate.childLanes, lane); } + + if (parent.tag === OffscreenComponent) { + const offscreenInstance: OffscreenInstance = parent.stateNode; + if (offscreenInstance.isHidden) { + isHidden = true; + } + } + + node = parent; parent = parent.return; } + + if (isHidden && update !== null && node.tag === HostRoot) { + const root: FiberRoot = node.stateNode; + markHiddenUpdate(root, update, lane); + } } function getRootForUpdatedFiber(sourceFiber: Fiber): FiberRoot | null { diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 697e87dd0137..c3ff781fb5f7 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -44,6 +44,7 @@ import { import { NoLane, SyncLane, + OffscreenLane, NoLanes, isSubsetOfLanes, includesBlockingLane, @@ -83,6 +84,7 @@ import { } from './ReactHookEffectTags'; import { getWorkInProgressRoot, + getWorkInProgressRootRenderLanes, scheduleUpdateOnFiber, requestUpdateLane, requestEventTime, @@ -811,8 +813,20 @@ function updateReducer( let newBaseQueueLast = null; let update = first; do { - const updateLane = update.lane; - if (!isSubsetOfLanes(renderLanes, updateLane)) { + // An extra OffscreenLane bit is added to updates that were made to + // a hidden tree, so that we can distinguish them from updates that were + // already there when the tree was hidden. + const updateLane = removeLanes(update.lane, OffscreenLane); + const isHiddenUpdate = updateLane !== update.lane; + + // Check if this update was made while the tree was hidden. If so, then + // it's not a "base" update and we should disregard the extra base lanes + // that were added to renderLanes when we entered the Offscreen tree. + const shouldSkipUpdate = isHiddenUpdate + ? !isSubsetOfLanes(getWorkInProgressRootRenderLanes(), updateLane) + : !isSubsetOfLanes(renderLanes, updateLane); + + if (shouldSkipUpdate) { // Priority is insufficient. Skip this update. If this is the first // skipped update, the previous update/state is the new base // update/state. diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index 708c9318b3a9..bb76950eb973 100644 --- a/packages/react-reconciler/src/ReactFiberLane.new.js +++ b/packages/react-reconciler/src/ReactFiberLane.new.js @@ -9,6 +9,7 @@ import type {FiberRoot} from './ReactInternalTypes'; import type {Transition} from './ReactFiberTracingMarkerComponent.new'; +import type {ConcurrentUpdate} from './ReactFiberConcurrentUpdates.new'; // TODO: Ideally these types would be opaque but that doesn't work well with // our reconciler fork infra, since these leak into non-reconciler packages. @@ -648,6 +649,7 @@ export function markRootFinished(root: FiberRoot, remainingLanes: Lanes) { const entanglements = root.entanglements; const eventTimes = root.eventTimes; const expirationTimes = root.expirationTimes; + const hiddenUpdates = root.hiddenUpdates; // Clear the lanes that no longer have pending work let lanes = noLongerPendingLanes; @@ -659,6 +661,21 @@ export function markRootFinished(root: FiberRoot, remainingLanes: Lanes) { eventTimes[index] = NoTimestamp; expirationTimes[index] = NoTimestamp; + const hiddenUpdatesForLane = hiddenUpdates[index]; + if (hiddenUpdatesForLane !== null) { + hiddenUpdates[index] = null; + // "Hidden" updates are updates that were made to a hidden component. They + // have special logic associated with them because they may be entangled + // with updates that occur outside that tree. But once the outer tree + // commits, they behave like regular updates. + for (let i = 0; i < hiddenUpdatesForLane.length; i++) { + const update = hiddenUpdatesForLane[i]; + if (update !== null) { + update.lane &= ~OffscreenLane; + } + } + } + lanes &= ~lane; } } @@ -694,6 +711,22 @@ export function markRootEntangled(root: FiberRoot, entangledLanes: Lanes) { } } +export function markHiddenUpdate( + root: FiberRoot, + update: ConcurrentUpdate, + lane: Lane, +) { + const index = laneToIndex(lane); + const hiddenUpdates = root.hiddenUpdates; + const hiddenUpdatesForLane = hiddenUpdates[index]; + if (hiddenUpdatesForLane === null) { + hiddenUpdates[index] = [update]; + } else { + hiddenUpdatesForLane.push(update); + } + update.lane = lane | OffscreenLane; +} + export function getBumpedLaneForHydration( root: FiberRoot, renderLanes: Lanes, diff --git a/packages/react-reconciler/src/ReactFiberOffscreenComponent.js b/packages/react-reconciler/src/ReactFiberOffscreenComponent.js index 05952f77a0fd..b43188103e6f 100644 --- a/packages/react-reconciler/src/ReactFiberOffscreenComponent.js +++ b/packages/react-reconciler/src/ReactFiberOffscreenComponent.js @@ -38,4 +38,6 @@ export type OffscreenQueue = {| transitions: Array | null, |} | null; -export type OffscreenInstance = {}; +export type OffscreenInstance = {| + isHidden: boolean, +|}; diff --git a/packages/react-reconciler/src/ReactFiberRoot.new.js b/packages/react-reconciler/src/ReactFiberRoot.new.js index ca9edcc3ded7..d8d275278717 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.new.js +++ b/packages/react-reconciler/src/ReactFiberRoot.new.js @@ -80,6 +80,8 @@ function FiberRootNode( this.entangledLanes = NoLanes; this.entanglements = createLaneMap(NoLanes); + this.hiddenUpdates = createLaneMap(null); + this.identifierPrefix = identifierPrefix; this.onRecoverableError = onRecoverableError; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 82963ea0fa9d..e7e95b2694e0 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -199,6 +199,7 @@ import { import { enqueueConcurrentRenderForLane, finishQueueingConcurrentUpdates, + getConcurrentlyUpdatedLanes, } from './ReactFiberConcurrentUpdates.new'; import { @@ -425,6 +426,10 @@ export function getWorkInProgressRoot(): FiberRoot | null { return workInProgressRoot; } +export function getWorkInProgressRootRenderLanes(): Lanes { + return workInProgressRootRenderLanes; +} + export function requestEventTime() { if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { // We're inside React, so it's fine to read the actual time. @@ -2059,7 +2064,7 @@ function commitRootImpl( // Make sure to account for lanes that were updated by a concurrent event // during the render phase; don't mark them as finished. - const concurrentlyUpdatedLanes = finishQueueingConcurrentUpdates(); + const concurrentlyUpdatedLanes = getConcurrentlyUpdatedLanes(); remainingLanes = mergeLanes(remainingLanes, concurrentlyUpdatedLanes); markRootFinished(root, remainingLanes); diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 618260b47c20..c6a7ed58b0b0 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -27,6 +27,7 @@ import type {RootTag} from './ReactRootTags'; import type {TimeoutHandle, NoTimeout} from './ReactFiberHostConfig'; import type {Cache} from './ReactFiberCacheComponent.old'; import type {Transition} from './ReactFiberTracingMarkerComponent.new'; +import type {ConcurrentUpdate} from './ReactFiberConcurrentUpdates.new'; // Unwind Circular: moved from ReactFiberHooks.old export type HookType = @@ -225,6 +226,7 @@ type BaseFiberRootProperties = {| callbackPriority: Lane, eventTimes: LaneMap, expirationTimes: LaneMap, + hiddenUpdates: LaneMap | null>, pendingLanes: Lanes, suspendedLanes: Lanes, diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js index d1e836c252ec..42b462100f31 100644 --- a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js +++ b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js @@ -6,6 +6,7 @@ let LegacyHidden; let Offscreen; let useState; let useLayoutEffect; +let useEffect; describe('ReactOffscreen', () => { beforeEach(() => { @@ -19,6 +20,7 @@ describe('ReactOffscreen', () => { Offscreen = React.unstable_Offscreen; useState = React.useState; useLayoutEffect = React.useLayoutEffect; + useEffect = React.useEffect; }); function Text(props) { @@ -473,4 +475,124 @@ describe('ReactOffscreen', () => { // Now it's visible expect(root).toMatchRenderedOutput(Hi); }); + + // Only works in new reconciler + // @gate variant + it('revealing a hidden tree at high priority does not cause tearing', async () => { + // When revealing an offscreen tree, we need to include updates that were + // previously deferred because the tree was hidden, even if they are lower + // priority than the current render. However, we should *not* include low + // priority updates that are entangled with updates outside of the hidden + // tree, because that can cause tearing. + // + // This test covers a scenario where an update multiple updates inside a + // hidden tree share the same lane, but are processed at different times + // because of the timing of when they were scheduled. + + let setInner; + function Child({outer}) { + const [inner, _setInner] = useState(0); + setInner = _setInner; + + useEffect(() => { + // Inner and outer values are always updated simultaneously, so they + // should always be consistent. + if (inner !== outer) { + Scheduler.unstable_yieldValue( + 'Tearing! Inner and outer are inconsistent!', + ); + } else { + Scheduler.unstable_yieldValue('Inner and outer are consistent'); + } + }, [inner, outer]); + + return ; + } + + let setOuter; + function App({show}) { + const [outer, _setOuter] = useState(0); + setOuter = _setOuter; + return ( + <> + + + + + + ); + } + + // Render a hidden tree + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded([ + 'Outer: 0', + 'Inner: 0', + 'Inner and outer are consistent', + ]); + expect(root).toMatchRenderedOutput( + <> + +