fix(ui): correct root-canvas demotion state and listener bookkeeping - #3070
cptbtptpbcptdtptp wants to merge 8 commits into
Conversation
Three coupled fixes ported from galacean#3068 (fix/shaderlab): - The demote loop dirtied the canvas itself instead of each element (setRootCanvasDirty(this) -> (element)), so demoted elements kept stale _rootCanvas/_indexInRootCanvas and never re-homed: the new root's walk re-assignment is gated on the element's dirty flag. - _registerListener truncated a shrinking listening chain without unregistering the dropped tail, leaking listeners on ex-ancestors. - Releasing that leak exposed that nested-canvas demotion relied on it: UpdateFlagManager.dispatch iterates a backward snapshot, so a listener appended mid-dispatch is never invoked by that dispatch, and the enabling canvas claims root status only after its dispatch returns — the cascade's search could not find it and promoted nested canvases to root. The demote cascade now receives the enabling canvas as an explicit successor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. Walkthrough
ChangesListener cleanup
Root canvas transitions
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change corrects root-canvas demotion state and listener cleanup within packages/ui. No concrete merge-blocking risk was found, and regression tests cover the root reassignment paths. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes stay within UI canvas lifecycle handling, with no identified new security boundary or attack path. Normal reassignment is tested, but listener removal during a notification leaves a limited ownership-consistency risk to verify. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. A rabbit checks the canvas tree Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3070 +/- ##
===========================================
+ Coverage 86.49% 86.67% +0.18%
===========================================
Files 809 809
Lines 94310 94394 +84
Branches 11737 11731 -6
===========================================
+ Hits 81570 81819 +249
+ Misses 12651 12488 -163
+ Partials 89 87 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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.
🧹 Nitpick comments (1)
tests/src/ui/UICanvas.test.ts (1)
354-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a second element to prevent hiding concurrent modification bugs.
Testing the demote cascade with only a single nested element (
image) masks concurrent modification bugs (such as removing elements from an array while looping over it, which succeeds for length 1 but skips elements for length 2+). Adding a second element ensures the logic robustly processes all children and would successfully catch theforEachiteration bug inUICanvas.ts.♻️ Proposed test update
it("re-homes elements to the new root canvas when theirs loses root status", () => { const outerEntity = root.createChild("outer"); const innerEntity = outerEntity.createChild("inner"); const innerCanvas = innerEntity.addComponent(UICanvas); const imageEntity = innerEntity.createChild("image"); const image = imageEntity.addComponent(Image); + const image2Entity = innerEntity.createChild("image2"); + const image2 = image2Entity.addComponent(Image); // Collected by the inner canvas while it is the root. // `@ts-ignore` innerCanvas._getRenderers(); // `@ts-ignore` expect(image._getRootCanvas()).to.eq(innerCanvas); + // `@ts-ignore` + expect(image2._getRootCanvas()).to.eq(innerCanvas); // `@ts-ignore` expect(innerCanvas._disorderedElements.length).to.be.greaterThan(0); // Enabling a canvas on an ancestor demotes the inner one. const outerCanvas = outerEntity.addComponent(UICanvas); // `@ts-ignore` expect(innerCanvas._isRootCanvas).to.be.false; // `@ts-ignore` expect(outerCanvas._isRootCanvas).to.be.true; // The element's canvas state must be reset (the walk's re-assignment is gated on the // dirty flag), otherwise it keeps rendering into the demoted canvas's dead queue. // `@ts-ignore` expect(image._isRootCanvasDirty).to.be.true; // `@ts-ignore` expect(image._rootCanvas).to.be.null; + // `@ts-ignore` + expect(image2._isRootCanvasDirty).to.be.true; + // `@ts-ignore` + expect(image2._rootCanvas).to.be.null; // The new root adopts it on its next walk. // `@ts-ignore` outerCanvas._getRenderers(); // `@ts-ignore` expect(image._getRootCanvas()).to.eq(outerCanvas); + // `@ts-ignore` + expect(image2._getRootCanvas()).to.eq(outerCanvas); outerEntity.destroy(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/ui/UICanvas.test.ts` around lines 354 - 389, Add a second child UI element with an Image component in the test “re-homes elements to the new root canvas when theirs loses root status,” then assert both images are marked dirty, have null root canvases after demotion, and are reassigned to outerCanvas after its renderer walk. Keep the existing assertions and cleanup behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/src/ui/UICanvas.test.ts`:
- Around line 354-389: Add a second child UI element with an Image component in
the test “re-homes elements to the new root canvas when theirs loses root
status,” then assert both images are marked dirty, have null root canvases after
demotion, and are reassigned to outerCanvas after its renderer walk. Keep the
existing assertions and cleanup behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fdadae4d-fb3c-4d95-a316-e04f616e96b7
📒 Files selected for processing (3)
packages/ui/src/Utils.tspackages/ui/src/component/UICanvas.tstests/src/ui/UICanvas.test.ts
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
把 #3068 的三处 packages/ui 修复原样 backport 到 dev/2.0。我在 #3068 已逐轮审过这三处（round-3 setRootCanvasDirty(this)→(element) 的 #2375-class bug、round-6 listener tail-release 泄漏、round-7 hosting 状态机 successor 线程化），本轮核心是校验：前像 bug 真在 dev/2.0 上、修复与 #3068 逐字等价、类型适配到 mainline ui 接口名正确、两个新测试可反向证伪。全部通过，无 P0/P1/P2/P3，可合。
核验结论（逐条）
1. Utils.ts — listener 链缩短的 tail 泄漏
dev/2.0前像确认：_registerListener走完 while 后直接listeningEntities.length = count，链变短时[count, oldLength)的尾部实体仍挂着_registerModifyListener→ 永久泄漏。- 修复补的
for (i=count; i<n) listeningEntities[i]._unRegisterModifyListener(listener)与 #3068 tip3c6c88c0逐字节相同（含注释）。✓
2. UICanvas.ts:414 — CanvasEnableInScene 分支缺 successor
- 前像
_setIsRootCanvas(false)无 successor → 嵌套 canvas demote cascade 里searchRootCanvasInParents(element)看不到「刚 enable、尚未 claim root」的祖先 canvas → 嵌套 canvas 被误提升为 root。 - 修复
_setIsRootCanvas(false, <UICanvas>param)+_setIsRootCanvas新增successor形参、cascade 里searchRootCanvasInParents(element) ?? successor，逻辑与 #3068 等价（#3068 用IUIElement/IUIGroupAble是它 round-5 把接口上移 core 后的名字，dev/2.0无此重构，backport 正确沿用 mainline 的IElement/IGroupAble，行为一致）。✓
3. UICanvas.ts:656 — 非 canvas 元素 setRootCanvasDirty(this)→(element)
- 前像 dirty 的是「正在 demote 的 canvas 自己」，而紧接着
disorderedElements.length = 0把这张 canvas 的列表清空 → 对元素 re-home 是 no-op；元素保留 stale_rootCanvas/_indexInRootCanvas+ 干净的 dirty flag，新 root 的 walk（以元素 dirty flag 为门）跳过它 → 渲染进已死队列、永不 re-home。 - 修复改为 dirty
element本身（setRootCanvasDirty内部置_isRootCanvasDirty=true+_registerRootCanvas(element,null)把_rootCanvas清 null），正解。✓
测试（两个新增均可反向证伪）
re-homes elements...：断言image._isRootCanvasDirty===true+image._rootCanvas===null。前像走setRootCanvasDirty(this)→ image 状态不动 →_isRootCanvasDirty仍 false → 前像 fail。✓re-roots nested canvases...：断言inner._isRootCanvas===false+inner._getRootCanvas()===outerCanvas。前像无 successor →searchRootCanvasInParents(inner)返 null →_setIsRootCanvas(!null)把 inner 误提升为 root → 断言 fail。✓Image经component/index.ts:6→index.ts export * from "./component"导出，IElement在dev/2.0声明了_rootCanvas/_indexInRootCanvas/_isRootCanvasDirty/_getRootCanvas()，测试引用符号全部存在。✓
补充说明
- #3068 里的「ui/Utils + spine 双源同步」债在此不适用：
dev/2.0无 spine 包（spine-in-UICanvas 是 #3068 stacked 在fix/shaderlab上的工作），backport 只需改ui/Utils.ts一处，已完整。 - 新增注释与 #3068 已接受版本逐字相同（多句解释性
//块，非 JSDoc），风格随 ported 代码一致，不另提。
LGTM。
Reorder _onEnableInScene to claim root status before broadcasting CanvasEnableInScene (Unity's AddToManager order), so demote cascades always find the new root via the ancestor search. Drops the newRootCanvas successor param entirely and merges the enable branch into the Parent branch.
Covers: enabler nested under an existing higher root, disable promoting the inner canvas back, and a disable/enable round-trip
关于分支上的后续改动:successor 参数 → 先认领后广播@cptbtptpbcptdtptp 分支上那几个 原修法的问题:正确,但 hack——补偿的是自己造的时序窗口原修法保持 具体表现为它补偿的窗口是实现顺序自己造出来的——整个 ui 包里, 这带来两个实际代价:
新修法:对齐"先应用、再派发"_onEnableInScene(): void {
const rootCanvas = Utils.searchRootCanvasInParents(this);
this._setIsRootCanvas(!rootCanvas); // 先认领(应用)
Utils.setRootCanvas(this, rootCanvas);
entity._dispatchModify(CanvasEnableInScene, this); // 后广播(派发)
}级联里的搜索在广播期间永远能命中已认领的启用 canvas,于是 successor 参数、 性能:无劣化,深树更优操作集合完全相同,差异只在级联内搜索的行走长度(以本 PR 测试层级 root/outer/mid/inner 计):
旧版每条级联搜索都走满到场景根 O(D);新版在启用 canvas 处早退 O(k)(k = 元素到启用者的层数)。30 层深、启用者在上方 2 层的树:30 跳 vs 2 跳。canvas 启停本身是冷路径,不涉每帧开销。代码量净 -3 行(13+/16−),删掉一个参数、一个分支、一个兜底。 bug 面:消除 2 项,新增 0 项
新增风险审计:全库消费 验证数据
对原始修复的三处诊断(脏错对象、尾部泄漏、时序依赖)没有任何改动,bug 1/2 的修法保持原样;这次只替换了 bug 3 的补偿方式。 |
2 participants
Summary
Ports three coupled
packages/uifixes from #3068 (fix/shaderlab) todev/2.0— all three bug sites are byte-identical here.UICanvas._setIsRootCanvas(false)calledUtils.setRootCanvasDirty(this)instead of(element), so demoted elements kept a stale_rootCanvas/_indexInRootCanvasand a clean dirty flag — but the new root's walk re-assignment is gated on the element's dirty flag, so they kept rendering into the demoted canvas's dead queue and never re-homed.Utils._registerListenertruncated a shrinking listening chain (listeningEntities.length = count) without unregistering the dropped tail, permanently leaking listeners on ex-ancestor entities.UpdateFlagManager.dispatchiterates a backward snapshot (a listener appended mid-dispatch is never invoked by that dispatch), and the enabling canvas claims root status only after itsCanvasEnableInScenedispatch returns — so the demote cascade'ssearchRootCanvasInParentscould not find it and promoted nested canvases to root. Convergence previously depended on a leaked listener happening to sit at a low index of the backward iteration. The cascade now receives the enabling canvas as an explicitsuccessor(_setIsRootCanvas(false, successor)).(2) and (3) must land together: fixing the leak alone breaks nested-canvas re-rooting.
Tests
Two regression tests added to
tests/src/ui/UICanvas.test.ts:re-homes elements to the new root canvas when theirs loses root status— pins (1)re-roots nested canvases to the enabling ancestor canvas— pins (3), exercised through (2)Negative validation: on unfixed
dev/2.0(rebuilt dist without the source fixes) both new tests fail and all 6 existing tests pass; with the fixes the full ui suite is green (66/66, node22b:module+b:types, vitest fromtests/).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests