Bug 1365092 - Move side effects of SetAttr and ParseAttribute to BeforeSetAttr and AfterSetAttr

* Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions
* Moves side effects of HTMLAnchorElement's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions
* Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions
* Moves side effects of SetAttr, UnsetAttr, and ParseAttribute functions to BeforeSetAttr and AfterSetAttr

Tag #1375

41 files changed, 983 insertions, 956 deletions

diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
index 11ad042a0e..96ccfd52a7 100644
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -2476,7 +2476,7 @@ Element::SetAttr(int32_t aNamespaceID, nsIAtom* aName,
 
   if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify,
                              oldValue, &modType, &hasListeners, &oldValueSet)) {
-    return NS_OK;
+    return OnAttrSetButNotChanged(aNamespaceID, aName, value, aNotify);
   }
 
   nsAttrValue attrValue;
@@ -2494,20 +2494,21 @@ Element::SetAttr(int32_t aNamespaceID, nsIAtom* aName,
                                      preparsedAttrValue);
   }
 
-  nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
   // Hold a script blocker while calling ParseAttribute since that can call
   // out to id-observers
   nsIDocument* document = GetComposedDoc();
   mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, aNotify);
 
-  // Even the value was pre-parsed, we still need to call ParseAttribute because
-  // it can have side effects.
-  if (!ParseAttribute(aNamespaceID, aName, aValue, attrValue)) {
+  nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  if (!preparsedAttrValue &&
+      !ParseAttribute(aNamespaceID, aName, aValue, attrValue)) {
     attrValue.SetTo(aValue);
   }
 
+  PreIdMaybeChange(aNamespaceID, aName, &value);
+
   return SetAttrAndNotify(aNamespaceID, aName, aPrefix,
                           oldValueSet ? &oldValue : nullptr,
                           attrValue, modType, hasListeners, aNotify,
@@ -2538,7 +2539,7 @@ Element::SetParsedAttr(int32_t aNamespaceID, nsIAtom* aName,
 
   if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify,
                              oldValue, &modType, &hasListeners, &oldValueSet)) {
-    return NS_OK;
+    return OnAttrSetButNotChanged(aNamespaceID, aName, value, aNotify);
   }
 
   if (aNotify) {
@@ -2549,6 +2550,8 @@ Element::SetParsedAttr(int32_t aNamespaceID, nsIAtom* aName,
   nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  PreIdMaybeChange(aNamespaceID, aName, &value);
+
   nsIDocument* document = GetComposedDoc();
   mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, aNotify);
   return SetAttrAndNotify(aNamespaceID, aName, aPrefix,
@@ -2608,6 +2611,8 @@ Element::SetAttrAndNotify(int32_t aNamespaceID,
   }
   NS_ENSURE_SUCCESS(rv, rv);
 
+  PostIdMaybeChange(aNamespaceID, aName, &valueForAfterSetAttr);
+
   // If the old value owns its own data, we know it is OK to keep using it.
   // oldValue will be null if there was no previously set value
   const nsAttrValue* oldValue;
@@ -2720,22 +2725,16 @@ Element::ParseAttribute(int32_t aNamespaceID,
                         nsAttrValue& aResult)
 {
   if (aNamespaceID == kNameSpaceID_None) {
-    if (aAttribute == nsGkAtoms::_class) {
-      SetMayHaveClass();
-      // Result should have been preparsed above.
-      return true;
-    }
+    MOZ_ASSERT(aAttribute != nsGkAtoms::_class,
+               "The class attribute should be preparsed and therefore should "
+               "never be passed to Element::ParseAttribute");
     if (aAttribute == nsGkAtoms::id) {
       // Store id as an atom.  id="" means that the element has no id,
       // not that it has an emptystring as the id.
-      RemoveFromIdTable();
       if (aValue.IsEmpty()) {
-        ClearHasID();
         return false;
       }
       aResult.ParseAtom(aValue);
-      SetHasID();
-      AddToIdTable(aResult.GetAtomValue());
       return true;
     }
   }
@@ -2754,6 +2753,57 @@ Element::SetAndSwapMappedAttribute(nsIDocument* aDocument,
   return false;
 }
 
+nsresult
+Element::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                       const nsAttrValueOrString* aValue, bool aNotify)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::_class) {
+      if (aValue) {
+        // Note: This flag is asymmetrical. It is never unset and isn't exact.
+        // If it is ever made to be exact, we probably need to handle this
+        // similarly to how ids are handled in PreIdMaybeChange and
+        // PostIdMaybeChange.
+        SetMayHaveClass();
+      }
+    }
+  }
+
+  return NS_OK;
+}
+
+void
+Element::PreIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName,
+                          const nsAttrValueOrString* aValue)
+{
+  if (aNamespaceID != kNameSpaceID_None || aName != nsGkAtoms::id) {
+    return;
+  }
+  RemoveFromIdTable();
+
+  return;
+}
+
+void
+Element::PostIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName,
+                           const nsAttrValue* aValue)
+{
+  if (aNamespaceID != kNameSpaceID_None || aName != nsGkAtoms::id) {
+    return;
+  }
+
+  // id="" means that the element has no id, not that it has an empty
+  // string as the id.
+  if (aValue && !aValue->IsEmptyString()) {
+    SetHasID();
+    AddToIdTable(aValue->GetAtomValue());
+  } else {
+    ClearHasID();
+  }
+
+  return;
+}
+
 EventListenerManager*
 Element::GetEventListenerManagerForAttr(nsIAtom* aAttrName,
                                         bool* aDefer)
@@ -2847,6 +2897,8 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                          NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
                                          this);
 
+  PreIdMaybeChange(aNameSpaceID, aName, nullptr);
+
   // Grab the attr node if needed before we remove it from the attr map
   RefPtr<Attr> attrNode;
   if (hasMutationListeners) {
@@ -2865,12 +2917,6 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
   // react to unexpected attribute changes.
   nsMutationGuard::DidMutate();
 
-  if (aName == nsGkAtoms::id && aNameSpaceID == kNameSpaceID_None) {
-    // Have to do this before clearing flag. See RemoveFromIdTable
-    RemoveFromIdTable();
-    ClearHasID();
-  }
-
   bool hadValidDir = false;
   bool hadDirAuto = false;
 
@@ -2883,6 +2929,8 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
   rv = mAttrsAndChildren.RemoveAttrAt(index, oldValue);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  PostIdMaybeChange(aNameSpaceID, aName, nullptr);
+
   if (document || HasFlag(NODE_FORCE_XBL_BINDINGS)) {
     RefPtr<nsXBLBinding> binding = GetXBLBinding();
     if (binding) {
diff --git a/dom/base/Element.h b/dom/base/Element.h
index 6f60c9ad0d..df0dbcc45e 100644
--- a/dom/base/Element.h
+++ b/dom/base/Element.h
@@ -523,6 +523,7 @@ public:
   already_AddRefed<mozilla::dom::NodeInfo>
   GetExistingAttrNameFromQName(const nsAString& aStr) const;
 
+  MOZ_ALWAYS_INLINE  // Avoid a crashy hook from Avast 10 Beta (Bug 1058131)
   nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                    const nsAString& aValue, bool aNotify)
   {
@@ -1378,14 +1379,9 @@ protected:
    *        will be null.
    * @param aNotify Whether we plan to notify document observers.
    */
-  // Note that this is inlined so that when subclasses call it it gets
-  // inlined.  Those calls don't go through a vtable.
   virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
                                  const nsAttrValueOrString* aValue,
-                                 bool aNotify)
-  {
-    return NS_OK;
-  }
+                                 bool aNotify);
 
   /**
    * Hook that is called by Element::SetAttr to allow subclasses to
@@ -1413,6 +1409,64 @@ protected:
   }
 
   /**
+   * This function shall be called just before the id attribute changes. It will
+   * be called after BeforeSetAttr. If the attribute being changed is not the id
+   * attribute, this function does nothing. Otherwise, it will remove the old id
+   * from the document's id cache.
+   *
+   * This must happen after BeforeSetAttr (rather than during) because the
+   * the subclasses' calls to BeforeSetAttr may notify on state changes. If they
+   * incorrectly determine whether the element had an id, the element may not be
+   * restyled properly.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aValue the new id value. Will be null if the id is being unset.
+   */
+  void PreIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName,
+                        const nsAttrValueOrString* aValue);
+
+  /**
+   * This function shall be called just after the id attribute changes. It will
+   * be called before AfterSetAttr. If the attribute being changed is not the id
+   * attribute, this function does nothing. Otherwise, it will add the new id to
+   * the document's id cache and properly set the ElementHasID flag.
+   *
+   * This must happen before AfterSetAttr (rather than during) because the
+   * the subclasses' calls to AfterSetAttr may notify on state changes. If they
+   * incorrectly determine whether the element now has an id, the element may
+   * not be restyled properly.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aValue the new id value. Will be null if the id is being unset.
+   */
+  void PostIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName,
+                         const nsAttrValue* aValue);
+
+  /**
+   * Usually, setting an attribute to the value that it already has results in
+   * no action. However, in some cases, setting an attribute to its current
+   * value should have the effect of, for example, forcing a reload of
+   * network data. To address that, this function will be called in this
+   * situation to allow the handling of such a case.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aValue the value it's being set to represented as either a string or
+   *        a parsed nsAttrValue.
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  // Note that this is inlined so that when subclasses call it it gets
+  // inlined.  Those calls don't go through a vtable.
+  virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify)
+  {
+    return NS_OK;
+  }
+
+  /**
    * Hook to allow subclasses to produce a different EventListenerManager if
    * needed for attachment of attribute-defined handlers
    */
diff --git a/dom/base/nsAttrValueOrString.h b/dom/base/nsAttrValueOrString.h
index 8e6c069532..1a62a84287 100644
--- a/dom/base/nsAttrValueOrString.h
+++ b/dom/base/nsAttrValueOrString.h
@@ -78,6 +78,20 @@ public:
     return aOther.EqualsAsStrings(*mAttrValue);
   }
 
+  /*
+   * Returns true if the value stored is empty
+   */
+  bool IsEmpty() const
+  {
+    if (mStringPtr) {
+      return mStringPtr->IsEmpty();
+    }
+    if (mAttrValue) {
+      return mAttrValue->IsEmptyString();
+    }
+    return true;
+  }
+
 protected:
   const nsAttrValue*       mAttrValue;
   mutable const nsAString* mStringPtr;
diff --git a/dom/base/nsStyledElement.cpp b/dom/base/nsStyledElement.cpp
index e3d8dfe574..8cd448d47e 100644
--- a/dom/base/nsStyledElement.cpp
+++ b/dom/base/nsStyledElement.cpp
@@ -40,7 +40,6 @@ nsStyledElement::ParseAttribute(int32_t aNamespaceID,
                                 nsAttrValue& aResult)
 {
   if (aAttribute == nsGkAtoms::style && aNamespaceID == kNameSpaceID_None) {
-    SetMayHaveStyle();
     ParseStyleAttribute(aValue, aResult, false);
     return true;
   }
@@ -50,6 +49,22 @@ nsStyledElement::ParseAttribute(int32_t aNamespaceID,
 }
 
 nsresult
+nsStyledElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                               const nsAttrValueOrString* aValue, bool aNotify)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::style) {
+      if (aValue) {
+        SetMayHaveStyle();
+      }
+    }
+  }
+
+  return nsStyledElementBase::BeforeSetAttr(aNamespaceID, aName, aValue,
+                                            aNotify);
+}
+
+nsresult
 nsStyledElement::SetInlineStyleDeclaration(DeclarationBlock* aDeclaration,
                                            const nsAString* aSerialized,
                                            bool aNotify)
diff --git a/dom/base/nsStyledElement.h b/dom/base/nsStyledElement.h
index c4894d87f1..cb6dfd54b9 100644
--- a/dom/base/nsStyledElement.h
+++ b/dom/base/nsStyledElement.h
@@ -80,6 +80,10 @@ protected:
    * document.
    */
   nsresult  ReparseStyleAttribute(bool aForceInDataDoc);
+
+  virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                 const nsAttrValueOrString* aValue,
+                                 bool aNotify) override;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(nsStyledElement, NS_STYLED_ELEMENT_IID)
diff --git a/dom/html/HTMLAnchorElement.cpp b/dom/html/HTMLAnchorElement.cpp
index 501d6e3d06..0759af3395 100644
--- a/dom/html/HTMLAnchorElement.cpp
+++ b/dom/html/HTMLAnchorElement.cpp
@@ -14,6 +14,7 @@
 #include "nsContentUtils.h"
 #include "nsGkAtoms.h"
 #include "nsHTMLDNSPrefetch.h"
+#include "nsAttrValueOrString.h"
 #include "nsIDocument.h"
 #include "nsIPresShell.h"
 #include "nsPresContext.h"
@@ -372,84 +373,37 @@ HTMLAnchorElement::GetHrefURI() const
 }
 
 nsresult
-HTMLAnchorElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify)
+HTMLAnchorElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                 const nsAttrValueOrString* aValue,
+                                 bool aNotify)
 {
-  bool reset = false;
-  if (aName == nsGkAtoms::href && kNameSpaceID_None == aNameSpaceID) {
-    // If we do not have a cached URI, we have some value here so we must reset
-    // our link state after calling the parent.
-    if (!Link::HasCachedURI()) {
-      reset = true;
-    }
-    // However, if we have a cached URI, we'll want to see if the value changed.
-    else {
-      nsAutoString val;
-      GetHref(val);
-      if (!val.Equals(aValue)) {
-        reset = true;
-      }
-    }
-    if (reset) {
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::href) {
       CancelDNSPrefetch(HTML_ANCHOR_DNS_PREFETCH_DEFERRED,
                         HTML_ANCHOR_DNS_PREFETCH_REQUESTED);
     }
   }
 
-  nsresult rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix,
-                                              aValue, aNotify);
-
-  // The ordering of the parent class's SetAttr call and Link::ResetLinkState
-  // is important here!  The attribute is not set until SetAttr returns, and
-  // we will need the updated attribute value because notifying the document
-  // that content states have changed will call IntrinsicState, which will try
-  // to get updated information about the visitedness from Link.
-  if (reset) {
-    Link::ResetLinkState(!!aNotify, true);
-    if (IsInComposedDoc()) {
-      TryDNSPrefetch();
-    }
-  }
-
-  return rv;
+  return nsGenericHTMLElement::BeforeSetAttr(aNamespaceID, aName, aValue,
+                                             aNotify);
 }
 
 nsresult
-HTMLAnchorElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify)
+HTMLAnchorElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue, bool aNotify)
 {
-  bool href =
-    (aAttribute == nsGkAtoms::href && kNameSpaceID_None == aNameSpaceID);
-
-  if (href) {
-    CancelDNSPrefetch(HTML_ANCHOR_DNS_PREFETCH_DEFERRED,
-                      HTML_ANCHOR_DNS_PREFETCH_REQUESTED);
-  }
-
-  nsresult rv = nsGenericHTMLElement::UnsetAttr(aNameSpaceID, aAttribute,
-                                                aNotify);
-
-  // The ordering of the parent class's UnsetAttr call and Link::ResetLinkState
-  // is important here!  The attribute is not unset until UnsetAttr returns, and
-  // we will need the updated attribute value because notifying the document
-  // that content states have changed will call IntrinsicState, which will try
-  // to get updated information about the visitedness from Link.
-  if (href) {
-    Link::ResetLinkState(!!aNotify, false);
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::href) {
+      Link::ResetLinkState(aNotify, !!aValue);
+      if (aValue && IsInComposedDoc()) {
+        TryDNSPrefetch();
+      }
+    }
   }
 
-  return rv;
-}
-
-bool
-HTMLAnchorElement::ParseAttribute(int32_t aNamespaceID,
-                                  nsIAtom* aAttribute,
-                                  const nsAString& aValue,
-                                  nsAttrValue& aResult)
-{
-  return nsGenericHTMLElement::ParseAttribute(aNamespaceID, aAttribute, aValue,
-                                              aResult);
+  return nsGenericHTMLElement::AfterSetAttr(aNamespaceID, aName,
+                                            aValue, aOldValue, aNotify);
 }
 
 EventStates
diff --git a/dom/html/HTMLAnchorElement.h b/dom/html/HTMLAnchorElement.h
index 027d8d689a..0875559644 100644
--- a/dom/html/HTMLAnchorElement.h
+++ b/dom/html/HTMLAnchorElement.h
@@ -66,20 +66,13 @@ public:
   virtual void GetLinkTarget(nsAString& aTarget) override;
   virtual already_AddRefed<nsIURI> GetHrefURI() const override;
 
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) override;
-  virtual bool ParseAttribute(int32_t aNamespaceID,
-                                nsIAtom* aAttribute,
-                                const nsAString& aValue,
-                                nsAttrValue& aResult) override;
+  virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                 const nsAttrValueOrString* aValue,
+                                 bool aNotify) override;
+  virtual nsresult AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
 
   virtual nsresult Clone(mozilla::dom::NodeInfo *aNodeInfo, nsINode **aResult) const override;
 
diff --git a/dom/html/HTMLAreaElement.cpp b/dom/html/HTMLAreaElement.cpp
index 213d4831dd..4c85774f45 100644
--- a/dom/html/HTMLAreaElement.cpp
+++ b/dom/html/HTMLAreaElement.cpp
@@ -153,42 +153,22 @@ HTMLAreaElement::UnbindFromTree(bool aDeep, bool aNullParent)
 }
 
 nsresult
-HTMLAreaElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                         nsIAtom* aPrefix, const nsAString& aValue,
-                         bool aNotify)
-{
-  nsresult rv =
-    nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix, aValue, aNotify);
-
-  // The ordering of the parent class's SetAttr call and Link::ResetLinkState
-  // is important here!  The attribute is not set until SetAttr returns, and
-  // we will need the updated attribute value because notifying the document
-  // that content states have changed will call IntrinsicState, which will try
-  // to get updated information about the visitedness from Link.
-  if (aName == nsGkAtoms::href && aNameSpaceID == kNameSpaceID_None) {
-    Link::ResetLinkState(!!aNotify, true);
+HTMLAreaElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                              const nsAttrValue* aValue,
+                              const nsAttrValue* aOldValue, bool aNotify)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    // This must happen after the attribute is set. We will need the updated
+    // attribute value because notifying the document that content states have
+    // changed will call IntrinsicState, which will try to get updated
+    // information about the visitedness from Link.
+    if (aName == nsGkAtoms::href) {
+      Link::ResetLinkState(aNotify, !!aValue);
+    }
   }
 
-  return rv;
-}
-
-nsresult
-HTMLAreaElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                           bool aNotify)
-{
-  nsresult rv = nsGenericHTMLElement::UnsetAttr(aNameSpaceID, aAttribute,
-                                                aNotify);
-
-  // The ordering of the parent class's UnsetAttr call and Link::ResetLinkState
-  // is important here!  The attribute is not unset until UnsetAttr returns, and
-  // we will need the updated attribute value because notifying the document
-  // that content states have changed will call IntrinsicState, which will try
-  // to get updated information about the visitedness from Link.
-  if (aAttribute == nsGkAtoms::href && kNameSpaceID_None == aNameSpaceID) {
-    Link::ResetLinkState(!!aNotify, false);
-  }
-
-  return rv;
+  return nsGenericHTMLElement::AfterSetAttr(aNamespaceID, aName, aValue,
+                                            aOldValue, aNotify);
 }
 
 #define IMPL_URI_PART(_part)                                 \
diff --git a/dom/html/HTMLAreaElement.h b/dom/html/HTMLAreaElement.h
index c2ed7a9283..d408934c99 100644
--- a/dom/html/HTMLAreaElement.h
+++ b/dom/html/HTMLAreaElement.h
@@ -56,16 +56,6 @@ public:
                               bool aCompileEventHandlers) override;
   virtual void UnbindFromTree(bool aDeep = true,
                               bool aNullParent = true) override;
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) override;
 
   virtual nsresult Clone(mozilla::dom::NodeInfo* aNodeInfo, nsINode** aResult) const override;
 
@@ -186,6 +176,11 @@ protected:
 
   virtual JSObject* WrapNode(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
 
+  virtual nsresult AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
+
   RefPtr<nsDOMTokenList > mRelList;
 };
 
diff --git a/dom/html/HTMLCanvasElement.cpp b/dom/html/HTMLCanvasElement.cpp
index 803c60e8ac..ba1fbedbb0 100644
--- a/dom/html/HTMLCanvasElement.cpp
+++ b/dom/html/HTMLCanvasElement.cpp
@@ -446,38 +446,37 @@ NS_IMPL_UINT_ATTR_DEFAULT_VALUE(HTMLCanvasElement, Height, height, DEFAULT_CANVA
 NS_IMPL_BOOL_ATTR(HTMLCanvasElement, MozOpaque, moz_opaque)
 
 nsresult
-HTMLCanvasElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify)
-{
-  nsresult rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix, aValue,
-                                              aNotify);
-  if (NS_SUCCEEDED(rv) && mCurrentContext &&
-      aNameSpaceID == kNameSpaceID_None &&
-      (aName == nsGkAtoms::width || aName == nsGkAtoms::height || aName == nsGkAtoms::moz_opaque))
-  {
-    ErrorResult dummy;
-    rv = UpdateContext(nullptr, JS::NullHandleValue, dummy);
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
+HTMLCanvasElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue, bool aNotify)
+{
+  AfterMaybeChangeAttr(aNamespaceID, aName, aNotify);
 
-  return rv;
+  return nsGenericHTMLElement::AfterSetAttr(aNamespaceID, aName, aValue,
+                                            aOldValue, aNotify);
 }
 
 nsresult
-HTMLCanvasElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                             bool aNotify)
+HTMLCanvasElement::OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify)
 {
-  nsresult rv = nsGenericHTMLElement::UnsetAttr(aNameSpaceID, aName, aNotify);
-  if (NS_SUCCEEDED(rv) && mCurrentContext &&
-      aNameSpaceID == kNameSpaceID_None &&
-      (aName == nsGkAtoms::width || aName == nsGkAtoms::height || aName == nsGkAtoms::moz_opaque))
-  {
+  AfterMaybeChangeAttr(aNamespaceID, aName, aNotify);
+
+  return nsGenericHTMLElement::OnAttrSetButNotChanged(aNamespaceID, aName,
+                                                      aValue, aNotify);
+}
+
+void
+HTMLCanvasElement::AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                        bool aNotify)
+{
+  if (mCurrentContext && aNamespaceID == kNameSpaceID_None &&
+      (aName == nsGkAtoms::width || aName == nsGkAtoms::height ||
+       aName == nsGkAtoms::moz_opaque)) {
     ErrorResult dummy;
-    rv = UpdateContext(nullptr, JS::NullHandleValue, dummy);
-    NS_ENSURE_SUCCESS(rv, rv);
+    UpdateContext(nullptr, JS::NullHandleValue, dummy);
   }
-  return rv;
 }
 
 void
diff --git a/dom/html/HTMLCanvasElement.h b/dom/html/HTMLCanvasElement.h
index de26c475a3..822f053978 100644
--- a/dom/html/HTMLCanvasElement.h
+++ b/dom/html/HTMLCanvasElement.h
@@ -295,20 +295,6 @@ public:
                                 nsAttrValue& aResult) override;
   nsChangeHint GetAttributeChangeHint(const nsIAtom* aAttribute, int32_t aModType) const override;
 
-  // SetAttr override.  C++ is stupid, so have to override both
-  // overloaded methods.
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                             bool aNotify) override;
-
   virtual nsresult Clone(mozilla::dom::NodeInfo *aNodeInfo, nsINode **aResult) const override;
   nsresult CopyInnerTo(mozilla::dom::Element* aDest);
 
@@ -373,6 +359,14 @@ protected:
                             nsISupports** aResult);
   void CallPrintCallback();
 
+  virtual nsresult AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
+  virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify) override;
+
   AsyncCanvasRenderer* GetAsyncCanvasRenderer();
 
   bool mResetLayer;
@@ -406,6 +400,18 @@ public:
   CanvasContextType GetCurrentContextType() {
     return mCurrentContextType;
   }
+
+private:
+  /**
+   * This function is called by AfterSetAttr and OnAttrSetButNotChanged.
+   * This function will be called by AfterSetAttr whether the attribute is being
+   * set or unset.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  void AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName, bool aNotify);
 };
 
 class HTMLCanvasPrintState final : public nsWrapperCache
diff --git a/dom/html/HTMLContentElement.cpp b/dom/html/HTMLContentElement.cpp
index 01c0158a03..158f8d0ce9 100644
--- a/dom/html/HTMLContentElement.cpp
+++ b/dom/html/HTMLContentElement.cpp
@@ -210,75 +210,63 @@ IsValidContentSelectors(nsCSSSelector* aSelector)
 }
 
 nsresult
-HTMLContentElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                            nsIAtom* aPrefix, const nsAString& aValue,
-                            bool aNotify)
+HTMLContentElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                 const nsAttrValue* aValue,
+                                 const nsAttrValue* aOldValue, bool aNotify)
 {
-  nsresult rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix,
-                                              aValue, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::select) {
-    // Select attribute was updated, the insertion point may match different
-    // elements.
-    nsIDocument* doc = OwnerDoc();
-    nsCSSParser parser(doc->CSSLoader());
-
-    mValidSelector = true;
-    mSelectorList = nullptr;
-
-    nsresult rv = parser.ParseSelectorString(aValue,
-                                             doc->GetDocumentURI(),
-                                             // Bug 11240
-                                             0, // XXX get the line number!
-                                             getter_Transfers(mSelectorList));
-
-    // We don't want to return an exception if parsing failed because
-    // the spec does not define it as an exception case.
-    if (NS_SUCCEEDED(rv)) {
-      // Ensure that all the selectors are valid
-      nsCSSSelectorList* selectors = mSelectorList;
-      while (selectors) {
-        if (!IsValidContentSelectors(selectors->mSelectors)) {
-          // If we have an invalid selector, we can not match anything.
-          mValidSelector = false;
-          mSelectorList = nullptr;
-          break;
+  if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::select) {
+    if (aValue) {
+      // Select attribute was updated, the insertion point may match different
+      // elements.
+      nsIDocument* doc = OwnerDoc();
+      nsCSSParser parser(doc->CSSLoader());
+
+      mValidSelector = true;
+      mSelectorList = nullptr;
+
+      nsAutoString valueStr;
+      aValue->ToString(valueStr);
+      nsresult rv = parser.ParseSelectorString(valueStr,
+                                               doc->GetDocumentURI(),
+                                               // Bug 11240
+                                               0, // XXX get the line number!
+                                               getter_Transfers(mSelectorList));
+
+      // We don't want to return an exception if parsing failed because
+      // the spec does not define it as an exception case.
+      if (NS_SUCCEEDED(rv)) {
+        // Ensure that all the selectors are valid
+        nsCSSSelectorList* selectors = mSelectorList;
+        while (selectors) {
+          if (!IsValidContentSelectors(selectors->mSelectors)) {
+            // If we have an invalid selector, we can not match anything.
+            mValidSelector = false;
+            mSelectorList = nullptr;
+            break;
+          }
+          selectors = selectors->mNext;
         }
-        selectors = selectors->mNext;
       }
-    }
-
-    ShadowRoot* containingShadow = GetContainingShadow();
-    if (containingShadow) {
-      containingShadow->DistributeAllNodes();
-    }
-  }
-
-  return NS_OK;
-}
 
-nsresult
-HTMLContentElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                              bool aNotify)
-{
-  nsresult rv = nsGenericHTMLElement::UnsetAttr(aNameSpaceID,
-                                                aAttribute, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (aNameSpaceID == kNameSpaceID_None && aAttribute == nsGkAtoms::select) {
-    // The select attribute was removed. This insertion point becomes
-    // a universal selector.
-    mValidSelector = true;
-    mSelectorList = nullptr;
-
-    ShadowRoot* containingShadow = GetContainingShadow();
-    if (containingShadow) {
-      containingShadow->DistributeAllNodes();
+      ShadowRoot* containingShadow = GetContainingShadow();
+      if (containingShadow) {
+        containingShadow->DistributeAllNodes();
+      }
+    } else {
+      // The select attribute was removed. This insertion point becomes
+      // a universal selector.
+      mValidSelector = true;
+      mSelectorList = nullptr;
+
+      ShadowRoot* containingShadow = GetContainingShadow();
+      if (containingShadow) {
+        containingShadow->DistributeAllNodes();
+      }
     }
   }
 
-  return NS_OK;
+  return nsGenericHTMLElement::AfterSetAttr(aNamespaceID, aName, aValue,
+                                            aOldValue, aNotify);
 }
 
 bool
diff --git a/dom/html/HTMLContentElement.h b/dom/html/HTMLContentElement.h
index f019e56cda..d2491a7928 100644
--- a/dom/html/HTMLContentElement.h
+++ b/dom/html/HTMLContentElement.h
@@ -63,13 +63,6 @@ public:
   void InsertMatchedNode(uint32_t aIndex, nsIContent* aContent);
   void ClearMatchedNodes();
 
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) override;
-
   // WebIDL methods.
   already_AddRefed<DistributedContentList> GetDistributedNodes();
   void GetSelect(nsAString& aSelect)
@@ -95,6 +88,11 @@ protected:
    */
   void UpdateFallbackDistribution();
 
+  virtual nsresult AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
+
   /**
    * An array of nodes from the ShadowRoot host that match the
    * content insertion selector.
diff --git a/dom/html/HTMLFormElement.cpp b/dom/html/HTMLFormElement.cpp
index 1fdfb602e1..58cba6863a 100644
--- a/dom/html/HTMLFormElement.cpp
+++ b/dom/html/HTMLFormElement.cpp
@@ -189,27 +189,31 @@ HTMLFormElement::GetElements(nsIDOMHTMLCollection** aElements)
 }
 
 nsresult
-HTMLFormElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                         nsIAtom* aPrefix, const nsAString& aValue,
-                         bool aNotify)
-{
-  if ((aName == nsGkAtoms::action || aName == nsGkAtoms::target) &&
-      aNameSpaceID == kNameSpaceID_None) {
-    if (mPendingSubmission) {
-      // aha, there is a pending submission that means we're in
-      // the script and we need to flush it. let's tell it
-      // that the event was ignored to force the flush.
-      // the second argument is not playing a role at all.
-      FlushPendingSubmission();
+HTMLFormElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                               const nsAttrValueOrString* aValue, bool aNotify)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::action || aName == nsGkAtoms::target) {
+      // This check is mostly to preserve previous behavior.
+      if (aValue) {
+        if (mPendingSubmission) {
+          // aha, there is a pending submission that means we're in
+          // the script and we need to flush it. let's tell it
+          // that the event was ignored to force the flush.
+          // the second argument is not playing a role at all.
+          FlushPendingSubmission();
+        }
+        // Don't forget we've notified the password manager already if the
+        // page sets the action/target in the during submit. (bug 343182)
+        bool notifiedObservers = mNotifiedObservers;
+        ForgetCurrentSubmission();
+        mNotifiedObservers = notifiedObservers;
+      }
     }
-    // Don't forget we've notified the password manager already if the
-    // page sets the action/target in the during submit. (bug 343182)
-    bool notifiedObservers = mNotifiedObservers;
-    ForgetCurrentSubmission();
-    mNotifiedObservers = notifiedObservers;
   }
-  return nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix, aValue,
-                                       aNotify);
+
+  return nsGenericHTMLElement::BeforeSetAttr(aNamespaceID, aName, aValue,
+                                             aNotify);
 }
 
 nsresult
diff --git a/dom/html/HTMLFormElement.h b/dom/html/HTMLFormElement.h
index 6102ea61f1..f06db4b51e 100644
--- a/dom/html/HTMLFormElement.h
+++ b/dom/html/HTMLFormElement.h
@@ -105,14 +105,9 @@ public:
                               bool aCompileEventHandlers) override;
   virtual void UnbindFromTree(bool aDeep = true,
                               bool aNullParent = true) override;
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
+  virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                 const nsAttrValueOrString* aValue,
+                                 bool aNotify) override;
   virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                 const nsAttrValue* aValue,
                                 const nsAttrValue* aOldValue,
diff --git a/dom/html/HTMLFrameSetElement.cpp b/dom/html/HTMLFrameSetElement.cpp
index 6d39caa19a..861dbfe9fc 100644
--- a/dom/html/HTMLFrameSetElement.cpp
+++ b/dom/html/HTMLFrameSetElement.cpp
@@ -9,6 +9,7 @@
 #include "mozilla/dom/EventHandlerBinding.h"
 #include "nsGlobalWindow.h"
 #include "mozilla/UniquePtrExtensions.h"
+#include "nsAttrValueOrString.h"
 
 NS_IMPL_NS_NEW_HTML_ELEMENT(FrameSet)
 
@@ -65,43 +66,45 @@ HTMLFrameSetElement::GetRows(nsAString& aRows)
 }
 
 nsresult
-HTMLFrameSetElement::SetAttr(int32_t aNameSpaceID,
-                             nsIAtom* aAttribute,
-                             nsIAtom* aPrefix,
-                             const nsAString& aValue,
-                             bool aNotify)
+HTMLFrameSetElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                   const nsAttrValueOrString* aValue,
+                                   bool aNotify)
 {
-  nsresult rv;
   /* The main goal here is to see whether the _number_ of rows or
-   *  columns has changed.  If it has, we need to reframe; otherwise
-   *  we want to reflow.  So we set mCurrentRowColHint here, then call
-   *  nsGenericHTMLElement::SetAttr, which will end up calling
-   *  GetAttributeChangeHint and notifying layout with that hint.
-   *  Once nsGenericHTMLElement::SetAttr returns, we want to go back to our
-   *  normal hint, which is NS_STYLE_HINT_REFLOW.
+   * columns has changed. If it has, we need to reframe; otherwise
+   * we want to reflow.
+   * Ideally, the style hint would be changed back to reflow after the reframe
+   * has been performed. Unfortunately, however, the reframe will be performed
+   * by the call to nsNodeUtils::AttributeChanged, which occurs *after*
+   * AfterSetAttr is called, leaving us with no convenient way of changing the
+   * value back to reflow afterwards. However, nsNodeUtils::AttributeChanged is
+   * effectively the only consumer of this value, so as long as we always set
+   * the value correctly here, we should be fine.
    */
-  if (aAttribute == nsGkAtoms::rows && aNameSpaceID == kNameSpaceID_None) {
-    int32_t oldRows = mNumRows;
-    ParseRowCol(aValue, mNumRows, &mRowSpecs);
+  mCurrentRowColHint = NS_STYLE_HINT_REFLOW;
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::rows) {
+      if (aValue) {
+        int32_t oldRows = mNumRows;
+        ParseRowCol(aValue->String(), mNumRows, &mRowSpecs);
 
-    if (mNumRows != oldRows) {
-      mCurrentRowColHint = nsChangeHint_ReconstructFrame;
-    }
-  } else if (aAttribute == nsGkAtoms::cols &&
-             aNameSpaceID == kNameSpaceID_None) {
-    int32_t oldCols = mNumCols;
-    ParseRowCol(aValue, mNumCols, &mColSpecs);
+        if (mNumRows != oldRows) {
+          mCurrentRowColHint = nsChangeHint_ReconstructFrame;
+        }
+      }
+    } else if (aName == nsGkAtoms::cols) {
+      if (aValue) {
+        int32_t oldCols = mNumCols;
+        ParseRowCol(aValue->String(), mNumCols, &mColSpecs);
 
-    if (mNumCols != oldCols) {
-      mCurrentRowColHint = nsChangeHint_ReconstructFrame;
+        if (mNumCols != oldCols) {
+          mCurrentRowColHint = nsChangeHint_ReconstructFrame;
+        }
+      }
     }
   }
 
-  rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aAttribute, aPrefix,
-                                     aValue, aNotify);
-  mCurrentRowColHint = NS_STYLE_HINT_REFLOW;
-
-  return rv;
+  return nsGenericHTMLElement::BeforeSetAttr(aNamespaceID, aName, aValue, aNotify);
 }
 
 nsresult
diff --git a/dom/html/HTMLFrameSetElement.h b/dom/html/HTMLFrameSetElement.h
index b6bbe5d957..fe3bc2341c 100644
--- a/dom/html/HTMLFrameSetElement.h
+++ b/dom/html/HTMLFrameSetElement.h
@@ -100,17 +100,6 @@ public:
 #undef WINDOW_EVENT_HELPER
 #undef EVENT
 
-  // These override the SetAttr methods in nsGenericHTMLElement (need
-  // both here to silence compiler warnings).
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-
    /**
     * GetRowSpec is used to get the "rows" spec.
     * @param out int32_t aNumValues The number of row sizes specified.
@@ -143,6 +132,10 @@ protected:
 
   virtual JSObject* WrapNode(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override;
 
+  virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                 const nsAttrValueOrString* aValue,
+                                 bool aNotify) override;
+
 private:
   nsresult ParseRowCol(const nsAString& aValue,
                        int32_t& aNumSpecs,
diff --git a/dom/html/HTMLIFrameElement.cpp b/dom/html/HTMLIFrameElement.cpp
index 6ae4ec9ddd..2b66cbd268 100644
--- a/dom/html/HTMLIFrameElement.cpp
+++ b/dom/html/HTMLIFrameElement.cpp
@@ -182,54 +182,49 @@ HTMLIFrameElement::GetAttributeMappingFunction() const
 }
 
 nsresult
-HTMLIFrameElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify)
-{
-  nsresult rv = nsGenericHTMLFrameElement::SetAttr(aNameSpaceID, aName,
-                                                   aPrefix, aValue, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::srcdoc) {
-    // Don't propagate error here. The attribute was successfully set, that's
-    // what we should reflect.
-    LoadSrc();
-  }
-
-  return NS_OK;
-}
-
-nsresult
 HTMLIFrameElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                 const nsAttrValue* aValue,
                                 const nsAttrValue* aOldValue, bool aNotify)
 {
-  if (aName == nsGkAtoms::sandbox &&
-      aNameSpaceID == kNameSpaceID_None && mFrameLoader) {
-    // If we have an nsFrameLoader, apply the new sandbox flags.
-    // Since this is called after the setter, the sandbox flags have
-    // alreay been updated.
-    mFrameLoader->ApplySandboxFlags(GetSandboxFlags());
+  AfterMaybeChangeAttr(aNameSpaceID, aName, aNotify);
+
+  if (aNameSpaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::sandbox) {
+      if (mFrameLoader) {
+        // If we have an nsFrameLoader, apply the new sandbox flags.
+        // Since this is called after the setter, the sandbox flags have
+        // alreay been updated.
+        mFrameLoader->ApplySandboxFlags(GetSandboxFlags());
+      }
+    }
   }
   return nsGenericHTMLFrameElement::AfterSetAttr(aNameSpaceID, aName, aValue,
                                                  aOldValue, aNotify);
 }
 
 nsresult
-HTMLIFrameElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify)
+HTMLIFrameElement::OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify)
 {
-  // Invoke on the superclass.
-  nsresult rv = nsGenericHTMLFrameElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (aNameSpaceID == kNameSpaceID_None &&
-      aAttribute == nsGkAtoms::srcdoc) {
-    // Fall back to the src attribute, if any
-    LoadSrc();
-  }
+  AfterMaybeChangeAttr(aNamespaceID, aName, aNotify);
+
+  return nsGenericHTMLFrameElement::OnAttrSetButNotChanged(aNamespaceID, aName,
+                                                           aValue, aNotify);
+}
 
-  return NS_OK;
+void
+HTMLIFrameElement::AfterMaybeChangeAttr(int32_t aNamespaceID,
+                                        nsIAtom* aName,
+                                        bool aNotify)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::srcdoc) {
+      // Don't propagate errors from LoadSrc. The attribute was successfully
+      // set/unset, that's what we should reflect.
+      LoadSrc();
+    }
+  }
 }
 
 uint32_t
diff --git a/dom/html/HTMLIFrameElement.h b/dom/html/HTMLIFrameElement.h
index 23afaaddd0..e94ae0bad6 100644
--- a/dom/html/HTMLIFrameElement.h
+++ b/dom/html/HTMLIFrameElement.h
@@ -46,21 +46,6 @@ public:
 
   virtual nsresult Clone(mozilla::dom::NodeInfo *aNodeInfo, nsINode **aResult) const override;
 
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-  virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                                const nsAttrValue* aValue,
-                                const nsAttrValue* aOldValue,
-                                bool aNotify) override;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) override;
-
   uint32_t GetSandboxFlags();
 
   // Web IDL binding methods
@@ -197,11 +182,30 @@ protected:
 
   virtual JSObject* WrapNode(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
 
+  virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
+  virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify) override;
+
 private:
   static void MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
                                     nsRuleData* aData);
 
   static const DOMTokenListSupportedToken sSupportedSandboxTokens[];
+
+  /**
+   * This function is called by AfterSetAttr and OnAttrSetButNotChanged.
+   * This function will be called by AfterSetAttr whether the attribute is being
+   * set or unset.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  void AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName, bool aNotify);
 };
 
 } // namespace dom
diff --git a/dom/html/HTMLImageElement.cpp b/dom/html/HTMLImageElement.cpp
index dd43ac36ad..444c352e2c 100644
--- a/dom/html/HTMLImageElement.cpp
+++ b/dom/html/HTMLImageElement.cpp
@@ -112,6 +112,7 @@ private:
 HTMLImageElement::HTMLImageElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo)
   : nsGenericHTMLElement(aNodeInfo)
   , mForm(nullptr)
+  , mForceReload(false)
   , mInDocResponsiveContent(false)
   , mCurrentDensity(1.0)
 {
@@ -372,6 +373,9 @@ HTMLImageElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                 const nsAttrValueOrString* aValue,
                                 bool aNotify)
 {
+  if (aValue) {
+    BeforeMaybeChangeAttr(aNameSpaceID, aName, *aValue, aNotify);
+  }
 
   if (aNameSpaceID == kNameSpaceID_None && mForm &&
       (aName == nsGkAtoms::name || aName == nsGkAtoms::id)) {
@@ -394,6 +398,10 @@ HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                const nsAttrValue* aValue,
                                const nsAttrValue* aOldValue, bool aNotify)
 {
+  if (aValue) {
+    AfterMaybeChangeAttr(aNameSpaceID, aName, aNotify);
+  }
+
   if (aNameSpaceID == kNameSpaceID_None && mForm &&
       (aName == nsGkAtoms::name || aName == nsGkAtoms::id) &&
       aValue && !aValue->IsEmptyString()) {
@@ -438,63 +446,22 @@ HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
 }
 
 nsresult
-HTMLImageElement::GetEventTargetParent(EventChainPreVisitor& aVisitor)
-{
-  // We handle image element with attribute ismap in its corresponding frame
-  // element. Set mMultipleActionsPrevented here to prevent the click event
-  // trigger the behaviors in Element::PostHandleEventForLinks
-  WidgetMouseEvent* mouseEvent = aVisitor.mEvent->AsMouseEvent();
-  if (mouseEvent && mouseEvent->IsLeftClickEvent() && IsMap()) {
-    mouseEvent->mFlags.mMultipleActionsPrevented = true;
-  }
-  return nsGenericHTMLElement::GetEventTargetParent(aVisitor);
-}
-
-bool
-HTMLImageElement::IsHTMLFocusable(bool aWithMouse,
-                                  bool *aIsFocusable, int32_t *aTabIndex)
+HTMLImageElement::OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                         const nsAttrValueOrString& aValue,
+                                         bool aNotify)
 {
-  int32_t tabIndex = TabIndex();
-
-  if (IsInUncomposedDoc()) {
-    nsAutoString usemap;
-    GetUseMap(usemap);
-    // XXXbz which document should this be using?  sXBL/XBL2 issue!  I
-    // think that OwnerDoc() is right, since we don't want to
-    // assume stuff about the document we're bound to.
-    if (OwnerDoc()->FindImageMap(usemap)) {
-      if (aTabIndex) {
-        // Use tab index on individual map areas
-        *aTabIndex = (sTabFocusModel & eTabFocus_linksMask)? 0 : -1;
-      }
-      // Image map is not focusable itself, but flag as tabbable
-      // so that image map areas get walked into.
-      *aIsFocusable = false;
-
-      return false;
-    }
-  }
-
-  if (aTabIndex) {
-    // Can be in tab order if tabindex >=0 and form controls are tabbable.
-    *aTabIndex = (sTabFocusModel & eTabFocus_formElementsMask)? tabIndex : -1;
-  }
-
-  *aIsFocusable =
-#ifdef XP_MACOSX
-    (!aWithMouse || nsFocusManager::sMouseFocusesFormControl) &&
-#endif
-    (tabIndex >= 0 || HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex));
+  BeforeMaybeChangeAttr(aNamespaceID, aName, aValue, aNotify);
+  AfterMaybeChangeAttr(aNamespaceID, aName, aNotify);
 
-  return false;
+  return nsGenericHTMLElement::OnAttrSetButNotChanged(aNamespaceID, aName,
+                                                      aValue, aNotify);
 }
 
-nsresult
-HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                          nsIAtom* aPrefix, const nsAString& aValue,
-                          bool aNotify)
+void
+HTMLImageElement::BeforeMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                        const nsAttrValueOrString& aValue,
+                                        bool aNotify)
 {
-  bool forceReload = false;
   // We need to force our image to reload.  This must be done here, not in
   // AfterSetAttr or BeforeSetAttr, because we want to do it even if the attr is
   // being set to its existing value, which is normally optimized away as a
@@ -505,16 +472,19 @@ HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
   // spec.
   //
   // Both cases handle unsetting src in AfterSetAttr
-  if (aNameSpaceID == kNameSpaceID_None &&
+  //
+  // Much of this should probably happen in AfterMaybeChangeAttr.
+  // See Bug 1370705
+  if (aNamespaceID == kNameSpaceID_None &&
       aName == nsGkAtoms::src) {
 
     if (InResponsiveMode()) {
       if (mResponsiveSelector &&
           mResponsiveSelector->Content() == this) {
-        mResponsiveSelector->SetDefaultSource(aValue);
+        mResponsiveSelector->SetDefaultSource(aValue.String());
       }
       QueueImageLoadTask(true);
-    } else if (aNotify) {
+    } else if (aNotify && OwnerDoc()->IsCurrentActiveDocument()) {
       // If aNotify is false, we are coming from the parser or some such place;
       // we'll get bound after all the attributes have been set, so we'll do the
       // sync image load from BindToTree. Skip the LoadImage call in that case.
@@ -529,23 +499,23 @@ HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
       // the state gets in Element's attr-setting happen around this
       // LoadImage call, we could start passing false instead of aNotify
       // here.
-      LoadImage(aValue, true, aNotify, eImageLoadType_Normal);
+      LoadImage(aValue.String(), true, aNotify, eImageLoadType_Normal);
 
       mNewRequestsWillNeedAnimationReset = false;
     }
-  } else if (aNameSpaceID == kNameSpaceID_None &&
+  } else if (aNamespaceID == kNameSpaceID_None &&
              aName == nsGkAtoms::crossorigin &&
              aNotify) {
     nsAttrValue attrValue;
-    ParseCORSValue(aValue, attrValue);
+    ParseCORSValue(aValue.String(), attrValue);
     if (GetCORSMode() != AttrValueToCORSMode(&attrValue)) {
       // Force a new load of the image with the new cross origin policy.
-      forceReload = true;
+      mForceReload = true;
     }
   } else if (aName == nsGkAtoms::referrerpolicy &&
-      aNameSpaceID == kNameSpaceID_None &&
+      aNamespaceID == kNameSpaceID_None &&
       aNotify) {
-    ReferrerPolicy referrerPolicy = AttributeReferrerPolicyFromString(aValue);
+    ReferrerPolicy referrerPolicy = AttributeReferrerPolicyFromString(aValue.String());
     if (!InResponsiveMode() &&
         referrerPolicy != RP_Unset &&
         referrerPolicy != GetImageReferrerPolicy()) {
@@ -554,22 +524,28 @@ HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
       // the attribute will neither trigger a reload nor update the referrer
       // policy of the loading channel (whether it has previously completed or
       // not). Force a new load of the image with the new referrerpolicy.
-      forceReload = true;
+      mForceReload = true;
     }
   }
 
-  nsresult rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix,
-                                              aValue, aNotify);
+  return;
+}
 
+void
+HTMLImageElement::AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                       bool aNotify)
+{
   // Because we load image synchronously in non-responsive-mode, we need to do
   // reload after the attribute has been set if the reload is triggerred by
   // cross origin changing.
-  if (forceReload) {
+  if (mForceReload) {
+    mForceReload = false;
+
     if (InResponsiveMode()) {
       // per spec, full selection runs when this changes, even though
       // it doesn't directly affect the source selection
       QueueImageLoadTask(true);
-    } else {
+    } else if (OwnerDoc()->IsCurrentActiveDocument()) {
       // Bug 1076583 - We still use the older synchronous algorithm in
       // non-responsive mode. Force a new load of the image with the
       // new cross origin policy
@@ -577,7 +553,59 @@ HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
     }
   }
 
-  return rv;
+  return;
+}
+
+nsresult
+HTMLImageElement::GetEventTargetParent(EventChainPreVisitor& aVisitor)
+{
+  // We handle image element with attribute ismap in its corresponding frame
+  // element. Set mMultipleActionsPrevented here to prevent the click event
+  // trigger the behaviors in Element::PostHandleEventForLinks
+  WidgetMouseEvent* mouseEvent = aVisitor.mEvent->AsMouseEvent();
+  if (mouseEvent && mouseEvent->IsLeftClickEvent() && IsMap()) {
+    mouseEvent->mFlags.mMultipleActionsPrevented = true;
+  }
+  return nsGenericHTMLElement::GetEventTargetParent(aVisitor);
+}
+
+bool
+HTMLImageElement::IsHTMLFocusable(bool aWithMouse,
+                                  bool *aIsFocusable, int32_t *aTabIndex)
+{
+  int32_t tabIndex = TabIndex();
+
+  if (IsInUncomposedDoc()) {
+    nsAutoString usemap;
+    GetUseMap(usemap);
+    // XXXbz which document should this be using?  sXBL/XBL2 issue!  I
+    // think that OwnerDoc() is right, since we don't want to
+    // assume stuff about the document we're bound to.
+    if (OwnerDoc()->FindImageMap(usemap)) {
+      if (aTabIndex) {
+        // Use tab index on individual map areas
+        *aTabIndex = (sTabFocusModel & eTabFocus_linksMask)? 0 : -1;
+      }
+      // Image map is not focusable itself, but flag as tabbable
+      // so that image map areas get walked into.
+      *aIsFocusable = false;
+
+      return false;
+    }
+  }
+
+  if (aTabIndex) {
+    // Can be in tab order if tabindex >=0 and form controls are tabbable.
+    *aTabIndex = (sTabFocusModel & eTabFocus_formElementsMask)? tabIndex : -1;
+  }
+
+  *aIsFocusable =
+#ifdef XP_MACOSX
+    (!aWithMouse || nsFocusManager::sMouseFocusesFormControl) &&
+#endif
+    (tabIndex >= 0 || HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex));
+
+  return false;
 }
 
 nsresult
diff --git a/dom/html/HTMLImageElement.h b/dom/html/HTMLImageElement.h
index 52480d0a05..bb4a098824 100644
--- a/dom/html/HTMLImageElement.h
+++ b/dom/html/HTMLImageElement.h
@@ -75,17 +75,6 @@ public:
 
   bool IsHTMLFocusable(bool aWithMouse, bool *aIsFocusable, int32_t *aTabIndex) override;
 
-  // SetAttr override.  C++ is stupid, so have to override both
-  // overloaded methods.
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-
   virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                               nsIContent* aBindingParent,
                               bool aCompileEventHandlers) override;
@@ -352,6 +341,10 @@ protected:
                                 const nsAttrValue* aOldValue,
                                 bool aNotify) override;
 
+  virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify) override;
+
   // This is a weak reference that this element and the HTMLFormElement
   // cooperate in maintaining.
   HTMLFormElement* mForm;
@@ -365,6 +358,36 @@ private:
   static void MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
                                     nsRuleData* aData);
 
+  /**
+   * This function is called by BeforeSetAttr and OnAttrSetButNotChanged.
+   * It will not be called if the value is being unset.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aValue the value it's being set to represented as either a string or
+   *        a parsed nsAttrValue.
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  void BeforeMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                             const nsAttrValueOrString& aValue,
+                             bool aNotify);
+  /**
+   * This function is called by AfterSetAttr and OnAttrSetButNotChanged.
+   * It will not be called if the value is being unset.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  void AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                            bool aNotify);
+  /**
+   * Used by BeforeMaybeChangeAttr and AfterMaybeChangeAttr to keep track of
+   * whether a reload needs to be forced after an attribute change that is
+   * currently in progress.
+   */
+  bool mForceReload;
+
   bool mInDocResponsiveContent;
   RefPtr<ImageLoadTask> mPendingImageLoadTask;
 
diff --git a/dom/html/HTMLLegendElement.cpp b/dom/html/HTMLLegendElement.cpp
index 1a593d7698..3f329de42d 100644
--- a/dom/html/HTMLLegendElement.cpp
+++ b/dom/html/HTMLLegendElement.cpp
@@ -71,21 +71,6 @@ HTMLLegendElement::GetAttributeChangeHint(const nsIAtom* aAttribute,
 }
 
 nsresult
-HTMLLegendElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify)
-{
-  return nsGenericHTMLElement::SetAttr(aNameSpaceID, aAttribute,
-                                       aPrefix, aValue, aNotify);
-}
-nsresult
-HTMLLegendElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify)
-{
-  return nsGenericHTMLElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify);
-}
-
-nsresult
 HTMLLegendElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                               nsIContent* aBindingParent,
                               bool aCompileEventHandlers)
diff --git a/dom/html/HTMLLegendElement.h b/dom/html/HTMLLegendElement.h
index e2d71d62dd..0b476c1194 100644
--- a/dom/html/HTMLLegendElement.h
+++ b/dom/html/HTMLLegendElement.h
@@ -42,16 +42,6 @@ public:
                                 nsAttrValue& aResult) override;
   virtual nsChangeHint GetAttributeChangeHint(const nsIAtom* aAttribute,
                                               int32_t aModType) const override;
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) override;
 
   virtual nsresult Clone(mozilla::dom::NodeInfo* aNodeInfo, nsINode** aResult) const override;
 
diff --git a/dom/html/HTMLMediaElement.cpp b/dom/html/HTMLMediaElement.cpp
index 4bcc188d39..05eaf3e8b5 100644
--- a/dom/html/HTMLMediaElement.cpp
+++ b/dom/html/HTMLMediaElement.cpp
@@ -3726,78 +3726,74 @@ int32_t HTMLMediaElement::TabIndexDefault()
   return 0;
 }
 
-nsresult HTMLMediaElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                                   nsIAtom* aPrefix, const nsAString& aValue,
-                                   bool aNotify)
-{
-  nsresult rv =
-    nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix, aValue,
-                                  aNotify);
-  if (NS_FAILED(rv))
-    return rv;
-  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::src) {
-    DoLoad();
-  }
-  if (aNotify && aNameSpaceID == kNameSpaceID_None) {
-    if (aName == nsGkAtoms::autoplay) {
-      StopSuspendingAfterFirstFrame();
-      CheckAutoplayDataReady();
-      // This attribute can affect AddRemoveSelfReference
-      AddRemoveSelfReference();
-      UpdatePreloadAction();
+nsresult
+HTMLMediaElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue, bool aNotify)
+{
+  if (aNameSpaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::src) {
+      mSrcMediaSource = nullptr;
+      if (aValue) {
+        nsString srcStr = aValue->GetStringValue();
+        nsCOMPtr<nsIURI> uri;
+        NewURIFromString(srcStr, getter_AddRefs(uri));
+        if (uri && IsMediaSourceURI(uri)) {
+          nsresult rv =
+            NS_GetSourceForMediaSourceURI(uri, getter_AddRefs(mSrcMediaSource));
+          if (NS_FAILED(rv)) {
+            nsAutoString spec;
+            GetCurrentSrc(spec);
+            const char16_t* params[] = { spec.get() };
+            ReportLoadError("MediaLoadInvalidURI", params, ArrayLength(params));
+          }
+        }
+      }
+    } else if (aName == nsGkAtoms::autoplay) {
+      if (aNotify) {
+        if (aValue) {
+          StopSuspendingAfterFirstFrame();
+          CheckAutoplayDataReady();
+        }
+        // This attribute can affect AddRemoveSelfReference
+        AddRemoveSelfReference();
+        UpdatePreloadAction();
+      }
     } else if (aName == nsGkAtoms::preload) {
       UpdatePreloadAction();
     }
   }
 
-  return rv;
+  // Since AfterMaybeChangeAttr may call DoLoad, make sure that it is called
+  // *after* any possible changes to mSrcMediaSource.
+  if (aValue) {
+    AfterMaybeChangeAttr(aNameSpaceID, aName, aNotify);
+  }
+
+  return nsGenericHTMLElement::AfterSetAttr(aNameSpaceID, aName,
+                                            aValue, aOldValue, aNotify);
 }
 
-nsresult HTMLMediaElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttr,
-                                     bool aNotify)
+nsresult
+HTMLMediaElement::OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                         const nsAttrValueOrString& aValue,
+                                         bool aNotify)
 {
-  nsresult rv = nsGenericHTMLElement::UnsetAttr(aNameSpaceID, aAttr, aNotify);
-  if (NS_FAILED(rv))
-    return rv;
-  if (aNotify && aNameSpaceID == kNameSpaceID_None) {
-    if (aAttr == nsGkAtoms::autoplay) {
-      // This attribute can affect AddRemoveSelfReference
-      AddRemoveSelfReference();
-      UpdatePreloadAction();
-    } else if (aAttr == nsGkAtoms::preload) {
-      UpdatePreloadAction();
-    }
-  }
+  AfterMaybeChangeAttr(aNamespaceID, aName, aNotify);
 
-  return rv;
+  return nsGenericHTMLElement::OnAttrSetButNotChanged(aNamespaceID, aName,
+                                                      aValue, aNotify);
 }
 
-nsresult
-HTMLMediaElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                                const nsAttrValue* aValue,
-                                const nsAttrValue* aOldValue, bool aNotify)
+void
+HTMLMediaElement::AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                       bool aNotify)
 {
-  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::src) {
-    mSrcMediaSource = nullptr;
-    if (aValue) {
-      nsString srcStr = aValue->GetStringValue();
-      nsCOMPtr<nsIURI> uri;
-      NewURIFromString(srcStr, getter_AddRefs(uri));
-      if (uri && IsMediaSourceURI(uri)) {
-        nsresult rv =
-          NS_GetSourceForMediaSourceURI(uri, getter_AddRefs(mSrcMediaSource));
-        if (NS_FAILED(rv)) {
-          nsAutoString spec;
-          GetCurrentSrc(spec);
-          const char16_t* params[] = { spec.get() };
-          ReportLoadError("MediaLoadInvalidURI", params, ArrayLength(params));
-        }
-      }
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::src) {
+      DoLoad();
     }
   }
-
-  return nsGenericHTMLElement::AfterSetAttr(aNameSpaceID, aName,
-                                            aValue, aOldValue, aNotify);
 }
 
 nsresult HTMLMediaElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
diff --git a/dom/html/HTMLMediaElement.h b/dom/html/HTMLMediaElement.h
index d81e9f5918..0d8d5d4e07 100644
--- a/dom/html/HTMLMediaElement.h
+++ b/dom/html/HTMLMediaElement.h
@@ -132,22 +132,6 @@ public:
                               nsIAtom* aAttribute,
                               const nsAString& aValue,
                               nsAttrValue& aResult) override;
-  // SetAttr override.  C++ is stupid, so have to override both
-  // overloaded methods.
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttr,
-                             bool aNotify) override;
-  virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                                const nsAttrValue* aValue,
-                                const nsAttrValue* aOldValue,
-                                bool aNotify) override;
 
   virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                               nsIContent* aBindingParent,
@@ -1315,6 +1299,14 @@ protected:
 
   already_AddRefed<Promise> CreateDOMPromise(ErrorResult& aRv) const;
 
+  virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
+  virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify) override;
+
   // The current decoder. Load() has been called on this decoder.
   // At most one of mDecoder and mSrcStream can be non-null.
   RefPtr<MediaDecoder> mDecoder;
@@ -1717,6 +1709,17 @@ private:
   // We keep track of these because the load algorithm resolves/rejects all
   // already-dispatched pending play promises.
   nsTArray<nsResolveOrRejectPendingPlayPromisesRunner*> mPendingPlayPromisesRunners;
+
+private:
+  /**
+   * This function is called by AfterSetAttr and OnAttrSetButNotChanged.
+   * It will not be called if the value is being unset.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  void AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName, bool aNotify);
 };
 
 } // namespace dom
diff --git a/dom/html/HTMLObjectElement.cpp b/dom/html/HTMLObjectElement.cpp
index 65f4078892..cd2f3f37a7 100644
--- a/dom/html/HTMLObjectElement.cpp
+++ b/dom/html/HTMLObjectElement.cpp
@@ -298,41 +298,45 @@ HTMLObjectElement::UnbindFromTree(bool aDeep,
 
 
 nsresult
-HTMLObjectElement::SetAttr(int32_t aNameSpaceID, nsIAtom *aName,
-                           nsIAtom *aPrefix, const nsAString &aValue,
-                           bool aNotify)
+HTMLObjectElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue, bool aNotify)
 {
-  nsresult rv = nsGenericHTMLFormElement::SetAttr(aNameSpaceID, aName, aPrefix,
-                                                  aValue, aNotify);
+  nsresult rv = AfterMaybeChangeAttr(aNamespaceID, aName, aNotify);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // if aNotify is false, we are coming from the parser or some such place;
-  // we'll get bound after all the attributes have been set, so we'll do the
-  // object load from BindToTree/DoneAddingChildren.
-  // Skip the LoadObject call in that case.
-  // We also don't want to start loading the object when we're not yet in
-  // a document, just in case that the caller wants to set additional
-  // attributes before inserting the node into the document.
-  if (aNotify && IsInComposedDoc() && mIsDoneAddingChildren &&
-      aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::data) {
-    return LoadObject(aNotify, true);
-  }
-
-  return NS_OK;
+  return nsGenericHTMLFormElement::AfterSetAttr(aNamespaceID, aName, aValue,
+                                                aOldValue, aNotify);
 }
 
 nsresult
-HTMLObjectElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify)
+HTMLObjectElement::OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify)
 {
-  nsresult rv = nsGenericHTMLFormElement::UnsetAttr(aNameSpaceID,
-                                                    aAttribute, aNotify);
+  nsresult rv = AfterMaybeChangeAttr(aNamespaceID, aName, aNotify);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // See comment in SetAttr
-  if (aNotify && IsInComposedDoc() && mIsDoneAddingChildren &&
-      aNameSpaceID == kNameSpaceID_None && aAttribute == nsGkAtoms::data) {
-    return LoadObject(aNotify, true);
+  return nsGenericHTMLFormElement::OnAttrSetButNotChanged(aNamespaceID, aName,
+                                                          aValue, aNotify);
+}
+
+nsresult
+HTMLObjectElement::AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                        bool aNotify)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    // if aNotify is false, we are coming from the parser or some such place;
+    // we'll get bound after all the attributes have been set, so we'll do the
+    // object load from BindToTree/DoneAddingChildren.
+    // Skip the LoadObject call in that case.
+    // We also don't want to start loading the object when we're not yet in
+    // a document, just in case that the caller wants to set additional
+    // attributes before inserting the node into the document.
+    if (aNotify && IsInComposedDoc() && mIsDoneAddingChildren &&
+        aName == nsGkAtoms::data) {
+      return LoadObject(aNotify, true);
+    }
   }
 
   return NS_OK;
diff --git a/dom/html/HTMLObjectElement.h b/dom/html/HTMLObjectElement.h
index 5226154da9..6ce09e1cc4 100644
--- a/dom/html/HTMLObjectElement.h
+++ b/dom/html/HTMLObjectElement.h
@@ -59,11 +59,6 @@ public:
                               bool aCompileEventHandlers) override;
   virtual void UnbindFromTree(bool aDeep = true,
                               bool aNullParent = true) override;
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom *aName,
-                           nsIAtom *aPrefix, const nsAString &aValue,
-                           bool aNotify) override;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) override;
 
   virtual bool IsHTMLFocusable(bool aWithMouse, bool *aIsFocusable, int32_t *aTabIndex) override;
   virtual IMEState GetDesiredIMEState() override;
@@ -245,6 +240,15 @@ public:
     return GetContentDocument(aSubjectPrincipal);
   }
 
+ protected:
+  virtual nsresult AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
+  virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify) override;
+
 private:
   /**
    * Calls LoadObject with the correct arguments to start the plugin load.
@@ -269,6 +273,18 @@ private:
   static void MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
                                     nsRuleData* aData);
 
+  /**
+   * This function is called by AfterSetAttr and OnAttrSetButNotChanged.
+   * This function will be called by AfterSetAttr whether the attribute is being
+   * set or unset.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  nsresult AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                bool aNotify);
+
   bool mIsDoneAddingChildren;
 };
 
diff --git a/dom/html/HTMLSelectElement.cpp b/dom/html/HTMLSelectElement.cpp
index d11f8b90f0..36dac0852b 100644
--- a/dom/html/HTMLSelectElement.cpp
+++ b/dom/html/HTMLSelectElement.cpp
@@ -1272,9 +1272,22 @@ HTMLSelectElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                  const nsAttrValueOrString* aValue,
                                  bool aNotify)
 {
-  if (aNotify && aName == nsGkAtoms::disabled &&
-      aNameSpaceID == kNameSpaceID_None) {
-    mDisabledChanged = true;
+  if (aNameSpaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::disabled) {
+      if (aNotify) {
+        mDisabledChanged = true;
+      }
+    } else if (aName == nsGkAtoms::multiple) {
+      if (!aValue && aNotify && mSelectedIndex >= 0) {
+        // We're changing from being a multi-select to a single-select.
+        // Make sure we only have one option selected before we do that.
+        // Note that this needs to come before we really unset the attr,
+        // since SetOptionsSelectedByIndex does some bail-out type
+        // optimization for cases when the select is not multiple that
+        // would lead to only a single option getting deselected.
+        SetSelectedIndexInternal(mSelectedIndex, aNotify);
+      }
+    }
   }
 
   return nsGenericHTMLFormElementWithState::BeforeSetAttr(aNameSpaceID, aName,
@@ -1294,6 +1307,12 @@ HTMLSelectElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
     } else if (aName == nsGkAtoms::autocomplete) {
       // Clear the cached @autocomplete attribute state
       mAutocompleteAttrState = nsContentUtils::eAutocompleteAttrState_Unknown;
+    } else if (aName == nsGkAtoms::multiple) {
+      if (!aValue && aNotify) {
+        // We might have become a combobox; make sure _something_ gets
+        // selected in that case
+        CheckSelectSomething(aNotify);
+      }
     }
   }
 
@@ -1302,37 +1321,6 @@ HTMLSelectElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                                          aNotify);
 }
 
-nsresult
-HTMLSelectElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify)
-{
-  if (aNotify && aNameSpaceID == kNameSpaceID_None &&
-      aAttribute == nsGkAtoms::multiple) {
-    // We're changing from being a multi-select to a single-select.
-    // Make sure we only have one option selected before we do that.
-    // Note that this needs to come before we really unset the attr,
-    // since SetOptionsSelectedByIndex does some bail-out type
-    // optimization for cases when the select is not multiple that
-    // would lead to only a single option getting deselected.
-    if (mSelectedIndex >= 0) {
-      SetSelectedIndexInternal(mSelectedIndex, aNotify);
-    }
-  }
-
-  nsresult rv = nsGenericHTMLFormElementWithState::UnsetAttr(aNameSpaceID, aAttribute,
-                                                             aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (aNotify && aNameSpaceID == kNameSpaceID_None &&
-      aAttribute == nsGkAtoms::multiple) {
-    // We might have become a combobox; make sure _something_ gets
-    // selected in that case
-    CheckSelectSomething(aNotify);
-  }
-
-  return rv;
-}
-
 void
 HTMLSelectElement::DoneAddingChildren(bool aHaveNotified)
 {
diff --git a/dom/html/HTMLSelectElement.h b/dom/html/HTMLSelectElement.h
index 90e13da7a2..298b1a8d16 100644
--- a/dom/html/HTMLSelectElement.h
+++ b/dom/html/HTMLSelectElement.h
@@ -381,8 +381,6 @@ public:
                                 const nsAttrValue* aValue,
                                 const nsAttrValue* aOldValue,
                                 bool aNotify) override;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) override;
 
   virtual void DoneAddingChildren(bool aHaveNotified) override;
   virtual bool IsDoneAddingChildren() override {
diff --git a/dom/html/HTMLSharedElement.cpp b/dom/html/HTMLSharedElement.cpp
index bcb84d29b2..a7a10b0823 100644
--- a/dom/html/HTMLSharedElement.cpp
+++ b/dom/html/HTMLSharedElement.cpp
@@ -231,52 +231,32 @@ SetBaseTargetUsingFirstBaseWithTarget(nsIDocument* aDocument,
 }
 
 nsresult
-HTMLSharedElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify)
+HTMLSharedElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue, bool aNotify)
 {
-  nsresult rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix,
-                                              aValue, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // If the href attribute of a <base> tag is changing, we may need to update
-  // the document's base URI, which will cause all the links on the page to be
-  // re-resolved given the new base.  If the target attribute is changing, we
-  // similarly need to change the base target.
-  if (mNodeInfo->Equals(nsGkAtoms::base) &&
-      aNameSpaceID == kNameSpaceID_None &&
-      IsInUncomposedDoc()) {
-    if (aName == nsGkAtoms::href) {
-      SetBaseURIUsingFirstBaseWithHref(GetUncomposedDoc(), this);
-    } else if (aName == nsGkAtoms::target) {
-      SetBaseTargetUsingFirstBaseWithTarget(GetUncomposedDoc(), this);
-    }
-  }
-
-  return NS_OK;
-}
-
-nsresult
-HTMLSharedElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                             bool aNotify)
-{
-  nsresult rv = nsGenericHTMLElement::UnsetAttr(aNameSpaceID, aName, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // If we're the first <base> with an href and our href attribute is being
-  // unset, then we're no longer the first <base> with an href, and we need to
-  // find the new one.  Similar for target.
-  if (mNodeInfo->Equals(nsGkAtoms::base) &&
-      aNameSpaceID == kNameSpaceID_None &&
-      IsInUncomposedDoc()) {
+  if (aNamespaceID == kNameSpaceID_None) {
     if (aName == nsGkAtoms::href) {
-      SetBaseURIUsingFirstBaseWithHref(GetUncomposedDoc(), nullptr);
+      // If the href attribute of a <base> tag is changing, we may need to
+      // update the document's base URI, which will cause all the links on the
+      // page to be re-resolved given the new base.
+      // If the href is being unset (aValue is null), we will need to find a new
+      // <base>.
+      if (mNodeInfo->Equals(nsGkAtoms::base) && IsInUncomposedDoc()) {
+        SetBaseURIUsingFirstBaseWithHref(GetUncomposedDoc(),
+                                         aValue ? this : nullptr);
+      }
     } else if (aName == nsGkAtoms::target) {
-      SetBaseTargetUsingFirstBaseWithTarget(GetUncomposedDoc(), nullptr);
+      // The target attribute is in pretty much the same situation as the href
+      // attribute, above.
+      if (mNodeInfo->Equals(nsGkAtoms::base) && IsInUncomposedDoc()) {
+        SetBaseTargetUsingFirstBaseWithTarget(GetUncomposedDoc(),
+                                              aValue ? this : nullptr);
+      }
     }
   }
-
-  return NS_OK;
+  return nsGenericHTMLElement::AfterSetAttr(aNamespaceID, aName, aValue,
+                                            aOldValue, aNotify);
 }
 
 nsresult
diff --git a/dom/html/HTMLSharedElement.h b/dom/html/HTMLSharedElement.h
index 205750cf6d..a12ee096df 100644
--- a/dom/html/HTMLSharedElement.h
+++ b/dom/html/HTMLSharedElement.h
@@ -59,17 +59,6 @@ public:
                                 nsIAtom* aAttribute,
                                 const nsAString& aValue,
                                 nsAttrValue& aResult) override;
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                             bool aNotify) override;
 
   virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                               nsIContent* aBindingParent,
@@ -181,6 +170,11 @@ protected:
   virtual ~HTMLSharedElement();
 
   virtual JSObject* WrapNode(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
+
+  virtual nsresult AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
 };
 
 } // namespace dom
diff --git a/dom/html/HTMLSharedObjectElement.cpp b/dom/html/HTMLSharedObjectElement.cpp
index 889fd5aef3..4f35068e1c 100644
--- a/dom/html/HTMLSharedObjectElement.cpp
+++ b/dom/html/HTMLSharedObjectElement.cpp
@@ -161,27 +161,53 @@ HTMLSharedObjectElement::UnbindFromTree(bool aDeep,
   nsGenericHTMLElement::UnbindFromTree(aDeep, aNullParent);
 }
 
+nsresult
+HTMLSharedObjectElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                      const nsAttrValue* aValue,
+                                      const nsAttrValue* aOldValue,
+                                      bool aNotify)
+{
+  if (aValue) {
+    nsresult rv = AfterMaybeChangeAttr(aNamespaceID, aName, aNotify);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  return nsGenericHTMLElement::AfterSetAttr(aNamespaceID, aName, aValue,
+                                            aOldValue, aNotify);
+}
 
 nsresult
-HTMLSharedObjectElement::SetAttr(int32_t aNameSpaceID, nsIAtom *aName,
-                                 nsIAtom *aPrefix, const nsAString &aValue,
-                                 bool aNotify)
+HTMLSharedObjectElement::OnAttrSetButNotChanged(int32_t aNamespaceID,
+                                                nsIAtom* aName,
+                                                const nsAttrValueOrString& aValue,
+                                                bool aNotify)
 {
-  nsresult rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix,
-                                              aValue, aNotify);
+  nsresult rv = AfterMaybeChangeAttr(aNamespaceID, aName, aNotify);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // if aNotify is false, we are coming from the parser or some such place;
-  // we'll get bound after all the attributes have been set, so we'll do the
-  // object load from BindToTree/DoneAddingChildren.
-  // Skip the LoadObject call in that case.
-  // We also don't want to start loading the object when we're not yet in
-  // a document, just in case that the caller wants to set additional
-  // attributes before inserting the node into the document.
-  if (aNotify && IsInComposedDoc() && mIsDoneAddingChildren &&
-      aNameSpaceID == kNameSpaceID_None && aName == URIAttrName()
-      && !BlockEmbedContentLoading()) {
-    return LoadObject(aNotify, true);
+  return nsGenericHTMLElement::OnAttrSetButNotChanged(aNamespaceID, aName,
+                                                      aValue, aNotify);
+}
+
+nsresult
+HTMLSharedObjectElement::AfterMaybeChangeAttr(int32_t aNamespaceID,
+                                              nsIAtom* aName,
+                                              bool aNotify)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == URIAttrName()) {
+      // If aNotify is false, we are coming from the parser or some such place;
+      // we'll get bound after all the attributes have been set, so we'll do the
+      // object load from BindToTree/DoneAddingChildren.
+      // Skip the LoadObject call in that case.
+      // We also don't want to start loading the object when we're not yet in
+      // a document, just in case that the caller wants to set additional
+      // attributes before inserting the node into the document.
+      if (aNotify && IsInComposedDoc() && mIsDoneAddingChildren) {
+        nsresult rv = LoadObject(aNotify, true);
+        NS_ENSURE_SUCCESS(rv, rv);
+      }
+    }
   }
 
   return NS_OK;
diff --git a/dom/html/HTMLSharedObjectElement.h b/dom/html/HTMLSharedObjectElement.h
index c70ba8ebd0..07cf86cb01 100644
--- a/dom/html/HTMLSharedObjectElement.h
+++ b/dom/html/HTMLSharedObjectElement.h
@@ -57,9 +57,6 @@ public:
                               bool aCompileEventHandlers) override;
   virtual void UnbindFromTree(bool aDeep = true,
                               bool aNullParent = true) override;
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom *aName,
-                           nsIAtom *aPrefix, const nsAString &aValue,
-                           bool aNotify) override;
 
   virtual bool IsHTMLFocusable(bool aWithMouse, bool *aIsFocusable, int32_t *aTabIndex) override;
   virtual IMEState GetDesiredIMEState() override;
@@ -200,6 +197,16 @@ public:
    * Calls LoadObject with the correct arguments to start the plugin load.
    */
   void StartObjectLoad(bool aNotify, bool aForceLoad);
+
+protected: 
+  virtual nsresult AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
+  virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify) override;
+
 private:
   virtual ~HTMLSharedObjectElement();
 
@@ -222,6 +229,17 @@ private:
                                     nsRuleData* aData);
 
   /**
+   * This function is called by AfterSetAttr and OnAttrSetButNotChanged.
+   * It will not be called if the value is being unset.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  nsresult AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                bool aNotify);
+
+  /**
    * Decides whether we should load embed node content.
    *
    * If this is an embed node there are cases in which we should not try to load
diff --git a/dom/html/nsGenericHTMLElement.cpp b/dom/html/nsGenericHTMLElement.cpp
index 569dae6c26..be78dc1cf0 100644
--- a/dom/html/nsGenericHTMLElement.cpp
+++ b/dom/html/nsGenericHTMLElement.cpp
@@ -655,6 +655,43 @@ nsGenericHTMLElement::GetHrefURIForAnchors() const
 }
 
 nsresult
+nsGenericHTMLElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                    const nsAttrValueOrString* aValue,
+                                    bool aNotify)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::accesskey) {
+      // Have to unregister before clearing flag. See UnregAccessKey
+      UnregAccessKey();
+      if (!aValue) {
+        UnsetFlags(NODE_HAS_ACCESSKEY);
+      }
+    } else if (aName == nsGkAtoms::name) {
+      // Have to do this before clearing flag. See RemoveFromNameTable
+      RemoveFromNameTable();
+      if (!aValue || aValue->IsEmpty()) {
+        ClearHasName();
+      }
+    } else if (aName == nsGkAtoms::contenteditable) {
+      if (aValue) {
+        // Set this before the attribute is set so that any subclass code that
+        // runs before the attribute is set won't think we're missing a
+        // contenteditable attr when we actually have one.
+        SetMayHaveContentEditableAttr();
+      }
+    }
+    if (!aValue && IsEventAttributeName(aName)) {
+      if (EventListenerManager* manager = GetExistingListenerManager()) {
+        manager->RemoveEventHandler(aName, EmptyString());
+      }
+    }
+  }
+
+  return nsGenericHTMLElementBase::BeforeSetAttr(aNamespaceID, aName, aValue,
+                                                 aNotify);
+}
+
+nsresult
 nsGenericHTMLElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
                                    const nsAttrValue* aValue,
                                    const nsAttrValue* aOldValue, bool aNotify)
@@ -694,6 +731,34 @@ nsGenericHTMLElement::AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
         }
       }
       SetDirectionalityOnDescendants(this, dir, aNotify);
+    } else if (aName == nsGkAtoms::contenteditable) {
+      int32_t editableCountDelta = 0;
+      if (aOldValue &&
+          (aOldValue->Equals(NS_LITERAL_STRING("true"), eIgnoreCase) ||
+          aOldValue->Equals(EmptyString(), eIgnoreCase))) {
+        editableCountDelta = -1;
+      }
+      if (aValue && (aValue->Equals(NS_LITERAL_STRING("true"), eIgnoreCase) ||
+                     aValue->Equals(EmptyString(), eIgnoreCase))) {
+        ++editableCountDelta;
+      }
+      ChangeEditableState(editableCountDelta);
+    } else if (aName == nsGkAtoms::accesskey) {
+      if (aValue && !aValue->Equals(EmptyString(), eIgnoreCase)) {
+        SetFlags(NODE_HAS_ACCESSKEY);
+        RegAccessKey();
+      }
+    } else if (aName == nsGkAtoms::name) {
+      if (aValue && !aValue->Equals(EmptyString(), eIgnoreCase) &&
+          CanHaveName(NodeInfo()->NameAtom())) {
+        // This may not be quite right because we can have subclass code run
+        // before here. But in practice subclasses don't care about this flag,
+        // and in particular selector matching does not care.  Otherwise we'd
+        // want to handle it like we handle id attributes (in PreIdMaybeChange
+        // and PostIdMaybeChange).
+        SetHasName();
+        AddToNameTable(aValue->GetAtomValue());
+      }
     }
   }
 
@@ -819,87 +884,6 @@ nsGenericHTMLElement::SetOn##name_(EventHandlerNonNull* handler)              \
 #undef FORWARDED_EVENT
 #undef EVENT
 
-nsresult
-nsGenericHTMLElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                              nsIAtom* aPrefix, const nsAString& aValue,
-                              bool aNotify)
-{
-  bool contentEditable = aNameSpaceID == kNameSpaceID_None &&
-                           aName == nsGkAtoms::contenteditable;
-  bool accessKey = aName == nsGkAtoms::accesskey && 
-                     aNameSpaceID == kNameSpaceID_None;
-
-  int32_t change = 0;
-  if (contentEditable) {
-    change = GetContentEditableValue() == eTrue ? -1 : 0;
-    SetMayHaveContentEditableAttr();
-  }
-
-  if (accessKey) {
-    UnregAccessKey();
-  }
-
-  nsresult rv = nsStyledElement::SetAttr(aNameSpaceID, aName, aPrefix, aValue,
-                                         aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (contentEditable) {
-    if (aValue.IsEmpty() || aValue.LowerCaseEqualsLiteral("true")) {
-      change += 1;
-    }
-
-    ChangeEditableState(change);
-  }
-
-  if (accessKey && !aValue.IsEmpty()) {
-    SetFlags(NODE_HAS_ACCESSKEY);
-    RegAccessKey();
-  }
-
-  return NS_OK;
-}
-
-nsresult
-nsGenericHTMLElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                                bool aNotify)
-{
-  bool contentEditable = false;
-  int32_t contentEditableChange = 0;
-
-  // Check for event handlers
-  if (aNameSpaceID == kNameSpaceID_None) {
-    if (aAttribute == nsGkAtoms::name) {
-      // Have to do this before clearing flag. See RemoveFromNameTable
-      RemoveFromNameTable();
-      ClearHasName();
-    }
-    else if (aAttribute == nsGkAtoms::contenteditable) {
-      contentEditable = true;
-      contentEditableChange = GetContentEditableValue() == eTrue ? -1 : 0;
-    }
-    else if (aAttribute == nsGkAtoms::accesskey) {
-      // Have to unregister before clearing flag. See UnregAccessKey
-      UnregAccessKey();
-      UnsetFlags(NODE_HAS_ACCESSKEY);
-    }
-    else if (IsEventAttributeName(aAttribute)) {
-      if (EventListenerManager* manager = GetExistingListenerManager()) {
-        manager->RemoveEventHandler(aAttribute, EmptyString());
-      }
-    }
-  }
-
-  nsresult rv = nsGenericHTMLElementBase::UnsetAttr(aNameSpaceID, aAttribute,
-                                                    aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (contentEditable) {
-    ChangeEditableState(contentEditableChange);
-  }
-
-  return NS_OK;
-}
-
 void
 nsGenericHTMLElement::GetBaseTarget(nsAString& aBaseTarget) const
 {
@@ -929,19 +913,13 @@ nsGenericHTMLElement::ParseAttribute(int32_t aNamespaceID,
 
     if (aAttribute == nsGkAtoms::name) {
       // Store name as an atom.  name="" means that the element has no name,
-      // not that it has an emptystring as the name.
-      RemoveFromNameTable();
+      // not that it has an empty string as the name.
       if (aValue.IsEmpty()) {
         ClearHasName();
         return false;
       }
 
       aResult.ParseAtom(aValue);
-
-      if (CanHaveName(NodeInfo()->NameAtom())) {
-        SetHasName();
-        AddToNameTable(aResult.GetAtomValue());
-      }
       
       return true;
     }
diff --git a/dom/html/nsGenericHTMLElement.h b/dom/html/nsGenericHTMLElement.h
index b9c0340493..16f438acc0 100644
--- a/dom/html/nsGenericHTMLElement.h
+++ b/dom/html/nsGenericHTMLElement.h
@@ -465,17 +465,6 @@ public:
   virtual void UnbindFromTree(bool aDeep = true,
                               bool aNullParent = true) override;
 
-  MOZ_ALWAYS_INLINE // Avoid a crashy hook from Avast 10 Beta
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                             bool aNotify) override;
   virtual bool IsFocusableInternal(int32_t *aTabIndex, bool aWithMouse) override
   {
     bool isFocusable = false;
@@ -914,6 +903,9 @@ private:
   void RegUnRegAccessKey(bool aDoReg);
 
 protected:
+  virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                 const nsAttrValueOrString* aValue,
+                                 bool aNotify) override;
   virtual nsresult AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
                                 const nsAttrValue* aValue,
                                 const nsAttrValue* aOldValue,
diff --git a/dom/html/nsGenericHTMLFrameElement.cpp b/dom/html/nsGenericHTMLFrameElement.cpp
index 1aeb0a7d2e..03e6ffb421 100644
--- a/dom/html/nsGenericHTMLFrameElement.cpp
+++ b/dom/html/nsGenericHTMLFrameElement.cpp
@@ -334,55 +334,6 @@ nsGenericHTMLFrameElement::UnbindFromTree(bool aDeep, bool aNullParent)
   nsGenericHTMLElement::UnbindFromTree(aDeep, aNullParent);
 }
 
-nsresult
-nsGenericHTMLFrameElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                                   nsIAtom* aPrefix, const nsAString& aValue,
-                                   bool aNotify)
-{
-  nsresult rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix,
-                                              aValue, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::src &&
-      (!IsHTMLElement(nsGkAtoms::iframe) ||
-       !HasAttr(kNameSpaceID_None,nsGkAtoms::srcdoc))) {
-    // Don't propagate error here. The attribute was successfully set, that's
-    // what we should reflect.
-    LoadSrc();
-  } else if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::name) {
-    // Propagate "name" to the docshell to make browsing context names live,
-    // per HTML5.
-    nsIDocShell *docShell = mFrameLoader ? mFrameLoader->GetExistingDocShell()
-                                         : nullptr;
-    if (docShell) {
-      docShell->SetName(aValue);
-    }
-  }
-
-  return NS_OK;
-}
-
-nsresult
-nsGenericHTMLFrameElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                                     bool aNotify)
-{
-  // Invoke on the superclass.
-  nsresult rv = nsGenericHTMLElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (aNameSpaceID == kNameSpaceID_None && aAttribute == nsGkAtoms::name) {
-    // Propagate "name" to the docshell to make browsing context names live,
-    // per HTML5.
-    nsIDocShell *docShell = mFrameLoader ? mFrameLoader->GetExistingDocShell()
-                                         : nullptr;
-    if (docShell) {
-      docShell->SetName(EmptyString());
-    }
-  }
-
-  return NS_OK;
-}
-
 /* static */ int32_t
 nsGenericHTMLFrameElement::MapScrollingAttribute(const nsAttrValue* aValue)
 {
@@ -415,39 +366,88 @@ nsGenericHTMLFrameElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                         const nsAttrValue* aValue,
                                         const nsAttrValue* aOldValue, bool aNotify)
 {
-  if (aName == nsGkAtoms::scrolling && aNameSpaceID == kNameSpaceID_None) {
-    if (mFrameLoader) {
-      nsIDocShell* docshell = mFrameLoader->GetExistingDocShell();
-      nsCOMPtr<nsIScrollable> scrollable = do_QueryInterface(docshell);
-      if (scrollable) {
-        int32_t cur;
-        scrollable->GetDefaultScrollbarPreferences(nsIScrollable::ScrollOrientation_X, &cur);
-        int32_t val = MapScrollingAttribute(aValue);
-        if (cur != val) {
-          scrollable->SetDefaultScrollbarPreferences(nsIScrollable::ScrollOrientation_X, val);
-          scrollable->SetDefaultScrollbarPreferences(nsIScrollable::ScrollOrientation_Y, val);
-          RefPtr<nsPresContext> presContext;
-          docshell->GetPresContext(getter_AddRefs(presContext));
-          nsIPresShell* shell = presContext ? presContext->GetPresShell() : nullptr;
-          nsIFrame* rootScroll = shell ? shell->GetRootScrollFrame() : nullptr;
-          if (rootScroll) {
-            shell->FrameNeedsReflow(rootScroll, nsIPresShell::eStyleChange,
-                                    NS_FRAME_IS_DIRTY);
+  if (aValue) {
+    nsAttrValueOrString value(aValue);
+    AfterMaybeChangeAttr(aNameSpaceID, aName, &value, aNotify);
+  } else {
+    AfterMaybeChangeAttr(aNameSpaceID, aName, nullptr, aNotify);
+  }
+
+  if (aNameSpaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::scrolling) {
+      if (mFrameLoader) {
+        nsIDocShell* docshell = mFrameLoader->GetExistingDocShell();
+        nsCOMPtr<nsIScrollable> scrollable = do_QueryInterface(docshell);
+        if (scrollable) {
+          int32_t cur;
+          scrollable->GetDefaultScrollbarPreferences(nsIScrollable::ScrollOrientation_X, &cur);
+          int32_t val = MapScrollingAttribute(aValue);
+          if (cur != val) {
+            scrollable->SetDefaultScrollbarPreferences(nsIScrollable::ScrollOrientation_X, val);
+            scrollable->SetDefaultScrollbarPreferences(nsIScrollable::ScrollOrientation_Y, val);
+            RefPtr<nsPresContext> presContext;
+            docshell->GetPresContext(getter_AddRefs(presContext));
+            nsIPresShell* shell = presContext ? presContext->GetPresShell() : nullptr;
+            nsIFrame* rootScroll = shell ? shell->GetRootScrollFrame() : nullptr;
+            if (rootScroll) {
+              shell->FrameNeedsReflow(rootScroll, nsIPresShell::eStyleChange,
+                                      NS_FRAME_IS_DIRTY);
+            }
           }
         }
       }
+    } else if (aName == nsGkAtoms::mozbrowser) {
+      mReallyIsBrowser = !!aValue && BrowserFramesEnabled() &&
+                         PrincipalAllowsBrowserFrame(NodePrincipal());
     }
   }
 
-  if (aName == nsGkAtoms::mozbrowser && aNameSpaceID == kNameSpaceID_None) {
-    mReallyIsBrowser = !!aValue && BrowserFramesEnabled() &&
-                       PrincipalAllowsBrowserFrame(NodePrincipal());
-  }
-
   return nsGenericHTMLElement::AfterSetAttr(aNameSpaceID, aName, aValue,
                                             aOldValue, aNotify);
 }
 
+nsresult
+nsGenericHTMLFrameElement::OnAttrSetButNotChanged(int32_t aNamespaceID,
+                                                  nsIAtom* aName,
+                                                  const nsAttrValueOrString& aValue,
+                                                  bool aNotify)
+{
+  AfterMaybeChangeAttr(aNamespaceID, aName, &aValue, aNotify);
+
+  return nsGenericHTMLElement::OnAttrSetButNotChanged(aNamespaceID, aName,
+                                                      aValue, aNotify);
+}
+
+void
+nsGenericHTMLFrameElement::AfterMaybeChangeAttr(int32_t aNamespaceID,
+                                                nsIAtom* aName,
+                                                const nsAttrValueOrString* aValue,
+                                                bool aNotify)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::src) {
+      if (aValue && (!IsHTMLElement(nsGkAtoms::iframe) ||
+          !HasAttr(kNameSpaceID_None, nsGkAtoms::srcdoc))) {
+        // Don't propagate error here. The attribute was successfully set,
+        // that's what we should reflect.
+        LoadSrc();
+      }
+    } else if (aName == nsGkAtoms::name) {
+      // Propagate "name" to the docshell to make browsing context names live,
+      // per HTML5.
+      nsIDocShell* docShell = mFrameLoader ? mFrameLoader->GetExistingDocShell()
+                                           : nullptr;
+      if (docShell) {
+        if (aValue) {
+          docShell->SetName(aValue->String());
+        } else {
+          docShell->SetName(EmptyString());
+        }
+      }
+    }
+  }
+}
+
 void
 nsGenericHTMLFrameElement::DestroyContent()
 {
diff --git a/dom/html/nsGenericHTMLFrameElement.h b/dom/html/nsGenericHTMLFrameElement.h
index 1570b19563..adcd17715e 100644
--- a/dom/html/nsGenericHTMLFrameElement.h
+++ b/dom/html/nsGenericHTMLFrameElement.h
@@ -53,20 +53,7 @@ public:
                               bool aCompileEventHandlers) override;
   virtual void UnbindFromTree(bool aDeep = true,
                               bool aNullParent = true) override;
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) override;
-  virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                                const nsAttrValue* aValue,
-                                const nsAttrValue* aOldValue,
-                                bool aNotify) override;
+
   virtual void DestroyContent() override;
 
   nsresult CopyInnerTo(mozilla::dom::Element* aDest);
@@ -111,6 +98,14 @@ protected:
   nsresult GetContentDocument(nsIDOMDocument** aContentDocument);
   already_AddRefed<nsPIDOMWindowOuter> GetContentWindow();
 
+  virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
+  virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify) override;
+
   RefPtr<nsFrameLoader> mFrameLoader;
   nsCOMPtr<nsPIDOMWindowOuter> mOpenerWindow;
 
@@ -133,6 +128,18 @@ protected:
 
 private:
   void GetManifestURL(nsAString& aOut);
+
+  /**
+   * This function is called by AfterSetAttr and OnAttrSetButNotChanged.
+   * It will be called whether the value is being set or unset.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aValue the value being set or null if the value is being unset
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  void AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                            const nsAttrValueOrString* aValue, bool aNotify);
 };
 
 #endif // nsGenericHTMLFrameElement_h
diff --git a/dom/mathml/nsMathMLElement.cpp b/dom/mathml/nsMathMLElement.cpp
index e2bc4f0083..a74d8168c6 100644
--- a/dom/mathml/nsMathMLElement.cpp
+++ b/dom/mathml/nsMathMLElement.cpp
@@ -1085,50 +1085,27 @@ nsMathMLElement::GetHrefURI() const
 }
 
 nsresult
-nsMathMLElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                         nsIAtom* aPrefix, const nsAString& aValue,
-                         bool aNotify)
+nsMathMLElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
+                              const nsAttrValue* aValue,
+                              const nsAttrValue* aOldValue, bool aNotify)
 {
-  nsresult rv = nsMathMLElementBase::SetAttr(aNameSpaceID, aName, aPrefix,
-                                           aValue, aNotify);
-
-  // The ordering of the parent class's SetAttr call and Link::ResetLinkState
-  // is important here!  The attribute is not set until SetAttr returns, and
-  // we will need the updated attribute value because notifying the document
+  // It is important that this be done after the attribute is set/unset.
+  // We will need the updated attribute value because notifying the document
   // that content states have changed will call IntrinsicState, which will try
   // to get updated information about the visitedness from Link.
   if (aName == nsGkAtoms::href &&
       (aNameSpaceID == kNameSpaceID_None ||
        aNameSpaceID == kNameSpaceID_XLink)) {
-    if (aNameSpaceID == kNameSpaceID_XLink) {
+    if (aValue && aNameSpaceID == kNameSpaceID_XLink) {
       WarnDeprecated(u"xlink:href", u"href", OwnerDoc());
     }
-    Link::ResetLinkState(!!aNotify, true);
-  }
-
-  return rv;
-}
-
-nsresult
-nsMathMLElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttr,
-                           bool aNotify)
-{
-  nsresult rv = nsMathMLElementBase::UnsetAttr(aNameSpaceID, aAttr, aNotify);
-
-  // The ordering of the parent class's UnsetAttr call and Link::ResetLinkState
-  // is important here!  The attribute is not unset until UnsetAttr returns, and
-  // we will need the updated attribute value because notifying the document
-  // that content states have changed will call IntrinsicState, which will try
-  // to get updated information about the visitedness from Link.
-  if (aAttr == nsGkAtoms::href &&
-      (aNameSpaceID == kNameSpaceID_None ||
-       aNameSpaceID == kNameSpaceID_XLink)) {
-    // Note: just because we removed a single href attr doesn't mean there's no href,
-    // since there are 2 possible namespaces.
-    Link::ResetLinkState(!!aNotify, Link::ElementHasHref());
+    // Note: When unsetting href, there may still be another href since there
+    // are 2 possible namespaces.
+    Link::ResetLinkState(aNotify, aValue || Link::ElementHasHref());
   }
 
-  return rv;
+  return nsMathMLElementBase::AfterSetAttr(aNameSpaceID, aName, aValue,
+                                           aOldValue, aNotify);
 }
 
 JSObject*
diff --git a/dom/mathml/nsMathMLElement.h b/dom/mathml/nsMathMLElement.h
index bbcdb9771b..0fdaf021f5 100644
--- a/dom/mathml/nsMathMLElement.h
+++ b/dom/mathml/nsMathMLElement.h
@@ -93,16 +93,6 @@ public:
   virtual bool IsLink(nsIURI** aURI) const override;
   virtual void GetLinkTarget(nsAString& aTarget) override;
   virtual already_AddRefed<nsIURI> GetHrefURI() const override;
-  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                   const nsAString& aValue, bool aNotify)
-  {
-    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
-  }
-  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                           nsIAtom* aPrefix, const nsAString& aValue,
-                           bool aNotify) override;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) override;
 
   virtual nsIDOMNode* AsDOMNode() override { return this; }
 
@@ -111,6 +101,11 @@ protected:
 
   virtual JSObject* WrapNode(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override;
 
+  virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
+                                const nsAttrValue* aValue,
+                                const nsAttrValue* aOldValue,
+                                bool aNotify) override;
+
 private:
   bool mIncrementScriptLevel;
 };
diff --git a/dom/xbl/XBLChildrenElement.cpp b/dom/xbl/XBLChildrenElement.cpp
index e4058a7890..f785a3058a 100644
--- a/dom/xbl/XBLChildrenElement.cpp
+++ b/dom/xbl/XBLChildrenElement.cpp
@@ -7,6 +7,7 @@
 #include "mozilla/dom/XBLChildrenElement.h"
 #include "nsCharSeparatedTokenizer.h"
 #include "mozilla/dom/NodeListBinding.h"
+#include "nsAttrValueOrString.h"
 
 namespace mozilla {
 namespace dom {
@@ -27,34 +28,24 @@ NS_INTERFACE_MAP_END_INHERITING(Element)
 NS_IMPL_ELEMENT_CLONE(XBLChildrenElement)
 
 nsresult
-XBLChildrenElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                              bool aNotify)
+XBLChildrenElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                  const nsAttrValueOrString* aValue,
+                                  bool aNotify)
 {
-  if (aAttribute == nsGkAtoms::includes &&
-      aNameSpaceID == kNameSpaceID_None) {
-    mIncludes.Clear();
-  }
-
-  return Element::UnsetAttr(aNameSpaceID, aAttribute, aNotify);
-}
-
-bool
-XBLChildrenElement::ParseAttribute(int32_t aNamespaceID,
-                                   nsIAtom* aAttribute,
-                                   const nsAString& aValue,
-                                   nsAttrValue& aResult)
-{
-  if (aAttribute == nsGkAtoms::includes &&
-      aNamespaceID == kNameSpaceID_None) {
-    mIncludes.Clear();
-    nsCharSeparatedTokenizer tok(aValue, '|',
-                                 nsCharSeparatedTokenizer::SEPARATOR_OPTIONAL);
-    while (tok.hasMoreTokens()) {
-      mIncludes.AppendElement(NS_Atomize(tok.nextToken()));
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aName == nsGkAtoms::includes) {
+      mIncludes.Clear();
+      if (aValue) {
+        nsCharSeparatedTokenizer tok(aValue->String(), '|',
+                                     nsCharSeparatedTokenizer::SEPARATOR_OPTIONAL);
+        while (tok.hasMoreTokens()) {
+          mIncludes.AppendElement(NS_Atomize(tok.nextToken()));
+        }
+      }
     }
   }
 
-  return false;
+  return nsXMLElement::BeforeSetAttr(aNamespaceID, aName, aValue, aNotify);
 }
 
 } // namespace dom
diff --git a/dom/xbl/XBLChildrenElement.h b/dom/xbl/XBLChildrenElement.h
index 4714da4a84..c010854742 100644
--- a/dom/xbl/XBLChildrenElement.h
+++ b/dom/xbl/XBLChildrenElement.h
@@ -37,14 +37,6 @@ public:
 
   virtual nsIDOMNode* AsDOMNode() override { return this; }
 
-  // nsIContent interface methods
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) override;
-  virtual bool ParseAttribute(int32_t aNamespaceID,
-                              nsIAtom* aAttribute,
-                              const nsAString& aValue,
-                              nsAttrValue& aResult) override;
-
   void AppendInsertedChild(nsIContent* aChild)
   {
     mInsertedChildren.AppendElement(aChild);
@@ -147,6 +139,10 @@ public:
 protected:
   ~XBLChildrenElement();
 
+  virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                 const nsAttrValueOrString* aValue,
+                                 bool aNotify) override;
+
 private:
   nsTArray<nsIContent*> mInsertedChildren; // WEAK
   nsTArray<nsCOMPtr<nsIAtom> > mIncludes;