From f3396bf8483bcffe4d61a34a743a8b03b26c6413 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 21 May 2020 12:17:28 +0100 Subject: [PATCH 1/2] Clear fiber.sibling field when clearing nextEffect --- packages/react-reconciler/src/ReactFiberWorkLoop.new.js | 6 ++++++ packages/react-reconciler/src/ReactFiberWorkLoop.old.js | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 0a29277d759f..d8934eb5c5e9 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) { + nextEffect.sibling = null; + } 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) { + effect.sibling = null; + } effect = nextNextEffect; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 456bd9670e92..3f5cd0befbae 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) { + nextEffect.sibling = null; + } 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) { + effect.sibling = null; + } effect = nextNextEffect; } From eb3dc35535401d5822cf52f3184faee859954833 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 21 May 2020 18:23:17 +0100 Subject: [PATCH 2/2] Address feedback --- .../react-reconciler/src/ReactFiberCommitWork.new.js | 9 +++++---- .../react-reconciler/src/ReactFiberCommitWork.old.js | 9 +++++---- packages/react-reconciler/src/ReactFiberWorkLoop.new.js | 8 ++++++-- packages/react-reconciler/src/ReactFiberWorkLoop.old.js | 8 ++++++-- 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 72e501a8d326..59ed975cd493 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 80a4673f9a28..5017ed66fb4f 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 d8934eb5c5e9..e00cc4e8e741 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1994,7 +1994,7 @@ function commitRootImpl(root, renderPriorityLevel) { const nextNextEffect = nextEffect.nextEffect; nextEffect.nextEffect = null; if (nextEffect.effectTag & Deletion) { - nextEffect.sibling = null; + detachFiberAfterEffects(nextEffect); } nextEffect = nextNextEffect; } @@ -2451,7 +2451,7 @@ function flushPassiveEffectsImpl() { // Remove nextEffect pointer to assist GC effect.nextEffect = null; if (effect.effectTag & Deletion) { - effect.sibling = null; + detachFiberAfterEffects(effect); } effect = nextNextEffect; } @@ -3555,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 3f5cd0befbae..b558ad7f4567 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2102,7 +2102,7 @@ function commitRootImpl(root, renderPriorityLevel) { const nextNextEffect = nextEffect.nextEffect; nextEffect.nextEffect = null; if (nextEffect.effectTag & Deletion) { - nextEffect.sibling = null; + detachFiberAfterEffects(nextEffect); } nextEffect = nextNextEffect; } @@ -2599,7 +2599,7 @@ function flushPassiveEffectsImpl() { // Remove nextEffect pointer to assist GC effect.nextEffect = null; if (effect.effectTag & Deletion) { - effect.sibling = null; + detachFiberAfterEffects(effect); } effect = nextNextEffect; } @@ -3718,3 +3718,7 @@ export function act(callback: () => Thenable): Thenable { }; } } + +function detachFiberAfterEffects(fiber: Fiber): void { + fiber.sibling = null; +}