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.

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

Clear fiber.sibling field when clearing nextEffect #18970

Merged
merged 2 commits into from May 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 5 additions & 4 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
Expand Up @@ -1124,15 +1124,16 @@ function commitNestedUnmounts(
}
}

function detachFiber(fiber: Fiber) {
function detachFiberMutation(fiber: Fiber) {
// Cut off the return pointers to disconnect it from the tree. Ideally, we
// should clear the child pointer of the parent alternate to let this
// get GC:ed but we don't know which for sure which parent is the current
// one so we'll settle for GC:ing the subtree of this child. This child
// itself will be GC:ed when the parent updates the next time.
// Note: we cannot null out sibling here, otherwise it can cause issues
// with findDOMNode and how it requires the sibling field to carry out
// traversal in a later effect. See PR #16820.
// traversal in a later effect. See PR #16820. We now clear the sibling
// field after effects, see: detachFiberAfterEffects.
fiber.alternate = null;
fiber.child = null;
fiber.dependencies_new = null;
Expand Down Expand Up @@ -1543,9 +1544,9 @@ function commitDeletion(
commitNestedUnmounts(finishedRoot, current, renderPriorityLevel);
}
const alternate = current.alternate;
detachFiber(current);
detachFiberMutation(current);
if (alternate !== null) {
detachFiber(alternate);
detachFiberMutation(alternate);
}
}

Expand Down
9 changes: 5 additions & 4 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
Expand Up @@ -1122,15 +1122,16 @@ function commitNestedUnmounts(
}
}

function detachFiber(fiber: Fiber) {
function detachFiberMutation(fiber: Fiber) {
// Cut off the return pointers to disconnect it from the tree. Ideally, we
// should clear the child pointer of the parent alternate to let this
// get GC:ed but we don't know which for sure which parent is the current
// one so we'll settle for GC:ing the subtree of this child. This child
// itself will be GC:ed when the parent updates the next time.
// Note: we cannot null out sibling here, otherwise it can cause issues
// with findDOMNode and how it requires the sibling field to carry out
// traversal in a later effect. See PR #16820.
// traversal in a later effect. See PR #16820. We now clear the sibling
// field after effects, see: detachFiberAfterEffects.
fiber.alternate = null;
fiber.child = null;
fiber.dependencies_old = null;
Expand Down Expand Up @@ -1541,9 +1542,9 @@ function commitDeletion(
commitNestedUnmounts(finishedRoot, current, renderPriorityLevel);
}
const alternate = current.alternate;
detachFiber(current);
detachFiberMutation(current);
if (alternate !== null) {
detachFiber(alternate);
detachFiberMutation(alternate);
}
}

Expand Down
10 changes: 10 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Expand Up @@ -1993,6 +1993,9 @@ function commitRootImpl(root, renderPriorityLevel) {
while (nextEffect !== null) {
const nextNextEffect = nextEffect.nextEffect;
nextEffect.nextEffect = null;
if (nextEffect.effectTag & Deletion) {
detachFiberAfterEffects(nextEffect);
}
nextEffect = nextNextEffect;
}
}
Expand Down Expand Up @@ -2447,6 +2450,9 @@ function flushPassiveEffectsImpl() {
const nextNextEffect = effect.nextEffect;
// Remove nextEffect pointer to assist GC
effect.nextEffect = null;
if (effect.effectTag & Deletion) {
detachFiberAfterEffects(effect);
}
effect = nextNextEffect;
}

Expand Down Expand Up @@ -3549,3 +3555,7 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {
};
}
}

function detachFiberAfterEffects(fiber: Fiber): void {
fiber.sibling = null;
}
10 changes: 10 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Expand Up @@ -2101,6 +2101,9 @@ function commitRootImpl(root, renderPriorityLevel) {
while (nextEffect !== null) {
const nextNextEffect = nextEffect.nextEffect;
nextEffect.nextEffect = null;
if (nextEffect.effectTag & Deletion) {
detachFiberAfterEffects(nextEffect);
}
nextEffect = nextNextEffect;
}
}
Expand Down Expand Up @@ -2595,6 +2598,9 @@ function flushPassiveEffectsImpl() {
const nextNextEffect = effect.nextEffect;
// Remove nextEffect pointer to assist GC
effect.nextEffect = null;
if (effect.effectTag & Deletion) {
detachFiberAfterEffects(effect);
}
effect = nextNextEffect;
}

Expand Down Expand Up @@ -3712,3 +3718,7 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {
};
}
}

function detachFiberAfterEffects(fiber: Fiber): void {
fiber.sibling = null;
}