Bug 123708 – ChildNodeList should not be LiveNodeList

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Antti Koivisto]

ChildNodeList is a poor fit to be LiveNodeList. It is heavily special-cased. It is also the only subtype that returns non-Elements thus preventing tightening.

[Antti Koivisto]

Created attachment 215871 [details]
patch

[Ryosuke Niwa]

Comment on attachment 215871 [details]
patch

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.

[Sam Weinig]

Comment on attachment 215871 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215871&action=review

> Source/WebCore/ChangeLog:8
> +        hildNodeList is a poor fit to be LiveNodeList. It is heavily special-cased. It is also

Typo. You mean ChildNodeList, not hildNodeList.

[Ryosuke Niwa]

I'll note that we don't have enough test cases for all the newly added code so we probably want to add some tests for childNodes.

[Antti Koivisto]

> 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.

The existing code is a mess and would not be a good starting point for any templates. 

I suspect caching everything to a vector would work better for child node list anyway (after access to index > 0). I'm just doing this now to avoid behaviour changes.

[Ryosuke Niwa]

(In reply to comment #5)
> I suspect caching everything to a vector would work better for child node list anyway (after access to index > 0). I'm just doing this now to avoid behaviour changes.

If we're taking that route, we should probably merge it with ChildNodesLazySnapshot.

[Ryosuke Niwa]

It's in ContainerNode.h

[Antti Koivisto]

https://trac.webkit.org/r158536

[Antti Koivisto]

Looks like this was ~13% progression in Dromaeo/dom-traverse

https://perf.webkit.org/#mode=charts&days=7&chartList=%5B%5B%22mac-mountainlion%22%2C%22Dromaeo%2Fdom-traverse%3ARuns%22%5D%5D

[Eric Seidel (no email)]

I wonder if this fixed bug 98823.  Sounds related at least to my uninformed ears.

[Eric Seidel (no email)]

(In reply to comment #10)
> I wonder if this fixed bug 98823.  Sounds related at least to my uninformed ears.

Nevermind.  That's HTMLCollection which is a related, but different beast.