Clear fiber.sibling field when clearing nextEffect (facebook#18970)

* Clear fiber.sibling field when clearing nextEffect

Author: trueadm

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

@@ -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;
@@ -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);
   }
 }
 

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

@@ -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;
@@ -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);
   }
 }
 

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

@@ -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;
     }
   }
@@ -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;
   }
 
@@ -3549,3 +3555,7 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {
     };
   }
 }
+
+function detachFiberAfterEffects(fiber: Fiber): void {
+  fiber.sibling = null;
+}

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

@@ -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;
     }
   }
@@ -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;
   }
 
@@ -3712,3 +3718,7 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {
     };
   }
 }
+
+function detachFiberAfterEffects(fiber: Fiber): void {
+  fiber.sibling = null;
+}