From b73ed8cd1c7fa2472886b36e2c3899f7833e265d Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Fri, 23 Apr 2021 11:53:36 -0500
Subject: [PATCH] Revert "Clean up host pointers in level 2 of clean-up flag
 (#21112)"

This reverts commit 8ed0c85bf174ce6e501be62d9ccec1889bbdbce1.

The host tree is a cyclical structure. Leaking a single DOM node can
retain a large amount of memory. React-managed DOM nodes also point
back to a fiber tree.

Perf testing suggests that disconnecting these fields has a big memory
impact. That suggests leaks in non-React code but since it's hard to
completely eliminate those, it may still be worth the extra work to
clear these fields.

I'm moving this to level 2 to confirm whether this alone is responsible
for the memory savings, or if there are other fields that are retaining
large amounts of memory.

In our plan for removing the alternate model, DOM nodes would not be
connected to fibers, except at the root of the whole tree, which is
easy to disconnect on deletion. So in that world, we likely won't have
to do any additional work.
---
 .../src/ReactFiberCommitWork.new.js           | 21 ++++++++-----------
 .../src/ReactFiberCommitWork.old.js           | 21 ++++++++-----------
 2 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
index 46f19294562c1..bbd6f56925225 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -1382,18 +1382,6 @@ function detachFiberAfterEffects(fiber: Fiber) {
     fiber.deletions = null;
     fiber.sibling = null;
 
-    // The `stateNode` is cyclical because on host nodes it points to the host
-    // tree, which has its own pointers to children, parents, and siblings.
-    // The other host nodes also point back to fibers, so we should detach that
-    // one, too.
-    if (fiber.tag === HostComponent) {
-      const hostInstance: Instance = fiber.stateNode;
-      if (hostInstance !== null) {
-        detachDeletedInstance(hostInstance);
-      }
-    }
-    fiber.stateNode = null;
-
     // I'm intentionally not clearing the `return` field in this level. We
     // already disconnect the `return` pointer at the root of the deleted
     // subtree (in `detachFiberMutation`). Besides, `return` by itself is not
@@ -1412,6 +1400,15 @@ function detachFiberAfterEffects(fiber: Fiber) {
       // The purpose of this branch is to be super aggressive so we can measure
       // if there's any difference in memory impact. If there is, that could
       // indicate a React leak we don't know about.
+
+      // For host components, disconnect host instance -> fiber pointer.
+      if (fiber.tag === HostComponent) {
+        const hostInstance: Instance = fiber.stateNode;
+        if (hostInstance !== null) {
+          detachDeletedInstance(hostInstance);
+        }
+      }
+
       fiber.return = null;
       fiber.dependencies = null;
       fiber.memoizedProps = null;
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
index f45a44083e236..40df79a1b8528 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -1382,18 +1382,6 @@ function detachFiberAfterEffects(fiber: Fiber) {
     fiber.deletions = null;
     fiber.sibling = null;
 
-    // The `stateNode` is cyclical because on host nodes it points to the host
-    // tree, which has its own pointers to children, parents, and siblings.
-    // The other host nodes also point back to fibers, so we should detach that
-    // one, too.
-    if (fiber.tag === HostComponent) {
-      const hostInstance: Instance = fiber.stateNode;
-      if (hostInstance !== null) {
-        detachDeletedInstance(hostInstance);
-      }
-    }
-    fiber.stateNode = null;
-
     // I'm intentionally not clearing the `return` field in this level. We
     // already disconnect the `return` pointer at the root of the deleted
     // subtree (in `detachFiberMutation`). Besides, `return` by itself is not
@@ -1412,6 +1400,15 @@ function detachFiberAfterEffects(fiber: Fiber) {
       // The purpose of this branch is to be super aggressive so we can measure
       // if there's any difference in memory impact. If there is, that could
       // indicate a React leak we don't know about.
+
+      // For host components, disconnect host instance -> fiber pointer.
+      if (fiber.tag === HostComponent) {
+        const hostInstance: Instance = fiber.stateNode;
+        if (hostInstance !== null) {
+          detachDeletedInstance(hostInstance);
+        }
+      }
+
       fiber.return = null;
       fiber.dependencies = null;
       fiber.memoizedProps = null;