MarkHasVariableReference also for shorthands with invalid var()

If a shorthand contains invalid var()-references, we would return
from ResolvePendingSubstitution with CSSUnsetValue before getting a
chance to mark the property as dependent on a variable. This would
cause StyleResolver to incorrectly cache the style in the matched
properties cache, causing bugs in subsequent recalcs.

Moving the call to MarkHasVariableReference before we attempt to resolve
var()s fixes the issue.

Drive-by: Remove some comments that should not have been committed in
the first place.

Bug: 1055035
Change-Id: I0749da2e8d3f8a74e2bc9418078704a319124fcf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2074237Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744656}

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

@@ -464,6 +464,8 @@ const CSSValue* StyleCascade::ResolvePendingSubstitution(
   DCHECK_NE(property.PropertyID(), CSSPropertyID::kVariable);
   DCHECK_NE(priority.GetOrigin(), CascadeOrigin::kNone);
 
+  MarkHasVariableReference(property);
+
   // If the previous call to ResolvePendingSubstitution parsed 'value', then
   // we don't need to do it again.
   bool is_cached = resolver.shorthand_cache_.value == &value;
@@ -501,8 +503,6 @@ const CSSValue* StyleCascade::ResolvePendingSubstitution(
   const CSSProperty* unvisited_property =
       property.IsVisited() ? property.GetUnvisitedProperty() : &property;
 
-  MarkHasVariableReference(*unvisited_property);
-
   unsigned parsed_properties_count = parsed_properties.size();
   for (unsigned i = 0; i < parsed_properties_count; ++i) {
     const CSSProperty& longhand = CSSProperty::Get(parsed_properties[i].Id());

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

@@ -600,30 +600,6 @@ TEST_F(StyleCascadeTest, ApplyingPendingSubstitutionLast) {
   EXPECT_EQ("4px", cascade.ComputedValue("margin-left"));
 }
 
-// TODO(andruud): This is not useful anymore.
-// TEST_F(StyleCascadeTest, ApplyingPendingSubstitutionModifiesCascade) {
-//  TestCascade cascade(GetDocument());
-//  cascade.Add("margin", "1px var(--x) 3px 4px");
-//  cascade.Add("--x", "2px");
-//  cascade.Add("margin-right", "5px");
-//
-//  // We expect the pending substitution value for all the shorthands,
-//  // except margin-right.
-//  EXPECT_EQ("1px var(--x) 3px 4px", cascade.GetValue("margin-top"));
-//  EXPECT_EQ("5px", cascade.GetValue("margin-right"));
-//  EXPECT_EQ("1px var(--x) 3px 4px", cascade.GetValue("margin-bottom"));
-//  EXPECT_EQ("1px var(--x) 3px 4px", cascade.GetValue("margin-left"));
-//
-//  // Apply a pending substitution value should modify the cascade for other
-//  // longhands with the same pending substitution value.
-//  cascade.Apply("margin-left");
-//
-//  EXPECT_EQ("1px", cascade.GetValue("margin-top"));
-//  EXPECT_EQ("5px", cascade.GetValue("margin-right"));
-//  EXPECT_EQ("3px", cascade.GetValue("margin-bottom"));
-//  EXPECT_FALSE(cascade.GetValue("margin-left"));
-//}
-
 TEST_F(StyleCascadeTest, ResolverDetectCycle) {
   TestCascade cascade(GetDocument());
   TestCascadeResolver resolver(GetDocument());
@@ -1999,6 +1975,56 @@ TEST_F(StyleCascadeTest, MarkReferenced) {
   EXPECT_FALSE(registry->WasReferenced("--y"));
 }
 
+TEST_F(StyleCascadeTest, MarkHasVariableReferenceLonghand) {
+  TestCascade cascade(GetDocument());
+  cascade.Add("--x", "1px");
+  cascade.Add("width", "var(--x)");
+  cascade.Apply();
+  auto style = cascade.TakeStyle();
+  EXPECT_TRUE(style->HasVariableReferenceFromNonInheritedProperty());
+}
+
+TEST_F(StyleCascadeTest, MarkHasVariableReferenceShorthand) {
+  TestCascade cascade(GetDocument());
+  cascade.Add("--x", "1px");
+  cascade.Add("margin", "var(--x)");
+  cascade.Apply();
+  auto style = cascade.TakeStyle();
+  EXPECT_TRUE(style->HasVariableReferenceFromNonInheritedProperty());
+}
+
+TEST_F(StyleCascadeTest, MarkHasVariableReferenceLonghandMissingVar) {
+  TestCascade cascade(GetDocument());
+  cascade.Add("width", "var(--x)");
+  cascade.Apply();
+  auto style = cascade.TakeStyle();
+  EXPECT_TRUE(style->HasVariableReferenceFromNonInheritedProperty());
+}
+
+TEST_F(StyleCascadeTest, MarkHasVariableReferenceShorthandMissingVar) {
+  TestCascade cascade(GetDocument());
+  cascade.Add("margin", "var(--x)");
+  cascade.Apply();
+  auto style = cascade.TakeStyle();
+  EXPECT_TRUE(style->HasVariableReferenceFromNonInheritedProperty());
+}
+
+TEST_F(StyleCascadeTest, NoMarkHasVariableReferenceInherited) {
+  TestCascade cascade(GetDocument());
+  cascade.Add("color", "var(--x)");
+  cascade.Apply();
+  auto style = cascade.TakeStyle();
+  EXPECT_FALSE(style->HasVariableReferenceFromNonInheritedProperty());
+}
+
+TEST_F(StyleCascadeTest, NoMarkHasVariableReferenceWithoutVar) {
+  TestCascade cascade(GetDocument());
+  cascade.Add("width", "1px");
+  cascade.Apply();
+  auto style = cascade.TakeStyle();
+  EXPECT_FALSE(style->HasVariableReferenceFromNonInheritedProperty());
+}
+
 TEST_F(StyleCascadeTest, InternalVisitedColorLonghand) {
   MatchResult result;
   result.FinishAddingUARules();
@@ -2566,7 +2592,4 @@ TEST_F(StyleCascadeTest, NoMarkHasReferenceForInherited) {
                    .HasVariableReferenceFromNonInheritedProperty());
 }
 
-// TODO: Check that tests don't crash when passing wrong match_result/
-// interpolations to Apply.
-
 }  // namespace blink