From b26a340a7e653cd2e0e70408a341e1ebbe5bc67d Mon Sep 17 00:00:00 2001 From: DWAA1660 Date: Tue, 13 Oct 2026 16:26:42 -0600 Subject: [PATCH] Project: comp_contains walks precomps with a visited set, not recursion (#1237) Project::comp_contains recursed through precomp layers with no visited set or depth limit. A damaged project with a precomp cycle that doesn't pass the queried comp (A and B nesting each other, C unrelated) recursed until the stack ran out, which aborts the process ("stack overflow, aborting"): it can't be caught as a command error, so unsaved work is lost. layer.addItem checks it before adding a precomp, and the renderer calls it for every nested comp and its audio. A long chain used the stack the same way, and comps shared by many paths were walked once per path (exponential for nested diamonds). It is now an explicit stack with a visited set: same answers (a comp contains itself; a target reached through a cycle is found), each comp visited once, no recursion. The renderer already skips a precomp that contains the comp being rendered, so a cyclic project now renders instead of crashing; nothing is repaired on load. Tests: project comp_contains_ends_on_cycles_long_chains_and_shared_ precomps (cycle with an unrelated target, self-nesting, a target through a cycle, 40 levels of diamonds, a 20 000-comp chain); engine a_project_with_a_precomp_cycle_opens_adds_and_renders_without_running_ out_of_stack (file.open, layer.addItem both ways, render). On main the engine test aborts with a stack overflow. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/tests_limits.rs | 35 ++++++++++++++++++++ crates/project/src/lib.rs | 25 ++++++++++++-- crates/project/src/tests.rs | 55 +++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/tests_limits.rs b/crates/engine/src/tests_limits.rs index 68f2c04c..524719bb 100644 --- a/crates/engine/src/tests_limits.rs +++ b/crates/engine/src/tests_limits.rs @@ -102,3 +102,38 @@ fn commands_cap_sizes_at_30000_px() { let f = s.project.items.values().find_map(|i| if let ItemKind::Footage(f) = &i.kind { Some(f.clone()) } else { None }).unwrap(); assert_eq!((f.width, f.height, f.frame_rate), (limits::MAX_SIZE, 8, FrameRate::FPS_29_97)); } + +#[test] +fn a_project_with_a_precomp_cycle_opens_adds_and_renders_without_running_out_of_stack() { + // Comps A and B nest each other (a damaged file); C is unrelated. Checking whether A + // contains C walked the A ⇄ B cycle until the stack ran out (#1237). + let mut s = Session::default(); + let mut ids = vec![]; + for name in ["A", "B", "C"] { + s.execute("comp.new", json!({"name": name, "width": 32, "height": 32, "frameRate": 30, "duration": 1})).unwrap(); + ids.push(s.active_comp_id().unwrap()); + } + let (a, b, c) = (ids[0], ids[1], ids[2]); + { + let proj = std::sync::Arc::make_mut(&mut s.project); + for (outer, inner) in [(a, b), (b, a)] { + let comp = proj.comp(outer).unwrap().clone(); + let l = effectcraft_project::build::layer(proj, &comp, "pre", effectcraft_project::LayerSource::Comp { item: inner }, (32, 32), None); + let ItemKind::Comp(cc) = &mut proj.item_mut(outer).unwrap().kind else { panic!("comp") }; + std::sync::Arc::make_mut(cc).layers.push(l); + } + } + let path = tmp("cycle.ecproj"); + std::fs::write(&path, s.project.to_file_json().unwrap()).unwrap(); + let mut t = Session::default(); + t.execute("file.open", json!({"path": path})).unwrap(); + t.open_comp(c); + // Adding A into C asks whether A contains C: it doesn't, so the layer goes in. + let r = t.execute("layer.addItem", json!({"comp": c.0, "item": a.0})).unwrap(); + assert!(r["layer"].as_u64().is_some(), "{r}"); + // Adding C into A would nest C in itself through A: refused, as before. + assert!(t.execute("layer.addItem", json!({"comp": a.0, "item": c.0})).is_err()); + // C renders: inside A the renderer skips B's copy of A instead of looping. + let img = t.render(c, Tick::ZERO, RenderOpts { scale: 1.0, ..Default::default() }); + assert_eq!((img.width, img.height), (32, 32)); +} diff --git a/crates/project/src/lib.rs b/crates/project/src/lib.rs index 16d78216..27f8870d 100644 --- a/crates/project/src/lib.rs +++ b/crates/project/src/lib.rs @@ -1466,11 +1466,32 @@ impl Project { } /// Whether comp `inner` is (transitively) nested inside comp `outer` (to prevent cycles). pub fn comp_contains(&self, outer: ItemId, inner: ItemId) -> bool { + // A walk with a stack and a visited set rather than recursion (#1237): a damaged project + // may hold a precomp cycle that doesn't pass `inner` (recursion never ended), a long + // chain must not use up the call stack, and comps shared by many paths are visited once + // (recursion re-walked them for every path). if outer == inner { return true; } - let Some(c) = self.comp(outer) else { return false }; - c.layers.iter().any(|l| matches!(l.source, LayerSource::Comp { item } if self.comp_contains(item, inner))) + let mut seen = std::collections::HashSet::new(); + let mut stack = vec![outer]; + while let Some(id) = stack.pop() { + if !seen.insert(id) { + continue; + } + let Some(c) = self.comp(id) else { continue }; + for l in &c.layers { + if let LayerSource::Comp { item } = l.source { + if item == inner { + return true; + } + if !seen.contains(&item) { + stack.push(item); + } + } + } + } + false } /// Make sure `next_id` exceeds every id in the project (after loading or merging). pub fn fix_next_id(&mut self) { diff --git a/crates/project/src/tests.rs b/crates/project/src/tests.rs index c19d81ee..508cb743 100644 --- a/crates/project/src/tests.rs +++ b/crates/project/src/tests.rs @@ -206,3 +206,58 @@ fn item_users_count_layers_in_every_comp() { assert!(p.item_users(unused).is_empty()); assert!(p.item_users(main).is_empty()); } + +/// A project of `n` empty comps; `links` adds a precomp layer of comp `b` inside comp `a` +/// (indexes into the returned ids). +fn comp_graph(n: usize, links: &[(usize, usize)]) -> (Project, Vec) { + let mut p = Project::default(); + let ids: Vec = (0..n) + .map(|i| { + p.add_item( + &format!("C{i}"), + effectcraft_color::Label::None, + None, + ItemKind::Comp(Comp::new(8, 8, FrameRate::FPS_25, Tick::from_seconds_f64(1.0)).into()), + ) + }) + .collect(); + for &(a, b) in links { + let outer = p.comp(ids[a]).unwrap().clone(); + let l = build::layer(&mut p, &outer, "pre", LayerSource::Comp { item: ids[b] }, (8, 8), None); + let ItemKind::Comp(c) = &mut p.item_mut(ids[a]).unwrap().kind else { panic!("comp") }; + std::sync::Arc::make_mut(c).layers.push(l); + } + (p, ids) +} + +#[test] +fn comp_contains_ends_on_cycles_long_chains_and_shared_precomps() { + // A ⇄ B cycle, C unrelated: the walk used to recurse until the stack ran out (#1237). + let (p, c) = comp_graph(3, &[(0, 1), (1, 0)]); + assert!(!p.comp_contains(c[0], c[2])); + assert!(p.comp_contains(c[0], c[1]) && p.comp_contains(c[1], c[0])); + // A comp nesting itself. + let (p, c) = comp_graph(2, &[(0, 0)]); + assert!(!p.comp_contains(c[0], c[1])); + assert!(p.comp_contains(c[0], c[0])); + // A target reached through a cycle. + let (p, c) = comp_graph(3, &[(0, 1), (1, 0), (1, 2)]); + assert!(p.comp_contains(c[0], c[2])); + assert!(!p.comp_contains(c[2], c[0])); + // 40 levels of diamonds: 2^40 paths, each comp visited once. + let mut links = vec![]; + for i in 0..40 { + let (top, l, r, bottom) = (i * 3, i * 3 + 1, i * 3 + 2, i * 3 + 3); + links.extend([(top, l), (top, r), (l, bottom), (r, bottom)]); + } + let (p, c) = comp_graph(121, &links); + let t0 = std::time::Instant::now(); + assert!(p.comp_contains(c[0], c[120])); + assert!(!p.comp_contains(c[120], c[0])); + assert!(t0.elapsed() < std::time::Duration::from_secs(1), "{:?}", t0.elapsed()); + // A chain 20 000 comps deep (a deep recursion's stack frames). + let n = 20_000; + let (p, c) = comp_graph(n, &(0..n - 1).map(|i| (i, i + 1)).collect::>()); + assert!(p.comp_contains(c[0], c[n - 1])); + assert!(!p.comp_contains(c[n - 1], c[0])); +}