Fix: Detach deleted fiber's alternate, too (#20587)
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.
Andrew Clark committed
Jan 14, 2021 at 10:18 UTC
42e04b46d19648968537a404f15cebc42b6fab54
4 files changed
+26
-10
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+9
-5
@@ -1084,17 +1084,17 @@ function detachFiberMutation(fiber: Fiber) {
1084
// Note that we can't clear child or sibling pointers yet.
1085
// They're needed for passive effects and for findDOMNode.
1086
// We defer those fields, and all other cleanup, to the passive phase (see detachFiberAfterEffects).
1087
- const alternate = fiber.alternate;
1088
- if (alternate !== null) {
1089
- alternate.return = null;
1090
- fiber.alternate = null;
1091
- }
1087
+ //
1088
+ // Don't reset the alternate yet, either. We need that so we can detach the
1089
+ // alternate's fields in the passive phase. Clearing the return pointer is
1090
+ // sufficient for findDOMNode semantics.
1091
fiber.return = null;
1092
}
1093
1094
export function detachFiberAfterEffects(fiber: Fiber): void {
1095
// Null out fields to improve GC for references that may be lingering (e.g. DevTools).
1096
// Note that we already cleared the return pointer in detachFiberMutation().
1097
+ fiber.alternate = null;
1098
fiber.child = null;
1099
fiber.deletions = null;
1100
fiber.dependencies = null;
@@ -1936,7 +1936,11 @@ function commitPassiveUnmountEffects_begin() {
1936
commitPassiveUnmountEffectsInsideOfDeletedTree_begin(fiberToDelete);
1937
1938
// Now that passive effects have been processed, it's safe to detach lingering pointers.
1939
+ const alternate = fiberToDelete.alternate;
1940
detachFiberAfterEffects(fiberToDelete);
1941
+ if (alternate !== null) {
1942
+ detachFiberAfterEffects(alternate);
1943
+ }
1944
}
1945
nextEffect = fiber;
1946
}
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+9
-5
@@ -1084,17 +1084,17 @@ function detachFiberMutation(fiber: Fiber) {
1084
// Note that we can't clear child or sibling pointers yet.
1085
// They're needed for passive effects and for findDOMNode.
1086
// We defer those fields, and all other cleanup, to the passive phase (see detachFiberAfterEffects).
1087
- const alternate = fiber.alternate;
1088
- if (alternate !== null) {
1089
- alternate.return = null;
1090
- fiber.alternate = null;
1091
- }
1087
+ //
1088
+ // Don't reset the alternate yet, either. We need that so we can detach the
1089
+ // alternate's fields in the passive phase. Clearing the return pointer is
1090
+ // sufficient for findDOMNode semantics.
1091
fiber.return = null;
1092
}
1093
1094
export function detachFiberAfterEffects(fiber: Fiber): void {
1095
// Null out fields to improve GC for references that may be lingering (e.g. DevTools).
1096
// Note that we already cleared the return pointer in detachFiberMutation().
1097
+ fiber.alternate = null;
1098
fiber.child = null;
1099
fiber.deletions = null;
1100
fiber.dependencies = null;
@@ -1936,7 +1936,11 @@ function commitPassiveUnmountEffects_begin() {
1936
commitPassiveUnmountEffectsInsideOfDeletedTree_begin(fiberToDelete);
1937
1938
// Now that passive effects have been processed, it's safe to detach lingering pointers.
1939
+ const alternate = fiberToDelete.alternate;
1940
detachFiberAfterEffects(fiberToDelete);
1941
+ if (alternate !== null) {
1942
+ detachFiberAfterEffects(alternate);
1943
+ }
1944
}
1945
nextEffect = fiber;
1946
}
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+4
@@ -2148,7 +2148,11 @@ function commitRootImpl(root, renderPriorityLevel) {
2148
if (deletions !== null) {
2149
for (let i = 0; i < deletions.length; i++) {
2150
const deletion = deletions[i];
2151
+ const alternate = deletion.alternate;
2152
detachFiberAfterEffects(deletion);
2153
+ if (alternate !== null) {
2154
+ detachFiberAfterEffects(alternate);
2155
+ }
2156
}
2157
}
2158
}
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+4
@@ -2148,7 +2148,11 @@ function commitRootImpl(root, renderPriorityLevel) {
2148
if (deletions !== null) {
2149
for (let i = 0; i < deletions.length; i++) {
2150
const deletion = deletions[i];
2151
+ const alternate = deletion.alternate;
2152
detachFiberAfterEffects(deletion);
2153
+ if (alternate !== null) {
2154
+ detachFiberAfterEffects(alternate);
2155
+ }
2156
}
2157
}
2158
}