[Webkit-unassigned] [Bug 123708] ChildNodeList should not be LiveNodeList

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=123708





--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org>  2013-11-03 11:18:35 PST ---
(From update of attachment 215871)
View in context: https://bugs.webkit.org/attachment.cgi?id=215871&action=review

> Source/WebCore/dom/ChildNodeList.cpp:132
> +Node* ChildNodeList::item(unsigned index) const
> +{
> +    if (m_cachedLengthValid && index >= m_cachedLength)
> +        return nullptr;
> +    if (m_cachedCurrentNode) {
> +        if (index > m_cachedCurrentPosition)
> +            return nodeAfterCached(index);
> +        if (index < m_cachedCurrentPosition)
> +            return nodeBeforeCached(index);
> +        return m_cachedCurrentNode;
> +    }
> +    if (m_cachedLengthValid && m_cachedLength - index < index)
> +        m_cachedCurrentNode = childFromLast(m_parent.get(), m_cachedLength - index - 1);
> +    else
> +        m_cachedCurrentNode = childFromFirst(m_parent.get(), index);
> +    m_cachedCurrentPosition = index;
> +    return m_cachedCurrentNode;
> +}

It's not great to duplicate the logic here.  We've had many use-after-free / correctness bugs in this area.
I'd imagine we should be able to templatize the logic in LiveNodeList and use it here.

> Source/WebCore/dom/ChildNodeList.cpp:136
> +    // FIXME: Why doesn't this look into the name attribute like HTMLCollection::namedItem does?

childNodes shouldn't have namedItem per spec so we should eventually remove this.

> Source/WebCore/dom/ChildNodeList.h:86
> +    Ref<ContainerNode> m_parent;
> +    mutable unsigned m_cachedLength : 31;
> +    mutable unsigned m_cachedLengthValid : 1;
> +    mutable unsigned m_cachedCurrentPosition;
> +    mutable Node* m_cachedCurrentNode;

I don't think we should duplicate all these caching mechanism here.
It would be better to extract a superclass of LiveNodeListBase and ChildNodeList if any.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.