From e522991c4172021e5d46c9ff4c3829eff2f57b0d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 14 Jan 2021 01:55:18 -0600 Subject: [PATCH] Fix: Detach deleted fiber's alternate, too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/ReactFiberCommitWork.new.js | 14 +++++++++----- .../src/ReactFiberCommitWork.old.js | 14 +++++++++----- .../react-reconciler/src/ReactFiberWorkLoop.new.js | 4 ++++ .../react-reconciler/src/ReactFiberWorkLoop.old.js | 4 ++++ 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index a048500e9ddf3..32bb1028add82 100644 --- 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 index 72f801981d30b..d3bfe78e39479 100644 --- 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 index 03831988e2b2b..0c50c804da36b 100644 --- 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 index 6966472e0d16f..caeab7ce67330 100644 --- 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); + } } } }