fix(compiler-core): prevent cached array children from retaining detached dom nodes #13691
Conversation
WalkthroughThis change eliminates internal slot cache key tracking in both the compiler and runtime, removes the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as E2E Test Runner
participant App as Vue App
participant Comp as Component with Cached Array/Text Nodes
participant GC as Garbage Collector
TestRunner->>App: Mounts component with large array/text nodes
App->>Comp: Renders cached array/text nodes
TestRunner->>App: Toggles component off (unmount)
App->>Comp: Unmounts, removes from DOM
loop Until WeakRef is cleared
TestRunner->>GC: Triggers garbage collection
GC->>Comp: Attempts to collect detached nodes
end
TestRunner->>TestRunner: Asserts WeakRef is cleared (no leak)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
edison1105
force-pushed
the
edison/fix/refix13211
branch
2 times, most recently
from
1f482ab to
eab33bf
Compare
edison1105
force-pushed
the
edison/fix/refix13211
branch
2 times, most recently
from
41c3435 to
ec921ec
Compare
edison1105
force-pushed
the
edison/fix/refix13211
branch
from
ec921ec to
c577ad7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/vue/__tests__/e2e/memory-leak.spec.ts (1)
88-88: Fix grammatical error in test description.The test description should use "retain" instead of "retaining".
- 'cached array vnodes should not retaining detached DOM nodes', + 'cached array vnodes should not retain detached DOM nodes',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/compiler-core/__tests__/transforms/__snapshots__/cacheStatic.spec.ts.snapis excluded by!**/*.snappackages/compiler-dom/__tests__/transforms/__snapshots__/stringifyStatic.spec.ts.snapis excluded by!**/*.snappackages/compiler-sfc/__tests__/__snapshots__/templateTransformAssetUrl.spec.ts.snapis excluded by!**/*.snappackages/compiler-sfc/__tests__/__snapshots__/templateTransformSrcset.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
packages/compiler-core/__tests__/transforms/cacheStatic.spec.ts(0 hunks)packages/compiler-core/src/ast.ts(2 hunks)packages/compiler-core/src/codegen.ts(2 hunks)packages/compiler-core/src/transforms/cacheStatic.ts(1 hunks)packages/runtime-core/__tests__/componentSlots.spec.ts(1 hunks)packages/runtime-core/src/componentSlots.ts(1 hunks)packages/runtime-core/src/renderer.ts(1 hunks)packages/runtime-core/src/vnode.ts(2 hunks)packages/vue/__tests__/e2e/memory-leak.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/compiler-core/tests/transforms/cacheStatic.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/runtime-core/src/vnode.ts (1)
packages/shared/src/general.ts (1)
isArray(39-39)
packages/compiler-core/src/transforms/cacheStatic.ts (1)
packages/compiler-core/src/ast.ts (2)
JSChildNode(349-359)CacheExpression(416-424)
packages/vue/__tests__/e2e/memory-leak.spec.ts (1)
packages/vue/__tests__/e2e/e2eUtils.ts (1)
E2E_TIMEOUT(8-8)
🔇 Additional comments (12)
packages/runtime-core/__tests__/componentSlots.spec.ts (1)
62-62: LGTM - Test correctly updated to reflect removed functionality.The test properly removes the
__property from the slots object, aligning with the removal of internal slot cache key tracking throughout the codebase.packages/runtime-core/src/vnode.ts (2)
683-687: Critical fix for memory leak prevention.This change correctly addresses the core issue by deep cloning cached array children to prevent them from retaining detached DOM nodes. The removal of the
__DEV__conditional ensures this fix applies in production environments, which is essential for preventing memory leaks.The logic properly checks for both
PatchFlags.CACHEDandisArray(children)before applying the expensive deep clone operation.
743-743: Minor documentation improvement.Good cleanup removing the "Dev only" prefix since this function is now used in production for cached vnode cloning.
packages/runtime-core/src/componentSlots.ts (1)
85-85: Properly removes internal cache key tracking.The removal of
"__"from the internal key check is consistent with eliminating the slot cache key tracking mechanism. This simplifies the slot handling by removing the unused cache key property.packages/compiler-core/src/ast.ts (2)
423-423: Good addition of array tracking for cached expressions.The
cachedAsArrayproperty provides the necessary metadata to determine when cached expressions need array cloning to prevent memory leaks.
788-788: Appropriate default initialization.Initializing
cachedAsArraytofalseis correct since most cached expressions are not arrays. This will be set totrueonly when needed by the transform phase.packages/compiler-core/src/codegen.ts (3)
1015-1015: Correctly extracts the new array flag.Proper destructuring of the
cachedAsArrayproperty for use in code generation logic.
1019-1019: Adds necessary parentheses for array cloning.The opening parenthesis ensures proper grouping when the
.slice()call is added later, generating code like(_cache[0] = value).slice().
1031-1031: Implements array cloning for memory leak prevention.The
.slice()call creates a shallow clone of cached arrays, preventing them from retaining references to detached DOM nodes. This is the key fix that complements the runtime changes invnode.ts.packages/runtime-core/src/renderer.ts (1)
2280-2280: LGTM! Clean removal of unused destructuring.This change appropriately removes the destructuring of unused variables as part of eliminating the slot cache key tracking mechanism.
packages/compiler-core/src/transforms/cacheStatic.ts (1)
217-230: LGTM! Proper implementation of array cache tracking.The addition of the
cachedAsArrayparameter and property assignment correctly enables the code generator to handle cached arrays differently, preventing memory leaks from retained DOM nodes. The default value oftrueis appropriate since most cached expressions in this context are arrays.packages/vue/__tests__/e2e/memory-leak.spec.ts (1)
86-145: Well-designed test for verifying the memory leak fix.The test effectively validates that cached array vnodes are properly released after component unmount, preventing memory leaks from retained DOM nodes. The use of WeakRef and forced garbage collection provides a reliable way to verify the fix.
edison1105
force-pushed
the
edison/fix/refix13211
branch
from
b2e450e to
6cd3e1f
Compare
edison1105
force-pushed
the
edison/fix/refix13211
branch
from
01fdb51 to
639fc0b
Compare
edison1105
added
scope: compiler
❗ p4-important
❌ Deploy Preview for vue-next-template-explorer failed. Why did it fail? →
|
❌ Deploy Preview for vue-sfc-playground failed. Why did it fail? →
|
fix element-plus/element-plus#21408
Re-fix #13211, as the associated PR #13215 did not resolve the issue correctly.
This PR reverts #13215 and re-applies a fix for #13211, addressing and covering memory leak issues present in the following two scenarios (the scenario is the same in both cases:
cache[1]is an array).Playground
Playground with slots
The cached vnodes get replaced by cloned ones during
mountChildren, which bind DOM elements. These DOM references persist after unmount, preventing garbage collection. Array spread avoids mutating the cached array, preventing memory leaks.Summary by CodeRabbit
Bug Fixes
Tests