Mostly remove refcount thrashes of value in setAttribute.

Relanding https://chromium-review.googlesource.com/c/chromium/src/+/2351950

This mostly removes the refcount thrashes in Element::setAttribute()
which accounts for nearly 13% of regression in blink_perf.bindings
set-attribute.html story when StringImpl is made threadsafe.

Because of some reentrant behavior in Element::setAttribute() ->
Element::SetAttributeInternal() -> Element::DidModifyAttribute() ->
Element::AttributeChanged() -> Element::ParseAttribute() ->
Element::setAttribute(), the qname for the Attribute, the old value,
and the new value cannot be easily transfered into the Element's internal
ElementData::Attributes() collection while also publishing the change to
observers via the DidModifyAttribute() call. Removal of that thrash will
be done in a follow-up CL.

Bug: 1083392
Change-Id: I7f7f4bfa22a2e553e703c0375bd04bb77f71de23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2369348Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Albert J. Wong <ajwong@chromium.org>
Auto-Submit: Albert J. Wong <ajwong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801214}

third_party/blink/renderer/bindings/core/v8/generated_code_helper.cc

@@ -395,7 +395,7 @@ void PerformAttributeSetCEReactionsReflectTypeString(
     const QualifiedName& content_attribute,
     const char* interface_name,
     const char* attribute_name) {
-  PerformAttributeSetCEReactionsReflect<IDLStringV2, const AtomicString&,
+  PerformAttributeSetCEReactionsReflect<IDLStringV2, AtomicString,
                                         &Element::setAttribute>(
       info, content_attribute, interface_name, attribute_name);
 }
@@ -406,8 +406,7 @@ void PerformAttributeSetCEReactionsReflectTypeStringLegacyNullToEmptyString(
     const char* interface_name,
     const char* attribute_name) {
   PerformAttributeSetCEReactionsReflect<IDLStringTreatNullAsEmptyStringV2,
-                                        const AtomicString&,
-                                        &Element::setAttribute>(
+                                        AtomicString, &Element::setAttribute>(
       info, content_attribute, interface_name, attribute_name);
 }
 
@@ -416,8 +415,8 @@ void PerformAttributeSetCEReactionsReflectTypeStringOrNull(
     const QualifiedName& content_attribute,
     const char* interface_name,
     const char* attribute_name) {
-  PerformAttributeSetCEReactionsReflect<
-      IDLNullable<IDLStringV2>, const AtomicString&, &Element::setAttribute>(
+  PerformAttributeSetCEReactionsReflect<IDLNullable<IDLStringV2>, AtomicString,
+                                        &Element::setAttribute>(
       info, content_attribute, interface_name, attribute_name);
 }
 

third_party/blink/renderer/core/dom/attribute.h

@@ -39,8 +39,8 @@ class Attribute {
   DISALLOW_NEW();
 
  public:
-  Attribute(const QualifiedName& name, const AtomicString& value)
-      : name_(name), value_(value) {}
+  Attribute(const QualifiedName& name, AtomicString value)
+      : name_(name), value_(std::move(value)) {}
 
   // NOTE: The references returned by these functions are only valid for as long
   // as the Attribute stays in place. For example, calling a function that
@@ -56,7 +56,10 @@ class Attribute {
   bool Matches(const QualifiedName&) const;
   bool MatchesCaseInsensitive(const QualifiedName&) const;
 
-  void SetValue(const AtomicString& value) { value_ = value; }
+  void SetValue(AtomicString value) { value_ = std::move(value); }
+  AtomicString ExchangeValue(AtomicString value) {
+    return std::exchange(value_, std::move(value));
+  }
 
   // Note: This API is only for HTMLTreeBuilder.  It is not safe to change the
   // name of an attribute once parseAttribute has been called as DOM

third_party/blink/renderer/core/dom/attribute_collection.h

@@ -160,13 +160,15 @@ class MutableAttributeCollection
             attributes) {}
 
   // These functions do no error/duplicate checking.
-  void Append(const QualifiedName&, const AtomicString& value);
+  const AtomicString& Append(const QualifiedName&, AtomicString value);
   void Remove(unsigned index);
 };
 
-inline void MutableAttributeCollection::Append(const QualifiedName& name,
-                                               const AtomicString& value) {
-  attributes_.push_back(Attribute(name, value));
+inline const AtomicString& MutableAttributeCollection::Append(
+    const QualifiedName& name,
+    AtomicString value) {
+  attributes_.emplace_back(name, std::move(value));
+  return attributes_.back().Value();
 }
 
 inline void MutableAttributeCollection::Remove(unsigned index) {

third_party/blink/renderer/core/dom/element-hot.cc

@@ -120,45 +120,45 @@ std::pair<wtf_size_t, const QualifiedName> Element::LookupAttributeQNameHinted(
                                  g_null_atom));
 }
 
-void Element::setAttribute(const QualifiedName& name,
-                           const AtomicString& value) {
+void Element::setAttribute(const QualifiedName& name, AtomicString value) {
   SynchronizeAttribute(name);
   wtf_size_t index = GetElementData()
                          ? GetElementData()->Attributes().FindIndex(name)
                          : kNotFound;
-  SetAttributeInternal(index, name, value,
+  SetAttributeInternal(index, name, std::move(value),
                        kNotInSynchronizationOfLazyAttribute);
 }
 
 void Element::setAttribute(const QualifiedName& name,
-                           const AtomicString& value,
+                           AtomicString value,
                            ExceptionState& exception_state) {
   SynchronizeAttribute(name);
   wtf_size_t index = GetElementData()
                          ? GetElementData()->Attributes().FindIndex(name)
                          : kNotFound;
 
-  AtomicString trusted_value(
-      TrustedTypesCheckFor(ExpectedTrustedTypeForAttribute(name), value,
-                           GetExecutionContext(), exception_state));
+  AtomicString trusted_value(TrustedTypesCheckFor(
+      ExpectedTrustedTypeForAttribute(name), std::move(value),
+      GetExecutionContext(), exception_state));
   if (exception_state.HadException())
     return;
 
-  SetAttributeInternal(index, name, trusted_value,
+  SetAttributeInternal(index, name, std::move(trusted_value),
                        kNotInSynchronizationOfLazyAttribute);
 }
 
 void Element::SetSynchronizedLazyAttribute(const QualifiedName& name,
-                                           const AtomicString& value) {
+                                           AtomicString value) {
   wtf_size_t index = GetElementData()
                          ? GetElementData()->Attributes().FindIndex(name)
                          : kNotFound;
-  SetAttributeInternal(index, name, value, kInSynchronizationOfLazyAttribute);
+  SetAttributeInternal(index, name, std::move(value),
+                       kInSynchronizationOfLazyAttribute);
 }
 
 void Element::SetAttributeHinted(const AtomicString& local_name,
                                  WTF::AtomicStringTable::WeakResult hint,
-                                 const AtomicString& value,
+                                 AtomicString value,
                                  ExceptionState& exception_state) {
   if (!Document::IsValidName(local_name)) {
     exception_state.ThrowDOMException(
@@ -178,7 +178,7 @@ void Element::SetAttributeHinted(const AtomicString& local_name,
   if (exception_state.HadException())
     return;
 
-  SetAttributeInternal(index, q_name, trusted_value,
+  SetAttributeInternal(index, q_name, std::move(trusted_value),
                        kNotInSynchronizationOfLazyAttribute);
 }
 
@@ -204,14 +204,14 @@ void Element::SetAttributeHinted(
       GetExecutionContext(), exception_state));
   if (exception_state.HadException())
     return;
-  SetAttributeInternal(index, q_name, value,
+  SetAttributeInternal(index, q_name, std::move(value),
                        kNotInSynchronizationOfLazyAttribute);
 }
 
 ALWAYS_INLINE void Element::SetAttributeInternal(
     wtf_size_t index,
     const QualifiedName& name,
-    const AtomicString& new_value,
+    AtomicString new_value,
     SynchronizationOfLazyAttribute in_synchronization_of_lazy_attribute) {
   if (new_value.IsNull()) {
     if (index != kNotFound)
@@ -220,25 +220,32 @@ ALWAYS_INLINE void Element::SetAttributeInternal(
   }
 
   if (index == kNotFound) {
-    AppendAttributeInternal(name, new_value,
+    AppendAttributeInternal(name, std::move(new_value),
                             in_synchronization_of_lazy_attribute);
     return;
   }
 
   const Attribute& existing_attribute =
       GetElementData()->Attributes().at(index);
-  AtomicString existing_attribute_value = existing_attribute.Value();
-  QualifiedName existing_attribute_name = existing_attribute.GetName();
 
   if (!in_synchronization_of_lazy_attribute) {
-    WillModifyAttribute(existing_attribute_name, existing_attribute_value,
-                        new_value);
+    WillModifyAttribute(existing_attribute.GetName(),
+                        existing_attribute.Value(), new_value);
   }
-  if (new_value != existing_attribute_value)
-    EnsureUniqueElementData().Attributes().at(index).SetValue(new_value);
+
+  AtomicString old_value_storage;  // Keep the old value alive in an update.
+  const AtomicString* old_val_ptr = &existing_attribute.Value();
+  const AtomicString* new_val_ptr = &new_value;
+  if (new_value != *old_val_ptr) {
+    Attribute& attribute = EnsureUniqueElementData().Attributes().at(index);
+    old_value_storage = attribute.ExchangeValue(std::move(new_value));
+    old_val_ptr = &old_value_storage;
+    new_val_ptr = &attribute.Value();
+  }
+
   if (!in_synchronization_of_lazy_attribute) {
-    DidModifyAttribute(existing_attribute_name, existing_attribute_value,
-                       new_value);
+    DidModifyAttribute(existing_attribute.GetName(), *old_val_ptr,
+                       *new_val_ptr);
   }
 }
 
@@ -301,7 +308,7 @@ Attr* Element::setAttributeNode(Attr* attr_node,
     }
   }
 
-  SetAttributeInternal(index, attr_node->GetQualifiedName(), value,
+  SetAttributeInternal(index, attr_node->GetQualifiedName(), std::move(value),
                        kNotInSynchronizationOfLazyAttribute);
 
   attr_node->AttachToElement(this, local_name);

third_party/blink/renderer/core/dom/element.cc

@@ -3794,7 +3794,7 @@ void Element::setAttributeNS(
   if (exception_state.HadException())
     return;
 
-  setAttribute(parsed_name, value);
+  setAttribute(parsed_name, std::move(value));
 }
 
 void Element::RemoveAttributeInternal(
@@ -3828,13 +3828,14 @@ void Element::RemoveAttributeInternal(
 
 void Element::AppendAttributeInternal(
     const QualifiedName& name,
-    const AtomicString& value,
+    AtomicString value,
     SynchronizationOfLazyAttribute in_synchronization_of_lazy_attribute) {
   if (!in_synchronization_of_lazy_attribute)
     WillModifyAttribute(name, g_null_atom, value);
-  EnsureUniqueElementData().Attributes().Append(name, value);
+  const AtomicString& stored_value =
+      EnsureUniqueElementData().Attributes().Append(name, std::move(value));
   if (!in_synchronization_of_lazy_attribute)
-    DidAddAttribute(name, value);
+    DidAddAttribute(name, stored_value);
 }
 
 void Element::removeAttributeNS(const AtomicString& namespace_uri,
@@ -5544,9 +5545,9 @@ void Element::DidAddAttribute(const QualifiedName& name,
   DispatchSubtreeModifiedEvent();
 }
 
-void Element::DidModifyAttribute(const QualifiedName& name,
-                                 const AtomicString& old_value,
-                                 const AtomicString& new_value) {
+void Element::DidModifyAttribute(QualifiedName name,
+                                 AtomicString old_value,
+                                 AtomicString new_value) {
   if (name == html_names::kIdAttr)
     UpdateId(old_value, new_value);
   AttributeChanged(AttributeModificationParams(

third_party/blink/renderer/core/dom/element.h

@@ -162,12 +162,9 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
 
   // Passing g_null_atom as the second parameter removes the attribute when
   // calling either of these set methods.
-  void setAttribute(const QualifiedName&, const AtomicString& value);
-  void setAttribute(const QualifiedName&,
-                    const AtomicString& value,
-                    ExceptionState&);
-  void SetSynchronizedLazyAttribute(const QualifiedName&,
-                                    const AtomicString& value);
+  void setAttribute(const QualifiedName&, AtomicString value);
+  void setAttribute(const QualifiedName&, AtomicString value, ExceptionState&);
+  void SetSynchronizedLazyAttribute(const QualifiedName&, AtomicString value);
 
   void removeAttribute(const QualifiedName&);
 
@@ -233,7 +230,7 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
   void setAttribute(const AtomicString& name,
                     AtomicString value,
                     ExceptionState& exception_state = ASSERT_NO_EXCEPTION) {
-    SetAttributeHinted(name, WeakLowercaseIfNecessary(name), value,
+    SetAttributeHinted(name, WeakLowercaseIfNecessary(name), std::move(value),
                        exception_state);
   }
 
@@ -265,7 +262,7 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
   bool toggleAttribute(const AtomicString&, bool force, ExceptionState&);
 
   const AtomicString& GetIdAttribute() const;
-  void SetIdAttribute(const AtomicString&);
+  void SetIdAttribute(AtomicString);
 
   const AtomicString& GetNameAttribute() const;
   const AtomicString& GetClassAttribute() const;
@@ -1081,9 +1078,12 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
   void WillModifyAttribute(const QualifiedName&,
                            const AtomicString& old_value,
                            const AtomicString& new_value);
-  void DidModifyAttribute(const QualifiedName&,
-                          const AtomicString& old_value,
-                          const AtomicString& new_value);
+  // Take arguments by copy as DidModifyAttribute() may modify the
+  // ElementData().Attributes(). If the arguments are references to entries of
+  // that array, this would cause a UaF.
+  void DidModifyAttribute(QualifiedName,
+                          AtomicString old_value,
+                          AtomicString new_value);
   void DidRemoveAttribute(const QualifiedName&, const AtomicString& old_value);
 
   void SynchronizeAllAttributes() const;
@@ -1102,10 +1102,10 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
 
   void SetAttributeInternal(wtf_size_t index,
                             const QualifiedName&,
-                            const AtomicString& value,
+                            AtomicString value,
                             SynchronizationOfLazyAttribute);
   void AppendAttributeInternal(const QualifiedName&,
-                               const AtomicString& value,
+                               AtomicString value,
                                SynchronizationOfLazyAttribute);
   void RemoveAttributeInternal(wtf_size_t index,
                                SynchronizationOfLazyAttribute);
@@ -1129,7 +1129,7 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
       WTF::AtomicStringTable::WeakResult hint) const;
   void SetAttributeHinted(const AtomicString& name,
                           WTF::AtomicStringTable::WeakResult hint,
-                          const AtomicString& value,
+                          AtomicString value,
                           ExceptionState& = ASSERT_NO_EXCEPTION);
   void SetAttributeHinted(
       const AtomicString& name,
@@ -1329,8 +1329,8 @@ inline const AtomicString& Element::GetClassAttribute() const {
   return FastGetAttribute(html_names::kClassAttr);
 }
 
-inline void Element::SetIdAttribute(const AtomicString& value) {
-  setAttribute(html_names::kIdAttr, value);
+inline void Element::SetIdAttribute(AtomicString value) {
+  setAttribute(html_names::kIdAttr, std::move(value));
 }
 
 inline const SpaceSplitString& Element::ClassNames() const {