Make Attr follow the spec again

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
4 years ago
10 days ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla44
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox44 fixed)

Details

Attachments

(1 attachment)

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.
The corresponding Blink bug, perhaps with a slightly wider scope, is https://code.google.com/p/chromium/issues/detail?id=502301
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.
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?
Flags: needinfo?(bugs)
"make createAttribute lowercase in HTML document"? Based on telemetry that should be fine.
51/8020000 ~= 0.00064%
Flags: needinfo?(bugs)
Depends on: 1205364
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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
Attachment #8663244 - Flags: review?(bugs) → review+
> 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.
https://hg.mozilla.org/mozilla-central/rev/40f004f4651b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.