From 3b90ddd5d164c51837190b1134d1f9abb82fcd26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 16 Jun 2018 02:50:28 -0700 Subject: [PATCH] style: Fix the broken invariants of the rule node cache. We were spuriously reframing the because it initially shared style with the
, which ended up being display: none, while the should've been display: contents from the beginning. lookup_by_rules seems pretty prone to obscure bugs, and also it's pretty complex... Probably we should try to get rid of it, I'm unconvinced that it's worth it. Even with that, in a normal restyle the
wouldn't have ended up with a style. It of course never had it before the reframe because the was display: none, but that doesn't mean it shouldn't have gotten one, since we detected we needed to go through kids in: https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/servo/components/style/matching.rs#500 That code did happen, but since it's an animation-only restyle, we don't look at unstyled stuff. That looks somewhat fishy, but I guess for now it's fine as long as display isn't animatable. Bug: 1469076 Reviewed-by: heycam MozReview-Commit-ID: B6NMSTNOKgK --- components/style/sharing/mod.rs | 17 +++++++++++++++-- components/style/style_adjuster.rs | 8 ++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 314e9877083..d6148d16b49 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -815,7 +815,11 @@ impl StyleSharingCache { Some(candidate.element.borrow_data().unwrap().share_styles()) } - /// Attempts to find an element in the cache with the given primary rule node and parent. + /// Attempts to find an element in the cache with the given primary rule + /// node and parent. + /// + /// FIXME(emilio): re-measure this optimization, and remove if it's not very + /// useful... It's probably not worth the complexity / obscure bugs. pub fn lookup_by_rules( &mut self, shared_context: &SharedStyleContext, @@ -841,7 +845,15 @@ impl StyleSharingCache { if style.visited_rules() != visited_rules { return None; } - + // NOTE(emilio): We only need to check name / namespace because we + // do name-dependent style adjustments, like the display: contents + // to display: none adjustment. + if target.namespace() != candidate.element.namespace() { + return None; + } + if target.local_name() != candidate.element.local_name() { + return None; + } // Rule nodes and styles are computed independent of the element's // actual visitedness, but at the end of the cascade (in // `adjust_for_visited`) we do store the visitedness as a flag in @@ -853,6 +865,7 @@ impl StyleSharingCache { // FIXME(jryans): This seems like it breaks the constant time // requirements of visited, assuming we get a cache hit on only one // of unvisited vs. visited. + // TODO(emilio): We no longer have such a flag, remove this check. if target.is_visited_link() != candidate.element.is_visited_link() { return None; } diff --git a/components/style/style_adjuster.rs b/components/style/style_adjuster.rs index 8bbc40ca8c6..3983c780894 100644 --- a/components/style/style_adjuster.rs +++ b/components/style/style_adjuster.rs @@ -19,6 +19,14 @@ use properties::longhands::position::computed_value::T as Position; /// NOTE(emilio): If new adjustments are introduced that depend on reset /// properties of the parent, you may need tweaking the /// `ChildCascadeRequirement` code in `matching.rs`. +/// +/// NOTE(emilio): Also, if new adjustments are introduced that break the +/// following invariant: +/// +/// Given same tag name, namespace, rules and parent style, two elements would +/// end up with exactly the same style. +/// +/// Then you need to adjust the lookup_by_rules conditions in the sharing cache. pub struct StyleAdjuster<'a, 'b: 'a> { style: &'a mut StyleBuilder<'b>, }