Skip to content

fix: under concurrency pressure, oldVNode may be empty, and preact may crash. Fix for that scenario #4854

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+1 −1
Open

fix: under concurrency pressure, oldVNode may be empty, and preact may crash. Fix for that scenario #4854

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/diff/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ export function diff(
}

newVNode._dom = oldVNode._dom;
newVNode._children = oldVNode._children;
newVNode._children = oldVNode._children || [];
Copy link
Member

@JoviDeCroock JoviDeCroock Jul 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We should have a test for this, I actually think an issue like this could also be related to our changing of the raf timing in signals/hooks. The main reason I want a test is to see whether this is the actual root-cause rather than a band-aid, there is little concurrency going on in Preact as the diff happens synchronously.

It's entirely possible that we somehow succeed at initiating a state-update while a component is suspended which, when the component is suspended upon mount, could lead to this. I've admittedly seen this issue occur once as well d55f62d and it started somewhere in the 10.26 line so my assumptions are the change in RAF timing or this one.

Another thought of something that got introduced in the 10.26.x release is that top-level Fragments are now cloned in the diff, which if the Suspense boundary captures this node could actually lead to this issue. as we'd rely on this being a diff and erroneously assume that this boundary has _children defined. This is in all scenarios prevent by this line but maybe there's a way to bypass it. It recursively removes the _vnodeId (_original) property of all of the suspended vnodes so it should never bail for the _original case which kind of disproves the #4724 assertion but maybe as you say there's a way to hit this case in a concurrent scenario.

I don't mean to be difficult about this, I would just love to get to the root of this problem and if you have a good way of reproducing then that is a great opportunity for that. I want to avoid us defensively setting _children and then realising that the DOM is clobbered because of it, or it duplicates vnodes.

newVNode._children.some(vnode => {
if (vnode) vnode._parent = newVNode;
});
Expand Down