fix(qwik): guard null/undefined vNode in markVNodeDirty#8563
Open
juslintek wants to merge 1 commit intoQwikDev:build/v2from
Open
fix(qwik): guard null/undefined vNode in markVNodeDirty#8563juslintek wants to merge 1 commit intoQwikDev:build/v2from
juslintek wants to merge 1 commit intoQwikDev:build/v2from
Conversation
When a container is destroyed during async qwikloader dispatch, DomContainer.$destroy$() replaces $getObjectById$ with () => undefined and truncates $stateData$. Pending event handlers (scheduleTask, _hmr, _val, _chk, _res) then deserialize undefined vNodes and pass them to markVNodeDirty, causing: TypeError: Cannot read properties of undefined (reading 'dirty') This single guard in markVNodeDirty protects all 20 callers at once, rather than requiring individual guards at each call site.
🦋 Changeset detectedLatest commit: 05d7ef1 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Varixo
requested changes
Apr 16, 2026
| // $stateData$, so deserializeCaptures returns [undefined] for pending event handlers. | ||
| // This affects scheduleTask, scheduleEffects, _hmr, _val, _chk, and _res. | ||
| return; | ||
| } |
Member
There was a problem hiding this comment.
Then its a bug that this funtion called on container destroyed, it is workaround not real fix
Comment on lines
+6
to
+23
| describe('markVNodeDirty', () => { | ||
| it('does not throw when vNode is undefined (destroyed container)', () => { | ||
| // After DomContainer.$destroy$(), $getObjectById$ returns undefined for all IDs. | ||
| // Callers like scheduleTask, _hmr, _val, _chk pass the deserialized result | ||
| // directly to markVNodeDirty — which would be undefined. | ||
| const container = {} as Container; | ||
| expect(() => { | ||
| markVNodeDirty(container, undefined as any, ChoreBits.TASKS); | ||
| }).not.toThrow(); | ||
| }); | ||
|
|
||
| it('does not throw when vNode is null', () => { | ||
| const container = {} as Container; | ||
| expect(() => { | ||
| markVNodeDirty(container, null as any, ChoreBits.TASKS); | ||
| }).not.toThrow(); | ||
| }); | ||
| }); |
Member
There was a problem hiding this comment.
you are testing a fix, not failing test case. If I understand it correctly, to test it correctly you need to test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is it?
Description
Fixes a runtime crash where
markVNodeDirtyreceives an undefined/null vNode, causing:This crash makes the entire application non-functional — no page renders, no login, all routes are dead.
Root Cause
When
DomContainer.$destroy$()is called during async qwikloader dispatch (e.g., SPA navigation destroys the container while queued event handlers are pending), it:$rawStateData$and$stateData$to length 0$getObjectById$with() => undefinedPending event handlers then call
deserializeCaptures()which invokescontainer.$getObjectById$(id)→ returnsundefined. The deserialized vNode references areundefined, and callers pass them directly tomarkVNodeDirty.Affected callers (all have the same vulnerability)
scheduleTaskuse-task.tstask.$el$scheduleEffectsreactive-primitives/utils.tsconsumer.$el$/consumer_hmruse-hmr.ts_captures![0]_valbind-handlers.ts_captures![0]_chkbind-handlers.ts_captures![0]_resbind-handlers.ts_captures![0]Why fix in
markVNodeDirtyinstead of each callerDomContainer$stateData$during lazy deserializationAlternative approaches considered
!task?.$el$)WrappedSignalImplpattern_hmr,_val,_chk,_res$destroyed$flag onDomContainerContainerinterface change; doesn't cover partial inflation; more invasiveelement.closest()re-check)markVNodeDirty(this PR)Changes
packages/qwik/src/core/shared/vnode/vnode-dirty.tsmarkVNodeDirtywith explanatory commentpackages/qwik/src/core/shared/vnode/vnode-dirty.unit.ts(new)undefined(destroyed container scenario)null.changeset/guard-null-vnode-mark-dirty.md@qwik.dev/coreDocs
No user-facing documentation changes needed —
markVNodeDirtyis an internal API.Checklist
pnpm change