Skip to content

Commit 2bbd282

Browse files
acdlitegaearon
authored and
rickhanlonii
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 4cd752b commit 2bbd282

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
@@ -1604,7 +1604,9 @@ function commitDeletionEffectsOnFiber(
16041604
// that don't modify the stack.
16051605
switch (deletedFiber.tag) {
16061606
case HostComponent: {
1607-
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1607+
if (!offscreenSubtreeWasHidden) {
1608+
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1609+
}
16081610
// Intentional fallthrough to next branch
16091611
}
16101612
// eslint-disable-next-line-no-fallthrough
@@ -1710,54 +1712,56 @@ function commitDeletionEffectsOnFiber(
17101712
case ForwardRef:
17111713
case MemoComponent:
17121714
case SimpleMemoComponent: {
1713-
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
1714-
if (updateQueue !== null) {
1715-
const lastEffect = updateQueue.lastEffect;
1716-
if (lastEffect !== null) {
1717-
const firstEffect = lastEffect.next;
1718-
1719-
let effect = firstEffect;
1720-
do {
1721-
const {destroy, tag} = effect;
1722-
if (destroy !== undefined) {
1723-
if ((tag & HookInsertion) !== NoHookEffect) {
1724-
safelyCallDestroy(
1725-
deletedFiber,
1726-
nearestMountedAncestor,
1727-
destroy,
1728-
);
1729-
} else if ((tag & HookLayout) !== NoHookEffect) {
1730-
if (enableSchedulingProfiler) {
1731-
markComponentLayoutEffectUnmountStarted(deletedFiber);
1732-
}
1733-
1734-
if (
1735-
enableProfilerTimer &&
1736-
enableProfilerCommitHooks &&
1737-
deletedFiber.mode & ProfileMode
1738-
) {
1739-
startLayoutEffectTimer();
1740-
safelyCallDestroy(
1741-
deletedFiber,
1742-
nearestMountedAncestor,
1743-
destroy,
1744-
);
1745-
recordLayoutEffectDuration(deletedFiber);
1746-
} else {
1715+
if (!offscreenSubtreeWasHidden) {
1716+
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
1717+
if (updateQueue !== null) {
1718+
const lastEffect = updateQueue.lastEffect;
1719+
if (lastEffect !== null) {
1720+
const firstEffect = lastEffect.next;
1721+
1722+
let effect = firstEffect;
1723+
do {
1724+
const {destroy, tag} = effect;
1725+
if (destroy !== undefined) {
1726+
if ((tag & HookInsertion) !== NoHookEffect) {
17471727
safelyCallDestroy(
17481728
deletedFiber,
17491729
nearestMountedAncestor,
17501730
destroy,
17511731
);
1752-
}
1732+
} else if ((tag & HookLayout) !== NoHookEffect) {
1733+
if (enableSchedulingProfiler) {
1734+
markComponentLayoutEffectUnmountStarted(deletedFiber);
1735+
}
17531736

1754-
if (enableSchedulingProfiler) {
1755-
markComponentLayoutEffectUnmountStopped();
1737+
if (
1738+
enableProfilerTimer &&
1739+
enableProfilerCommitHooks &&
1740+
deletedFiber.mode & ProfileMode
1741+
) {
1742+
startLayoutEffectTimer();
1743+
safelyCallDestroy(
1744+
deletedFiber,
1745+
nearestMountedAncestor,
1746+
destroy,
1747+
);
1748+
recordLayoutEffectDuration(deletedFiber);
1749+
} else {
1750+
safelyCallDestroy(
1751+
deletedFiber,
1752+
nearestMountedAncestor,
1753+
destroy,
1754+
);
1755+
}
1756+
1757+
if (enableSchedulingProfiler) {
1758+
markComponentLayoutEffectUnmountStopped();
1759+
}
17561760
}
17571761
}
1758-
}
1759-
effect = effect.next;
1760-
} while (effect !== firstEffect);
1762+
effect = effect.next;
1763+
} while (effect !== firstEffect);
1764+
}
17611765
}
17621766
}
17631767

@@ -1769,14 +1773,16 @@ function commitDeletionEffectsOnFiber(
17691773
return;
17701774
}
17711775
case ClassComponent: {
1772-
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1773-
const instance = deletedFiber.stateNode;
1774-
if (typeof instance.componentWillUnmount === 'function') {
1775-
safelyCallComponentWillUnmount(
1776-
deletedFiber,
1777-
nearestMountedAncestor,
1778-
instance,
1779-
);
1776+
if (!offscreenSubtreeWasHidden) {
1777+
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1778+
const instance = deletedFiber.stateNode;
1779+
if (typeof instance.componentWillUnmount === 'function') {
1780+
safelyCallComponentWillUnmount(
1781+
deletedFiber,
1782+
nearestMountedAncestor,
1783+
instance,
1784+
);
1785+
}
17801786
}
17811787
recursivelyTraverseDeletionEffects(
17821788
finishedRoot,
@@ -1796,6 +1802,27 @@ function commitDeletionEffectsOnFiber(
17961802
);
17971803
return;
17981804
}
1805+
case OffscreenComponent: {
1806+
// If this offscreen component is hidden, we already unmounted it. Before
1807+
// deleting the children, track that it's already unmounted so that we
1808+
// don't attempt to unmount the effects again.
1809+
// TODO: If the tree is hidden, in most cases we should be able to skip
1810+
// over the nested children entirely. An exception is we haven't yet found
1811+
// the topmost host node to delete, which we already track on the stack.
1812+
// But the other case is portals, which need to be detached no matter how
1813+
// deeply they are nested. We should use a subtree flag to track whether a
1814+
// subtree includes a nested portal.
1815+
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
1816+
offscreenSubtreeWasHidden =
1817+
prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
1818+
recursivelyTraverseDeletionEffects(
1819+
finishedRoot,
1820+
nearestMountedAncestor,
1821+
deletedFiber,
1822+
);
1823+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
1824+
break;
1825+
}
17991826
default: {
18001827
recursivelyTraverseDeletionEffects(
18011828
finishedRoot,
@@ -2203,13 +2230,21 @@ function commitMutationEffectsOnFiber(
22032230
return;
22042231
}
22052232
case OffscreenComponent: {
2233+
const wasHidden = current !== null && current.memoizedState !== null;
2234+
2235+
// Before committing the children, track on the stack whether this
2236+
// offscreen subtree was already hidden, so that we don't unmount the
2237+
// effects again.
2238+
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
2239+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
22062240
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2241+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
2242+
22072243
commitReconciliationEffects(finishedWork);
22082244

22092245
if (flags & Visibility) {
22102246
const newState: OffscreenState | null = finishedWork.memoizedState;
22112247
const isHidden = newState !== null;
2212-
const wasHidden = current !== null && current.memoizedState !== null;
22132248
const offscreenBoundary: Fiber = finishedWork;
22142249

22152250
if (supportsMutation) {

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

Lines changed: 85 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1604,7 +1604,9 @@ function commitDeletionEffectsOnFiber(
16041604
// that don't modify the stack.
16051605
switch (deletedFiber.tag) {
16061606
case HostComponent: {
1607-
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1607+
if (!offscreenSubtreeWasHidden) {
1608+
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1609+
}
16081610
// Intentional fallthrough to next branch
16091611
}
16101612
// eslint-disable-next-line-no-fallthrough
@@ -1710,54 +1712,56 @@ function commitDeletionEffectsOnFiber(
17101712
case ForwardRef:
17111713
case MemoComponent:
17121714
case SimpleMemoComponent: {
1713-
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
1714-
if (updateQueue !== null) {
1715-
const lastEffect = updateQueue.lastEffect;
1716-
if (lastEffect !== null) {
1717-
const firstEffect = lastEffect.next;
1718-
1719-
let effect = firstEffect;
1720-
do {
1721-
const {destroy, tag} = effect;
1722-
if (destroy !== undefined) {
1723-
if ((tag & HookInsertion) !== NoHookEffect) {
1724-
safelyCallDestroy(
1725-
deletedFiber,
1726-
nearestMountedAncestor,
1727-
destroy,
1728-
);
1729-
} else if ((tag & HookLayout) !== NoHookEffect) {
1730-
if (enableSchedulingProfiler) {
1731-
markComponentLayoutEffectUnmountStarted(deletedFiber);
1732-
}
1733-
1734-
if (
1735-
enableProfilerTimer &&
1736-
enableProfilerCommitHooks &&
1737-
deletedFiber.mode & ProfileMode
1738-
) {
1739-
startLayoutEffectTimer();
1740-
safelyCallDestroy(
1741-
deletedFiber,
1742-
nearestMountedAncestor,
1743-
destroy,
1744-
);
1745-
recordLayoutEffectDuration(deletedFiber);
1746-
} else {
1715+
if (!offscreenSubtreeWasHidden) {
1716+
const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any);
1717+
if (updateQueue !== null) {
1718+
const lastEffect = updateQueue.lastEffect;
1719+
if (lastEffect !== null) {
1720+
const firstEffect = lastEffect.next;
1721+
1722+
let effect = firstEffect;
1723+
do {
1724+
const {destroy, tag} = effect;
1725+
if (destroy !== undefined) {
1726+
if ((tag & HookInsertion) !== NoHookEffect) {
17471727
safelyCallDestroy(
17481728
deletedFiber,
17491729
nearestMountedAncestor,
17501730
destroy,
17511731
);
1752-
}
1732+
} else if ((tag & HookLayout) !== NoHookEffect) {
1733+
if (enableSchedulingProfiler) {
1734+
markComponentLayoutEffectUnmountStarted(deletedFiber);
1735+
}
17531736

1754-
if (enableSchedulingProfiler) {
1755-
markComponentLayoutEffectUnmountStopped();
1737+
if (
1738+
enableProfilerTimer &&
1739+
enableProfilerCommitHooks &&
1740+
deletedFiber.mode & ProfileMode
1741+
) {
1742+
startLayoutEffectTimer();
1743+
safelyCallDestroy(
1744+
deletedFiber,
1745+
nearestMountedAncestor,
1746+
destroy,
1747+
);
1748+
recordLayoutEffectDuration(deletedFiber);
1749+
} else {
1750+
safelyCallDestroy(
1751+
deletedFiber,
1752+
nearestMountedAncestor,
1753+
destroy,
1754+
);
1755+
}
1756+
1757+
if (enableSchedulingProfiler) {
1758+
markComponentLayoutEffectUnmountStopped();
1759+
}
17561760
}
17571761
}
1758-
}
1759-
effect = effect.next;
1760-
} while (effect !== firstEffect);
1762+
effect = effect.next;
1763+
} while (effect !== firstEffect);
1764+
}
17611765
}
17621766
}
17631767

@@ -1769,14 +1773,16 @@ function commitDeletionEffectsOnFiber(
17691773
return;
17701774
}
17711775
case ClassComponent: {
1772-
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1773-
const instance = deletedFiber.stateNode;
1774-
if (typeof instance.componentWillUnmount === 'function') {
1775-
safelyCallComponentWillUnmount(
1776-
deletedFiber,
1777-
nearestMountedAncestor,
1778-
instance,
1779-
);
1776+
if (!offscreenSubtreeWasHidden) {
1777+
safelyDetachRef(deletedFiber, nearestMountedAncestor);
1778+
const instance = deletedFiber.stateNode;
1779+
if (typeof instance.componentWillUnmount === 'function') {
1780+
safelyCallComponentWillUnmount(
1781+
deletedFiber,
1782+
nearestMountedAncestor,
1783+
instance,
1784+
);
1785+
}
17801786
}
17811787
recursivelyTraverseDeletionEffects(
17821788
finishedRoot,
@@ -1796,6 +1802,27 @@ function commitDeletionEffectsOnFiber(
17961802
);
17971803
return;
17981804
}
1805+
case OffscreenComponent: {
1806+
// If this offscreen component is hidden, we already unmounted it. Before
1807+
// deleting the children, track that it's already unmounted so that we
1808+
// don't attempt to unmount the effects again.
1809+
// TODO: If the tree is hidden, in most cases we should be able to skip
1810+
// over the nested children entirely. An exception is we haven't yet found
1811+
// the topmost host node to delete, which we already track on the stack.
1812+
// But the other case is portals, which need to be detached no matter how
1813+
// deeply they are nested. We should use a subtree flag to track whether a
1814+
// subtree includes a nested portal.
1815+
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
1816+
offscreenSubtreeWasHidden =
1817+
prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
1818+
recursivelyTraverseDeletionEffects(
1819+
finishedRoot,
1820+
nearestMountedAncestor,
1821+
deletedFiber,
1822+
);
1823+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
1824+
break;
1825+
}
17991826
default: {
18001827
recursivelyTraverseDeletionEffects(
18011828
finishedRoot,
@@ -2203,13 +2230,21 @@ function commitMutationEffectsOnFiber(
22032230
return;
22042231
}
22052232
case OffscreenComponent: {
2233+
const wasHidden = current !== null && current.memoizedState !== null;
2234+
2235+
// Before committing the children, track on the stack whether this
2236+
// offscreen subtree was already hidden, so that we don't unmount the
2237+
// effects again.
2238+
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
2239+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
22062240
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2241+
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
2242+
22072243
commitReconciliationEffects(finishedWork);
22082244

22092245
if (flags & Visibility) {
22102246
const newState: OffscreenState | null = finishedWork.memoizedState;
22112247
const isHidden = newState !== null;
2212-
const wasHidden = current !== null && current.memoizedState !== null;
22132248
const offscreenBoundary: Fiber = finishedWork;
22142249

22152250
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)