| Summary: | ChildNodeList should not be LiveNodeList | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||
| Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | commit-queue, eric, esprehn+autocc, gyuyoung.kim, kangil.han, kling, rniwa | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Antti Koivisto
2013-11-03 10:48:18 PST
Created attachment 215871 [details]
patch
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 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. 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. > 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.
(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. It's in ContainerNode.h 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 I wonder if this fixed bug 98823. Sounds related at least to my uninformed ears. (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. |