fix(core): fix crash when removeChild during deactivation callbacks#2948
fix(core): fix crash when removeChild during deactivation callbacks#2948GuoLei1990 wants to merge 1 commit intogalacean:dev/2.0from
Conversation
…alacean#2947) Children-first deactivation order + scene guard for deferred callback safety. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThe PR fixes a bug where removing child entities during a parent's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2948 +/- ##
===========================================
+ Coverage 77.07% 77.44% +0.36%
===========================================
Files 899 899
Lines 98224 98296 +72
Branches 9678 9671 -7
===========================================
+ Hits 75705 76122 +417
+ Misses 22351 22008 -343
+ Partials 168 166 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/Entity.ts`:
- Around line 696-699: The current loop skips calling component._setActive when
a component was detached (if !isActive && !component._entity._scene), which
prevents phased flags (_phasedActive / _phasedActiveInScene) from being cleared;
instead remove the early continue and ensure component._setActive(isActive,
activeChangeFlag) is always invoked for that component so the phased inactive
transition runs even when _scene is null (the callback invocation itself can
still be gated inside _setActive/_processInActive if needed). Target the loop
handling activeChangedComponents and update the logic around
component._setActive to always call it for deactivate transitions.
- Around line 693-702: The loop in Entity._setActiveComponents can throw if a
child onDisable clears component._entity._scene, leaking
this._activeChangedComponents; capture the current scene (e.g., const scene =
this._scene) before the loop, then wrap the for-loop and component._setActive
call in a try/finally where in finally you call
scene._componentsManager.putActiveChangedTempList(activeChangedComponents) and
set this._activeChangedComponents = null so the temp list is always returned and
the field cleared even if reentrant mutations throw; keep the existing skip
check (if (!isActive && !component._entity._scene) continue) inside the try.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 64654315-da1f-4baf-895a-51e10f65ce4f
📒 Files selected for processing (2)
packages/core/src/Entity.tstests/src/core/Entity.test.ts
| private _setActiveComponents(isActive: boolean, activeChangeFlag: ActiveChangeFlag): void { | ||
| const activeChangedComponents = this._activeChangedComponents; | ||
| for (let i = 0, length = activeChangedComponents.length; i < length; ++i) { | ||
| activeChangedComponents[i]._setActive(isActive, activeChangeFlag); | ||
| const component = activeChangedComponents[i]; | ||
| // Skip components whose scene was already cleared by an earlier callback's removeChild | ||
| if (!isActive && !component._entity._scene) continue; | ||
| component._setActive(isActive, activeChangeFlag); | ||
| } | ||
| this._scene._componentsManager.putActiveChangedTempList(activeChangedComponents); | ||
| this._activeChangedComponents = null; |
There was a problem hiding this comment.
Always release _activeChangedComponents from the original scene.
A child onDisable can still clear this._scene on the entity currently running _setActiveComponents. Then Line 701 throws before Line 702 runs, so the temp list never goes back to the pool and the entity stays stuck with _activeChangedComponents !== null. Capture the scene up front and do the release/reset in finally, even if the reentrant mutation is supposed to throw.
Possible fix
private _setActiveComponents(isActive: boolean, activeChangeFlag: ActiveChangeFlag): void {
+ const scene = this._scene;
const activeChangedComponents = this._activeChangedComponents;
- for (let i = 0, length = activeChangedComponents.length; i < length; ++i) {
- const component = activeChangedComponents[i];
- // Skip components whose scene was already cleared by an earlier callback's removeChild
- if (!isActive && !component._entity._scene) continue;
- component._setActive(isActive, activeChangeFlag);
- }
- this._scene._componentsManager.putActiveChangedTempList(activeChangedComponents);
- this._activeChangedComponents = null;
+ try {
+ for (let i = 0, length = activeChangedComponents.length; i < length; ++i) {
+ const component = activeChangedComponents[i];
+ // Skip components whose scene was already cleared by an earlier callback's removeChild
+ if (!isActive && !component._entity._scene) continue;
+ component._setActive(isActive, activeChangeFlag);
+ }
+ } finally {
+ scene._componentsManager.putActiveChangedTempList(activeChangedComponents);
+ this._activeChangedComponents = null;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private _setActiveComponents(isActive: boolean, activeChangeFlag: ActiveChangeFlag): void { | |
| const activeChangedComponents = this._activeChangedComponents; | |
| for (let i = 0, length = activeChangedComponents.length; i < length; ++i) { | |
| activeChangedComponents[i]._setActive(isActive, activeChangeFlag); | |
| const component = activeChangedComponents[i]; | |
| // Skip components whose scene was already cleared by an earlier callback's removeChild | |
| if (!isActive && !component._entity._scene) continue; | |
| component._setActive(isActive, activeChangeFlag); | |
| } | |
| this._scene._componentsManager.putActiveChangedTempList(activeChangedComponents); | |
| this._activeChangedComponents = null; | |
| private _setActiveComponents(isActive: boolean, activeChangeFlag: ActiveChangeFlag): void { | |
| const scene = this._scene; | |
| const activeChangedComponents = this._activeChangedComponents; | |
| try { | |
| for (let i = 0, length = activeChangedComponents.length; i < length; ++i) { | |
| const component = activeChangedComponents[i]; | |
| // Skip components whose scene was already cleared by an earlier callback's removeChild | |
| if (!isActive && !component._entity._scene) continue; | |
| component._setActive(isActive, activeChangeFlag); | |
| } | |
| } finally { | |
| scene._componentsManager.putActiveChangedTempList(activeChangedComponents); | |
| this._activeChangedComponents = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/Entity.ts` around lines 693 - 702, The loop in
Entity._setActiveComponents can throw if a child onDisable clears
component._entity._scene, leaking this._activeChangedComponents; capture the
current scene (e.g., const scene = this._scene) before the loop, then wrap the
for-loop and component._setActive call in a try/finally where in finally you
call scene._componentsManager.putActiveChangedTempList(activeChangedComponents)
and set this._activeChangedComponents = null so the temp list is always returned
and the field cleared even if reentrant mutations throw; keep the existing skip
check (if (!isActive && !component._entity._scene) continue) inside the try.
| const component = activeChangedComponents[i]; | ||
| // Skip components whose scene was already cleared by an earlier callback's removeChild | ||
| if (!isActive && !component._entity._scene) continue; | ||
| component._setActive(isActive, activeChangeFlag); |
There was a problem hiding this comment.
Don't skip the deactivate state transition for detached components.
When a sibling callback removes C, C._setParent(null) clears _scene after C._isActiveInHierarchy / C._isActiveInScene were already flipped, so there is no second _processInActive(). This continue then skips the Component._setActive(false) path that clears _phasedActive / _phasedActiveInScene, which means reattaching C later can miss onEnable / onEnableInScene. If the callback itself must be skipped once _scene is gone, the phased state still needs to transition to inactive here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/Entity.ts` around lines 696 - 699, The current loop skips
calling component._setActive when a component was detached (if !isActive &&
!component._entity._scene), which prevents phased flags (_phasedActive /
_phasedActiveInScene) from being cleared; instead remove the early continue and
ensure component._setActive(isActive, activeChangeFlag) is always invoked for
that component so the phased inactive transition runs even when _scene is null
(the callback invocation itself can still be gated inside
_setActive/_processInActive if needed). Target the loop handling
activeChangedComponents and update the logic around component._setActive to
always call it for deactivate transitions.
1 participant
Summary
Fixes #2947
_setInActiveInHierarchynow recurses children (reverse traversal) before processing the current node, aligning with Unity's deactivation order. This ensures child callbacks complete before parent callbacks fire_setActiveComponentsskips components whose entity's_scenewas cleared by an earlier callback'sremoveChild, preventing null access crashes in sibling removal scenariosRoot cause
The two-phase deferred callback mechanism (Phase 1: collect components, Phase 2: fire callbacks) caused a timing issue:
_isActiveInScene = falseupfrontremoveChild(B)→_processInActiveis skipped (flag already false) → but_traverseSetOwnerScene(B, null)still runs, clearingB._scene_onDisableInScene→this.scene._componentsManager→ crash (scene is null)Known limitation / future improvement
The
_scenenull check and_phasedActiveInSceneguard handle deferred callback safety at different layers:_scenecheck in_setActiveComponents: prevents crash fromremoveChildclearing scene_phasedActiveInSceneinComponent._setActive: prevents duplicate callback dispatchA unified mechanism (e.g.
_deactivateCallbackPendingflag set during Phase 1, cleared after Phase 2) could replace both checks at the source, providing consistent protection for all cases (removeChild,isActive = false, etc.) in a single layer. This is deferred for a future PR.Test plan
removeChild during parent's onDisable— original crash scenariosibling removes another sibling during onDisable— scene guard scenariochild tries to removeChild its deactivating parent— reentrant protectionsetting sibling isActive=false during onDisable— no duplicate dispatchdeactivation callbacks in children-first order— order verification (C, B, A)InActiveAndActiveandScriptlifecycle tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests