From 730ae7afa2a2f620a77490ad4e2fbcc98f326da2 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 21 May 2020 18:53:56 +0100 Subject: [PATCH] Clear fiber.sibling field when clearing nextEffect (#18970) * Clear fiber.sibling field when clearing nextEffect --- .../react-reconciler/src/ReactFiberCommitWork.new.js | 9 +++++---- .../react-reconciler/src/ReactFiberCommitWork.old.js | 9 +++++---- .../react-reconciler/src/ReactFiberWorkLoop.new.js | 10 ++++++++++ .../react-reconciler/src/ReactFiberWorkLoop.old.js | 10 ++++++++++ 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 72e501a8d326e..59ed975cd4937 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1124,7 +1124,7 @@ 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 @@ -1132,7 +1132,8 @@ function detachFiber(fiber: Fiber) { // 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); } } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 80a4673f9a288..5017ed66fb4f6 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1122,7 +1122,7 @@ 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 @@ -1130,7 +1130,8 @@ function detachFiber(fiber: Fiber) { // 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); } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 0a29277d759ff..e00cc4e8e741a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/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): Thenable { }; } } + +function detachFiberAfterEffects(fiber: Fiber): void { + fiber.sibling = null; +} diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 456bd9670e922..b558ad7f4567f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/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): Thenable { }; } } + +function detachFiberAfterEffects(fiber: Fiber): void { + fiber.sibling = null; +}