Bug 123708 - ChildNodeList should not be LiveNodeList
Summary: ChildNodeList should not be LiveNodeList
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-03 10:48 PST by Antti Koivisto
Modified: 2013-11-11 22:41 PST (History)
7 users (show)

See Also:


Attachments
patch (24.42 KB, patch)
2013-11-03 11:09 PST, Antti Koivisto
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2013-11-03 10:48:18 PST
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.
Comment 1 Antti Koivisto 2013-11-03 11:09:02 PST
Created attachment 215871 [details]
patch
Comment 2 Ryosuke Niwa 2013-11-03 11:18:35 PST
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.
Comment 3 Sam Weinig 2013-11-03 11:20:27 PST
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.
Comment 4 Ryosuke Niwa 2013-11-03 11:32:00 PST
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.
Comment 5 Antti Koivisto 2013-11-03 11:34:26 PST
> 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.
Comment 6 Ryosuke Niwa 2013-11-03 11:40:50 PST
(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.
Comment 7 Ryosuke Niwa 2013-11-03 11:41:19 PST
It's in ContainerNode.h
Comment 8 Antti Koivisto 2013-11-03 11:51:05 PST
https://trac.webkit.org/r158536
Comment 9 Antti Koivisto 2013-11-04 00:25:33 PST
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
Comment 10 Eric Seidel (no email) 2013-11-11 22:38:26 PST
I wonder if this fixed bug 98823.  Sounds related at least to my uninformed ears.
Comment 11 Eric Seidel (no email) 2013-11-11 22:41:56 PST
(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.