REGRESSION (r149652): Videos do not play on cnn.com, just black box
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 May 2013 16:38:21 +0000 (16:38 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 May 2013 16:38:21 +0000 (16:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=115887

Reviewed by Antti Koivisto.

Source/WebCore:

The bug was caused by window and document named item maps counting the same element twice
when it has the same id and name attribute values. Fixed the bug by avoiding to add or remove
an element per id and name attribute updates when it had already been added or removed by
name and id attribute updates respectively.

We do this by checking whether the other attribute affects the element's precense in window
and document named item maps and avoiding to add or remove the attribute when they do and
the other attribute is present in updateId and updateName.

Consider a scenario when an object element has id "foo", and name attribute is about to be also
set to "foo". If the id attribute doesn't affect element's presense in window or document
named item maps, we're done. If it does, then the maps already have this element so we don't
want to add it again. Conversely, if the element already has id and name attributes set to
"foo", and we're moving the id attribute, then we want to remove the element from the maps only
if the id doesn't affect the presence of the element in the maps.

Unfortuntely, this logic doesn't work when we're inserting or removing an element on its entirely
because updateId and updateName are called when both id and name attributes are present so skip
this step (AlwaysUpdateHTMLDocumentNamedItemMaps) for the id attribute to break the symmetry.

Test: fast/dom/HTMLDocument/image-with-same-id-and-name.html
      fast/dom/HTMLDocument/object-with-same-id-and-name.html

* dom/Element.cpp:
(WebCore::Element::insertedInto): Call updateId and updateName with
AlwaysUpdateHTMLDocumentNamedItemMaps.
(WebCore::Element::removedFrom): Ditto.
(WebCore::Element::updateName): Don't add or remove this element if the id attribute has already
done so except when we're inserting, removing, or cloning an element.
(WebCore::Element::updateId): Ditto for the name attribute.
(WebCore::Element::cloneAttributesFromElement): Added a comment and assert that we never call this
function when this element is in the document. We can't update window and documemt named item
maps here because image element's id attribute value, for example, is present in the document's
named item map if it has a name attribute. Since this function calls updateId and updateName
before updating attributes, this check is going to fail in DocumentNameCollection's
nodeMatchesIfIdAttributeMatch and bad things will happen.

* dom/Element.h:

* editing/ReplaceNodeWithSpanCommand.cpp:
(WebCore::swapInNodePreservingAttributesAndChildren): Clone children and attributes before
inserting the swapped span to avoid hitting the assertion in cloneAttributesFromElement we added.

* html/HTMLImageElement.cpp:
(WebCore::HTMLImageElement::parseAttribute):

* html/HTMLObjectElement.cpp:
(WebCore::HTMLObjectElement::updateDocNamedItem):

LayoutTests:

Add regression tests.

* fast/dom/HTMLDocument/image-with-same-id-and-name-expected.txt: Added.
* fast/dom/HTMLDocument/image-with-same-id-and-name.html: Added.
* fast/dom/HTMLDocument/object-with-same-id-and-name-expected.txt: Added.
* fast/dom/HTMLDocument/object-with-same-id-and-name.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/dom/HTMLDocument/object-with-same-id-and-name-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/HTMLDocument/object-with-same-id-and-name.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h
Source/WebCore/editing/ReplaceNodeWithSpanCommand.cpp
Source/WebCore/html/HTMLImageElement.cpp
Source/WebCore/html/HTMLObjectElement.cpp

index fb83db0ce22228215efdffb4bbc3a126ee53eb2e..550c9f5723dc3f28b6935db98d0fc34e485c6965 100644 (file)
@@ -1,3 +1,17 @@
+2013-05-10  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION (r149652): Videos do not play on cnn.com, just black box
+        https://bugs.webkit.org/show_bug.cgi?id=115887
+
+        Reviewed by Antti Koivisto.
+
+        Add regression tests.
+
+        * fast/dom/HTMLDocument/image-with-same-id-and-name-expected.txt: Added.
+        * fast/dom/HTMLDocument/image-with-same-id-and-name.html: Added.
+        * fast/dom/HTMLDocument/object-with-same-id-and-name-expected.txt: Added.
+        * fast/dom/HTMLDocument/object-with-same-id-and-name.html: Added.
+
 2013-05-10  Christophe Dumez  <ch.dumez@sisa.samsung.com>
 
         Unreviewed EFL gardening.
diff --git a/LayoutTests/fast/dom/HTMLDocument/object-with-same-id-and-name-expected.txt b/LayoutTests/fast/dom/HTMLDocument/object-with-same-id-and-name-expected.txt
new file mode 100644 (file)
index 0000000..83a611e
--- /dev/null
@@ -0,0 +1,22 @@
+Test that the document's name getter returns an object element with id and name attributes set to the same value.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+objectElement = document.querySelector("object#someName");
+PASS objectElement.getAttribute("name") is "someName"
+PASS document.someName is objectElement
+PASS objectElement.removeAttribute("id"); document.someName is objectElement
+PASS objectElement.setAttribute("name", "otherName"); document.someName is undefined.
+PASS document.otherName is objectElement
+PASS objectElement.setAttribute("id", "otherName"); document.otherName is objectElement
+PASS objectElement.parentNode.removeChild(objectElement); document.otherName is undefined.
+PASS document.body.appendChild(objectElement); document.otherName is objectElement
+PASS objectElement.setAttribute("id", "anotherName"); document.anotherName is objectElement
+PASS document.otherName is objectElement
+PASS objectElement.setAttribute("name", "anotherName"); document.otherName is undefined.
+PASS document.anotherName is objectElement
+imageElement = document.createElement("img"); document.body.appendChild(imageElement); imageElement.setAttribute("name", "anotherName");
+PASS objectElement.parentNode.removeChild(objectElement); document.otherName is undefined.
+PASS document.anotherName is imageElement
diff --git a/LayoutTests/fast/dom/HTMLDocument/object-with-same-id-and-name.html b/LayoutTests/fast/dom/HTMLDocument/object-with-same-id-and-name.html
new file mode 100644 (file)
index 0000000..127c70e
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<body>
+<object id="someName" name="someName"></object>
+<script src="../../js/resources/js-test-pre.js"></script>
+<script>
+
+description("Test that the document's name getter returns an object element with id and name attributes set to the same value.");
+
+evalAndLog('objectElement = document.querySelector("object#someName");');
+shouldBe('objectElement.getAttribute("name")', '"someName"');
+shouldBe('document.someName', 'objectElement');
+shouldBe('objectElement.removeAttribute("id"); document.someName', 'objectElement');
+shouldBeUndefined('objectElement.setAttribute("name", "otherName"); document.someName');
+shouldBe('document.otherName', 'objectElement');
+shouldBe('objectElement.setAttribute("id", "otherName"); document.otherName', 'objectElement');
+shouldBeUndefined('objectElement.parentNode.removeChild(objectElement); document.otherName');
+shouldBe('document.body.appendChild(objectElement); document.otherName', 'objectElement');
+shouldBe('objectElement.setAttribute("id", "anotherName"); document.anotherName', 'objectElement');
+shouldBe('document.otherName', 'objectElement');
+shouldBeUndefined('objectElement.setAttribute("name", "anotherName"); document.otherName');
+shouldBe('document.anotherName', 'objectElement');
+evalAndLog('imageElement = document.createElement("img"); document.body.appendChild(imageElement); imageElement.setAttribute("name", "anotherName");');
+shouldBeUndefined('objectElement.parentNode.removeChild(objectElement); document.otherName');
+shouldBe('document.anotherName', 'imageElement');
+
+</script>
+</body>
+</html>
index 7b69696b0df0b58f045563794b5ddc22babb6316..bfc1f6b45d57ea8cc762ca796bdf82d5d97b5bf2 100644 (file)
@@ -1,3 +1,59 @@
+2013-05-10  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION (r149652): Videos do not play on cnn.com, just black box
+        https://bugs.webkit.org/show_bug.cgi?id=115887
+
+        Reviewed by Antti Koivisto.
+
+        The bug was caused by window and document named item maps counting the same element twice
+        when it has the same id and name attribute values. Fixed the bug by avoiding to add or remove
+        an element per id and name attribute updates when it had already been added or removed by
+        name and id attribute updates respectively.
+
+        We do this by checking whether the other attribute affects the element's precense in window
+        and document named item maps and avoiding to add or remove the attribute when they do and
+        the other attribute is present in updateId and updateName.
+
+        Consider a scenario when an object element has id "foo", and name attribute is about to be also
+        set to "foo". If the id attribute doesn't affect element's presense in window or document
+        named item maps, we're done. If it does, then the maps already have this element so we don't
+        want to add it again. Conversely, if the element already has id and name attributes set to
+        "foo", and we're moving the id attribute, then we want to remove the element from the maps only
+        if the id doesn't affect the presence of the element in the maps.
+
+        Unfortuntely, this logic doesn't work when we're inserting or removing an element on its entirely
+        because updateId and updateName are called when both id and name attributes are present so skip
+        this step (AlwaysUpdateHTMLDocumentNamedItemMaps) for the id attribute to break the symmetry.
+
+        Test: fast/dom/HTMLDocument/image-with-same-id-and-name.html
+              fast/dom/HTMLDocument/object-with-same-id-and-name.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::insertedInto): Call updateId and updateName with
+        AlwaysUpdateHTMLDocumentNamedItemMaps.
+        (WebCore::Element::removedFrom): Ditto.
+        (WebCore::Element::updateName): Don't add or remove this element if the id attribute has already
+        done so except when we're inserting, removing, or cloning an element.
+        (WebCore::Element::updateId): Ditto for the name attribute.
+        (WebCore::Element::cloneAttributesFromElement): Added a comment and assert that we never call this
+        function when this element is in the document. We can't update window and documemt named item
+        maps here because image element's id attribute value, for example, is present in the document's
+        named item map if it has a name attribute. Since this function calls updateId and updateName
+        before updating attributes, this check is going to fail in DocumentNameCollection's
+        nodeMatchesIfIdAttributeMatch and bad things will happen.
+
+        * dom/Element.h:
+
+        * editing/ReplaceNodeWithSpanCommand.cpp:
+        (WebCore::swapInNodePreservingAttributesAndChildren): Clone children and attributes before
+        inserting the swapped span to avoid hitting the assertion in cloneAttributesFromElement we added.
+
+        * html/HTMLImageElement.cpp:
+        (WebCore::HTMLImageElement::parseAttribute):
+
+        * html/HTMLObjectElement.cpp:
+        (WebCore::HTMLObjectElement::updateDocNamedItem):
+
 2013-05-09  Dean Jackson  <dino@apple.com>
 
         Don't trust character widths for internal OS X fonts in form controls
index a3384c9f514e7b838d3c4e045652bc7eeb84f82d..31c243d3428822135ad50e4e0524744eadd7a69f 100644 (file)
@@ -1177,7 +1177,7 @@ Node::InsertionNotificationRequest Element::insertedInto(ContainerNode* insertio
 
     const AtomicString& idValue = getIdAttribute();
     if (!idValue.isNull())
-        updateId(scope, nullAtom, idValue);
+        updateId(scope, nullAtom, idValue, AlwaysUpdateHTMLDocumentNamedItemMaps);
 
     const AtomicString& nameValue = getNameAttribute();
     if (!nameValue.isNull())
@@ -1220,7 +1220,7 @@ void Element::removedFrom(ContainerNode* insertionPoint)
     if (insertionPoint->isInTreeScope() && treeScope() == document()) {
         const AtomicString& idValue = getIdAttribute();
         if (!idValue.isNull())
-            updateId(insertionPoint->treeScope(), idValue, nullAtom);
+            updateId(insertionPoint->treeScope(), idValue, nullAtom, AlwaysUpdateHTMLDocumentNamedItemMaps);
 
         const AtomicString& nameValue = getNameAttribute();
         if (!nameValue.isNull())
@@ -2648,16 +2648,18 @@ void Element::updateName(TreeScope* scope, const AtomicString& oldName, const At
         return;
 
     if (WindowNameCollection::nodeMatchesIfNameAttributeMatch(this)) {
-        if (!oldName.isEmpty())
+        const AtomicString& id = WindowNameCollection::nodeMatchesIfIdAttributeMatch(this) ? getIdAttribute() : nullAtom;
+        if (!oldName.isEmpty() && oldName != id)
             toHTMLDocument(ownerDocument)->windowNamedItemMap().remove(oldName.impl(), this);
-        if (!newName.isEmpty())
+        if (!newName.isEmpty() && newName != id)
             toHTMLDocument(ownerDocument)->windowNamedItemMap().add(newName.impl(), this);
     }
 
     if (DocumentNameCollection::nodeMatchesIfNameAttributeMatch(this)) {
-        if (!oldName.isEmpty())
+        const AtomicString& id = DocumentNameCollection::nodeMatchesIfIdAttributeMatch(this) ? getIdAttribute() : nullAtom;
+        if (!oldName.isEmpty() && oldName != id)
             toHTMLDocument(ownerDocument)->documentNamedItemMap().remove(oldName.impl(), this);
-        if (!newName.isEmpty())
+        if (!newName.isEmpty() && newName != id)
             toHTMLDocument(ownerDocument)->documentNamedItemMap().add(newName.impl(), this);
     }
 }
@@ -2670,10 +2672,10 @@ inline void Element::updateId(const AtomicString& oldId, const AtomicString& new
     if (oldId == newId)
         return;
 
-    updateId(treeScope(), oldId, newId);
+    updateId(treeScope(), oldId, newId, UpdateHTMLDocumentNamedItemMapsOnlyIfDiffersFromNameAttribute);
 }
 
-void Element::updateId(TreeScope* scope, const AtomicString& oldId, const AtomicString& newId)
+void Element::updateId(TreeScope* scope, const AtomicString& oldId, const AtomicString& newId, HTMLDocumentNamedItemMapsUpdatingCondition condition)
 {
     ASSERT(isInTreeScope());
     ASSERT(oldId != newId);
@@ -2691,16 +2693,18 @@ void Element::updateId(TreeScope* scope, const AtomicString& oldId, const Atomic
         return;
 
     if (WindowNameCollection::nodeMatchesIfIdAttributeMatch(this)) {
-        if (!oldId.isEmpty())
+        const AtomicString& name = condition == UpdateHTMLDocumentNamedItemMapsOnlyIfDiffersFromNameAttribute && WindowNameCollection::nodeMatchesIfNameAttributeMatch(this) ? getNameAttribute() : nullAtom;
+        if (!oldId.isEmpty() && oldId != name)
             toHTMLDocument(ownerDocument)->windowNamedItemMap().remove(oldId.impl(), this);
-        if (!newId.isEmpty())
+        if (!newId.isEmpty() && newId != name)
             toHTMLDocument(ownerDocument)->windowNamedItemMap().add(newId.impl(), this);
     }
 
     if (DocumentNameCollection::nodeMatchesIfIdAttributeMatch(this)) {
-        if (!oldId.isEmpty())
+        const AtomicString& name = condition == UpdateHTMLDocumentNamedItemMapsOnlyIfDiffersFromNameAttribute && DocumentNameCollection::nodeMatchesIfNameAttributeMatch(this) ? getNameAttribute() : nullAtom;
+        if (!oldId.isEmpty() && oldId != name)
             toHTMLDocument(ownerDocument)->documentNamedItemMap().remove(oldId.impl(), this);
-        if (!newId.isEmpty())
+        if (!newId.isEmpty() && newId != name)
             toHTMLDocument(ownerDocument)->documentNamedItemMap().add(newId.impl(), this);
     }
 }
@@ -2886,6 +2890,10 @@ void Element::cloneAttributesFromElement(const Element& other)
         return;
     }
 
+    // We can't update window and document's named item maps since the presence of image and object elements depend on other attributes and children.
+    // Fortunately, those named item maps are only updated when this element is in the document, which should never be the case.
+    ASSERT(!inDocument());
+
     const AtomicString& oldID = getIdAttribute();
     const AtomicString& newID = other.getIdAttribute();
 
index 442e950345449e35e6db3ff98d6c16cbbc2f3b81..26f9b7e3684ebc9ec0f01e19b7c9c3d011255df2 100644 (file)
@@ -674,7 +674,8 @@ private:
     void synchronizeAttribute(const AtomicString& localName) const;
 
     void updateId(const AtomicString& oldId, const AtomicString& newId);
-    void updateId(TreeScope*, const AtomicString& oldId, const AtomicString& newId);
+    enum HTMLDocumentNamedItemMapsUpdatingCondition { AlwaysUpdateHTMLDocumentNamedItemMaps, UpdateHTMLDocumentNamedItemMapsOnlyIfDiffersFromNameAttribute };
+    void updateId(TreeScope*, const AtomicString& oldId, const AtomicString& newId, HTMLDocumentNamedItemMapsUpdatingCondition);
     void updateName(const AtomicString& oldName, const AtomicString& newName);
     void updateName(TreeScope*, const AtomicString& oldName, const AtomicString& newName);
     void updateLabel(TreeScope*, const AtomicString& oldForAttributeValue, const AtomicString& newForAttributeValue);
index f507d288682bf783b4bb3e6d94bb0b42475dda20..75c47434d63b3cb86a95afca859a598838021c4b 100644 (file)
@@ -52,16 +52,15 @@ static void swapInNodePreservingAttributesAndChildren(HTMLElement* newNode, HTML
 {
     ASSERT(nodeToReplace->inDocument());
     RefPtr<ContainerNode> parentNode = nodeToReplace->parentNode();
-    parentNode->insertBefore(newNode, nodeToReplace, ASSERT_NO_EXCEPTION);
 
+    // FIXME: Fix this to send the proper MutationRecords when MutationObservers are present.
+    newNode->cloneDataFromElement(*nodeToReplace);
     NodeVector children;
     getChildNodes(nodeToReplace, children);
     for (size_t i = 0; i < children.size(); ++i)
         newNode->appendChild(children[i], ASSERT_NO_EXCEPTION);
 
-    // FIXME: Fix this to send the proper MutationRecords when MutationObservers are present.
-    newNode->cloneDataFromElement(*nodeToReplace);
-
+    parentNode->insertBefore(newNode, nodeToReplace, ASSERT_NO_EXCEPTION);
     parentNode->removeChild(nodeToReplace, ASSERT_NO_EXCEPTION);
 }
 
index 69dd80ac993923940030c73772016b57c2a01911..9f9b0bcb5944efd08d3f4639353f8f62cb4fe52f 100644 (file)
@@ -130,7 +130,7 @@ void HTMLImageElement::parseAttribute(const QualifiedName& name, const AtomicStr
             if (hasName() != willHaveName && inDocument() && document()->isHTMLDocument()) {
                 HTMLDocument* document = toHTMLDocument(this->document());
                 const AtomicString& id = getIdAttribute();
-                if (!id.isEmpty()) {
+                if (!id.isEmpty() && id != getNameAttribute()) {
                     if (willHaveName)
                         document->documentNamedItemMap().add(id.impl(), this);
                     else
index 3a205131f0221b05e16c4d58aa6ec1fda5861076..a93192aa2f857fbd50ca58b5a885cd635056befe 100644 (file)
@@ -451,7 +451,7 @@ void HTMLObjectElement::updateDocNamedItem()
         }
 
         const AtomicString& name = getNameAttribute();
-        if (!name.isEmpty()) {
+        if (!name.isEmpty() && id != name) {
             if (isNamedItem)
                 document->documentNamedItemMap().add(name.impl(), this);
             else