1176313 - Make Attr follow the spec again

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This is basically about relanding bug 1075702.  What needs to be done is a reversion of https://hg.mozilla.org/mozilla-central/rev/03682dcd82bb plus a resolution for bug 1165851, however we decide to resolve it.  That decision is going to need the telemetry we're adding in bug 1175031.

Comment 1

[Philip Jägenstedt [:foolip]]

The corresponding Blink bug, perhaps with a slightly wider scope, is https://code.google.com/p/chromium/issues/detail?id=502301

Comment 2

[Philip Jägenstedt [:foolip]]

The use counter data from Blink is now reliable as it has reached stable:
https://www.chromestatus.com/metrics/feature/timeline/popularity/844
https://www.chromestatus.com/metrics/feature/timeline/popularity/845

Counter 845 is a proxy for the most problematic cases and its usage doesn't rise to the 0.0001% required to be visibily non-zero in the graphs. Per counter 844, only around 0.0015% of page views should be affected at all, so this has a good chance of working out, I think.

Comment 3

[Philip Jägenstedt [:foolip]]

Spec bug: https://github.com/whatwg/dom/issues/72

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, the telemetry from bug 1175031 is not on the release channel yet, but for the nightly/aurora/beta channels it's showing absolutely no instances of non-HTML elements in an HTML document having setAttributeNode called on them with a non-lowercase Attr that was created with createAttribute.

Olli, are you ok with the "make createAttribute lowercase" approach given that?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

"make createAttribute lowercase in HTML document"? Based on telemetry that should be fine.
51/8020000 ~= 0.00064%

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Reland the changes to make Attr handling follow the spec again. This is a relanding of bug 1075702, effectively

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73ec5f840d9d

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8663244 [details] [diff] [review]
Reland the changes to make Attr handling follow the spec again.  This is a relanding of bug 1075702, effectively

> Attr::GetValue(nsAString& aValue)
> {
>   Element* element = GetElement();
>   if (element) {
>-    nsCOMPtr<nsIAtom> nameAtom = GetNameAtom(element);
>+    nsCOMPtr<nsIAtom> nameAtom = mNodeInfo->NameAtom();
>     element->GetAttr(mNodeInfo->NamespaceID(), nameAtom, aValue);
>   }
>   else {
>     aValue = mValue;
>   }
> 
>   return NS_OK;
> }
>@@ -198,17 +198,17 @@ void
> Attr::SetValue(const nsAString& aValue, ErrorResult& aRv)
> {
>   Element* element = GetElement();
>   if (!element) {
>     mValue = aValue;
>     return;
>   }
> 
>-  nsCOMPtr<nsIAtom> nameAtom = GetNameAtom(element);
>+  nsCOMPtr<nsIAtom> nameAtom = mNodeInfo->NameAtom();
So we don't have any callers for GetNameAtom anymore, want to remove it?



>-  return Attributes()->RemoveNamedItem(aAttribute.NodeName(), aError);
>+  nsAutoString nameSpaceURI;
>+  aAttribute.NodeInfo()->GetNamespaceURI(nameSpaceURI);
>+  return Attributes()->RemoveNamedItemNS(nameSpaceURI, aAttribute.NodeInfo()->LocalName(), aError);
A bit annoying that we don't have nsIAtom* taking methods for this
>+++ b/dom/base/nsDOMAttributeMap.cpp
>@@ -295,72 +295,86 @@ nsDOMAttributeMap::SetNamedItemInternal(
>     if (aError.Failed()) {
>       return nullptr;
>     }
> 
>     NS_ASSERTION(adoptedNode == &aAttr, "Uh, adopt node changed nodes?");
>   }
> 
>   // Get nodeinfo and preexisting attribute (if it exists)
>-  nsAutoString name;
>-  nsRefPtr<mozilla::dom::NodeInfo> ni;
>+  nsRefPtr<NodeInfo> oldNi;
>+
>+  if (!aWithNS) {
>+    nsAutoString name;
>+    aAttr.GetName(name);
>+    oldNi = mContent->GetExistingAttrNameFromQName(name);
>+  }
>+  else {
Nit, } else {


>+    if (oldAttr) {
>+      attr = RemoveNamedItem(oldNi, aError);
>+      NS_ASSERTION(attr->NodeInfo()->NameAndNamespaceEquals(oldNi),
>+        "RemoveNamedItem() called, attr->NodeInfo() should be equal to oldNi!");
Not really about this bug, but I wonder what happens if mutation event listener does something mean here, like
move the to-be-set-attribute to some other element...


>   // Add the new attribute to the attribute map before updating
>   // its value in the element. @see bug 364413.
>   nsAttrKey attrkey(ni->NamespaceID(), ni->NameAtom());
>   mAttributeCache.Put(attrkey, &aAttr);
>   aAttr.SetMap(this);
... looks like it will end up being in both mAttributeCaches (current and new element), but
attr.ownerElement will point to this element here.
Sure, not about this bug, but perhaps we should add a check after RemoveNamedItem that
aAttr's GetMap is still null (or 'this') and that its ownerDocument is still the right one.
Feel free to add such check in this bug or file a followup

Comment 9

[Boris Zbarsky [:bzbarsky]]

> So we don't have any callers for GetNameAtom anymore, want to remove it?

Done.

> A bit annoying that we don't have nsIAtom* taking methods for this

I suppose we could add something taking namespace id and localname atom.  Seems fairly edge-case.

> Nit, } else {

Fixed.

> Not really about this bug, but I wonder what happens if mutation event listener does
> something mean here, like move the to-be-set-attribute to some other element...

Seems like a spec issue, except the spec pretends mutation events don't exist.  :(

Anyway, I added this inside that block:

+
+      // That might have run mutation event listeners, so re-verify
+      // our assumptions.
+      nsDOMAttributeMap* newOwner = aAttr.GetMap();
+      if (newOwner) {
+        if (newOwner == this) {
+          // OK, we're just done here.
+          return attr.forget();
+        }
+
+        // The attr we're trying to set got stuck on some other
+        // element.  Just throw, for lack of anything better to do.
+        aError.Throw(NS_ERROR_DOM_INUSE_ATTRIBUTE_ERR);
+        return nullptr;
+      } else if (mContent->OwnerDoc() != aAttr.OwnerDoc()) {
+        // Got moved into a different document, boo.
+        aError.Throw(NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
+        return nullptr;
+      }

Let me know if you think it's off the rails somehow.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40f004f4651b

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/40f004f4651b

Comment 12

[Kohei Yoshino [:kohei]]

Posted the site compat doc: https://www.fxsitecompat.com/en-US/docs/2015/document-createattribute-now-lowercases-the-input/

Comment 13

[Jean-Yves Perrier [:teoli]]

Updated:
https://developer.mozilla.org/en-US/docs/Web/API/Document/createAttribute
and
https://developer.mozilla.org/en-US/Firefox/Releases/44#DOM_HTML_DOM