Fix AnimationComponent leaking asset event handlers - #9141
Merged
Conversation
AnimationComponent registered its asset handlers as closures, so they could
never be unsubscribed:
- `assets.on('add:<id>', onAssetAdd)` on the asset registry
- `asset.once('load', onAssetReady)` on the asset
and `onBeforeRemove` only unsubscribed from assets it could resolve through
the registry at that moment (`if (!asset) continue`). A component removed
while its asset was missing from the registry therefore stayed subscribed,
and a later `assets.add()` re-subscribed the removed component to the
asset's `change`/`remove` events, keeping the component, its entity, model
and animation resources alive - and dispatching events to it when the app
was destroyed.
The handlers are now private methods, the subscriptions are tracked, and
they are all removed both when the component is removed and when the asset
array is replaced.
Fixes #3972
Build size reportThis PR changes the size of the minified bundles.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a memory-leak / stale-callback problem in AnimationComponent by making asset-registry and asset load subscriptions fully unsubscribeable, ensuring removed components cannot remain referenced via event handlers (including during app.destroy() asset unload flows).
Changes:
- Replaces closure-based asset callbacks with private instance methods so
off()can reliably unsubscribe them. - Tracks both bound assets and pending
add:[id]registry subscriptions, and centralizes cleanup in_unbindAssets()(called fromonBeforeRemove()and theassetssetter). - Adds regression tests covering multiple unbinding paths (in-registry, in-flight load, late registry add, registry removal, and asset-array replacement).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/framework/components/animation/component.js | Introduces tracked asset binding/unbinding to prevent leaked event handlers and callbacks after component removal or asset-list changes. |
| test/framework/components/animation/component.test.mjs | Adds regression tests verifying asset event handlers are correctly removed across several lifecycle and registry edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Fixes #3972.
AnimationComponentregistered its asset event handlers as closures, so they could never be unsubscribed:and
onBeforeRemove()only unsubscribed from assets it could resolve through the registry at that moment (if (!asset) continue). So a component removed while its animation asset was missing from the registry stayed subscribed, and a laterassets.add()re-subscribed the removed component to the asset'schange/removeevents. That keeps the component, its entity, model and animation resources alive, and dispatches events to it - including duringapp.destroy(), whereAsset.unload()fireschangeon every asset. In 1.x this threwCannot read properties of null (reading 'animations'), since a removed component's data-backedanimationsaccessor reads from a store record that no longer exists.Changes:
onAssetAdd/onAssetReadyclosures become private methods (_bindAsset,_onAssetAdded,_onAssetReady), so they can be unsubscribed.add:[id]registry events) and all removed by a new private_unbindAssets(), called fromonBeforeRemove()and from theassetssetter. This covers assets that are no longer in the registry, assets added after the component was removed, and loads still in flight.assetssetter now resolves ids for entries holdingAssetinstances, which previously skipped their cleanup (registry.get(assetInstance)returnsundefined).add:[id]subscriptions are no longer registered for the same asset id.No public API changes.