From 3e251f50fcf7f7755269510ca7fa3e57961e4793 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 27 May 2023 07:21:38 +0200 Subject: [PATCH] style: Fix anonymous name handling in presence of stylesheet sharing We need to compute the anonymous name on the fly while building the CascadeData, otherwise we may see the same layer rule in two places due to stylehseet sharing and make them incorrectly share a name. Differential Revision: https://phabricator.services.mozilla.com/D125175 --- components/style/stylesheets/import_rule.rs | 32 ++++------- components/style/stylesheets/layer_rule.rs | 18 ++---- components/style/stylesheets/rule_parser.rs | 12 ++-- components/style/stylist.rs | 63 +++++++++++++++------ 4 files changed, 65 insertions(+), 60 deletions(-) diff --git a/components/style/stylesheets/import_rule.rs b/components/style/stylesheets/import_rule.rs index d60802cf0ab..48473ff61cd 100644 --- a/components/style/stylesheets/import_rule.rs +++ b/components/style/stylesheets/import_rule.rs @@ -137,12 +137,10 @@ impl DeepCloneWithLock for ImportSheet { } /// The layer keyword or function in an import rule. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ImportLayer { - /// Whether the layer is anonymous. - pub is_anonymous: bool, - /// The layer name. - pub name: LayerName, + /// The layer name, or None for an anonymous layer. + pub name: Option, } @@ -151,12 +149,13 @@ impl ToCss for ImportLayer { where W: Write, { - if self.is_anonymous { - dest.write_str("layer") - } else { - dest.write_str("layer(")?; - self.name.to_css(dest)?; - dest.write_char(')') + match self.name { + None => dest.write_str("layer"), + Some(ref name) => { + dest.write_str("layer(")?; + name.to_css(dest)?; + dest.write_char(')') + }, } } } @@ -199,16 +198,7 @@ impl DeepCloneWithLock for ImportRule { ImportRule { url: self.url.clone(), stylesheet: self.stylesheet.deep_clone_with_lock(lock, guard, params), - layer: self.layer.as_ref().map(|layer| { - ImportLayer { - is_anonymous: layer.is_anonymous, - name: if layer.is_anonymous { - LayerName::new_anonymous() - } else { - layer.name.clone() - }, - } - }), + layer: self.layer.clone(), source_location: self.source_location.clone(), } } diff --git a/components/style/stylesheets/layer_rule.rs b/components/style/stylesheets/layer_rule.rs index c246d5e1b0f..45649dcfece 100644 --- a/components/style/stylesheets/layer_rule.rs +++ b/components/style/stylesheets/layer_rule.rs @@ -202,13 +202,10 @@ impl ToCss for LayerName { pub enum LayerRuleKind { /// A block `@layer ? { ... }` Block { - /// The layer name. - name: LayerName, + /// The layer name, or `None` if anonymous. + name: Option, /// The nested rules. rules: Arc>, - /// Whether the layer name is synthesized (and thus shouldn't be - /// serialized). - is_anonymous: bool, }, /// A statement `@layer , , ;` Statement { @@ -239,9 +236,8 @@ impl ToCssWithGuard for LayerRule { LayerRuleKind::Block { ref name, ref rules, - ref is_anonymous, } => { - if !*is_anonymous { + if let Some(ref name) = *name { dest.write_char(' ')?; name.to_css(&mut CssWriter::new(dest))?; } @@ -277,13 +273,8 @@ impl DeepCloneWithLock for LayerRule { LayerRuleKind::Block { ref name, ref rules, - ref is_anonymous, } => LayerRuleKind::Block { - name: if *is_anonymous { - LayerName::new_anonymous() - } else { - name.clone() - }, + name: name.clone(), rules: Arc::new( lock.wrap( rules @@ -291,7 +282,6 @@ impl DeepCloneWithLock for LayerRule { .deep_clone_with_lock(lock, guard, params), ), ), - is_anonymous: *is_anonymous, }, LayerRuleKind::Statement { ref names } => LayerRuleKind::Statement { names: names.clone(), diff --git a/components/style/stylesheets/rule_parser.rs b/components/style/stylesheets/rule_parser.rs index 9ee3301094d..dd85144a45f 100644 --- a/components/style/stylesheets/rule_parser.rs +++ b/components/style/stylesheets/rule_parser.rs @@ -216,8 +216,7 @@ impl<'a, 'i> AtRuleParser<'i> for TopLevelRuleParser<'a> { None } else if input.try_parse(|input| input.expect_ident_matching("layer")).is_ok() { Some(ImportLayer { - is_anonymous: true, - name: LayerName::new_anonymous(), + name: None, }) } else { input.try_parse(|input| { @@ -225,8 +224,7 @@ impl<'a, 'i> AtRuleParser<'i> for TopLevelRuleParser<'a> { input.parse_nested_block(|input| { LayerName::parse(&self.context, input) }).map(|name| ImportLayer { - is_anonymous: false, - name, + name: Some(name), }) }).ok() }; @@ -603,9 +601,8 @@ impl<'a, 'b, 'i> AtRuleParser<'i> for NestedRuleParser<'a, 'b> { )))) }, AtRulePrelude::Layer(names) => { - let (name, is_anonymous) = match names.len() { - 0 => (LayerName::new_anonymous(), true), - 1 => (names.into_iter().next().unwrap(), false), + let name = match names.len() { + 0 | 1 => names.into_iter().next(), _ => return Err(input.new_error(BasicParseErrorKind::AtRuleBodyInvalid)), }; Ok(CssRule::Layer(Arc::new(self.shared_lock.wrap( @@ -613,7 +610,6 @@ impl<'a, 'b, 'i> AtRuleParser<'i> for NestedRuleParser<'a, 'b> { kind: LayerRuleKind::Block { name, rules: self.parse_nested_rules(input, CssRuleType::Layer), - is_anonymous, }, source_location: start.source_location(), }, diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 4be6e035378..d3d1a24a9d9 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -2150,7 +2150,7 @@ impl CascadeData { stylesheet: &S, guard: &SharedRwLockReadGuard, rebuild_kind: SheetRebuildKind, - current_layer: &mut LayerName, + mut current_layer: &mut LayerName, current_layer_order: LayerOrder, mut precomputed_pseudo_element_decls: Option<&mut PrecomputedPseudoElementDeclarations>, ) -> Result<(), FailedAllocationError> @@ -2337,7 +2337,6 @@ impl CascadeData { continue; } - fn maybe_register_layer(data: &mut CascadeData, layer: &LayerName) -> LayerOrder { // TODO: Measure what's more common / expensive, if // layer.clone() or the double hash lookup in the insert @@ -2366,6 +2365,31 @@ impl CascadeData { order } + fn maybe_register_layers( + data: &mut CascadeData, + name: Option<&LayerName>, + current_layer: &mut LayerName, + pushed_layers: &mut usize, + ) -> LayerOrder { + let anon_name; + let name = match name { + Some(name) => name, + None => { + anon_name = LayerName::new_anonymous(); + &anon_name + }, + }; + + let mut order = LayerOrder::top_level(); + for name in name.layer_names() { + current_layer.0.push(name.clone()); + order = maybe_register_layer(data, ¤t_layer); + *pushed_layers += 1; + } + debug_assert_ne!(order, LayerOrder::top_level()); + order + } + let mut layer_names_to_pop = 0; let mut children_layer_order = current_layer_order; match *rule { @@ -2376,11 +2400,12 @@ impl CascadeData { .saw_effective(import_rule); } if let Some(ref layer) = import_rule.layer { - for name in layer.name.layer_names() { - current_layer.0.push(name.clone()); - children_layer_order = maybe_register_layer(self, ¤t_layer); - layer_names_to_pop += 1; - } + children_layer_order = maybe_register_layers( + self, + layer.name.as_ref(), + &mut current_layer, + &mut layer_names_to_pop, + ); } }, @@ -2396,20 +2421,24 @@ impl CascadeData { let layer_rule = lock.read_with(guard); match layer_rule.kind { LayerRuleKind::Block { ref name, .. } => { - for name in name.layer_names() { - current_layer.0.push(name.clone()); - children_layer_order = maybe_register_layer(self, ¤t_layer); - layer_names_to_pop += 1; - } + children_layer_order = maybe_register_layers( + self, + name.as_ref(), + &mut current_layer, + &mut layer_names_to_pop, + ); } LayerRuleKind::Statement { ref names } => { for name in &**names { let mut pushed = 0; - for name in name.layer_names() { - current_layer.0.push(name.clone()); - maybe_register_layer(self, ¤t_layer); - pushed += 1; - } + // There are no children, so we can ignore the + // return value. + maybe_register_layers( + self, + Some(name), + &mut current_layer, + &mut pushed, + ); for _ in 0..pushed { current_layer.0.pop(); }