Fix: Detach deleted fiber's alternate, too (#20587)

acdlite · web-flow · commit 42e04b46d196 · 2021-01-14T08:18:55.000-08:00
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.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -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;
@@ -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;
       }
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -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;
@@ -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;
       }
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -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);
+            }
           }
         }
       }
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -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);
+            }
           }
         }
       }
