From c25009c80b18a6af1a0f243757d426220507563b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 2 Sep 2019 10:43:52 +0000 Subject: [PATCH] Bug 1577721 - Unify attribute cloning between XUL / HTML and everything else. r=bzbarsky This contains an (intentional) behavior change, which is that we always copy (i.e. don't reparse) style attributes, even across documents. XUL and HTML already had this behavior. This makes stuff like SVG and MathML consistent with that. Depends on D44128 Differential Revision: https://phabricator.services.mozilla.com/D44129 --HG-- extra : moz-landing-system : lando --- dom/base/Element.cpp | 40 ++++++++++---- dom/base/Element.h | 4 +- dom/html/nsGenericHTMLElement.cpp | 46 ++-------------- dom/xul/nsXULElement.cpp | 52 +------------------ ...yle-allowed-while-cloning-objects.sub.html | 9 ++++ 5 files changed, 47 insertions(+), 104 deletions(-) diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp index b67d20da9a41..77f774ad12a3 100644 --- a/dom/base/Element.cpp +++ b/dom/base/Element.cpp @@ -3226,19 +3226,39 @@ nsDOMTokenList* Element::GetTokenList( return list; } -nsresult Element::CopyInnerTo(Element* aDst) { +nsresult Element::CopyInnerTo(Element* aDst, ReparseAttributes aReparse) { nsresult rv = aDst->mAttrs.EnsureCapacityToClone(mAttrs); NS_ENSURE_SUCCESS(rv, rv); - uint32_t i, count = mAttrs.AttrCount(); - for (i = 0; i < count; ++i) { - const nsAttrName* name = mAttrs.AttrNameAt(i); - const nsAttrValue* value = mAttrs.AttrAt(i); - nsAutoString valStr; - value->ToString(valStr); - rv = aDst->SetAttr(name->NamespaceID(), name->LocalName(), - name->GetPrefix(), valStr, false); - NS_ENSURE_SUCCESS(rv, rv); + const bool reparse = aReparse == ReparseAttributes::Yes; + + uint32_t count = mAttrs.AttrCount(); + for (uint32_t i = 0; i < count; ++i) { + BorrowedAttrInfo info = mAttrs.AttrInfoAt(i); + const nsAttrName* name = info.mName; + const nsAttrValue* value = info.mValue; + if (value->Type() == nsAttrValue::eCSSDeclaration) { + MOZ_ASSERT(name->Equals(nsGkAtoms::style, kNameSpaceID_None)); + // We still clone CSS attributes, even in the `reparse` (cross-document) + // case. https://github.com/w3c/webappsec-csp/issues/212 + nsAttrValue valueCopy(*value); + rv = aDst->SetParsedAttr(name->NamespaceID(), name->LocalName(), + name->GetPrefix(), valueCopy, false); + NS_ENSURE_SUCCESS(rv, rv); + + value->GetCSSDeclarationValue()->SetImmutable(); + } else if (reparse) { + nsAutoString valStr; + value->ToString(valStr); + rv = aDst->SetAttr(name->NamespaceID(), name->LocalName(), + name->GetPrefix(), valStr, false); + NS_ENSURE_SUCCESS(rv, rv); + } else { + nsAttrValue valueCopy(*value); + rv = aDst->SetParsedAttr(name->NamespaceID(), name->LocalName(), + name->GetPrefix(), valueCopy, false); + NS_ENSURE_SUCCESS(rv, rv); + } } return NS_OK; diff --git a/dom/base/Element.h b/dom/base/Element.h index 7944c56bd408..061be0bf28e6 100644 --- a/dom/base/Element.h +++ b/dom/base/Element.h @@ -1910,11 +1910,13 @@ class Element : public FragmentOrElement { nsAtom* aAtom, const DOMTokenListSupportedTokenArray aSupportedTokens = nullptr); + enum class ReparseAttributes { No, Yes }; /** * Copy attributes and state to another element * @param aDest the object to copy to */ - nsresult CopyInnerTo(Element* aDest); + nsresult CopyInnerTo(Element* aDest, + ReparseAttributes = ReparseAttributes::Yes); private: /** diff --git a/dom/html/nsGenericHTMLElement.cpp b/dom/html/nsGenericHTMLElement.cpp index b24c50f29ef8..bfb35bd3ae23 100644 --- a/dom/html/nsGenericHTMLElement.cpp +++ b/dom/html/nsGenericHTMLElement.cpp @@ -111,49 +111,9 @@ nsresult nsGenericHTMLElement::CopyInnerTo(Element* aDst) { MOZ_ASSERT(!aDst->GetUncomposedDoc(), "Should not CopyInnerTo an Element in a document"); - bool reparse = (aDst->OwnerDoc() != OwnerDoc()); - - nsresult rv = - static_cast(aDst)->mAttrs.EnsureCapacityToClone( - mAttrs); - NS_ENSURE_SUCCESS(rv, rv); - - int32_t i, count = GetAttrCount(); - for (i = 0; i < count; ++i) { - const nsAttrName* name = mAttrs.AttrNameAt(i); - const nsAttrValue* value = mAttrs.AttrAt(i); - - if (name->Equals(nsGkAtoms::style, kNameSpaceID_None) && - value->Type() == nsAttrValue::eCSSDeclaration) { - // We still clone CSS attributes, even in the cross-document case. - // https://github.com/w3c/webappsec-csp/issues/212 - - // We can't just set this as a string, because that will fail - // to reparse the string into style data until the node is - // inserted into the document. Clone the Rule instead. - nsAttrValue valueCopy(*value); - rv = aDst->SetParsedAttr(name->NamespaceID(), name->LocalName(), - name->GetPrefix(), valueCopy, false); - NS_ENSURE_SUCCESS(rv, rv); - - DeclarationBlock* cssDeclaration = value->GetCSSDeclarationValue(); - cssDeclaration->SetImmutable(); - } else if (reparse) { - nsAutoString valStr; - value->ToString(valStr); - - rv = aDst->SetAttr(name->NamespaceID(), name->LocalName(), - name->GetPrefix(), valStr, false); - NS_ENSURE_SUCCESS(rv, rv); - } else { - nsAttrValue valueCopy(*value); - rv = aDst->SetParsedAttr(name->NamespaceID(), name->LocalName(), - name->GetPrefix(), valueCopy, false); - NS_ENSURE_SUCCESS(rv, rv); - } - } - - return NS_OK; + auto reparse = aDst->OwnerDoc() == OwnerDoc() ? ReparseAttributes::No + : ReparseAttributes::Yes; + return Element::CopyInnerTo(aDst, reparse); } static const nsAttrValue::EnumTable kDirTable[] = { diff --git a/dom/xul/nsXULElement.cpp b/dom/xul/nsXULElement.cpp index 363a8a1c7ffe..950d5faaaa23 100644 --- a/dom/xul/nsXULElement.cpp +++ b/dom/xul/nsXULElement.cpp @@ -307,60 +307,12 @@ nsresult nsXULElement::Clone(mozilla::dom::NodeInfo* aNodeInfo, RefPtr ni = aNodeInfo; RefPtr element = Construct(ni.forget()); - nsresult rv = element->mAttrs.EnsureCapacityToClone(mAttrs); + nsresult rv = const_cast(this)->CopyInnerTo( + element, ReparseAttributes::No); NS_ENSURE_SUCCESS(rv, rv); // Note that we're _not_ copying mControllers. - uint32_t count = mAttrs.AttrCount(); - rv = NS_OK; - // FIXME(emilio): This setup looks somewhat error prone. Other than the - // AddListenerFor bit (what is that for?), everything else could be handled by - // the generic Element::CopyInnerTo, or the equivalent in - // nsGenericHTMLElement (somehow), both of which go through AfterSetAttr... - for (uint32_t i = 0; i < count; ++i) { - const nsAttrName* originalName = mAttrs.AttrNameAt(i); - const nsAttrValue* originalValue = mAttrs.AttrAt(i); - nsAttrValue attrValue; - - // Style rules need to be cloned. - if (originalValue->Type() == nsAttrValue::eCSSDeclaration) { - DeclarationBlock* decl = originalValue->GetCSSDeclarationValue(); - RefPtr declClone = decl->Clone(); - - nsString stringValue; - originalValue->ToString(stringValue); - - attrValue.SetTo(declClone.forget(), &stringValue); - } else { - attrValue.SetTo(*originalValue); - } - - bool oldValueSet; - if (originalName->IsAtom()) { - rv = element->mAttrs.SetAndSwapAttr(originalName->Atom(), attrValue, - &oldValueSet); - } else { - rv = element->mAttrs.SetAndSwapAttr(originalName->NodeInfo(), attrValue, - &oldValueSet); - } - NS_ENSURE_SUCCESS(rv, rv); - element->AddListenerFor(*originalName); - if (originalName->Equals(nsGkAtoms::id) && - !originalValue->IsEmptyString()) { - element->SetHasID(); - } - if (originalName->Equals(nsGkAtoms::_class)) { - element->SetMayHaveClass(); - } - if (originalName->Equals(nsGkAtoms::style)) { - element->SetMayHaveStyle(); - } - if (originalName->Equals(nsGkAtoms::part)) { - element->SetHasPartAttribute(true); - } - } - element.forget(aResult); return rv; } diff --git a/testing/web-platform/tests/content-security-policy/style-src/inline-style-allowed-while-cloning-objects.sub.html b/testing/web-platform/tests/content-security-policy/style-src/inline-style-allowed-while-cloning-objects.sub.html index 7c95f47aff88..d222743049e3 100644 --- a/testing/web-platform/tests/content-security-policy/style-src/inline-style-allowed-while-cloning-objects.sub.html +++ b/testing/web-platform/tests/content-security-policy/style-src/inline-style-allowed-while-cloning-objects.sub.html @@ -111,6 +111,14 @@ test(function() { assert_equals(ops.id, clonedOps.id) }); + test(function() { + let el = document.getElementById("svg"); + assert_equals(el.getAttribute("style"), ""); + el.style.background = violetOps.style.background; + assert_not_equals(el.style.background, ""); + let clone = el.cloneNode(true); + assert_equals(el.style.background, clone.style.background) + }, "non-HTML namespace"); } @@ -131,6 +139,7 @@
Yet another div.
+