Skip to content

Commit

Permalink
Fix: Detach deleted fiber's alternate, too
Browse files Browse the repository at this point in the history
We need to call `detachFiberAfterEffects` on the fiber's alternate to
clean it up. We're currently not, because the `alternate` field is
cleared during `detachFiberMutation`. So I deferred detaching the
`alternate` until the passive phase. Only the `return` pointer needs to
be detached for semantic purposes.

I don't think there's any good way to test this without using
reflection. It's not even observable using out our "supported"
reflection APIs (`findDOMNode`), or at least not that I can think of.
Which is a good thing, in a way.

It's not really a memory leak, either, because the only reference to the
alternate fiber is from the parent's alternate. Which will be
disconnected the next time the parent is updated or deleted.

It's really only observable if you mess around with internals in ways
you're not supposed to — I found it because a product test in www that
uses Enzyme was doing just that.

In lieu of a new unit test, I confirmed this patch fixes the broken
product test.
  • Loading branch information
acdlite committed Jan 14, 2021
1 parent a656ace commit e522991
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 10 deletions.
14 changes: 9 additions & 5 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -1084,17 +1084,17 @@ function detachFiberMutation(fiber: Fiber) {
// Note that we can't clear child or sibling pointers yet.
// They're needed for passive effects and for findDOMNode.
// We defer those fields, and all other cleanup, to the passive phase (see detachFiberAfterEffects).
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.return = null;
fiber.alternate = null;
}
//
// Don't reset the alternate yet, either. We need that so we can detach the
// alternate's fields in the passive phase. Clearing the return pointer is
// sufficient for findDOMNode semantics.
fiber.return = null;
}

export function detachFiberAfterEffects(fiber: Fiber): void {
// Null out fields to improve GC for references that may be lingering (e.g. DevTools).
// Note that we already cleared the return pointer in detachFiberMutation().
fiber.alternate = null;
fiber.child = null;
fiber.deletions = null;
fiber.dependencies = null;
Expand Down Expand Up @@ -1936,7 +1936,11 @@ function commitPassiveUnmountEffects_begin() {
commitPassiveUnmountEffectsInsideOfDeletedTree_begin(fiberToDelete);

// Now that passive effects have been processed, it's safe to detach lingering pointers.
const alternate = fiberToDelete.alternate;
detachFiberAfterEffects(fiberToDelete);
if (alternate !== null) {
detachFiberAfterEffects(alternate);
}
}
nextEffect = fiber;
}
Expand Down
14 changes: 9 additions & 5 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -1084,17 +1084,17 @@ function detachFiberMutation(fiber: Fiber) {
// Note that we can't clear child or sibling pointers yet.
// They're needed for passive effects and for findDOMNode.
// We defer those fields, and all other cleanup, to the passive phase (see detachFiberAfterEffects).
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.return = null;
fiber.alternate = null;
}
//
// Don't reset the alternate yet, either. We need that so we can detach the
// alternate's fields in the passive phase. Clearing the return pointer is
// sufficient for findDOMNode semantics.
fiber.return = null;
}

export function detachFiberAfterEffects(fiber: Fiber): void {
// Null out fields to improve GC for references that may be lingering (e.g. DevTools).
// Note that we already cleared the return pointer in detachFiberMutation().
fiber.alternate = null;
fiber.child = null;
fiber.deletions = null;
fiber.dependencies = null;
Expand Down Expand Up @@ -1936,7 +1936,11 @@ function commitPassiveUnmountEffects_begin() {
commitPassiveUnmountEffectsInsideOfDeletedTree_begin(fiberToDelete);

// Now that passive effects have been processed, it's safe to detach lingering pointers.
const alternate = fiberToDelete.alternate;
detachFiberAfterEffects(fiberToDelete);
if (alternate !== null) {
detachFiberAfterEffects(alternate);
}
}
nextEffect = fiber;
}
Expand Down
4 changes: 4 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -2148,7 +2148,11 @@ function commitRootImpl(root, renderPriorityLevel) {
if (deletions !== null) {
for (let i = 0; i < deletions.length; i++) {
const deletion = deletions[i];
const alternate = deletion.alternate;
detachFiberAfterEffects(deletion);
if (alternate !== null) {
detachFiberAfterEffects(alternate);
}
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -2148,7 +2148,11 @@ function commitRootImpl(root, renderPriorityLevel) {
if (deletions !== null) {
for (let i = 0; i < deletions.length; i++) {
const deletion = deletions[i];
const alternate = deletion.alternate;
detachFiberAfterEffects(deletion);
if (alternate !== null) {
detachFiberAfterEffects(alternate);
}
}
}
}
Expand Down

0 comments on commit e522991

Please sign in to comment.