Skip to content

Commit 4df33b6

Browse files
committed
Fix: Detach deleted fiber's alternate, too
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.
1 parent a656ace commit 4df33b6

File tree

4 files changed

+20
-4
lines changed

4 files changed

+20
-4
lines changed

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1084,17 +1084,17 @@ function detachFiberMutation(fiber: Fiber) {
10841084
// Note that we can't clear child or sibling pointers yet.
10851085
// They're needed for passive effects and for findDOMNode.
10861086
// We defer those fields, and all other cleanup, to the passive phase (see detachFiberAfterEffects).
1087+
fiber.return = null;
10871088
const alternate = fiber.alternate;
10881089
if (alternate !== null) {
10891090
alternate.return = null;
1090-
fiber.alternate = null;
10911091
}
1092-
fiber.return = null;
10931092
}
10941093

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

19381938
// Now that passive effects have been processed, it's safe to detach lingering pointers.
1939+
const alternate = fiberToDelete.alternate;
19391940
detachFiberAfterEffects(fiberToDelete);
1941+
if (alternate !== null) {
1942+
detachFiberAfterEffects(alternate);
1943+
}
19401944
}
19411945
nextEffect = fiber;
19421946
}

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1084,17 +1084,17 @@ function detachFiberMutation(fiber: Fiber) {
10841084
// Note that we can't clear child or sibling pointers yet.
10851085
// They're needed for passive effects and for findDOMNode.
10861086
// We defer those fields, and all other cleanup, to the passive phase (see detachFiberAfterEffects).
1087+
fiber.return = null;
10871088
const alternate = fiber.alternate;
10881089
if (alternate !== null) {
10891090
alternate.return = null;
1090-
fiber.alternate = null;
10911091
}
1092-
fiber.return = null;
10931092
}
10941093

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

19381938
// Now that passive effects have been processed, it's safe to detach lingering pointers.
1939+
const alternate = fiberToDelete.alternate;
19391940
detachFiberAfterEffects(fiberToDelete);
1941+
if (alternate !== null) {
1942+
detachFiberAfterEffects(alternate);
1943+
}
19401944
}
19411945
nextEffect = fiber;
19421946
}

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2148,7 +2148,11 @@ function commitRootImpl(root, renderPriorityLevel) {
21482148
if (deletions !== null) {
21492149
for (let i = 0; i < deletions.length; i++) {
21502150
const deletion = deletions[i];
2151+
const alternate = deletion.alternate;
21512152
detachFiberAfterEffects(deletion);
2153+
if (alternate !== null) {
2154+
detachFiberAfterEffects(alternate);
2155+
}
21522156
}
21532157
}
21542158
}

packages/react-reconciler/src/ReactFiberWorkLoop.old.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2148,7 +2148,11 @@ function commitRootImpl(root, renderPriorityLevel) {
21482148
if (deletions !== null) {
21492149
for (let i = 0; i < deletions.length; i++) {
21502150
const deletion = deletions[i];
2151+
const alternate = deletion.alternate;
21512152
detachFiberAfterEffects(deletion);
2153+
if (alternate !== null) {
2154+
detachFiberAfterEffects(alternate);
2155+
}
21522156
}
21532157
}
21542158
}

0 commit comments

Comments
 (0)