From 8b13f372d884270bbf07ceece93fa096f70b975f Mon Sep 17 00:00:00 2001 From: ldm0 Date: Wed, 9 Sep 2026 00:33:11 +0800 Subject: [PATCH] fix(style): reject stale sibling topology during deferred invalidation Check retained child-list relations when draining structural mutation batches. Fall back to lazy source-scope invalidation when removed nodes or old neighbors have moved, covering both old and new scopes without mixing historical and live sibling links. Keep safe insertion batches precise, defer disconnected child-list effects consistently, and cover reorder, reinsert, moved-neighbor, relative-selector, and cross-shadow regressions. --- .../script_vm/tests/dom_xhr/computed_style.rs | 2 + .../dom_xhr/computed_style/child_list.rs | 203 ++++++++++++++++++ .../src/style_engine/eligibility.rs | 93 +++++++- .../src/style_engine/mutation_effect.rs | 1 + moli-renderer-v8/src/style_engine/query.rs | 1 + .../src/style_engine/runtime_invalidation.rs | 9 +- .../src/style_engine/tests/eligibility.rs | 101 +++++++++ .../src/style_engine/tests/invalidator.rs | 17 +- .../src/style_engine/tests/mod.rs | 1 + 9 files changed, 422 insertions(+), 6 deletions(-) create mode 100644 moli-renderer-v8/src/script_vm/tests/dom_xhr/computed_style/child_list.rs create mode 100644 moli-renderer-v8/src/style_engine/tests/eligibility.rs diff --git a/moli-renderer-v8/src/script_vm/tests/dom_xhr/computed_style.rs b/moli-renderer-v8/src/script_vm/tests/dom_xhr/computed_style.rs index 1191edbff6..93f16fe133 100644 --- a/moli-renderer-v8/src/script_vm/tests/dom_xhr/computed_style.rs +++ b/moli-renderer-v8/src/script_vm/tests/dom_xhr/computed_style.rs @@ -1,5 +1,7 @@ use super::*; +mod child_list; + #[test] fn element_current_css_zoom_observes_fresh_effective_style_for_rendered_boxes() { let mut vm = new_parsed_test_vm( diff --git a/moli-renderer-v8/src/script_vm/tests/dom_xhr/computed_style/child_list.rs b/moli-renderer-v8/src/script_vm/tests/dom_xhr/computed_style/child_list.rs new file mode 100644 index 0000000000..b5ba444851 --- /dev/null +++ b/moli-renderer-v8/src/script_vm/tests/dom_xhr/computed_style/child_list.rs @@ -0,0 +1,203 @@ +use super::*; + +#[test] +fn removed_source_with_moved_old_neighbor_invalidates_remaining_target() { + let mut vm = new_parsed_test_vm( + "https://moved-old-neighbor.test/", + r#"
ABC
"#, + ); + let result = vm + .eval( + r#"(() => { + const oldParent = document.getElementById('old'); + const newParent = document.getElementById('new'); + const held = getComputedStyle(document.getElementById('target')); + const values = [held.color]; + document.getElementById('source').remove(); + newParent.appendChild(document.getElementById('neighbor')); + values.push(oldParent.innerText, newParent.innerText, held.color); + return values.join('|'); + })()"#, + ) + .expect("moving an old neighbor must not hide the remaining affected siblings"); + assert_eq!(result, "rgb(255, 0, 0)|C|B|rgb(0, 0, 255)"); +} + +#[test] +fn sibling_move_between_shadow_roots_invalidates_both_style_scopes() { + let mut vm = new_parsed_test_vm( + "https://cross-shadow-sibling-move.test/", + "
", + ); + let result = vm + .eval( + r#"(() => { + const oldRoot = document.getElementById('old').attachShadow({mode:'open'}); + const newRoot = document.getElementById('new').attachShadow({mode:'open'}); + const css = ''; + oldRoot.innerHTML = css + 'AB'; + newRoot.innerHTML = css + 'C'; + const oldTarget = oldRoot.querySelector('.target'); + const newTarget = newRoot.querySelector('.target'); + const oldStyle = getComputedStyle(oldTarget); + const newStyle = getComputedStyle(newTarget); + const values = [oldStyle.color, newStyle.color]; + newRoot.insertBefore(oldRoot.querySelector('.source'), newTarget); + values.push(oldTarget.innerText, newTarget.innerText, oldStyle.color, newStyle.color); + return values.join('|'); + })()"#, + ) + .expect("source-scope fallback must include the old and new shadow trees"); + assert_eq!( + result, + "rgb(255, 0, 0)|rgb(0, 0, 255)|B|C|rgb(0, 0, 255)|rgb(255, 0, 0)" + ); +} + +#[test] +fn detached_then_reinserted_sibling_does_not_reuse_queued_old_links() { + let mut vm = new_parsed_test_vm( + "https://two-step-sibling-move.test/", + r#"
AB
"#, + ); + let result = vm + .eval( + r#"(() => { + const container = document.getElementById('container'); + const source = document.getElementById('source'); + const held = getComputedStyle(document.getElementById('target')); + const values = [held.color]; + source.remove(); + container.appendChild(source); + values.push(container.innerText, held.color); + return values.join('|'); + })()"#, + ) + .expect("separate remove and insert calls must validate topology together at drain time"); + assert_eq!(result, "rgb(255, 0, 0)|BA|rgb(0, 0, 255)"); +} + +#[test] +fn inner_text_and_held_style_update_after_sibling_reorder() { + let mut vm = new_parsed_test_vm( + "https://sibling-reorder.test/", + r#"
AB
"#, + ); + let result = vm + .eval( + r#"(() => { + const container = document.getElementById('container'); + const source = document.getElementById('source'); + const held = getComputedStyle(document.getElementById('target')); + const values = [held.color]; + container.appendChild(source); + values.push(container.innerText, held.color); + container.prepend(source); + values.push(container.innerText, held.color); + return values.join('|'); + })()"#, + ) + .expect("innerText must finish after a sibling move and preserve held style liveness"); + assert_eq!(result, "rgb(255, 0, 0)|BA|rgb(0, 0, 255)|AB|rgb(255, 0, 0)"); +} + +#[test] +fn relative_selector_updates_after_backward_sibling_reorder() { + let mut vm = new_parsed_test_vm( + "https://backward-sibling-reorder.test/", + r#"
AB
"#, + ); + let result = vm + .eval( + r#"(() => { + const container = document.getElementById('container'); + const target = document.getElementById('target'); + const held = getComputedStyle(document.getElementById('source')); + const values = [held.color]; + container.prepend(target); + values.push(container.innerText, held.color); + container.appendChild(target); + values.push(container.innerText, held.color); + return values.join('|'); + })()"#, + ) + .expect("relative invalidation must finish after reversing a sibling relation"); + assert_eq!(result, "rgb(255, 0, 0)|BA|rgb(0, 0, 255)|AB|rgb(255, 0, 0)"); +} + +#[test] +fn sibling_move_between_parents_invalidates_old_and_new_targets() { + let mut vm = new_parsed_test_vm( + "https://cross-parent-sibling-move.test/", + r#" +
AB
+
C
"#, + ); + let result = vm + .eval( + r#"(() => { + const oldParent = document.getElementById('old'); + const newParent = document.getElementById('new'); + const source = document.getElementById('source'); + const oldStyle = getComputedStyle(document.getElementById('old-target')); + const newStyle = getComputedStyle(document.getElementById('new-target')); + const values = [oldStyle.color, newStyle.color]; + newParent.prepend(source); + values.push(oldParent.innerText, newParent.innerText, oldStyle.color, newStyle.color); + return values.join('|'); + })()"#, + ) + .expect("moving a source must invalidate both sibling regions"); + assert_eq!( + result, + "rgb(255, 0, 0)|rgb(0, 0, 255)|B|AC|rgb(0, 0, 255)|rgb(255, 0, 0)" + ); +} + +#[test] +fn batched_sibling_reorders_update_adjacent_and_general_sibling_styles() { + let mut vm = new_parsed_test_vm( + "https://batched-sibling-reorder.test/", + r#"
ABC
"#, + ); + let result = vm + .eval( + r#"(() => { + const container = document.getElementById('container'); + const source = document.getElementById('source'); + const other = document.getElementById('other'); + const held = getComputedStyle(document.getElementById('target')); + const values = [held.color, held.backgroundColor]; + container.appendChild(source); + container.prepend(other); + values.push(container.innerText, held.color, held.backgroundColor); + container.prepend(source); + values.push(container.innerText, held.color, held.backgroundColor); + return values.join('|'); + })()"#, + ) + .expect("batched moves must not retain obsolete sibling edges or computed styles"); + assert_eq!( + result, + "rgb(255, 0, 0)|rgb(0, 0, 0)|CBA|rgb(0, 0, 255)|rgb(255, 255, 255)|ACB|rgb(255, 0, 0)|rgb(255, 255, 255)" + ); +} diff --git a/moli-renderer-v8/src/style_engine/eligibility.rs b/moli-renderer-v8/src/style_engine/eligibility.rs index 1d11769bfc..398552102f 100644 --- a/moli-renderer-v8/src/style_engine/eligibility.rs +++ b/moli-renderer-v8/src/style_engine/eligibility.rs @@ -1,6 +1,97 @@ +use std::collections::HashSet; + use moli_selector::stylo_attribute_change_can_use_retained_invalidator; -use super::StyleAttributeImpact; +use crate::{ + document_runtime::DomHandle, + dom::native::{DomHost, Node}, +}; + +use super::{StyleAttributeImpact, StyleMutationEffect}; + +/// Retained sibling traversal can restore one removed element's links, not a +/// historical tree. Only use it while each recorded splice still agrees with +/// the live tree, excluding subsequent insertions validated in reverse order. +/// Check at drain time: later mutations can move either the changed nodes or +/// their old neighbors, including within the same parent. +pub(super) fn child_list_effects_have_current_topology( + host: &DomHost, + effects: &[StyleMutationEffect], +) -> bool { + let mut later_insertions = HashSet::new(); + effects.iter().rev().all(|effect| { + let StyleMutationEffect::ChildList { + parent, + added_nodes, + removed_nodes, + removed_element_snapshots, + previous_sibling, + next_sibling, + } = effect + else { + return true; + }; + if removed_nodes.iter().any(|&root| { + host.node(root) + .is_none_or(|node| node.parent_node().is_some()) + }) { + return false; + } + // A removed root can stay detached while one of its snapshotted + // descendants is moved back into the live tree. + if removed_element_snapshots.iter().any(|snapshot| { + let mut current = Some(snapshot.handle()); + while let Some(handle) = current { + if removed_nodes.contains(&handle) { + return false; + } + current = host.node(handle).and_then(Node::parent_node); + } + true + }) { + return false; + } + if previous_sibling + .iter() + .chain(next_sibling) + .any(|&sibling| host.node(sibling).and_then(Node::parent_node) != Some(*parent)) + { + return false; + } + let Some(parent_node) = host.node(*parent) else { + return false; + }; + let mut current = match previous_sibling { + Some(previous) => host.next_sibling(*previous), + None => parent_node.first_child(), + }; + current = skip_later_insertions(host, current, &later_insertions); + for &added in added_nodes { + if current != Some(added) { + return false; + } + current = skip_later_insertions(host, host.next_sibling(added), &later_insertions); + } + if current != *next_sibling { + return false; + } + // Pure insertion batches are safe: every new subtree is also queried. + // Do not mistake an appended suffix for a rearranged old sibling chain. + later_insertions.extend(added_nodes.iter().copied()); + true + }) +} + +fn skip_later_insertions( + host: &DomHost, + mut current: Option, + later_insertions: &HashSet, +) -> Option { + while let Some(handle) = current.filter(|handle| later_insertions.contains(handle)) { + current = host.next_sibling(handle); + } + current +} pub(super) fn attribute_effect_can_use_retained_stylo_invalidator(name: &str) -> bool { stylo_attribute_change_can_use_retained_invalidator( diff --git a/moli-renderer-v8/src/style_engine/mutation_effect.rs b/moli-renderer-v8/src/style_engine/mutation_effect.rs index 87a0e056c5..ea25c3be90 100644 --- a/moli-renderer-v8/src/style_engine/mutation_effect.rs +++ b/moli-renderer-v8/src/style_engine/mutation_effect.rs @@ -331,6 +331,7 @@ pub(super) fn style_mutation_effects_are_child_list_structural( effect, StyleMutationEffect::ChildList { .. } | StyleMutationEffect::ConnectedSubtrees { .. } + | StyleMutationEffect::DisconnectedSubtrees { .. } | StyleMutationEffect::SlotAssignment { .. } ) }) diff --git a/moli-renderer-v8/src/style_engine/query.rs b/moli-renderer-v8/src/style_engine/query.rs index 39198848ec..546747f2c2 100644 --- a/moli-renderer-v8/src/style_engine/query.rs +++ b/moli-renderer-v8/src/style_engine/query.rs @@ -211,6 +211,7 @@ fn retained_stylo_invalidation_queries_for_child_list_mutations( if matches!( effect, StyleMutationEffect::ConnectedSubtrees { .. } + | StyleMutationEffect::DisconnectedSubtrees { .. } | StyleMutationEffect::SlotAssignment { .. } ) { continue; diff --git a/moli-renderer-v8/src/style_engine/runtime_invalidation.rs b/moli-renderer-v8/src/style_engine/runtime_invalidation.rs index 2813fe7cd3..43ff0e2a79 100644 --- a/moli-renderer-v8/src/style_engine/runtime_invalidation.rs +++ b/moli-renderer-v8/src/style_engine/runtime_invalidation.rs @@ -17,6 +17,7 @@ use super::{ MoliStyleEngine, StyleMutationEffect, StyleViewport, cause::PendingStyleInvalidationCause, document_world::DocumentStyleWorld, + eligibility::child_list_effects_have_current_topology, mutation_effect::detached_style_subtree_roots_for_mutations, schedule::queue_style_invalidation_for_scope, scope::{ @@ -219,8 +220,10 @@ impl MoliStyleEngine { emulated_media: &EmulatedMediaOverrides, viewport: StyleViewport, ) { - if effects.len() <= IMMEDIATE_STRUCTURAL_MUTATION_EFFECT_LIMIT { - self.invalidate_detached_style_subtrees_for_mutations(host, effects); + self.invalidate_detached_style_subtrees_for_mutations(host, effects); + if effects.len() <= IMMEDIATE_STRUCTURAL_MUTATION_EFFECT_LIMIT + && child_list_effects_have_current_topology(host, effects) + { self.queue_style_invalidation_for_mutations( document, world, @@ -250,6 +253,8 @@ impl MoliStyleEngine { emulated_media: &EmulatedMediaOverrides, viewport: StyleViewport, ) { + // Use all mutation handles to cover both the old and new tree scopes. + // Cause-local fallback roots are not enough when old neighbors moved. let source_scope = source_scope_for_mutations(host, effects); let cause = PendingStyleInvalidationCause::Mutation(Vec::new()); self.queue_style_invalidation_scope( diff --git a/moli-renderer-v8/src/style_engine/tests/eligibility.rs b/moli-renderer-v8/src/style_engine/tests/eligibility.rs new file mode 100644 index 0000000000..6ba656432d --- /dev/null +++ b/moli-renderer-v8/src/style_engine/tests/eligibility.rs @@ -0,0 +1,101 @@ +use super::super::eligibility::child_list_effects_have_current_topology; +use super::*; +use crate::dom::native::Node; + +fn sibling_fixture() -> (DomHost, DomHandle, [DomHandle; 3]) { + let mut host = test_host(); + let parent = host.create_element("div"); + assert!(host.append_child(host.document_handle(), parent)); + let children = std::array::from_fn(|_| { + let child = host.create_element("span"); + assert!(host.append_child(parent, child)); + child + }); + (host, parent, children) +} + +#[test] +fn stable_single_insertions_and_removals_keep_retained_topology() { + for index in 0..3 { + let (mut host, parent, children) = sibling_fixture(); + let removed = host.remove_child_effects(parent, children[index]); + let removed = StyleMutationEffect::from_dom_mutation_effects(&host, &removed); + assert!(child_list_effects_have_current_topology(&host, &removed)); + + let inserted = + host.insert_before_effects(parent, children[index], children.get(index + 1).copied()); + let inserted = StyleMutationEffect::from_dom_mutation_effects(&host, &inserted); + assert!(child_list_effects_have_current_topology(&host, &inserted)); + assert!( + !child_list_effects_have_current_topology(&host, &removed), + "reinserting the removed node invalidates the earlier snapshot" + ); + } +} + +#[test] +fn consecutive_insertions_keep_retained_topology() { + let (mut host, parent, [_, b, _]) = sibling_fixture(); + let mut effects = Vec::new(); + for reference in [None, None, Some(b), Some(b)] { + let added = host.create_element("span"); + let inserted = host.insert_before_effects(parent, added, reference); + effects.extend(StyleMutationEffect::from_dom_mutation_effects( + &host, &inserted, + )); + } + assert!(child_list_effects_have_current_topology(&host, &effects)); +} + +#[test] +fn removal_snapshot_rejects_old_neighbor_moved_to_another_parent() { + let (mut host, parent, [a, b, _]) = sibling_fixture(); + let removed = host.remove_child_effects(parent, a); + let removed = StyleMutationEffect::from_dom_mutation_effects(&host, &removed); + let other = host.create_element("div"); + assert!(host.append_child(host.document_handle(), other)); + assert!(host.append_child(other, b)); + + assert_eq!(host.node(a).and_then(Node::parent_node), None); + assert!(!child_list_effects_have_current_topology(&host, &removed)); +} + +#[test] +fn removal_snapshot_rejects_old_neighbor_reordered_within_parent() { + let (mut host, parent, [a, b, _]) = sibling_fixture(); + let removed = host.remove_child_effects(parent, b); + let removed = StyleMutationEffect::from_dom_mutation_effects(&host, &removed); + assert!(host.append_child(parent, a)); + + assert_eq!(host.node(b).and_then(Node::parent_node), None); + assert_eq!(host.node(a).and_then(Node::parent_node), Some(parent)); + assert!(!child_list_effects_have_current_topology(&host, &removed)); +} + +#[test] +fn removal_snapshot_rejects_reattached_descendant_of_detached_root() { + let (mut host, parent, [_, b, c]) = sibling_fixture(); + let descendant = host.create_element("span"); + assert!(host.append_child(b, descendant)); + let removed = host.remove_child_effects(parent, b); + let removed = StyleMutationEffect::from_dom_mutation_effects(&host, &removed); + assert!(child_list_effects_have_current_topology(&host, &removed)); + assert!(host.append_child(c, descendant)); + + assert_eq!(host.node(b).and_then(Node::parent_node), None); + assert!(!child_list_effects_have_current_topology(&host, &removed)); +} + +#[test] +fn insertion_snapshot_rejects_changed_range_at_consumption_time() { + let (mut host, parent, [a, _, _]) = sibling_fixture(); + let added = host.create_element("span"); + let inserted = host.insert_before_effects(parent, added, Some(a)); + let inserted = StyleMutationEffect::from_dom_mutation_effects(&host, &inserted); + assert!(child_list_effects_have_current_topology(&host, &inserted)); + assert!(host.append_child(parent, added)); + + // Check the old record alone, without relying on the later mutation being + // in the same viewport/media group. + assert!(!child_list_effects_have_current_topology(&host, &inserted)); +} diff --git a/moli-renderer-v8/src/style_engine/tests/invalidator.rs b/moli-renderer-v8/src/style_engine/tests/invalidator.rs index 7f8855cfd3..3a42b2ac8b 100644 --- a/moli-renderer-v8/src/style_engine/tests/invalidator.rs +++ b/moli-renderer-v8/src/style_engine/tests/invalidator.rs @@ -4219,7 +4219,7 @@ fn retained_stylo_invalidator_narrows_child_list_inserted_sibling_invalidation() } #[test] -fn retained_stylo_invalidator_keeps_ua_structural_boundary_when_author_source_has_no_target() { +fn reordered_summary_invalidates_ua_styles_when_author_source_has_no_target() { let mut host = test_host(); let document = host.document_handle(); let details = host.create_element("details"); @@ -4306,8 +4306,19 @@ fn retained_stylo_invalidator_keeps_ua_structural_boundary_when_author_source_ha ); engine.drain_pending_style_invalidations_for_document_for_test(&host, document); - assert!(!engine.computed_style_cache_contains_handle_for_document_for_test(document, first)); - assert!(!engine.computed_style_cache_contains_handle_for_document_for_test(document, second)); + // Reordering uses lazy source-scope invalidation, not eager eviction of + // exact sibling targets. Both cached styles must be stale before a read. + let world = engine.world_for_document(document); + let invalidation = &world.document_state.lazy_invalidation_roots; + for handle in [first, second] { + assert!( + invalidation + .validation_path(&host, document, handle) + .iter() + .any(|entry| entry.element == handle + && !invalidation.element_is_current(handle, entry.required_generation)) + ); + } assert!(engine.computed_style_cache_contains_handle_for_document_for_test(document, unrelated)); assert_eq!( engine.computed_style_property_value( diff --git a/moli-renderer-v8/src/style_engine/tests/mod.rs b/moli-renderer-v8/src/style_engine/tests/mod.rs index 55c550530a..f76d706fa7 100644 --- a/moli-renderer-v8/src/style_engine/tests/mod.rs +++ b/moli-renderer-v8/src/style_engine/tests/mod.rs @@ -524,6 +524,7 @@ fn collect_source_invalidation_roots_for_test( mod char_child; mod dependency; +mod eligibility; mod invalidator; mod lifecycle; mod outcome;