Skip to content

Commit 04d140c

Browse files
acdlitegaearon
acdlite
and
gaearon
committed
Fix: Don't call cWU if already unmounted
When a tree goes offscreen, we unmount all the effects just like we would in a normal deletion. (Conceptually it _is_ a deletion; we keep the fiber around so we can reuse its state if the tree mounts again.) If an offscreen component gets deleted "for real", we shouldn't unmount it again. The fix is to track on the stack whether we're inside a hidden tree. We already had a stack variable for this purpose, called `offscreenSubtreeWasHidden`, in another part of the commit phase, so I reused that variable instead of creating a new one. (The name is a bit confusing: "was" refers to the current tree before this commit. So, the "previous current".) Co-authored-by: dan <[email protected]>
1 parent 5a20865 commit 04d140c

File tree

4 files changed

+555
-102
lines changed

4 files changed

+555
-102
lines changed

‎packages/react-reconciler/src/ReactFiberCommitWork.new.js‎

Lines changed: 85 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1596,7 +1596,9 @@ function commitDeletionEffectsOnFiber(
15961596
switch (deletedFiber.tag) {
15971597
case HostComponent:
15981598
case HostText: {
1599-
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1599+
if (!offscreenSubtreeWasHidden) {
1600+
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1601+
}
16001602
// We only need to remove the nearest host child. Set the host parent
16011603
// to `null` on the stack to indicate that nested children don't
16021604
// need to be removed.
@@ -1696,54 +1698,56 @@ function commitDeletionEffectsOnFiber(
16961698
case ForwardRef:
16971699
case MemoComponent:
16981700
case SimpleMemoComponent: {
1699-
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
1700-
if (updateQueue !== null) {
1701-
const lastEffect = updateQueue.lastEffect;
1702-
if (lastEffect !== null) {
1703-
const firstEffect = lastEffect.next;
1704-
1705-
let effect = firstEffect;
1706-
do {
1707-
const {destroy, tag} = effect;
1708-
if (destroy !== undefined) {
1709-
if ((tag & HookInsertion) !== NoHookEffect) {
1710-
safelyCallDestroy(
1711-
deletedFiber,
1712-
nearestMountedAncestor,
1713-
destroy,
1714-
);
1715-
} else if ((tag & HookLayout) !== NoHookEffect) {
1716-
if (enableSchedulingProfiler) {
1717-
markComponentLayoutEffectUnmountStarted(deletedFiber);
1718-
}
1719-
1720-
if (
1721-
enableProfilerTimer &&
1722-
enableProfilerCommitHooks &&
1723-
deletedFiber.mode & ProfileMode
1724-
) {
1725-
startLayoutEffectTimer();
1726-
safelyCallDestroy(
1727-
deletedFiber,
1728-
nearestMountedAncestor,
1729-
destroy,
1730-
);
1731-
recordLayoutEffectDuration(deletedFiber);
1732-
} else {
1701+
if (!offscreenSubtreeWasHidden) {
1702+
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
1703+
if (updateQueue !== null) {
1704+
const lastEffect = updateQueue.lastEffect;
1705+
if (lastEffect !== null) {
1706+
const firstEffect = lastEffect.next;
1707+
1708+
let effect = firstEffect;
1709+
do {
1710+
const {destroy, tag} = effect;
1711+
if (destroy !== undefined) {
1712+
if ((tag & HookInsertion) !== NoHookEffect) {
17331713
safelyCallDestroy(
17341714
deletedFiber,
17351715
nearestMountedAncestor,
17361716
destroy,
17371717
);
1738-
}
1718+
} else if ((tag & HookLayout) !== NoHookEffect) {
1719+
if (enableSchedulingProfiler) {
1720+
markComponentLayoutEffectUnmountStarted(deletedFiber);
1721+
}
17391722

1740-
if (enableSchedulingProfiler) {
1741-
markComponentLayoutEffectUnmountStopped();
1723+
if (
1724+
enableProfilerTimer &&
1725+
enableProfilerCommitHooks &&
1726+
deletedFiber.mode & ProfileMode
1727+
) {
1728+
startLayoutEffectTimer();
1729+
safelyCallDestroy(
1730+
deletedFiber,
1731+
nearestMountedAncestor,
1732+
destroy,
1733+
);
1734+
recordLayoutEffectDuration(deletedFiber);
1735+
} else {
1736+
safelyCallDestroy(
1737+
deletedFiber,
1738+
nearestMountedAncestor,
1739+
destroy,
1740+
);
1741+
}
1742+
1743+
if (enableSchedulingProfiler) {
1744+
markComponentLayoutEffectUnmountStopped();
1745+
}
17421746
}
17431747
}
1744-
}
1745-
effect = effect.next;
1746-
} while (effect !== firstEffect);
1748+
effect = effect.next;
1749+
} while (effect !== firstEffect);
1750+
}
17471751
}
17481752
}
17491753

@@ -1755,14 +1759,16 @@ function commitDeletionEffectsOnFiber(
17551759
return;
17561760
}
17571761
case ClassComponent: {
1758-
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1759-
const instance = deletedFiber.stateNode;
1760-
if (typeof instance.componentWillUnmount === 'function') {
1761-
safelyCallComponentWillUnmount(
1762-
deletedFiber,
1763-
nearestMountedAncestor,
1764-
instance,
1765-
);
1762+
if (!offscreenSubtreeWasHidden) {
1763+
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1764+
const instance = deletedFiber.stateNode;
1765+
if (typeof instance.componentWillUnmount === 'function') {
1766+
safelyCallComponentWillUnmount(
1767+
deletedFiber,
1768+
nearestMountedAncestor,
1769+
instance,
1770+
);
1771+
}
17661772
}
17671773
recursivelyTraverseDeletionEffects(
17681774
finishedRoot,
@@ -1782,6 +1788,27 @@ function commitDeletionEffectsOnFiber(
17821788
);
17831789
return;
17841790
}
1791+
case OffscreenComponent: {
1792+
// If this offscreen component is hidden, we already unmounted it. Before
1793+
// deleting the children, track that it's already unmounted so that we
1794+
// don't attempt to unmount the effects again.
1795+
// TODO: If the tree is hidden, in most cases we should be able to skip
1796+
// over the nested children entirely. An exception is we haven't yet found
1797+
// the topmost host node to delete, which we already track on the stack.
1798+
// But the other case is portals, which need to be detached no matter how
1799+
// deeply they are nested. We should use a subtree flag to track whether a
1800+
// subtree includes a nested portal.
1801+
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
1802+
offscreenSubtreeWasHidden =
1803+
prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
1804+
recursivelyTraverseDeletionEffects(
1805+
finishedRoot,
1806+
nearestMountedAncestor,
1807+
deletedFiber,
1808+
);
1809+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
1810+
break;
1811+
}
17851812
default: {
17861813
recursivelyTraverseDeletionEffects(
17871814
finishedRoot,
@@ -2216,13 +2243,21 @@ function commitMutationEffectsOnFiber(
22162243
return;
22172244
}
22182245
case OffscreenComponent: {
2246+
const wasHidden = current !== null && current.memoizedState !== null;
2247+
2248+
// Before committing the children, track on the stack whether this
2249+
// offscreen subtree was already hidden, so that we don't unmount the
2250+
// effects again.
2251+
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
2252+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
22192253
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2254+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
2255+
22202256
commitReconciliationEffects(finishedWork);
22212257

22222258
if (flags & Visibility) {
22232259
const newState: OffscreenState | null = finishedWork.memoizedState;
22242260
const isHidden = newState !== null;
2225-
const wasHidden = current !== null && current.memoizedState !== null;
22262261
const offscreenBoundary: Fiber = finishedWork;
22272262

22282263
if (supportsMutation) {

‎packages/react-reconciler/src/ReactFiberCommitWork.old.js‎

Lines changed: 85 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1596,7 +1596,9 @@ function commitDeletionEffectsOnFiber(
15961596
switch (deletedFiber.tag) {
15971597
case HostComponent:
15981598
case HostText: {
1599-
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1599+
if (!offscreenSubtreeWasHidden) {
1600+
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1601+
}
16001602
// We only need to remove the nearest host child. Set the host parent
16011603
// to `null` on the stack to indicate that nested children don't
16021604
// need to be removed.
@@ -1696,54 +1698,56 @@ function commitDeletionEffectsOnFiber(
16961698
case ForwardRef:
16971699
case MemoComponent:
16981700
case SimpleMemoComponent: {
1699-
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
1700-
if (updateQueue !== null) {
1701-
const lastEffect = updateQueue.lastEffect;
1702-
if (lastEffect !== null) {
1703-
const firstEffect = lastEffect.next;
1704-
1705-
let effect = firstEffect;
1706-
do {
1707-
const {destroy, tag} = effect;
1708-
if (destroy !== undefined) {
1709-
if ((tag & HookInsertion) !== NoHookEffect) {
1710-
safelyCallDestroy(
1711-
deletedFiber,
1712-
nearestMountedAncestor,
1713-
destroy,
1714-
);
1715-
} else if ((tag & HookLayout) !== NoHookEffect) {
1716-
if (enableSchedulingProfiler) {
1717-
markComponentLayoutEffectUnmountStarted(deletedFiber);
1718-
}
1719-
1720-
if (
1721-
enableProfilerTimer &&
1722-
enableProfilerCommitHooks &&
1723-
deletedFiber.mode & ProfileMode
1724-
) {
1725-
startLayoutEffectTimer();
1726-
safelyCallDestroy(
1727-
deletedFiber,
1728-
nearestMountedAncestor,
1729-
destroy,
1730-
);
1731-
recordLayoutEffectDuration(deletedFiber);
1732-
} else {
1701+
if (!offscreenSubtreeWasHidden) {
1702+
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
1703+
if (updateQueue !== null) {
1704+
const lastEffect = updateQueue.lastEffect;
1705+
if (lastEffect !== null) {
1706+
const firstEffect = lastEffect.next;
1707+
1708+
let effect = firstEffect;
1709+
do {
1710+
const {destroy, tag} = effect;
1711+
if (destroy !== undefined) {
1712+
if ((tag & HookInsertion) !== NoHookEffect) {
17331713
safelyCallDestroy(
17341714
deletedFiber,
17351715
nearestMountedAncestor,
17361716
destroy,
17371717
);
1738-
}
1718+
} else if ((tag & HookLayout) !== NoHookEffect) {
1719+
if (enableSchedulingProfiler) {
1720+
markComponentLayoutEffectUnmountStarted(deletedFiber);
1721+
}
17391722

1740-
if (enableSchedulingProfiler) {
1741-
markComponentLayoutEffectUnmountStopped();
1723+
if (
1724+
enableProfilerTimer &&
1725+
enableProfilerCommitHooks &&
1726+
deletedFiber.mode & ProfileMode
1727+
) {
1728+
startLayoutEffectTimer();
1729+
safelyCallDestroy(
1730+
deletedFiber,
1731+
nearestMountedAncestor,
1732+
destroy,
1733+
);
1734+
recordLayoutEffectDuration(deletedFiber);
1735+
} else {
1736+
safelyCallDestroy(
1737+
deletedFiber,
1738+
nearestMountedAncestor,
1739+
destroy,
1740+
);
1741+
}
1742+
1743+
if (enableSchedulingProfiler) {
1744+
markComponentLayoutEffectUnmountStopped();
1745+
}
17421746
}
17431747
}
1744-
}
1745-
effect = effect.next;
1746-
} while (effect !== firstEffect);
1748+
effect = effect.next;
1749+
} while (effect !== firstEffect);
1750+
}
17471751
}
17481752
}
17491753

@@ -1755,14 +1759,16 @@ function commitDeletionEffectsOnFiber(
17551759
return;
17561760
}
17571761
case ClassComponent: {
1758-
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1759-
const instance = deletedFiber.stateNode;
1760-
if (typeof instance.componentWillUnmount === 'function') {
1761-
safelyCallComponentWillUnmount(
1762-
deletedFiber,
1763-
nearestMountedAncestor,
1764-
instance,
1765-
);
1762+
if (!offscreenSubtreeWasHidden) {
1763+
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1764+
const instance = deletedFiber.stateNode;
1765+
if (typeof instance.componentWillUnmount === 'function') {
1766+
safelyCallComponentWillUnmount(
1767+
deletedFiber,
1768+
nearestMountedAncestor,
1769+
instance,
1770+
);
1771+
}
17661772
}
17671773
recursivelyTraverseDeletionEffects(
17681774
finishedRoot,
@@ -1782,6 +1788,27 @@ function commitDeletionEffectsOnFiber(
17821788
);
17831789
return;
17841790
}
1791+
case OffscreenComponent: {
1792+
// If this offscreen component is hidden, we already unmounted it. Before
1793+
// deleting the children, track that it's already unmounted so that we
1794+
// don't attempt to unmount the effects again.
1795+
// TODO: If the tree is hidden, in most cases we should be able to skip
1796+
// over the nested children entirely. An exception is we haven't yet found
1797+
// the topmost host node to delete, which we already track on the stack.
1798+
// But the other case is portals, which need to be detached no matter how
1799+
// deeply they are nested. We should use a subtree flag to track whether a
1800+
// subtree includes a nested portal.
1801+
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
1802+
offscreenSubtreeWasHidden =
1803+
prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
1804+
recursivelyTraverseDeletionEffects(
1805+
finishedRoot,
1806+
nearestMountedAncestor,
1807+
deletedFiber,
1808+
);
1809+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
1810+
break;
1811+
}
17851812
default: {
17861813
recursivelyTraverseDeletionEffects(
17871814
finishedRoot,
@@ -2216,13 +2243,21 @@ function commitMutationEffectsOnFiber(
22162243
return;
22172244
}
22182245
case OffscreenComponent: {
2246+
const wasHidden = current !== null && current.memoizedState !== null;
2247+
2248+
// Before committing the children, track on the stack whether this
2249+
// offscreen subtree was already hidden, so that we don't unmount the
2250+
// effects again.
2251+
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
2252+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
22192253
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2254+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
2255+
22202256
commitReconciliationEffects(finishedWork);
22212257

22222258
if (flags & Visibility) {
22232259
const newState: OffscreenState | null = finishedWork.memoizedState;
22242260
const isHidden = newState !== null;
2225-
const wasHidden = current !== null && current.memoizedState !== null;
22262261
const offscreenBoundary: Fiber = finishedWork;
22272262

22282263
if (supportsMutation) {

‎packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1980,7 +1980,6 @@ describe('ReactSuspenseEffectsSemantics', () => {
19801980

19811981
// Destroy layout and passive effects in the errored tree.
19821982
'App destroy layout',
1983-
'ThrowsInWillUnmount componentWillUnmount',
19841983
'Text:Fallback destroy layout',
19851984
'Text:Outside destroy layout',
19861985
'Text:Inside destroy passive',

0 commit comments

Comments
 (0)