Removing Attr can delete a wrong Attribute in ElementData
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 May 2013 16:52:30 +0000 (16:52 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 May 2013 16:52:30 +0000 (16:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=116077

Source/WebCore:

Reviewed by Benjamin Poulain.

Merge https://chromium.googlesource.com/chromium/blink/+/e861452a292e185501e48940305947aa6a4e23c2
after simplifying and renaming functions to be more WebKit style.

The XML parser can produce elements with attributes whose names have
distinct prefixes, but the same expanded name. When one of these
attributes is put up for adoption, it may be its similarly named
sibling that is removed from its owner element. As a result the
original owner hangs onto the adopted attribute, despite the fact that
it is now in a different document. Sometimes it's just hard to let go.

Test: fast/dom/adopt-attribute-crash.svg

* dom/Element.cpp:
(WebCore::Element::setAttributeNode):
(WebCore::Element::removeAttributeNode):
(WebCore::ElementData::getAttributeItemIndex):
* dom/Element.h:
(ElementData):
(Element):

LayoutTests:

Reviewed by Benjamin Poulain.

Add a regression test by importing
https://chromium.googlesource.com/chromium/blink/+/e861452a292e185501e48940305947aa6a4e23c2

* fast/dom/adopt-attribute-crash-expected.txt: Added.
* fast/dom/adopt-attribute-crash.svg: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@150072 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/dom/adopt-attribute-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/adopt-attribute-crash.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h

index b890e49793b8930d5decdd49a16713051dd0d181..7cc4cbd8ac9028766c2ba4a97b72243b29ab1d34 100644 (file)
@@ -1,3 +1,16 @@
+2013-05-13  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Removing Attr can delete a wrong Attribute in ElementData
+        https://bugs.webkit.org/show_bug.cgi?id=116077
+
+        Reviewed by Benjamin Poulain.
+
+        Add a regression test by importing
+        https://chromium.googlesource.com/chromium/blink/+/e861452a292e185501e48940305947aa6a4e23c2
+
+        * fast/dom/adopt-attribute-crash-expected.txt: Added.
+        * fast/dom/adopt-attribute-crash.svg: Added.
+
 2013-05-14  Andrei Bucur  <abucur@adobe.com>
 
         Modify checkLayout to receive the log container as an optional parameter
diff --git a/LayoutTests/fast/dom/adopt-attribute-crash-expected.txt b/LayoutTests/fast/dom/adopt-attribute-crash-expected.txt
new file mode 100644 (file)
index 0000000..fcd3bae
--- /dev/null
@@ -0,0 +1,8 @@
+ALERT: Tests adopting two Attr nodes of the same qualified name. WebKit shouldn't crash or assert.
+This page contains the following errors:
+
+error on line 3 at column 35: Namespaced Attribute href in 'http://www.w3.org/1999/xlink' redefined
+error on line 18 at column 10: Extra content at the end of the document
+Below is a rendering of the page up to the first error.
+
+
diff --git a/LayoutTests/fast/dom/adopt-attribute-crash.svg b/LayoutTests/fast/dom/adopt-attribute-crash.svg
new file mode 100644 (file)
index 0000000..2dd65e7
--- /dev/null
@@ -0,0 +1,18 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<svg xmlns:xl="http://www.w3.org/1999/xlink" xmlns:xlink="http://www.w3.org/1999/xlink">
+<linearGradient xl:href="#tCF2" xlink:href="resources/smiley.png"></linearGradient>
+</svg>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+var element = document.querySelector('linearGradient');
+var attr1 = element.attributes[0];
+var attr2 = element.attributes[1];
+var doc2 = document.implementation.createHTMLDocument('Doc 2');
+var attr3 = doc2.adoptNode(attr2);
+var doc3 = document.implementation.createHTMLDocument('Doc 3');
+var attr4 = doc3.adoptNode(attr3);
+
+alert("Tests adopting two Attr nodes of the same qualified name. WebKit shouldn't crash or assert.");
+</script>
index 3dfe71c3f94828d394916dd0984ec7436994e865..9571a122640c500f0316487c80cfa6961719d7f2 100644 (file)
@@ -1,3 +1,30 @@
+2013-05-13  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Removing Attr can delete a wrong Attribute in ElementData
+        https://bugs.webkit.org/show_bug.cgi?id=116077
+
+        Reviewed by Benjamin Poulain.
+        
+        Merge https://chromium.googlesource.com/chromium/blink/+/e861452a292e185501e48940305947aa6a4e23c2
+        after simplifying and renaming functions to be more WebKit style.
+
+        The XML parser can produce elements with attributes whose names have
+        distinct prefixes, but the same expanded name. When one of these
+        attributes is put up for adoption, it may be its similarly named
+        sibling that is removed from its owner element. As a result the
+        original owner hangs onto the adopted attribute, despite the fact that
+        it is now in a different document. Sometimes it's just hard to let go.
+
+        Test: fast/dom/adopt-attribute-crash.svg
+
+        * dom/Element.cpp:
+        (WebCore::Element::setAttributeNode):
+        (WebCore::Element::removeAttributeNode):
+        (WebCore::ElementData::getAttributeItemIndex):
+        * dom/Element.h:
+        (ElementData):
+        (Element):
+
 2013-05-14  Antti Koivisto  <antti@apple.com>
 
         Remove ::-webkit-distributed()
index 31c243d3428822135ad50e4e0524744eadd7a69f..df78cdfe6c3ef3d331ba3c7ac7c13755703816f9 100644 (file)
@@ -1729,7 +1729,7 @@ PassRefPtr<Attr> Element::setAttributeNode(Attr* attrNode, ExceptionCode& ec)
     synchronizeAllAttributes();
     UniqueElementData* elementData = ensureUniqueElementData();
 
-    unsigned index = elementData->getAttributeItemIndex(attrNode->qualifiedName());
+    unsigned index = elementData->getAttributeItemIndexForAttributeNode(attrNode);
     if (index != ElementData::attributeNotFound) {
         if (oldAttrNode)
             detachAttrNodeFromElementWithValue(oldAttrNode.get(), elementData->attributeItem(index)->value());
@@ -1765,13 +1765,16 @@ PassRefPtr<Attr> Element::removeAttributeNode(Attr* attr, ExceptionCode& ec)
 
     synchronizeAttribute(attr->qualifiedName());
 
-    unsigned index = elementData()->getAttributeItemIndex(attr->qualifiedName());
+    unsigned index = elementData()->getAttributeItemIndexForAttributeNode(attr);
     if (index == ElementData::attributeNotFound) {
         ec = NOT_FOUND_ERR;
         return 0;
     }
 
-    return detachAttribute(index);
+    RefPtr<Attr> attrNode = attr;
+    detachAttrNodeFromElementWithValue(attr, elementData()->attributeItem(index)->value());
+    removeAttributeInternal(index, NotInSynchronizationOfLazyAttribute);
+    return attrNode.release();
 }
 
 bool Element::parseAttributeName(QualifiedName& out, const AtomicString& namespaceURI, const AtomicString& qualifiedName, ExceptionCode& ec)
@@ -3140,9 +3143,22 @@ unsigned ElementData::getAttributeItemIndexSlowCase(const AtomicString& name, bo
     return attributeNotFound;
 }
 
+unsigned ElementData::getAttributeItemIndexForAttributeNode(const Attr* attr) const
+{
+    ASSERT(attr);
+    const Attribute* attributes = attributeBase();
+    unsigned count = length();
+    for (unsigned i = 0; i < count; ++i) {
+        if (attributes[i].name() == attr->qualifiedName())
+            return i;
+    }
+    return attributeNotFound;
+}
+
 Attribute* UniqueElementData::getAttributeItem(const QualifiedName& name)
 {
-    for (unsigned i = 0; i < length(); ++i) {
+    unsigned count = length();
+    for (unsigned i = 0; i < count; ++i) {
         if (m_attributeVector.at(i).name().matches(name))
             return &m_attributeVector.at(i);
     }
index f8d3047a93406a1ac5281adbf11039ced986eabb..3bc7e8ef9f5078df99a4ea9c9fe8d54e13f7ad3d 100644 (file)
@@ -78,6 +78,7 @@ public:
     const Attribute* getAttributeItem(const QualifiedName&) const;
     unsigned getAttributeItemIndex(const QualifiedName&) const;
     unsigned getAttributeItemIndex(const AtomicString& name, bool shouldIgnoreAttributeCase) const;
+    unsigned getAttributeItemIndexForAttributeNode(const Attr*) const;
 
     bool hasID() const { return !m_idForStyleResolution.isNull(); }
     bool hasClass() const { return !m_classNames.isNull(); }
@@ -729,6 +730,7 @@ private:
 
     void detachAllAttrNodesFromElement();
     void detachAttrNodeFromElementWithValue(Attr*, const AtomicString& value);
+    void detachAttrNodeAtIndex(Attr*, size_t index);
 
     void createRendererIfNeeded();