From c896cf9617626be54f4ef599578e6f8a3a874205 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 10 Nov 2020 13:20:04 -0600 Subject: [PATCH] Set return pointer when reusing current tree (#20212) * Do not fix return pointers during commit phase In the commit phase, we should be able to assume that the `return` pointers in the just-completed tree are consistent. The render phase should be responsible for ensuring these are always correct. I've removed the `return` pointer assignments from the render phase traversal logic. This isn't all of them, only the ones added recently during the effects refactor. The other ones have been around longer so I'll leave those for a later clean up. This breaks a few SuspenseList tests; I'll fix in the next commit. * Set return pointer when reusing current tree We always set the return pointer on freshly cloned, work-in-progress fibers. However, we were neglecting to set them on trees that are reused from current. I fixed this in the same path of the complete phase where we reset the fiber flags. This is a code smell because it assumes the commit phase is never concurrent with the render phase. Our eventual goal is to make fibers a lock free data structure. Will address further during refactor to alternate model. --- .../src/ReactFiberCommitWork.new.js | 14 -------------- .../src/ReactFiberCompleteWork.new.js | 10 ++++++++++ 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 6f0b923c14445..55c7c986effc7 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -463,7 +463,6 @@ function iterativelyCommitBeforeMutationEffects_begin() { (fiber.subtreeFlags & BeforeMutationMask) !== NoFlags && child !== null ) { - child.return = fiber; nextEffect = child; } else { iterativelyCommitBeforeMutationEffects_complete(); @@ -497,7 +496,6 @@ function iterativelyCommitBeforeMutationEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } @@ -715,7 +713,6 @@ function iterativelyCommitMutationEffects_begin( const child = fiber.child; if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) { - child.return = fiber; nextEffect = child; } else { iterativelyCommitMutationEffects_complete(root, renderPriorityLevel); @@ -754,7 +751,6 @@ function iterativelyCommitMutationEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } @@ -1176,14 +1172,12 @@ function iterativelyCommitLayoutEffects_begin( } const sibling = finishedWork.sibling; if (sibling !== null) { - sibling.return = finishedWork.return; nextEffect = sibling; } else { nextEffect = finishedWork.return; iterativelyCommitLayoutEffects_complete(subtreeRoot, finishedRoot); } } else { - firstChild.return = finishedWork; nextEffect = firstChild; } } else { @@ -1230,7 +1224,6 @@ function iterativelyCommitLayoutEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } @@ -1764,14 +1757,12 @@ function iterativelyCommitPassiveMountEffects_begin( } const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; } else { nextEffect = fiber.return; iterativelyCommitPassiveMountEffects_complete(subtreeRoot, root); } } else { - firstChild.return = fiber; nextEffect = firstChild; } } else { @@ -1817,7 +1808,6 @@ function iterativelyCommitPassiveMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } @@ -1896,7 +1886,6 @@ function iterativelyCommitPassiveUnmountEffects_begin() { } if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && child !== null) { - child.return = fiber; nextEffect = child; } else { iterativelyCommitPassiveUnmountEffects_complete(); @@ -1915,7 +1904,6 @@ function iterativelyCommitPassiveUnmountEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } @@ -1953,7 +1941,6 @@ function iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_begin( const fiber = nextEffect; const child = fiber.child; if ((fiber.subtreeFlags & PassiveStatic) !== NoFlags && child !== null) { - child.return = fiber; nextEffect = child; } else { iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_complete( @@ -1981,7 +1968,6 @@ function iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_complete( const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index 0a187f9c84b07..1b353a097268d 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -738,6 +738,11 @@ function bubbleProperties(completedWork: Fiber) { subtreeFlags |= child.subtreeFlags; subtreeFlags |= child.flags; + // Update the return pointer so the tree is consistent. This is a code + // smell because it assumes the commit phase is never concurrent with + // the render phase. Will address during refactor to alternate model. + child.return = completedWork; + child = child.sibling; } } @@ -784,6 +789,11 @@ function bubbleProperties(completedWork: Fiber) { subtreeFlags |= child.subtreeFlags & StaticMask; subtreeFlags |= child.flags & StaticMask; + // Update the return pointer so the tree is consistent. This is a code + // smell because it assumes the commit phase is never concurrent with + // the render phase. Will address during refactor to alternate model. + child.return = completedWork; + child = child.sibling; } }