Reduce complexity of lifetime management in DynamicNodeList caches

<https://bugs.webkit.org/show_bug.cgi?id=27068>

Reviewed by Maciej Stachowiak

Switch the Cache object used by DynamicNodeList into a normal
refcounted object rather than having a weird flag controlled
refcounting system, where positive refcount did not automatically
imply the cache object would actually still be live.

Canonical link: https://commits.webkit.org/37277@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@45620 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog

@@ -1,3 +1,31 @@
+2009-07-07  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Maciej Stachowiak.
+
+        Reduce complexity of lifetime management in DynamicNodeList caches
+        <https://bugs.webkit.org/show_bug.cgi?id=27068>
+
+        Switch the Cache object used by DynamicNodeList into a normal
+        refcounted object rather than having a weird flag controlled
+        refcounting system, where positive refcount did not automatically
+        imply the cache object would actually still be live.
+
+        * dom/DynamicNodeList.cpp:
+        (WebCore::DynamicNodeList::DynamicNodeList):
+        (WebCore::DynamicNodeList::~DynamicNodeList):
+        (WebCore::DynamicNodeList::Caches::Caches):
+        (WebCore::DynamicNodeList::Caches::create):
+        * dom/DynamicNodeList.h:
+        * dom/Node.cpp:
+        (WebCore::Node::childNodes):
+        (WebCore::Node::getElementsByTagNameNS):
+        (WebCore::Node::getElementsByName):
+        (WebCore::Node::getElementsByClassName):
+        (WebCore::NodeListsNodeData::invalidateCaches):
+        (WebCore::NodeListsNodeData::isEmpty):
+        * dom/NodeRareData.h:
+        (WebCore::NodeListsNodeData::NodeListsNodeData):
+
 2009-07-07  Simon Fraser  <simon.fraser@apple.com>
 
         Reviewed by Dan Bernstein.

WebCore/dom/DynamicNodeList.cpp

@@ -30,7 +30,7 @@ namespace WebCore {
 
 DynamicNodeList::DynamicNodeList(PassRefPtr<Node> rootNode)
     : m_rootNode(rootNode)
-    , m_caches(new Caches)
+    , m_caches(Caches::create())
     , m_ownsCaches(true)
 {
     m_rootNode->registerDynamicNodeList(this);
@@ -42,16 +42,11 @@ DynamicNodeList::DynamicNodeList(PassRefPtr<Node> rootNode, DynamicNodeList::Cac
     , m_ownsCaches(false)
 {
     m_rootNode->registerDynamicNodeList(this);
-    ++caches->refCount;
 }    
 
 DynamicNodeList::~DynamicNodeList()
 {
     m_rootNode->unregisterDynamicNodeList(this);
-    if (m_ownsCaches)
-        delete m_caches;
-    else
-        --m_caches->refCount;
 }
 
 unsigned DynamicNodeList::length() const
@@ -158,10 +153,14 @@ DynamicNodeList::Caches::Caches()
     : lastItem(0)
     , isLengthCacheValid(false)
     , isItemCacheValid(false)
-    , refCount(0)
 {
 }
 
+PassRefPtr<DynamicNodeList::Caches> DynamicNodeList::Caches::create()
+{
+    return adoptRef(new Caches());
+}
+
 void DynamicNodeList::Caches::reset()
 {
     lastItem = 0;

WebCore/dom/DynamicNodeList.h

@@ -37,16 +37,17 @@ namespace WebCore {
 
     class DynamicNodeList : public NodeList {
     public:
-        struct Caches {
-            Caches();
+        struct Caches : RefCounted<Caches>{
+            static PassRefPtr<Caches> create();
             void reset();
 
             unsigned cachedLength;
             Node* lastItem;
             unsigned lastItemOffset;
             bool isLengthCacheValid : 1;
             bool isItemCacheValid : 1;
-            unsigned refCount;
+        protected:
+            Caches();
         };
 
         virtual ~DynamicNodeList();
@@ -68,7 +69,7 @@ namespace WebCore {
         virtual bool nodeMatches(Element*) const = 0;
 
         RefPtr<Node> m_rootNode;
-        mutable Caches* m_caches;
+        mutable RefPtr<Caches> m_caches;
         bool m_ownsCaches;
 
     private:

WebCore/dom/Node.cpp

@@ -517,7 +517,7 @@ PassRefPtr<NodeList> Node::childNodes()
         document()->addNodeListCache();
     }
 
-    return ChildNodeList::create(this, &data->nodeLists()->m_childNodeListCaches);
+    return ChildNodeList::create(this, data->nodeLists()->m_childNodeListCaches.get());
 }
 
 Node *Node::lastDescendant() const
@@ -1500,9 +1500,9 @@ PassRefPtr<NodeList> Node::getElementsByTagNameNS(const AtomicString& namespaceU
 
     pair<NodeListsNodeData::TagCacheMap::iterator, bool> result = data->nodeLists()->m_tagNodeListCaches.add(QualifiedName(nullAtom, localNameAtom, namespaceURI), 0);
     if (result.second)
-        result.first->second = new DynamicNodeList::Caches;
+        result.first->second = DynamicNodeList::Caches::create();
 
-    return TagNodeList::create(this, namespaceURI.isEmpty() ? nullAtom : namespaceURI, localNameAtom, result.first->second);
+    return TagNodeList::create(this, namespaceURI.isEmpty() ? nullAtom : namespaceURI, localNameAtom, result.first->second.get());
 }
 
 PassRefPtr<NodeList> Node::getElementsByName(const String& elementName)
@@ -1515,9 +1515,9 @@ PassRefPtr<NodeList> Node::getElementsByName(const String& elementName)
 
     pair<NodeListsNodeData::CacheMap::iterator, bool> result = data->nodeLists()->m_nameNodeListCaches.add(elementName, 0);
     if (result.second)
-        result.first->second = new DynamicNodeList::Caches;
+        result.first->second = DynamicNodeList::Caches::create();
 
-    return NameNodeList::create(this, elementName, result.first->second);
+    return NameNodeList::create(this, elementName, result.first->second.get());
 }
 
 PassRefPtr<NodeList> Node::getElementsByClassName(const String& classNames)
@@ -1530,9 +1530,9 @@ PassRefPtr<NodeList> Node::getElementsByClassName(const String& classNames)
 
     pair<NodeListsNodeData::CacheMap::iterator, bool> result = data->nodeLists()->m_classNodeListCaches.add(classNames, 0);
     if (result.second)
-        result.first->second = new DynamicNodeList::Caches;
+        result.first->second = DynamicNodeList::Caches::create();
 
-    return ClassNodeList::create(this, classNames, result.first->second);
+    return ClassNodeList::create(this, classNames, result.first->second.get());
 }
 
 template <typename Functor>
@@ -2185,7 +2185,7 @@ void Node::formatForDebugger(char* buffer, unsigned length) const
 
 void NodeListsNodeData::invalidateCaches()
 {
-    m_childNodeListCaches.reset();
+    m_childNodeListCaches->reset();
     TagCacheMap::const_iterator tagCachesEnd = m_tagNodeListCaches.end();
     for (TagCacheMap::const_iterator it = m_tagNodeListCaches.begin(); it != tagCachesEnd; ++it)
         it->second->reset();
@@ -2208,24 +2208,24 @@ bool NodeListsNodeData::isEmpty() const
     if (!m_listsWithCaches.isEmpty())
         return false;
 
-    if (m_childNodeListCaches.refCount)
+    if (m_childNodeListCaches->refCount())
         return false;
 
     TagCacheMap::const_iterator tagCachesEnd = m_tagNodeListCaches.end();
     for (TagCacheMap::const_iterator it = m_tagNodeListCaches.begin(); it != tagCachesEnd; ++it) {
-        if (it->second->refCount)
+        if (it->second->refCount())
             return false;
     }
 
     CacheMap::const_iterator classCachesEnd = m_classNodeListCaches.end();
     for (CacheMap::const_iterator it = m_classNodeListCaches.begin(); it != classCachesEnd; ++it) {
-        if (it->second->refCount)
+        if (it->second->refCount())
             return false;
     }
 
     CacheMap::const_iterator nameCachesEnd = m_nameNodeListCaches.end();
     for (CacheMap::const_iterator it = m_nameNodeListCaches.begin(); it != nameCachesEnd; ++it) {
-        if (it->second->refCount)
+        if (it->second->refCount())
             return false;
     }
 

WebCore/dom/NodeRareData.h

@@ -37,32 +37,28 @@ struct NodeListsNodeData {
     typedef HashSet<DynamicNodeList*> NodeListSet;
     NodeListSet m_listsWithCaches;
 
-    DynamicNodeList::Caches m_childNodeListCaches;
+    RefPtr<DynamicNodeList::Caches> m_childNodeListCaches;
 
-    typedef HashMap<String, DynamicNodeList::Caches*> CacheMap;
+    typedef HashMap<String, RefPtr<DynamicNodeList::Caches> > CacheMap;
     CacheMap m_classNodeListCaches;
     CacheMap m_nameNodeListCaches;
 
-    typedef HashMap<QualifiedName, DynamicNodeList::Caches*> TagCacheMap;
+    typedef HashMap<QualifiedName, RefPtr<DynamicNodeList::Caches> > TagCacheMap;
     TagCacheMap m_tagNodeListCaches;
 
     static PassOwnPtr<NodeListsNodeData> create() {
         return new NodeListsNodeData;
     }
-
-    ~NodeListsNodeData()
-    {
-        deleteAllValues(m_classNodeListCaches);
-        deleteAllValues(m_nameNodeListCaches);
-        deleteAllValues(m_tagNodeListCaches);
-    }
 
     void invalidateCaches();
     void invalidateCachesThatDependOnAttributes();
     bool isEmpty() const;
 
 private:
-    NodeListsNodeData() { }
+    NodeListsNodeData()
+        : m_childNodeListCaches(DynamicNodeList::Caches::create())
+    {
+    }
 };
 
 class NodeRareData {