Ensure string data is kept alive as long as there are CSSVariableData tokens pointing to it

This change is part of animating registered custom properties.
Animated CSSVariableData objects won't have the same
ownership guarantees as ones owned by CSSValues.

This change removes the assumption that the tokens spliced into a
CSSVariableData during var() resolution will have their backing strings
kept alive somewhere else and instead keeps all necessary string handles
alongside the tokens.

This patch introduces no changes in behaviour.

BUG=671904

Review-Url: https://codereview.chromium.org/2901213005
Cr-Commit-Position: refs/heads/master@{#474970}

third_party/WebKit/Source/core/css/CSSVariableData.cpp

@@ -22,21 +22,23 @@ StylePropertySet* CSSVariableData::PropertySet() {
 }
 
 template <typename CharacterType>
-void CSSVariableData::UpdateTokens(const CSSParserTokenRange& range) {
+static void UpdateTokens(const CSSParserTokenRange& range,
+                         const String& backing_string,
+                         Vector<CSSParserToken>& result) {
   const CharacterType* current_offset =
-      backing_string_.GetCharacters<CharacterType>();
+      backing_string.GetCharacters<CharacterType>();
   for (const CSSParserToken& token : range) {
     if (token.HasStringBacking()) {
       unsigned length = token.Value().length();
       StringView string(current_offset, length);
-      tokens_.push_back(token.CopyWithUpdatedString(string));
+      result.push_back(token.CopyWithUpdatedString(string));
       current_offset += length;
     } else {
-      tokens_.push_back(token);
+      result.push_back(token);
     }
   }
-  DCHECK(current_offset == backing_string_.GetCharacters<CharacterType>() +
-                               backing_string_.length());
+  DCHECK(current_offset == backing_string.GetCharacters<CharacterType>() +
+                               backing_string.length());
 }
 
 bool CSSVariableData::operator==(const CSSVariableData& other) const {
@@ -44,6 +46,8 @@ bool CSSVariableData::operator==(const CSSVariableData& other) const {
 }
 
 void CSSVariableData::ConsumeAndUpdateTokens(const CSSParserTokenRange& range) {
+  DCHECK_EQ(tokens_.size(), 0u);
+  DCHECK_EQ(backing_strings_.size(), 0u);
   StringBuilder string_builder;
   CSSParserTokenRange local_range = range;
 
@@ -52,11 +56,12 @@ void CSSVariableData::ConsumeAndUpdateTokens(const CSSParserTokenRange& range) {
     if (token.HasStringBacking())
       string_builder.Append(token.Value());
   }
-  backing_string_ = string_builder.ToString();
-  if (backing_string_.Is8Bit())
-    UpdateTokens<LChar>(range);
+  String backing_string = string_builder.ToString();
+  backing_strings_.push_back(backing_string);
+  if (backing_string.Is8Bit())
+    UpdateTokens<LChar>(range, backing_string, tokens_);
   else
-    UpdateTokens<UChar>(range);
+    UpdateTokens<UChar>(range, backing_string, tokens_);
 }
 
 CSSVariableData::CSSVariableData(const CSSParserTokenRange& range,

third_party/WebKit/Source/core/css/CSSVariableData.h

@@ -31,16 +31,16 @@ class CORE_EXPORT CSSVariableData : public RefCounted<CSSVariableData> {
 
   static PassRefPtr<CSSVariableData> CreateResolved(
       const Vector<CSSParserToken>& resolved_tokens,
-      const CSSVariableData& unresolved_data,
+      Vector<String> backing_strings,
       bool is_animation_tainted) {
-    return AdoptRef(new CSSVariableData(resolved_tokens,
-                                        unresolved_data.backing_string_,
-                                        is_animation_tainted));
+    return AdoptRef(new CSSVariableData(
+        resolved_tokens, std::move(backing_strings), is_animation_tainted));
   }
 
   CSSParserTokenRange TokenRange() const { return tokens_; }
 
   const Vector<CSSParserToken>& Tokens() const { return tokens_; }
+  const Vector<String>& BackingStrings() const { return backing_strings_; }
 
   bool operator==(const CSSVariableData& other) const;
 
@@ -57,25 +57,21 @@ class CORE_EXPORT CSSVariableData : public RefCounted<CSSVariableData> {
                   bool is_animation_tainted,
                   bool needs_variable_resolution);
 
-  // We can safely copy the tokens (which have raw pointers to substrings)
-  // because StylePropertySets contain references to
-  // CSSCustomPropertyDeclarations, which point to the unresolved
-  // CSSVariableData values that own the backing strings this will potentially
-  // reference.
   CSSVariableData(const Vector<CSSParserToken>& resolved_tokens,
-                  String backing_string,
+                  Vector<String> backing_strings,
                   bool is_animation_tainted)
-      : backing_string_(backing_string),
+      : backing_strings_(std::move(backing_strings)),
         tokens_(resolved_tokens),
         is_animation_tainted_(is_animation_tainted),
         needs_variable_resolution_(false),
         cached_property_set_(false) {}
 
   void ConsumeAndUpdateTokens(const CSSParserTokenRange&);
-  template <typename CharacterType>
-  void UpdateTokens(const CSSParserTokenRange&);
 
-  String backing_string_;
+  // tokens_ may have raw pointers to string data, we store the String objects
+  // owning that data in backing_strings_ to keep it alive alongside the
+  // tokens_.
+  Vector<String> backing_strings_;
   Vector<CSSParserToken> tokens_;
   const bool is_animation_tainted_;
   const bool needs_variable_resolution_;

third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp

@@ -27,16 +27,18 @@
 
 namespace blink {
 
-bool CSSVariableResolver::ResolveFallback(CSSParserTokenRange range,
-                                          bool disallow_animation_tainted,
-                                          Vector<CSSParserToken>& result,
-                                          bool& result_is_animation_tainted) {
+bool CSSVariableResolver::ResolveFallback(
+    CSSParserTokenRange range,
+    bool disallow_animation_tainted,
+    Vector<CSSParserToken>& result,
+    Vector<String>& result_backing_strings,
+    bool& result_is_animation_tainted) {
   if (range.AtEnd())
     return false;
   DCHECK_EQ(range.Peek().GetType(), kCommaToken);
   range.Consume();
   return ResolveTokenRange(range, disallow_animation_tainted, result,
-                           result_is_animation_tainted);
+                           result_backing_strings, result_is_animation_tainted);
 }
 
 CSSVariableData* CSSVariableResolver::ValueForCustomProperty(
@@ -96,22 +98,19 @@ PassRefPtr<CSSVariableData> CSSVariableResolver::ResolveCustomProperty(
   bool disallow_animation_tainted = false;
   bool is_animation_tainted = variable_data.IsAnimationTainted();
   Vector<CSSParserToken> tokens;
+  Vector<String> backing_strings;
+  backing_strings.AppendVector(variable_data.BackingStrings());
   variables_seen_.insert(name);
   bool success =
       ResolveTokenRange(variable_data.Tokens(), disallow_animation_tainted,
-                        tokens, is_animation_tainted);
+                        tokens, backing_strings, is_animation_tainted);
   variables_seen_.erase(name);
 
-  // The old variable data holds onto the backing string the new resolved
-  // CSSVariableData relies on. Ensure it will live beyond us overwriting the
-  // RefPtr in StyleInheritedVariables.
-  DCHECK_GT(variable_data.RefCount(), 1);
-
   if (!success || !cycle_start_points_.IsEmpty()) {
     cycle_start_points_.erase(name);
     return nullptr;
   }
-  return CSSVariableData::CreateResolved(tokens, variable_data,
+  return CSSVariableData::CreateResolved(tokens, std::move(backing_strings),
                                          is_animation_tainted);
 }
 
@@ -119,6 +118,7 @@ bool CSSVariableResolver::ResolveVariableReference(
     CSSParserTokenRange range,
     bool disallow_animation_tainted,
     Vector<CSSParserToken>& result,
+    Vector<String>& result_backing_strings,
     bool& result_is_animation_tainted) {
   range.ConsumeWhitespace();
   DCHECK_EQ(range.Peek().GetType(), kIdentToken);
@@ -132,21 +132,26 @@ bool CSSVariableResolver::ResolveVariableReference(
     // TODO(alancutter): Append the registered initial custom property value if
     // we are disallowing an animation tainted value.
     return ResolveFallback(range, disallow_animation_tainted, result,
-                           result_is_animation_tainted);
+                           result_backing_strings, result_is_animation_tainted);
   }
 
   result.AppendVector(variable_data->Tokens());
+  // TODO(alancutter): Avoid adding backing strings multiple times in a row.
+  result_backing_strings.AppendVector(variable_data->BackingStrings());
   result_is_animation_tainted |= variable_data->IsAnimationTainted();
 
   Vector<CSSParserToken> trash;
+  Vector<String> trash_backing_strings;
   bool trash_is_animation_tainted;
   ResolveFallback(range, disallow_animation_tainted, trash,
-                  trash_is_animation_tainted);
+                  trash_backing_strings, trash_is_animation_tainted);
   return true;
 }
 
-void CSSVariableResolver::ResolveApplyAtRule(CSSParserTokenRange& range,
-                                             Vector<CSSParserToken>& result) {
+void CSSVariableResolver::ResolveApplyAtRule(
+    CSSParserTokenRange& range,
+    Vector<CSSParserToken>& result,
+    Vector<String>& result_backing_strings) {
   DCHECK(range.Peek().GetType() == kAtKeywordToken &&
          EqualIgnoringASCIICase(range.Peek().Value(), "apply"));
   range.ConsumeIncludingWhitespace();
@@ -170,22 +175,25 @@ void CSSVariableResolver::ResolveApplyAtRule(CSSParserTokenRange& range,
     return;
 
   result.AppendRange(rule_contents.begin(), rule_contents.end());
+  result_backing_strings.AppendVector(variable_data->BackingStrings());
 }
 
-bool CSSVariableResolver::ResolveTokenRange(CSSParserTokenRange range,
-                                            bool disallow_animation_tainted,
-                                            Vector<CSSParserToken>& result,
-                                            bool& result_is_animation_tainted) {
+bool CSSVariableResolver::ResolveTokenRange(
+    CSSParserTokenRange range,
+    bool disallow_animation_tainted,
+    Vector<CSSParserToken>& result,
+    Vector<String>& result_backing_strings,
+    bool& result_is_animation_tainted) {
   bool success = true;
   while (!range.AtEnd()) {
     if (range.Peek().FunctionId() == CSSValueVar) {
-      success &= ResolveVariableReference(range.ConsumeBlock(),
-                                          disallow_animation_tainted, result,
-                                          result_is_animation_tainted);
+      success &= ResolveVariableReference(
+          range.ConsumeBlock(), disallow_animation_tainted, result,
+          result_backing_strings, result_is_animation_tainted);
     } else if (range.Peek().GetType() == kAtKeywordToken &&
                EqualIgnoringASCIICase(range.Peek().Value(), "apply") &&
                RuntimeEnabledFeatures::cssApplyAtRulesEnabled()) {
-      ResolveApplyAtRule(range, result);
+      ResolveApplyAtRule(range, result, result_backing_strings);
     } else {
       result.push_back(range.Consume());
     }
@@ -223,10 +231,11 @@ const CSSValue* CSSVariableResolver::ResolveVariableReferences(
     bool disallow_animation_tainted) {
   CSSVariableResolver resolver(state);
   Vector<CSSParserToken> tokens;
+  Vector<String> backing_strings;
   bool is_animation_tainted = false;
   if (!resolver.ResolveTokenRange(value.VariableDataValue()->Tokens(),
                                   disallow_animation_tainted, tokens,
-                                  is_animation_tainted))
+                                  backing_strings, is_animation_tainted))
     return CSSUnsetValue::Create();
   const CSSValue* result =
       CSSPropertyParser::ParseSingleValue(id, tokens, value.ParserContext());
@@ -254,11 +263,12 @@ const CSSValue* CSSVariableResolver::ResolvePendingSubstitutions(
     CSSVariableResolver resolver(state);
 
     Vector<CSSParserToken> tokens;
+    Vector<String> backing_strings;
     bool is_animation_tainted = false;
     if (resolver.ResolveTokenRange(
             shorthand_value->VariableDataValue()->Tokens(),
-            disallow_animation_tainted, tokens, is_animation_tainted)) {
-
+            disallow_animation_tainted, tokens, backing_strings,
+            is_animation_tainted)) {
       HeapVector<CSSProperty, 256> parsed_properties;
 
       if (CSSPropertyParser::ParseValue(

third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.h

@@ -58,20 +58,25 @@ class CSSVariableResolver {
   bool ResolveTokenRange(CSSParserTokenRange,
                          bool disallow_animation_tainted,
                          Vector<CSSParserToken>& result,
+                         Vector<String>& result_backing_strings,
                          bool& result_is_animation_tainted);
   // Resolves the fallback (if present) of a var() reference, starting from the
   // comma.
   bool ResolveFallback(CSSParserTokenRange,
                        bool disallow_animation_tainted,
                        Vector<CSSParserToken>& result,
+                       Vector<String>& result_backing_strings,
                        bool& result_is_animation_tainted);
   // Resolves the contents of a var() reference.
   bool ResolveVariableReference(CSSParserTokenRange,
                                 bool disallow_animation_tainted,
                                 Vector<CSSParserToken>& result,
+                                Vector<String>& result_backing_strings,
                                 bool& result_is_animation_tainted);
   // Consumes and resolves an @apply rule.
-  void ResolveApplyAtRule(CSSParserTokenRange&, Vector<CSSParserToken>& result);
+  void ResolveApplyAtRule(CSSParserTokenRange&,
+                          Vector<CSSParserToken>& result,
+                          Vector<String>& result_backing_strings);
 
   // These return null if the custom property is invalid.