Clear memoizedState on unmount of fiber to avoid memory leak #14218
Conversation
|
Oh and |
|
I still don't understand why this is necessary. Isn't the expectation that all the pointers to the unmounted Fiber are removed? In which case you don't need to null out all its fields. |
|
@sophiebits The parent alternate could still have a reference via its child pointer |
|
Can't we null out .return.alternate.child? If I understand our pooling correctly, we would never end up reusing the parent alternate fiber without re-cloning anyway. |
|
The code comment explains that we don't detach the alternate pointer because we're not sure if it's current but that should be fixable. Excepting resuming. |
|
That's a riskier change, though. Return pointers are confusing. Could be worth it. |
|
I'm all about risky. |
|
I guess it's not safe. With this diff diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js
index bed780c2b..2fdc77ee0 100644
--- a/packages/react-reconciler/src/ReactChildFiber.js
+++ b/packages/react-reconciler/src/ReactChildFiber.js
@@ -256,6 +256,7 @@ function ChildReconciler(shouldTrackSideEffects) {
}
childToDelete.nextEffect = null;
childToDelete.effectTag = Deletion;
+ childToDelete.return = returnFiber;
}
function deleteRemainingChildren(
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
index 55a39ee64..eee0659dc 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.js
@@ -726,6 +726,10 @@ function detachFiber(current: Fiber) {
// get GC:ed but we don't know which for sure which parent is the current
// one so we'll settle for GC:ing the subtree of this child. This child
// itself will be GC:ed when the parent updates the next time.
+ const returnAlternate = current.return.alternate;
+ if (returnAlternate !== null) {
+ returnAlternate.child = null;
+ }
current.return = null;
current.child = null;
if (current.alternate) {
diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js
index fbd3b5b65..4da60e633 100644
--- a/packages/react-reconciler/src/ReactFiberCompleteWork.js
+++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js
@@ -725,6 +725,7 @@ function completeWork(
currentFallbackChild.nextEffect = null;
}
currentFallbackChild.effectTag = Deletion;
+ currentFallbackChild.return = current.child;
}
}
all but one of our tests pass. But I realized that some of our traversals – like in findDOMNode – can run in between render and commit on the alternate tree and will mutate the .return pointers. So we can't rely on this to do the right thing. If we could store the return pointer for a deletion somewhere else where it wouldn't be clobbered then I guess it would work. (Or we could do a variant of findCurrentFiberUsingSlowPath where we iterate the children of .return and .return.alternate, but it's still easy to get n^2 doing that.) |
|
ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.1% Details of bundled changes.Comparing: d204747...5d65390 react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
|
I added a bunch of fields as suggested by @acdlite, minus |
| @@ -726,11 +726,20 @@ function detachFiber(current: Fiber) { | |||
| // get GC:ed but we don't know which for sure which parent is the current | |||
| // one so we'll settle for GC:ing the subtree of this child. This child | |||
| // itself will be GC:ed when the parent updates the next time. | |||
| // We do not null out the 'nextEffect' field as it causes tests to fail. | |||
There was a problem hiding this comment.
Do we have a better understanding of this? "Causes tests to fail" doesn't say anything about the intent and whether those tests should fail or if there's some other mistake.
This feels dangerous to me if we don't understand it. How do you know clearing firstEffect is safe if nextEffect is unsafe? Maybe we just accidentally hit some cases in our tests but not others.
There was a problem hiding this comment.
In general I'd prefer that we don't use vague wording like "causes tests to fail" in the comments. Either we know why it fails and explain the conceptual reason. Or we don't know, and should understand before merging.
|
Follow up PR created #14276. |
…k#14218) * Clear properties on unmount of fiber to ensure objects are not retained
…acebook#14218)" (facebook#14275) This reverts commit 9b2fb24.
…k#14218) * Clear properties on unmount of fiber to ensure objects are not retained
…acebook#14218)" (facebook#14275) This reverts commit 9b2fb24.
This is an alternative approach to #14153. This PR aims at clearing the memory leak when the associated fibers containing the references to the closures (stored in
memoizedState) still exist after being unmounted. The fix is to simplynullout thememoizedStatefield which seems to clear up the retained functions.