Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up unmounted fiber references for GC #15157

Closed
wants to merge 14 commits into from
Closed

Clean up unmounted fiber references for GC #15157

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c6947e4
Clear nextEffect pointer for Deletion effects
paulshen Mar 17, 2019
7aa4b6c
clear stateNode
paulshen Mar 18, 2019
50de1fa
remove stateNode from alternate as well
paulshen Mar 19, 2019
c2bbc8d
add comment
paulshen Mar 19, 2019
2100482
Unhook deletion effects from effect chain
paulshen Mar 20, 2019
c1f16f3
move to commitAllHostEffects and undo passive effect changes
paulshen Mar 20, 2019
80b908c
fix flow
paulshen Mar 20, 2019
41a983b
cleanup
paulshen Mar 20, 2019
4cae222
Merge remote-tracking branch 'facebook/master' into clean-references-…
paulshen Mar 21, 2019
137d9b0
Merge remote-tracking branch 'facebook/master' into clean-references-…
paulshen Apr 8, 2019
de27cd4
clear nextEffect chain
paulshen Apr 8, 2019
2edaa1f
remove all nextEffect pointers
paulshen Apr 8, 2019
ea2c6d7
update comment
paulshen Apr 8, 2019
568ef3e
break instead of return
paulshen Jun 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,12 @@ function commitUnmount(current: Fiber): void {
}
}
}

// Remove reference for GC
current.stateNode = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We won't want to do this indiscriminately for all types of Fibers. They might not all have this field. We're looking to use Flow disjoint unions to enforce that they're consistently used for each type. They don't all use it in the same way where this makes sense.

We should be more specific and only do this for the type where it's needed.

However, which type is this supposed to clean up? Is it just DOM nodes? Is it class instance references? What about the equivalent pattern for Hooks? It's hard to tell without knowing more about the particular pattern this fix is solving for.

I'm guessing that this is somehow meant to release DOM nodes that might be part of larger subtrees and therefore might have back pointers into Fibers. However, where is the root that hold onto the first Fiber that might point back into those anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Here's an example where mounted DOM/component link back to detached DOM elements. The root cause in this example is probably the effect pointers from mounted fibers to unmounted ones.

They don't all use it in the same way where this makes sense.

I'm not familiar enough with the internals to know where it doesn't make sense. I believe it though 😄 This was from a naive reading that FiberNodes have this field. Mainly, I was thinking about stateNode pointers for HostText/HostComponent (DOM) and ClassComponent but it didn't seem costly to just reset for all. Should I limit to HostText and HostComponent?

However, where is the root that hold onto the first Fiber that might point back into those anyway?

I know this is the crux of this whole thing but the chains were long, sometimes ending in V8 internals. I very much believe it could be a user-space issue but it was not obvious (to me) and these changes seemed cheap/reasonable enough to prevent cascading effects.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular example is rooted in a nextEffect. With your other fix, that would be reset to null. So would that be sufficient to fix this particular leak?

Maybe we can split out this part to a separate PR and just land the nextEffect part like you suggested? That way we can see if that is sufficient or not.

if (current.alternate != null) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be if (current.alternate !== null) {?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably because of this:

> const a = {}
undefined
> a.x != null
false
> a.x == null
true
> a.x === null
false

It's a nice way to cover null and undefined scenarios.

current.alternate.stateNode = null;
}
}

function commitNestedUnmounts(root: Fiber): void {
Expand Down Expand Up @@ -1050,19 +1056,15 @@ function unmountHostComponents(current): void {
}

if (node.tag === HostComponent || node.tag === HostText) {
// Save stateNode reference so commitUnmount can clear it.
const stateNode: Instance | TextInstance = node.stateNode;
commitNestedUnmounts(node);
// After all the children have unmounted, it is now safe to remove the
// node from the tree.
if (currentParentIsContainer) {
removeChildFromContainer(
((currentParent: any): Container),
(node.stateNode: Instance | TextInstance),
);
removeChildFromContainer(((currentParent: any): Container), stateNode);
} else {
removeChild(
((currentParent: any): Instance),
(node.stateNode: Instance | TextInstance),
);
removeChild(((currentParent: any): Instance), stateNode);
}
// Don't visit children because we already visited them.
} else if (
Expand Down
14 changes: 13 additions & 1 deletion packages/react-reconciler/src/ReactFiberScheduler.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,6 +1404,15 @@ function commitRootImpl(root, expirationTime) {
rootWithPendingPassiveEffects = root;
pendingPassiveEffectsExpirationTime = expirationTime;
} else {
// We are done with the effect chain at this point so let's clear the
// nextEffect pointers to assist with GC. If we have passive effects, we'll
// clear this in flushPassiveEffects.
nextEffect = firstEffect;
while (nextEffect !== null) {
const nextNextEffect = nextEffect.nextEffect;
nextEffect.nextEffect = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part of the fix makes sense because otherwise if this node (nextEffect) never gets any updates ever again, it'll now have a reference to a node that might later get deleted. Should be pretty easy to come up with a reduced test case for this.

Copy link
Contributor Author

@paulshen paulshen Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to come up with an isolated repro. https://github.com/paulshen/react-next-effect-leak/blob/master/src/App.js Live nextEffect pointers can point to zombie nodes. Here is a running sandbox https://etc33.codesandbox.io/. Examine the nextEffect pointer of <Sidebar> after clicking "Remount InnerBody" several times. You can see that it remains pointing at the original (and unmounted) <InnerBody>.

I tried to create this repro in the past but was bamboozled by cloneChildFibers and

react/packages/react-reconciler/src/ReactFiber.js

Line 418 in 67e3f3f

workInProgress.nextEffect = null;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just add a comment next to it with a link to your repro. We don't need to add any infra just for this. We can figure that out later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebmarkbage in case it was missed, I opened #16115

nextEffect = nextNextEffect;
}
if (enableSchedulerTracing) {
// If there are no passive effects, then we can complete the pending
// interactions. Otherwise, we'll wait until after the passive effects
Expand Down Expand Up @@ -1620,7 +1629,10 @@ export function flushPassiveEffects() {
captureCommitPhaseError(effect, error);
}
}
effect = effect.nextEffect;
const nextNextEffect = effect.nextEffect;
// Remove nextEffect pointer to assist GC
effect.nextEffect = null;
effect = nextNextEffect;
}

if (enableSchedulerTracing) {
Expand Down
15 changes: 14 additions & 1 deletion packages/react-reconciler/src/ReactFiberScheduler.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,10 @@ function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {
captureCommitPhaseError(effect, error);
}
}
effect = effect.nextEffect;
const nextNextEffect = effect.nextEffect;
// Remove nextEffect pointer to assist GC
effect.nextEffect = null;
effect = nextNextEffect;
} while (effect !== null);
if (__DEV__) {
resetCurrentFiber();
Expand Down Expand Up @@ -831,6 +834,16 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
}
passiveEffectCallbackHandle = scheduleCallback(NormalPriority, callback);
passiveEffectCallback = callback;
} else {
// We are done with the effect chain at this point so let's clear the
// nextEffect pointers to assist with GC. If we have passive effects, we'll
// clear this in commitPassiveEffects.
nextEffect = firstEffect;
while (nextEffect !== null) {
const nextNextEffect = nextEffect.nextEffect;
nextEffect.nextEffect = null;
nextEffect = nextNextEffect;
}
}

isCommitting = false;
Expand Down