Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't clean up twice in hidden trees #24282

Closed
wants to merge 5 commits into from
Closed

Don't clean up twice in hidden trees #24282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ce0a24
Failing test: componentWillUnmount called twice
gaearon Apr 5, 2022
3ae6285
Add a pair of tests for removing the parent
gaearon Apr 5, 2022
fd77aa4
Add tests for refs
gaearon Apr 5, 2022
36a1726
Convert tests to use ReactDOM
gaearon Apr 6, 2022
01e3269
Don't clean up twice in hidden trees
gaearon Apr 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
176 changes: 126 additions & 50 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
Expand Up @@ -1209,6 +1209,7 @@ function commitUnmount(
finishedRoot: FiberRoot,
current: Fiber,
nearestMountedAncestor: Fiber,
outermostHiddenFiber: Fiber | null,
): void {
onCommitUnmount(current);

Expand All @@ -1217,68 +1218,79 @@ function commitUnmount(
case ForwardRef:
case MemoComponent:
case SimpleMemoComponent: {
const updateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any);
if (updateQueue !== null) {
const lastEffect = updateQueue.lastEffect;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;

let effect = firstEffect;
do {
const {destroy, tag} = effect;
if (destroy !== undefined) {
if ((tag & HookInsertion) !== NoHookEffect) {
safelyCallDestroy(current, nearestMountedAncestor, destroy);
} else if ((tag & HookLayout) !== NoHookEffect) {
if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStarted(current);
}

if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
current.mode & ProfileMode
) {
startLayoutEffectTimer();
safelyCallDestroy(current, nearestMountedAncestor, destroy);
recordLayoutEffectDuration(current);
} else {
if (outermostHiddenFiber === null) {
const updateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any);
if (updateQueue !== null) {
const lastEffect = updateQueue.lastEffect;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;

let effect = firstEffect;
do {
const {destroy, tag} = effect;
if (destroy !== undefined) {
if ((tag & HookInsertion) !== NoHookEffect) {
safelyCallDestroy(current, nearestMountedAncestor, destroy);
}
} else if ((tag & HookLayout) !== NoHookEffect) {
if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStarted(current);
}

if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
current.mode & ProfileMode
) {
startLayoutEffectTimer();
safelyCallDestroy(current, nearestMountedAncestor, destroy);
recordLayoutEffectDuration(current);
} else {
safelyCallDestroy(current, nearestMountedAncestor, destroy);
}

if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStopped();
if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStopped();
}
}
}
}
effect = effect.next;
} while (effect !== firstEffect);
effect = effect.next;
} while (effect !== firstEffect);
}
}
}
return;
}
case ClassComponent: {
safelyDetachRef(current, nearestMountedAncestor);
const instance = current.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(
current,
nearestMountedAncestor,
instance,
);
if (outermostHiddenFiber === null) {
safelyDetachRef(current, nearestMountedAncestor);
const instance = current.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(
current,
nearestMountedAncestor,
instance,
);
}
}
return;
}
case HostComponent: {
safelyDetachRef(current, nearestMountedAncestor);
if (outermostHiddenFiber === null) {
safelyDetachRef(current, nearestMountedAncestor);
}
return;
}
case HostPortal: {
// TODO: this is recursive.
// We are also not using this parent because
// the portal will get pushed immediately.
if (supportsMutation) {
unmountHostComponents(finishedRoot, current, nearestMountedAncestor);
unmountHostComponents(
finishedRoot,
current,
nearestMountedAncestor,
outermostHiddenFiber,
);
} else if (supportsPersistence) {
emptyPortalContainer(current);
}
Expand All @@ -1297,8 +1309,10 @@ function commitUnmount(
return;
}
case ScopeComponent: {
if (enableScopeAPI) {
safelyDetachRef(current, nearestMountedAncestor);
if (outermostHiddenFiber === null) {
if (enableScopeAPI) {
safelyDetachRef(current, nearestMountedAncestor);
}
}
return;
}
Expand All @@ -1309,15 +1323,29 @@ function commitNestedUnmounts(
finishedRoot: FiberRoot,
root: Fiber,
nearestMountedAncestor: Fiber,
outermostHiddenFiber: Fiber | null,
): void {
let currentOutermostHiddenFiber = outermostHiddenFiber;
// While we're inside a removed host node we don't want to call
// removeChild on the inner nodes because they're removed by the top
// call anyway. We also want to call componentWillUnmount on all
// composites before this host node is removed from the tree. Therefore
// we do an inner loop while we're still inside the host node.
let node: Fiber = root;
while (true) {
commitUnmount(finishedRoot, node, nearestMountedAncestor);
if (
currentOutermostHiddenFiber === null &&
node.tag === OffscreenComponent &&
node.memoizedState !== null
) {
currentOutermostHiddenFiber = node;
}
commitUnmount(
finishedRoot,
node,
nearestMountedAncestor,
currentOutermostHiddenFiber,
);
// Visit children because they may contain more composite or host nodes.
// Skip portals because commitUnmount() currently visits them recursively.
if (
Expand All @@ -1338,6 +1366,9 @@ function commitNestedUnmounts(
return;
}
node = node.return;
if (node === currentOutermostHiddenFiber) {
currentOutermostHiddenFiber = null;
}
}
node.sibling.return = node.return;
node = node.sibling;
Expand Down Expand Up @@ -1666,6 +1697,7 @@ function unmountHostComponents(
finishedRoot: FiberRoot,
current: Fiber,
nearestMountedAncestor: Fiber,
outermostHiddenFiber: Fiber | null,
): void {
// We only have the top Fiber that was deleted but we need to recurse down its
// children to find all the terminal nodes.
Expand All @@ -1679,6 +1711,8 @@ function unmountHostComponents(
let currentParent;
let currentParentIsContainer;

let currentOutermostHiddenFiber = outermostHiddenFiber;

while (true) {
if (!currentParentIsValid) {
let parent = node.return;
Expand Down Expand Up @@ -1711,7 +1745,12 @@ function unmountHostComponents(
}

if (node.tag === HostComponent || node.tag === HostText) {
commitNestedUnmounts(finishedRoot, node, nearestMountedAncestor);
commitNestedUnmounts(
finishedRoot,
node,
nearestMountedAncestor,
currentOutermostHiddenFiber,
);
// After all the children have unmounted, it is now safe to remove the
// node from the tree.
if (currentParentIsContainer) {
Expand Down Expand Up @@ -1764,7 +1803,19 @@ function unmountHostComponents(
continue;
}
} else {
commitUnmount(finishedRoot, node, nearestMountedAncestor);
if (
node.tag === OffscreenComponent &&
currentOutermostHiddenFiber === null &&
node.memoizedState !== null
) {
currentOutermostHiddenFiber = node;
}
commitUnmount(
finishedRoot,
node,
nearestMountedAncestor,
currentOutermostHiddenFiber,
);
// Visit children because we may find more host components below.
if (node.child !== null) {
node.child.return = node;
Expand All @@ -1784,6 +1835,8 @@ function unmountHostComponents(
// When we go out of the portal, we need to restore the parent.
// Since we don't keep a stack of them, we will search for it.
currentParentIsValid = false;
} else if (node === currentOutermostHiddenFiber) {
currentOutermostHiddenFiber = null;
}
}
node.sibling.return = node.return;
Expand All @@ -1795,14 +1848,25 @@ function commitDeletion(
finishedRoot: FiberRoot,
current: Fiber,
nearestMountedAncestor: Fiber,
outermostHiddenFiber: Fiber | null,
): void {
if (supportsMutation) {
// Recursively delete all host nodes from the parent.
// Detach refs and call componentWillUnmount() on the whole subtree.
unmountHostComponents(finishedRoot, current, nearestMountedAncestor);
unmountHostComponents(
finishedRoot,
current,
nearestMountedAncestor,
outermostHiddenFiber,
);
} else {
// Detach refs and call componentWillUnmount() on the whole subtree.
commitNestedUnmounts(finishedRoot, current, nearestMountedAncestor);
commitNestedUnmounts(
finishedRoot,
current,
nearestMountedAncestor,
outermostHiddenFiber,
);
}

detachFiberMutation(current);
Expand Down Expand Up @@ -2144,6 +2208,17 @@ export function commitMutationEffects(
inProgressRoot = null;
}

function findOutermostHiddenFiber(fiber: Fiber): Fiber | null {
let node = fiber;
while (node !== null) {
if (node.tag === OffscreenComponent && node.memoizedState !== null) {
return node;
}
node = node.return;
}
return null;
}

function commitMutationEffects_begin(root: FiberRoot, lanes: Lanes) {
while (nextEffect !== null) {
const fiber = nextEffect;
Expand All @@ -2153,8 +2228,9 @@ function commitMutationEffects_begin(root: FiberRoot, lanes: Lanes) {
if (deletions !== null) {
for (let i = 0; i < deletions.length; i++) {
const childToDelete = deletions[i];
const outermostHiddenFiber = findOutermostHiddenFiber(childToDelete);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This searches up to the root for each fiber in the deletion list. I don't know if there's a way to avoid this. We need to somehow know where we are in the tree before we start descending.

try {
commitDeletion(root, childToDelete, fiber);
commitDeletion(root, childToDelete, fiber, outermostHiddenFiber);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(childToDelete, fiber, error);
Expand Down