Skip to content

Drop animation targets that fall outside the binding's scope in resolveTargets.#3583

Open
ooLittlePanda wants to merge 2 commits into
mainfrom
bugfix/limit_node_scope
Open

Drop animation targets that fall outside the binding's scope in resolveTargets.#3583
ooLittlePanda wants to merge 2 commits into
mainfrom
bugfix/limit_node_scope

Conversation

@ooLittlePanda

@ooLittlePanda ooLittlePanda commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

PAGTimeline::resolveTargets 和 DataBindRuntime::bind之前通过 findNode 扁平查找 target,未按 binding 作用域过滤,document 级动画/数据绑定可能越界命中嵌套 Composition 内部节点。
修复:遍历动画对象时用 binding.contains() 过滤掉不在当前 scope 内的目标(scope filter),并新增树结构变化或 binding 指针变化时能够重新resolveTarget避免信息过时

@codecov-commenter

codecov-commenter commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.39474% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.78%. Comparing base (9f25435) to head (81f7388).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/pagx/DataBindRuntime.cpp 40.00% 1 Missing and 2 partials ⚠️
src/pagx/PAGScene.cpp 50.00% 0 Missing and 2 partials ⚠️
src/pagx/PAGAnimation.cpp 87.50% 0 Missing and 1 partial ⚠️
src/pagx/runtime/PAGComposition.cpp 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3583      +/-   ##
==========================================
+ Coverage   82.57%   82.78%   +0.21%     
==========================================
  Files         707      707              
  Lines       91382    92924    +1542     
  Branches    25871    26197     +326     
==========================================
+ Hits        75459    76931    +1472     
+ Misses      10435    10434       -1     
- Partials     5488     5559      +71     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ooLittlePanda
ooLittlePanda force-pushed the bugfix/limit_node_scope branch 3 times, most recently from c6176bf to 2d70790 Compare July 14, 2026 11:18
Comment thread src/renderer/LayerBuilder.h Outdated
Comment thread src/pagx/PAGTimeline.cpp
Comment thread src/pagx/PAGTimeline.cpp Outdated
Comment thread src/pagx/DataBindRuntime.cpp
Comment thread src/pagx/PAGTimeline.cpp Outdated
@ooLittlePanda
ooLittlePanda requested a review from shlzxjp July 20, 2026 08:27
@ooLittlePanda
ooLittlePanda force-pushed the bugfix/limit_node_scope branch 2 times, most recently from de6fa4b to 31a8e2e Compare July 20, 2026 11:52
…scope.

PAGTimeline::resolveTargets 和 DataBindRuntime::bind之前通过 findNode 扁平查找 target,未按 binding 作用域过滤,document 级动画/数据绑定可能越界命中嵌套 Composition 内部节点。
修复:resolveTargets/bind 用 contains()提前过滤越界 target。
@ooLittlePanda
ooLittlePanda force-pushed the bugfix/limit_node_scope branch from 31a8e2e to 0040491 Compare July 21, 2026 03:48
Comment thread src/pagx/PAGAnimation.cpp
}
if (!resolved) {
resolveTargets();
// Re-resolve only when needed: targetsDirty covers an incremental refresh that changed binding

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

【PAGStateMachine 存在同类 binding 失效隐患,建议与本次一并修复并把机制上提基类】

本 PR 为顶层 PAGAnimation 建立了正确的 binding 失效机制:顶层 timeline 用 null binding + apply 时 lazy 取 scene->mutableBinding(),再靠 effectiveBinding != lastResolvedBinding 指针比较,在 runtime-tree 重建(root binding 被替换、旧 binding 释放)后自愈重解析,避免悬空。

但 PAGStateMachine 没有对齐这套机制,存在同一场景的 use-after-free:

  1. 顶层 SM 在 PAGScene.cpp(getStateMachineTimeline 与 buildRuntimeTree 两处创建点)是用 _rootComposition->binding.get() 作为固定 binding 传入构造函数保存的,而非 PAGAnimation 的 null-lazy 模式。
  2. SM::createTimelineForState 用这个固定 binding 创建内部 PAGAnimation(new PAGAnimation(anim, binding, ...))。
  3. 用户持有 getStateMachineTimeline() 返回的 handle 跨 runtime-tree 重建时(外部文档编辑等):instantiatedTimelines 被 clear、新 root composition/binding 建立、旧 binding 释放,但用户手里的旧 SM 仍存活,其 binding 成员指向已释放的旧 binding。
  4. SM::apply 的 owner.expired() 只在 scene 对象销毁时保护;runtime-tree 重建时 scene 未销毁、owner 未 expire,挡不住。→ SM advance/apply → createTimelineForState 用悬空 binding → UAF。

这不是本 PR 引入的(SM 的 binding 生命周期是 #3574 遗留),但本 PR 正是在完善 timeline 的 binding 失效机制,建议一并对齐,否则两个 PAGTimeline 子类对「顶层 timeline binding 从哪来」的处理不一致,SM 侧留坑。

建议做法:把「effective binding 解析」上提到 PAGTimeline 基类,两个子类共用:

  • 基类收拢 owner / binding / contextDoc 三个共有成员,提供 protected 的 effectiveBinding():binding 非空时返回固定 binding(composition 内嵌 timeline),为空时 lazy 取 scene->mutableBinding()(顶层 timeline,跟随重建)。
  • PAGStateMachine 顶层实例改为传 null binding(PAGScene.cpp 两处创建点),apply / createTimelineForState 改用 effectiveBinding();内部 PAGAnimation 也传 null,让其走和顶层一致的 lazy + 指针比较自愈路径。
  • PAGAnimation 的 owner/binding/contextDoc 上提基类复用;resolvedTargets/targetsDirty/lastResolvedBinding 仍留在 PAGAnimation(Animation 特有缓存)。

这样顶层 timeline 的 binding 失效行为在基类统一实现一次,SM 自动获得和 Animation 相同的重建自愈能力,也避免后续新增 PAGTimeline 子类时再次踩坑。

Comment thread src/pagx/PAGAnimation.cpp
}
if (!resolved) {
resolveTargets();
// Re-resolve only when needed: targetsDirty covers an incremental refresh that changed binding

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

【补充上一条:binding、内部 PAGAnimation 的 binding、contextDoc 三者都应统一到基类,不留子类差异】

承接上一条关于 SM binding 失效隐患的建议,明确统一范围——避免只统一 effectiveBinding 一处、其余仍各行其是:

1. effectiveBinding 解析(基类统一):binding 非空返回固定 binding,为空 lazy 取 scene->mutableBinding()。顶层 timeline(PAGAnimation 与 PAGStateMachine)一律传 null。

2. SM 内部 PAGAnimation 的 binding 也统一为 null-lazy:createTimelineForState 创建内部 PAGAnimation 时应传 null,而不是传 effectiveBinding() 的当次值。若传当次值,等于又固定了一个可能过期的指针,runtime-tree 重建后仍悬空。传 null 让内部 PAGAnimation 走和顶层完全一致的 lazy 取当前 binding + 指针比较自愈路径,SM 内部 timeline 天然跟随最新 binding,无需额外失效信号。

3. contextDoc 也统一到基类:owner / binding / contextDoc 三个共有成员一起上提 PAGTimeline 基类,contextDoc 的解析(顶层用 scene 主文档、外部 composition timeline 用 layer 的 externalDoc)也在基类处理一次,不保留子类各自设置。

统一后:顶层 timeline 的「binding 从哪来、contextDoc 从哪来、重建后如何自愈」全部在基类实现一份,PAGAnimation 与 PAGStateMachine(及未来子类)行为一致,SM 内部 PAGAnimation 也走同一路径,彻底消除「某一处忘了对齐」的留坑空间。PAGAnimation 特有的 resolvedTargets/targetsDirty/lastResolvedBinding 缓存仍留在 PAGAnimation。

Comment thread src/pagx/PAGAnimation.cpp
if (targetNode == nullptr) {
continue;
}
// Drop targets outside this binding's scope. findNode does a flat document-wide lookup, so it

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

【架构建议:scope 校验应前移到数据层,而非在运行时 apply 兜底】

梳理完整链路后,认为本 PR 的过滤放错了层,根因是两套 scope 模型不一致:

现状链路

  • 数据层 PAGXDocument::findNode 是 document 域扁平查找(单个 nodeMap,全局 id 空间,无 scope)——这是「资源引用」语义,合理。
  • 运行时 RuntimeBinding 是 composition 域隔离(每个 Composition 实例一个 binding,嵌套 composition 有独立 binding,已确认 layers 子树边界 == binding 隔离边界)——这是「实例 scope」语义。
  • resolveTargets 用无 scope 的 findNode 解析 target,可能跨 composition 命中(A 的 animation 查到 B 内部节点);再传给有 scope 的 binding apply,靠 find miss 静默兜底过滤。

问题:scope 不是任何一层显式校验保证的,而是 apply 的 find miss「碰巧」兜出来的。全链路无校验——越界 target 从解析一路传到 apply 才被默默丢弃,无人报告哪个 timeline 的哪个 target 越界。本 PR 在 resolveTargets 加 contains 过滤,方向是对的(引入 scope 校验),但放在运行时每帧路径校验结果依赖 binding,于是缓存含 binding、逼出 lastResolvedBinding 指针比较(僵硬、且有 binding 地址复用隐患),同时 resolveTargets 过滤与 apply miss 兜底双重存在,职责更乱。

更合理的方向:给 Composition 加 composition 域的 scoped 查找

让 timeline/DataBind 的 target 解析用「Composition 域查找」而非「document 域 findNode」,两套 scope 模型即对齐:

  • Composition 新增 findNodeInScope(id):只在本 composition 的 layers 子树内查(遇到引用子 composition 的 Layer 即停,不跨边界),与 RuntimeBinding 隔离边界一致。
  • resolveTargets / DataBindRuntime::bind 改用 composition 域查找解析 target:跨界 id 从一开始就查不到,天然过滤,不需要 binding 参与。
  • target 查不到 = malformed,在构建期一次性 ReportError/LOGE,有明确报错点。
  • 运行时回归无 scope 逻辑:resolveTargets 只缓存 node 指针(不含 binding);apply 直接写值,find miss 降级为 DEBUG_ASSERT(scope 已在数据层保证,miss 是真 bug 而非正常过滤)。

收益:三个问题一起消失——(1) 指针比较删除(缓存不含 binding,runtime-tree 重建后 apply 用新 binding 查稳定的 node 指针,自愈免费);(2) apply 不再承担 scope 兜底,miss 变回 bug 信号;(3) scope 成为数据层显式契约,有报错点,不再是运行时隐式副作用。

这是比「修僵硬的指针比较」更根本的调整,建议评估是否将 scope 校验前移,而不是在运行时 apply 层堆兜底。

Comment thread src/pagx/PAGAnimation.cpp
if (targetNode == nullptr) {
continue;
}
// Drop targets outside this binding's scope. findNode does a flat document-wide lookup, so it

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

命名补充:上一条提到的 Composition 域查找方法就叫 findNode 即可,不必加 InScope 后缀。它是 Composition 域的 findNode,与 PAGXDocument::findNode(document 域)同名不同宿主,scope 差异由宿主体现:composition->findNode(id) vs document->findNode(id) 已一目了然,后缀反而冗余。

@ooLittlePanda
ooLittlePanda force-pushed the bugfix/limit_node_scope branch from 0040491 to 81f7388 Compare July 21, 2026 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants