Changeset 149881 in webkit


Ignore:
Timestamp:
May 10, 2013 9:38:21 AM (11 years ago)
Author:
rniwa@webkit.org
Message:

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.

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.
Location:
trunk
Files:
2 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r149874 r149881  
     12013-05-10  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        REGRESSION (r149652): Videos do not play on cnn.com, just black box
     4        https://bugs.webkit.org/show_bug.cgi?id=115887
     5
     6        Reviewed by Antti Koivisto.
     7
     8        Add regression tests.
     9
     10        * fast/dom/HTMLDocument/image-with-same-id-and-name-expected.txt: Added.
     11        * fast/dom/HTMLDocument/image-with-same-id-and-name.html: Added.
     12        * fast/dom/HTMLDocument/object-with-same-id-and-name-expected.txt: Added.
     13        * fast/dom/HTMLDocument/object-with-same-id-and-name.html: Added.
     14
    1152013-05-10  Christophe Dumez  <ch.dumez@sisa.samsung.com>
    216

  • trunk/Source/WebCore/ChangeLog

    r149880 r149881  
     12013-05-10  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        REGRESSION (r149652): Videos do not play on cnn.com, just black box
     4        https://bugs.webkit.org/show_bug.cgi?id=115887
     5
     6        Reviewed by Antti Koivisto.
     7
     8        The bug was caused by window and document named item maps counting the same element twice
     9        when it has the same id and name attribute values. Fixed the bug by avoiding to add or remove
     10        an element per id and name attribute updates when it had already been added or removed by
     11        name and id attribute updates respectively.
     12
     13        We do this by checking whether the other attribute affects the element's precense in window
     14        and document named item maps and avoiding to add or remove the attribute when they do and
     15        the other attribute is present in updateId and updateName.
     16
     17        Consider a scenario when an object element has id "foo", and name attribute is about to be also
     18        set to "foo". If the id attribute doesn't affect element's presense in window or document
     19        named item maps, we're done. If it does, then the maps already have this element so we don't
     20        want to add it again. Conversely, if the element already has id and name attributes set to
     21        "foo", and we're moving the id attribute, then we want to remove the element from the maps only
     22        if the id doesn't affect the presence of the element in the maps.
     23
     24        Unfortuntely, this logic doesn't work when we're inserting or removing an element on its entirely
     25        because updateId and updateName are called when both id and name attributes are present so skip
     26        this step (AlwaysUpdateHTMLDocumentNamedItemMaps) for the id attribute to break the symmetry.
     27
     28        Test: fast/dom/HTMLDocument/image-with-same-id-and-name.html
     29              fast/dom/HTMLDocument/object-with-same-id-and-name.html
     30
     31        * dom/Element.cpp:
     32        (WebCore::Element::insertedInto): Call updateId and updateName with
     33        AlwaysUpdateHTMLDocumentNamedItemMaps.
     34        (WebCore::Element::removedFrom): Ditto.
     35        (WebCore::Element::updateName): Don't add or remove this element if the id attribute has already
     36        done so except when we're inserting, removing, or cloning an element.
     37        (WebCore::Element::updateId): Ditto for the name attribute.
     38        (WebCore::Element::cloneAttributesFromElement): Added a comment and assert that we never call this
     39        function when this element is in the document. We can't update window and documemt named item
     40        maps here because image element's id attribute value, for example, is present in the document's
     41        named item map if it has a name attribute. Since this function calls updateId and updateName
     42        before updating attributes, this check is going to fail in DocumentNameCollection's
     43        nodeMatchesIfIdAttributeMatch and bad things will happen.
     44
     45        * dom/Element.h:
     46
     47        * editing/ReplaceNodeWithSpanCommand.cpp:
     48        (WebCore::swapInNodePreservingAttributesAndChildren): Clone children and attributes before
     49        inserting the swapped span to avoid hitting the assertion in cloneAttributesFromElement we added.
     50
     51        * html/HTMLImageElement.cpp:
     52        (WebCore::HTMLImageElement::parseAttribute):
     53
     54        * html/HTMLObjectElement.cpp:
     55        (WebCore::HTMLObjectElement::updateDocNamedItem):
     56
    1572013-05-09  Dean Jackson  <dino@apple.com>
    258

  • trunk/Source/WebCore/dom/Element.cpp

    r149708 r149881  
    11781178    const AtomicString& idValue = getIdAttribute();
    11791179    if (!idValue.isNull())
    1180         updateId(scope, nullAtom, idValue);
     1180        updateId(scope, nullAtom, idValue, AlwaysUpdateHTMLDocumentNamedItemMaps);
    11811181
    11821182    const AtomicString& nameValue = getNameAttribute();
    … …  
    12211221        const AtomicString& idValue = getIdAttribute();
    12221222        if (!idValue.isNull())
    1223             updateId(insertionPoint->treeScope(), idValue, nullAtom);
     1223            updateId(insertionPoint->treeScope(), idValue, nullAtom, AlwaysUpdateHTMLDocumentNamedItemMaps);
    12241224
    12251225        const AtomicString& nameValue = getNameAttribute();
    … …  
    26492649
    26502650    if (WindowNameCollection::nodeMatchesIfNameAttributeMatch(this)) {
    2651         if (!oldName.isEmpty())
     2651        const AtomicString& id = WindowNameCollection::nodeMatchesIfIdAttributeMatch(this) ? getIdAttribute() : nullAtom;
     2652        if (!oldName.isEmpty() && oldName != id)
    26522653            toHTMLDocument(ownerDocument)->windowNamedItemMap().remove(oldName.impl(), this);
    2653         if (!newName.isEmpty())
     2654        if (!newName.isEmpty() && newName != id)
    26542655            toHTMLDocument(ownerDocument)->windowNamedItemMap().add(newName.impl(), this);
    26552656    }
    26562657
    26572658    if (DocumentNameCollection::nodeMatchesIfNameAttributeMatch(this)) {
    2658         if (!oldName.isEmpty())
     2659        const AtomicString& id = DocumentNameCollection::nodeMatchesIfIdAttributeMatch(this) ? getIdAttribute() : nullAtom;
     2660        if (!oldName.isEmpty() && oldName != id)
    26592661            toHTMLDocument(ownerDocument)->documentNamedItemMap().remove(oldName.impl(), this);
    2660         if (!newName.isEmpty())
     2662        if (!newName.isEmpty() && newName != id)
    26612663            toHTMLDocument(ownerDocument)->documentNamedItemMap().add(newName.impl(), this);
    26622664    }
    … …  
    26712673        return;
    26722674
    2673     updateId(treeScope(), oldId, newId);
    2674 }
    2675 
    2676 void Element::updateId(TreeScope* scope, const AtomicString& oldId, const AtomicString& newId)
     2675    updateId(treeScope(), oldId, newId, UpdateHTMLDocumentNamedItemMapsOnlyIfDiffersFromNameAttribute);
     2676}
     2677
     2678void Element::updateId(TreeScope* scope, const AtomicString& oldId, const AtomicString& newId, HTMLDocumentNamedItemMapsUpdatingCondition condition)
    26772679{
    26782680    ASSERT(isInTreeScope());
    … …  
    26922694
    26932695    if (WindowNameCollection::nodeMatchesIfIdAttributeMatch(this)) {
    2694         if (!oldId.isEmpty())
     2696        const AtomicString& name = condition == UpdateHTMLDocumentNamedItemMapsOnlyIfDiffersFromNameAttribute && WindowNameCollection::nodeMatchesIfNameAttributeMatch(this) ? getNameAttribute() : nullAtom;
     2697        if (!oldId.isEmpty() && oldId != name)
    26952698            toHTMLDocument(ownerDocument)->windowNamedItemMap().remove(oldId.impl(), this);
    2696         if (!newId.isEmpty())
     2699        if (!newId.isEmpty() && newId != name)
    26972700            toHTMLDocument(ownerDocument)->windowNamedItemMap().add(newId.impl(), this);
    26982701    }
    26992702
    27002703    if (DocumentNameCollection::nodeMatchesIfIdAttributeMatch(this)) {
    2701         if (!oldId.isEmpty())
     2704        const AtomicString& name = condition == UpdateHTMLDocumentNamedItemMapsOnlyIfDiffersFromNameAttribute && DocumentNameCollection::nodeMatchesIfNameAttributeMatch(this) ? getNameAttribute() : nullAtom;
     2705        if (!oldId.isEmpty() && oldId != name)
    27022706            toHTMLDocument(ownerDocument)->documentNamedItemMap().remove(oldId.impl(), this);
    2703         if (!newId.isEmpty())
     2707        if (!newId.isEmpty() && newId != name)
    27042708            toHTMLDocument(ownerDocument)->documentNamedItemMap().add(newId.impl(), this);
    27052709    }
    … …  
    28872891    }
    28882892
     2893    // 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.
     2894    // Fortunately, those named item maps are only updated when this element is in the document, which should never be the case.
     2895    ASSERT(!inDocument());
     2896
    28892897    const AtomicString& oldID = getIdAttribute();
    28902898    const AtomicString& newID = other.getIdAttribute();

  • trunk/Source/WebCore/dom/Element.h

    r149718 r149881  
    675675
    676676    void updateId(const AtomicString& oldId, const AtomicString& newId);
    677     void updateId(TreeScope*, const AtomicString& oldId, const AtomicString& newId);
     677    enum HTMLDocumentNamedItemMapsUpdatingCondition { AlwaysUpdateHTMLDocumentNamedItemMaps, UpdateHTMLDocumentNamedItemMapsOnlyIfDiffersFromNameAttribute };
     678    void updateId(TreeScope*, const AtomicString& oldId, const AtomicString& newId, HTMLDocumentNamedItemMapsUpdatingCondition);
    678679    void updateName(const AtomicString& oldName, const AtomicString& newName);
    679680    void updateName(TreeScope*, const AtomicString& oldName, const AtomicString& newName);

  • trunk/Source/WebCore/editing/ReplaceNodeWithSpanCommand.cpp

    r142247 r149881  
    5353    ASSERT(nodeToReplace->inDocument());
    5454    RefPtr<ContainerNode> parentNode = nodeToReplace->parentNode();
    55     parentNode->insertBefore(newNode, nodeToReplace, ASSERT_NO_EXCEPTION);
    5655
     56    // FIXME: Fix this to send the proper MutationRecords when MutationObservers are present.
     57    newNode->cloneDataFromElement(*nodeToReplace);
    5758    NodeVector children;
    5859    getChildNodes(nodeToReplace, children);
    … …  
    6061        newNode->appendChild(children[i], ASSERT_NO_EXCEPTION);
    6162
    62     // FIXME: Fix this to send the proper MutationRecords when MutationObservers are present.
    63     newNode->cloneDataFromElement(*nodeToReplace);
    64 
     63    parentNode->insertBefore(newNode, nodeToReplace, ASSERT_NO_EXCEPTION);
    6564    parentNode->removeChild(nodeToReplace, ASSERT_NO_EXCEPTION);
    6665}

  • trunk/Source/WebCore/html/HTMLImageElement.cpp

    r149652 r149881  
    131131                HTMLDocument* document = toHTMLDocument(this->document());
    132132                const AtomicString& id = getIdAttribute();
    133                 if (!id.isEmpty()) {
     133                if (!id.isEmpty() && id != getNameAttribute()) {
    134134                    if (willHaveName)
    135135                        document->documentNamedItemMap().add(id.impl(), this);

  • trunk/Source/WebCore/html/HTMLObjectElement.cpp

    r149652 r149881  
    452452
    453453        const AtomicString& name = getNameAttribute();
    454         if (!name.isEmpty()) {
     454        if (!name.isEmpty() && id != name) {
    455455            if (isNamedItem)
    456456                document->documentNamedItemMap().add(name.impl(), this);
Note: See TracChangeset for help on using the changeset viewer.