[mpc] Only mark dependencies from non-inherited properties

Before the dependency-aware MatchedPropertiesCache, we would set a
flag HasVariableReferenceFromNonInheritedProperty whenever some non-
inherited property referenced a custom property.

However, with the dependency-aware MPC enabled, var() references from
inherited properties can also incur dependencies. This caused a
regression in the CustomPropertiesVarAlias.htm benchmark, since it
creates a rule with 2000 var() references that are referenced from
other custom property declarations.

Since (regular) custom property declarations are _inherited_
properties, they would not be excluded from the cache with the old
code, but they are excluded now, since we exceed the dependency limit.

To fix this, we only produce a dependency from a var() if it is
referenced from a non-inherited property.

However, this creates a new problem for the "apply inherited only"-type
of cache hit: applying inherited properties mean custom properties
(that are not registered as non-inherited) will be applied as well, and
this affects any var() references in non-inherited properties. This
problem is exactly the same as "apply inherited only" when it affects
the font or zoom (which may affect lots of other non-inherited
properties). Therefore, this CL changes ::EffectiveZoomOrFontChanged
to ::IsUsableAfterApplyInheritedOnly (name inspired by Webkit's similar
code), which also takes changes to inherited variables into account.

Sidenote: The reason for using CascadeResolver to figure out the
current CSSProperty rather than propagating it as an argument is to
avoid having multiple sources of truth. If we passed a CSSProperty as
a parameter, it would be possible to pass the wrong thing (vs. what is
contained by CascadeResolver), and that would be very bad.

Bug: 1057072, 1095460
Change-Id: I137fb8ffc69eb2b913c8494ab9b1d1debc8038eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2247767
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#780079}

third_party/blink/renderer/core/css/resolver/cascade_resolver.cc

@@ -18,7 +18,7 @@ bool CascadeResolver::IsLocked(const CSSProperty& property) const {
 
 bool CascadeResolver::AllowSubstitution(CSSVariableData* data) const {
   if (data && data->IsAnimationTainted() && stack_.size()) {
-    const CSSProperty* property = stack_.back();
+    const CSSProperty* property = CurrentProperty();
     if (IsA<CustomProperty>(*property))
       return true;
     return !CSSAnimations::IsAnimationAffectingProperty(*property);

third_party/blink/renderer/core/css/resolver/cascade_resolver.h

@@ -41,6 +41,11 @@ class CORE_EXPORT CascadeResolver {
   // a cycle, and is therefore an error.
   bool IsLocked(const CSSProperty&) const;
 
+  // Returns the property we're currently applying.
+  const CSSProperty* CurrentProperty() const {
+    return stack_.size() ? stack_.back() : nullptr;
+  }
+
   // We do not allow substitution of animation-tainted values into
   // an animation-affecting property.
   //

third_party/blink/renderer/core/css/resolver/style_cascade.cc

@@ -749,7 +749,8 @@ bool StyleCascade::ResolveVarInto(CSSParserTokenRange range,
   // Any custom property referenced (by anything, even just once) in the
   // document can currently not be animated on the compositor. Hence we mark
   // properties that have been referenced.
-  MarkIsReferenced(property);
+  DCHECK(resolver.CurrentProperty());
+  MarkIsReferenced(*resolver.CurrentProperty(), property);
 
   if (!resolver.DetectCycle(property)) {
     // We are about to substitute var(property). In order to do that, we must
@@ -870,15 +871,16 @@ bool StyleCascade::ValidateFallback(const CustomProperty& property,
   return property.ParseSingleValue(range, *context, local_context);
 }
 
-void StyleCascade::MarkIsReferenced(const CustomProperty& property) {
+void StyleCascade::MarkIsReferenced(const CSSProperty& referencer,
+                                    const CustomProperty& referenced) {
   // For simplicity, we mark all inherited custom property references as
   // dependencies, even though it might not be a dependency if this cascade
   // defines a value for that property.
-  if (property.IsInherited())
-    MarkDependency(property);
-  if (!property.IsRegistered())
+  if (!referencer.IsInherited() && referenced.IsInherited())
+    MarkDependency(referenced);
+  if (!referenced.IsRegistered())
     return;
-  const AtomicString& name = property.GetPropertyNameAtomicString();
+  const AtomicString& name = referenced.GetPropertyNameAtomicString();
   state_.GetDocument().EnsurePropertyRegistry().MarkReferenced(name);
 }
 

third_party/blink/renderer/core/css/resolver/style_cascade.h

@@ -305,7 +305,8 @@ class CORE_EXPORT StyleCascade {
   bool ValidateFallback(const CustomProperty&, CSSParserTokenRange) const;
   // Marks the CustomProperty as referenced by something. Needed to avoid
   // animating these custom properties on the compositor.
-  void MarkIsReferenced(const CustomProperty&);
+  void MarkIsReferenced(const CSSProperty& referencer,
+                        const CustomProperty& referenced);
   // Marks a CSSProperty as having a reference to a custom property. Needed to
   // disable the matched property cache in some cases.
   void MarkHasVariableReference(const CSSProperty&);

third_party/blink/renderer/core/css/resolver/style_cascade_test.cc

@@ -71,6 +71,9 @@ class TestCascadeResolver {
   }
   uint8_t GetGeneration() { return resolver_.generation_; }
   CascadeResolver& InnerResolver() { return resolver_; }
+  const CSSProperty* CurrentProperty() const {
+    return resolver_.CurrentProperty();
+  }
 
  private:
   friend class TestCascadeAutoLock;
@@ -814,6 +817,34 @@ TEST_F(StyleCascadeTest, ResolverMarkApplied) {
   EXPECT_EQ(2, priority.GetGeneration());
 }
 
+TEST_F(StyleCascadeTest, CurrentProperty) {
+  using AutoLock = TestCascadeAutoLock;
+
+  TestCascade cascade(GetDocument());
+  TestCascadeResolver resolver;
+
+  CustomProperty a("--a", GetDocument());
+  CustomProperty b("--b", GetDocument());
+  CustomProperty c("--c", GetDocument());
+
+  EXPECT_FALSE(resolver.CurrentProperty());
+  {
+    AutoLock lock(a, resolver);
+    EXPECT_EQ(&a, resolver.CurrentProperty());
+    {
+      AutoLock lock(b, resolver);
+      EXPECT_EQ(&b, resolver.CurrentProperty());
+      {
+        AutoLock lock(c, resolver);
+        EXPECT_EQ(&c, resolver.CurrentProperty());
+      }
+      EXPECT_EQ(&b, resolver.CurrentProperty());
+    }
+    EXPECT_EQ(&a, resolver.CurrentProperty());
+  }
+  EXPECT_FALSE(resolver.CurrentProperty());
+}
+
 TEST_F(StyleCascadeTest, ResolverMarkUnapplied) {
   TestCascadeResolver resolver(7);
 

third_party/blink/renderer/core/css/resolver/style_resolver.cc

@@ -1834,9 +1834,18 @@ bool StyleResolver::CacheSuccess::FontChanged(
          style.GetFontDescription();
 }
 
-bool StyleResolver::CacheSuccess::EffectiveZoomOrFontChanged(
+bool StyleResolver::CacheSuccess::InheritedVariablesChanged(
     const ComputedStyle& style) const {
-  return EffectiveZoomChanged(style) || FontChanged(style);
+  if (!cached_matched_properties)
+    return false;
+  return cached_matched_properties->computed_style->InheritedVariables() !=
+         style.InheritedVariables();
+}
+
+bool StyleResolver::CacheSuccess::IsUsableAfterApplyInheritedOnly(
+    const ComputedStyle& style) const {
+  return !EffectiveZoomChanged(style) && !FontChanged(style) &&
+         !InheritedVariablesChanged(style);
 }
 
 StyleResolver::CacheSuccess StyleResolver::ApplyMatchedCache(
@@ -2175,7 +2184,7 @@ void StyleResolver::CascadeAndApplyMatchedProperties(StyleResolverState& state,
 
   if (cache_success.ShouldApplyInheritedOnly()) {
     cascade.Apply(CascadeFilter(CSSProperty::kInherited, false));
-    if (cache_success.EffectiveZoomOrFontChanged(state.StyleRef()))
+    if (!cache_success.IsUsableAfterApplyInheritedOnly(state.StyleRef()))
       cascade.Apply(CascadeFilter(CSSProperty::kInherited, true));
   } else {
     cascade.Apply();

third_party/blink/renderer/core/css/resolver/style_resolver.h

@@ -212,7 +212,8 @@ class CORE_EXPORT StyleResolver final : public GarbageCollected<StyleResolver> {
     }
     bool EffectiveZoomChanged(const ComputedStyle&) const;
     bool FontChanged(const ComputedStyle&) const;
-    bool EffectiveZoomOrFontChanged(const ComputedStyle&) const;
+    bool InheritedVariablesChanged(const ComputedStyle&) const;
+    bool IsUsableAfterApplyInheritedOnly(const ComputedStyle&) const;
   };
 
   // These flags indicate whether an apply pass for a given CSSPropertyPriority

third_party/blink/renderer/core/css/resolver/style_resolver_test.cc

@@ -561,4 +561,30 @@ TEST_F(StyleResolverTest, CSSMarkerPseudoElement) {
   }
 }
 
+TEST_F(StyleResolverTest, ApplyInheritedOnlyCustomPropertyChange) {
+  ScopedCSSMatchedPropertiesCacheDependenciesForTest scoped_feature(true);
+
+  // This test verifies that when we get a "apply inherited only"-type
+  // hit in the MatchesPropertiesCache, we're able to detect that custom
+  // properties changed, and that we therefore need to apply the non-inherited
+  // properties as well.
+
+  GetDocument().body()->setInnerHTML(R"HTML(
+    <style>
+      #parent1 { --a: 10px; }
+      #parent2 { --a: 20px; }
+      #child1, #child2 {
+        --b: var(--a);
+        width: var(--b);
+      }
+    </style>
+    <div id=parent1><div id=child1></div></div>
+    <div id=parent2><div id=child2></div></div>
+  )HTML");
+  UpdateAllLifecyclePhasesForTest();
+
+  EXPECT_EQ("10px", ComputedValue("width", *StyleForId("child1")));
+  EXPECT_EQ("20px", ComputedValue("width", *StyleForId("child2")));
+}
+
 }  // namespace blink