Revert "Mostly remove refcount thrashes of value in setAttribute."

This reverts commit 6a1fac8e.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> 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/+/2369348
> Reviewed-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}

TBR=ajwong@chromium.org,jbroman@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1083392,1121429
Change-Id: Ia42214b62e1efc218afb055fcc60bea52039587c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2376705Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801763}

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, AtomicString,
+  PerformAttributeSetCEReactionsReflect<IDLStringV2, const AtomicString&,
                                         &Element::setAttribute>(
       info, content_attribute, interface_name, attribute_name);
 }
@@ -406,7 +406,8 @@ void PerformAttributeSetCEReactionsReflectTypeStringLegacyNullToEmptyString(
     const char* interface_name,
     const char* attribute_name) {
   PerformAttributeSetCEReactionsReflect<IDLStringTreatNullAsEmptyStringV2,
-                                        AtomicString, &Element::setAttribute>(
+                                        const AtomicString&,
+                                        &Element::setAttribute>(
       info, content_attribute, interface_name, attribute_name);
 }
 
@@ -415,8 +416,8 @@ void PerformAttributeSetCEReactionsReflectTypeStringOrNull(
     const QualifiedName& content_attribute,
     const char* interface_name,
     const char* attribute_name) {
-  PerformAttributeSetCEReactionsReflect<IDLNullable<IDLStringV2>, AtomicString,
-                                        &Element::setAttribute>(
+  PerformAttributeSetCEReactionsReflect<
+      IDLNullable<IDLStringV2>, const 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, AtomicString value)
-      : name_(name), value_(std::move(value)) {}
+  Attribute(const QualifiedName& name, const AtomicString& value)
+      : name_(name), value_(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,10 +56,7 @@ class Attribute {
   bool Matches(const QualifiedName&) const;
   bool MatchesCaseInsensitive(const QualifiedName&) const;
 
-  void SetValue(AtomicString value) { value_ = std::move(value); }
-  AtomicString ExchangeValue(AtomicString value) {
-    return std::exchange(value_, std::move(value));
-  }
+  void SetValue(const AtomicString& value) { value_ = 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,15 +160,13 @@ class MutableAttributeCollection
             attributes) {}
 
   // These functions do no error/duplicate checking.
-  const AtomicString& Append(const QualifiedName&, AtomicString value);
+  void Append(const QualifiedName&, const AtomicString& value);
   void Remove(unsigned index);
 };
 
-inline const AtomicString& MutableAttributeCollection::Append(
-    const QualifiedName& name,
-    AtomicString value) {
-  attributes_.emplace_back(name, std::move(value));
-  return attributes_.back().Value();
+inline void MutableAttributeCollection::Append(const QualifiedName& name,
+                                               const AtomicString& value) {
+  attributes_.push_back(Attribute(name, 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, AtomicString value) {
+void Element::setAttribute(const QualifiedName& name,
+                           const AtomicString& value) {
   SynchronizeAttribute(name);
   wtf_size_t index = GetElementData()
                          ? GetElementData()->Attributes().FindIndex(name)
                          : kNotFound;
-  SetAttributeInternal(index, name, std::move(value),
+  SetAttributeInternal(index, name, value,
                        kNotInSynchronizationOfLazyAttribute);
 }
 
 void Element::setAttribute(const QualifiedName& name,
-                           AtomicString value,
+                           const AtomicString& value,
                            ExceptionState& exception_state) {
   SynchronizeAttribute(name);
   wtf_size_t index = GetElementData()
                          ? GetElementData()->Attributes().FindIndex(name)
                          : kNotFound;
 
-  AtomicString trusted_value(TrustedTypesCheckFor(
-      ExpectedTrustedTypeForAttribute(name), std::move(value),
-      GetExecutionContext(), exception_state));
+  AtomicString trusted_value(
+      TrustedTypesCheckFor(ExpectedTrustedTypeForAttribute(name), value,
+                           GetExecutionContext(), exception_state));
   if (exception_state.HadException())
     return;
 
-  SetAttributeInternal(index, name, std::move(trusted_value),
+  SetAttributeInternal(index, name, trusted_value,
                        kNotInSynchronizationOfLazyAttribute);
 }
 
 void Element::SetSynchronizedLazyAttribute(const QualifiedName& name,
-                                           AtomicString value) {
+                                           const AtomicString& value) {
   wtf_size_t index = GetElementData()
                          ? GetElementData()->Attributes().FindIndex(name)
                          : kNotFound;
-  SetAttributeInternal(index, name, std::move(value),
-                       kInSynchronizationOfLazyAttribute);
+  SetAttributeInternal(index, name, value, kInSynchronizationOfLazyAttribute);
 }
 
 void Element::SetAttributeHinted(const AtomicString& local_name,
                                  WTF::AtomicStringTable::WeakResult hint,
-                                 AtomicString value,
+                                 const 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, std::move(trusted_value),
+  SetAttributeInternal(index, q_name, trusted_value,
                        kNotInSynchronizationOfLazyAttribute);
 }
 
@@ -204,14 +204,14 @@ void Element::SetAttributeHinted(
       GetExecutionContext(), exception_state));
   if (exception_state.HadException())
     return;
-  SetAttributeInternal(index, q_name, std::move(value),
+  SetAttributeInternal(index, q_name, value,
                        kNotInSynchronizationOfLazyAttribute);
 }
 
 ALWAYS_INLINE void Element::SetAttributeInternal(
     wtf_size_t index,
     const QualifiedName& name,
-    AtomicString new_value,
+    const AtomicString& new_value,
     SynchronizationOfLazyAttribute in_synchronization_of_lazy_attribute) {
   if (new_value.IsNull()) {
     if (index != kNotFound)
@@ -220,32 +220,25 @@ ALWAYS_INLINE void Element::SetAttributeInternal(
   }
 
   if (index == kNotFound) {
-    AppendAttributeInternal(name, std::move(new_value),
+    AppendAttributeInternal(name, 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.GetName(),
-                        existing_attribute.Value(), new_value);
+    WillModifyAttribute(existing_attribute_name, existing_attribute_value,
+                        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 (new_value != existing_attribute_value)
+    EnsureUniqueElementData().Attributes().at(index).SetValue(new_value);
   if (!in_synchronization_of_lazy_attribute) {
-    DidModifyAttribute(existing_attribute.GetName(), *old_val_ptr,
-                       *new_val_ptr);
+    DidModifyAttribute(existing_attribute_name, existing_attribute_value,
+                       new_value);
   }
 }
 
@@ -308,7 +301,7 @@ Attr* Element::setAttributeNode(Attr* attr_node,
     }
   }
 
-  SetAttributeInternal(index, attr_node->GetQualifiedName(), std::move(value),
+  SetAttributeInternal(index, attr_node->GetQualifiedName(), 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, std::move(value));
+  setAttribute(parsed_name, value);
 }
 
 void Element::RemoveAttributeInternal(
@@ -3828,14 +3828,13 @@ void Element::RemoveAttributeInternal(
 
 void Element::AppendAttributeInternal(
     const QualifiedName& name,
-    AtomicString value,
+    const AtomicString& value,
     SynchronizationOfLazyAttribute in_synchronization_of_lazy_attribute) {
   if (!in_synchronization_of_lazy_attribute)
     WillModifyAttribute(name, g_null_atom, value);
-  const AtomicString& stored_value =
-      EnsureUniqueElementData().Attributes().Append(name, std::move(value));
+  EnsureUniqueElementData().Attributes().Append(name, value);
   if (!in_synchronization_of_lazy_attribute)
-    DidAddAttribute(name, stored_value);
+    DidAddAttribute(name, value);
 }
 
 void Element::removeAttributeNS(const AtomicString& namespace_uri,
@@ -5545,9 +5544,9 @@ void Element::DidAddAttribute(const QualifiedName& name,
   DispatchSubtreeModifiedEvent();
 }
 
-void Element::DidModifyAttribute(QualifiedName name,
-                                 AtomicString old_value,
-                                 AtomicString new_value) {
+void Element::DidModifyAttribute(const QualifiedName& name,
+                                 const AtomicString& old_value,
+                                 const AtomicString& new_value) {
   if (name == html_names::kIdAttr)
     UpdateId(old_value, new_value);
   AttributeChanged(AttributeModificationParams(

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

@@ -162,9 +162,12 @@ 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&, AtomicString value);
-  void setAttribute(const QualifiedName&, AtomicString value, ExceptionState&);
-  void SetSynchronizedLazyAttribute(const QualifiedName&, AtomicString value);
+  void setAttribute(const QualifiedName&, const AtomicString& value);
+  void setAttribute(const QualifiedName&,
+                    const AtomicString& value,
+                    ExceptionState&);
+  void SetSynchronizedLazyAttribute(const QualifiedName&,
+                                    const AtomicString& value);
 
   void removeAttribute(const QualifiedName&);
 
@@ -230,7 +233,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), std::move(value),
+    SetAttributeHinted(name, WeakLowercaseIfNecessary(name), value,
                        exception_state);
   }
 
@@ -262,7 +265,7 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
   bool toggleAttribute(const AtomicString&, bool force, ExceptionState&);
 
   const AtomicString& GetIdAttribute() const;
-  void SetIdAttribute(AtomicString);
+  void SetIdAttribute(const AtomicString&);
 
   const AtomicString& GetNameAttribute() const;
   const AtomicString& GetClassAttribute() const;
@@ -1078,12 +1081,9 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
   void WillModifyAttribute(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 DidModifyAttribute(const QualifiedName&,
+                          const AtomicString& old_value,
+                          const 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&,
-                            AtomicString value,
+                            const AtomicString& value,
                             SynchronizationOfLazyAttribute);
   void AppendAttributeInternal(const QualifiedName&,
-                               AtomicString value,
+                               const 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,
-                          AtomicString value,
+                          const 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(AtomicString value) {
-  setAttribute(html_names::kIdAttr, std::move(value));
+inline void Element::SetIdAttribute(const AtomicString& value) {
+  setAttribute(html_names::kIdAttr, value);
 }
 
 inline const SpaceSplitString& Element::ClassNames() const {