Changeset 225613 in webkit

Timestamp:

Dec 6, 2017 4:55:01 PM (6 years ago)

Author:

Alan Bujtas

Message:

Remove nodes from AXObjectCache when the associated subframe document is getting destroyed.
​https://bugs.webkit.org/show_bug.cgi?id=180503
<rdar://problem/35891328

Reviewed by Chris Fleizach.

While AXObjectCache lives on the mainframe's document, it caches nodes from every subframe document.
When a node is being destroyed, we deregister it from the AX cache through the Node's destructor.
Soon after the document is detached from the frame/frame is detached from the frame tree, this codepath
is no longer available (no access to the AXObjectCache object) and from this point we are unable to deregister
nodes associated with the current document.
In AXObjectCache::prepareForDocumentDestruction(), we preemptively remove all the cached nodes associated
with the about-to-be-destroyed document.

Covered by existing tests.

• accessibility/AXObjectCache.cpp:

(WebCore::AXObjectCache::remove):
(WebCore::filterForRemoval):
(WebCore::AXObjectCache::prepareForDocumentDestruction): Collecting the nodes and removing them later is
not the most performant way but in order to have a single code path for the de-registration (AXObjectCache::remove)
I think it's worth going down the slower path -which should not really be that slower anyway since those
lists tend to stay small.
(WebCore::AXObjectCache::clearTextMarkerNodesInUse): Deleted.

• accessibility/AXObjectCache.h:

(WebCore::AXObjectCache::removeNodeForUse):
(WebCore::AXObjectCache::remove):

• dom/Document.cpp:

(WebCore::Document::prepareForDestruction):

• dom/Node.cpp:

(WebCore::Node::willBeDeletedFrom):
(WebCore::Node::moveNodeToNewDocument):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• accessibility/AXObjectCache.cpp (modified) (2 diffs)
• accessibility/AXObjectCache.h (modified) (5 diffs)
• dom/Document.cpp (modified) (1 diff)
• dom/Node.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-12-06  Zalan Bujtas  <zalan@apple.com>
+
+        Remove nodes from AXObjectCache when the associated subframe document is getting destroyed.
+        https://bugs.webkit.org/show_bug.cgi?id=180503
+        <rdar://problem/35891328
+
+        Reviewed by Chris Fleizach.
+
+        While AXObjectCache lives on the mainframe's document, it caches nodes from every subframe document.
+        When a node is being destroyed, we deregister it from the AX cache through the Node's destructor.
+        Soon after the document is detached from the frame/frame is detached from the frame tree, this codepath
+        is no longer available (no access to the AXObjectCache object) and from this point we are unable to deregister
+        nodes associated with the current document.
+        In AXObjectCache::prepareForDocumentDestruction(), we preemptively remove all the cached nodes associated
+        with the about-to-be-destroyed document.
+
+        Covered by existing tests.
+
+        * accessibility/AXObjectCache.cpp:
+        (WebCore::AXObjectCache::remove):
+        (WebCore::filterForRemoval):
+        (WebCore::AXObjectCache::prepareForDocumentDestruction): Collecting the nodes and removing them later is
+        not the most performant way but in order to have a single code path for the de-registration (AXObjectCache::remove)
+        I think it's worth going down the slower path -which should not really be that slower anyway since those
+        lists tend to stay small.
+        (WebCore::AXObjectCache::clearTextMarkerNodesInUse): Deleted.
+        * accessibility/AXObjectCache.h:
+        (WebCore::AXObjectCache::removeNodeForUse):
+        (WebCore::AXObjectCache::remove):
+        * dom/Document.cpp:
+        (WebCore::Document::prepareForDestruction):
+        * dom/Node.cpp:
+        (WebCore::Node::willBeDeletedFrom):
+        (WebCore::Node::moveNodeToNewDocument):
+
 2017-12-06  Brady Eidson  <beidson@apple.com>
 

trunk/Source/WebCore/accessibility/AXObjectCache.cpp

@@ -717 +717 @@
 }
 
-void AXObjectCache::remove(Node* node)
-{
-    if (!node)
-        return;
-
-    if (is<Element>(*node)) {
-        m_deferredRecomputeIsIgnoredList.remove(downcast<Element>(node));
-        m_deferredSelectedChildredChangedList.remove(downcast<Element>(node));
-    }
-    m_deferredTextChangedList.remove(node);
+void AXObjectCache::remove(Node& node)
+{
+    if (is<Element>(node)) {
+        m_deferredRecomputeIsIgnoredList.remove(downcast<Element>(&node));
+        m_deferredSelectedChildredChangedList.remove(downcast<Element>(&node));
+    }
+    m_deferredTextChangedList.remove(&node);
     removeNodeForUse(node);
 
-    remove(m_nodeObjectMapping.take(node));
-
-    if (m_currentModalNode == node)
+    remove(m_nodeObjectMapping.take(&node));
+
+    if (m_currentModalNode == &node)
         m_currentModalNode = nullptr;
-    m_modalNodesSet.remove(node);
-
-    remove(node->renderer());
+    m_modalNodesSet.remove(&node);
+
+    remove(node.renderer());
 }
 
@@ -2729 +2726 @@
 }
 
-void AXObjectCache::clearTextMarkerNodesInUse(Document* document)
-{
-    if (!document)
-        return;
-    
-    // Check each node to see if it's inside the document being deleted, of if it no longer belongs to a document.
-    HashSet<Node*> nodesToDelete;
-    for (const auto& node : m_textMarkerNodes) {
-        if (!node->isConnected() || &(node)->document() == document)
-            nodesToDelete.add(node);
-    }
-    
-    for (const auto& node : nodesToDelete)
-        m_textMarkerNodes.remove(node);
+template<typename T>
+static void filterForRemoval(const ListHashSet<T>& list, const Document& document, HashSet<Node*>& nodesToRemove)
+{
+    for (auto* node : list) {
+        if (node->isConnected() && &node->document() != &document)
+            continue;
+        nodesToRemove.add(node);
+    }
+}
+
+void AXObjectCache::prepareForDocumentDestruction(const Document& document)
+{
+    HashSet<Node*> nodesToRemove;
+    filterForRemoval(m_textMarkerNodes, document, nodesToRemove);
+    filterForRemoval(m_modalNodesSet, document, nodesToRemove);
+    filterForRemoval(m_deferredRecomputeIsIgnoredList, document, nodesToRemove);
+    filterForRemoval(m_deferredTextChangedList, document, nodesToRemove);
+    filterForRemoval(m_deferredSelectedChildredChangedList, document, nodesToRemove);
+
+    for (auto* node : nodesToRemove)
+        remove(*node);
 }
     

trunk/Source/WebCore/accessibility/AXObjectCache.h

@@ -159 +159 @@
     
     void remove(RenderObject*);
-    void remove(Node*);
+    void remove(Node&);
     void remove(Widget*);
     void remove(AXID);
@@ -321 +321 @@
     void frameLoadingEventNotification(Frame*, AXLoadingEvent);
 
-    void clearTextMarkerNodesInUse(Document*);
+    void prepareForDocumentDestruction(const Document&);
 
     void startCachingComputedObjectAttributesUntilTreeMutates();
@@ -362 +362 @@
     // This is a weak reference cache for knowing if Nodes used by TextMarkers are valid.
     void setNodeInUse(Node* n) { m_textMarkerNodes.add(n); }
-    void removeNodeForUse(Node* n) { m_textMarkerNodes.remove(n); }
+    void removeNodeForUse(Node& n) { m_textMarkerNodes.remove(&n); }
     bool isNodeInUse(Node* n) { return m_textMarkerNodes.contains(n); }
     
@@ -420 +420 @@
     HashMap<Widget*, AXID> m_widgetObjectMapping;
     HashMap<Node*, AXID> m_nodeObjectMapping;
-    HashSet<Node*> m_textMarkerNodes;
+    ListHashSet<Node*> m_textMarkerNodes;
     std::unique_ptr<AXComputedObjectAttributeCache> m_computedObjectAttributeCache;
     WEBCORE_EXPORT static bool gAccessibilityEnabled;
@@ -529 +529 @@
 inline void AXObjectCache::remove(AXID) { }
 inline void AXObjectCache::remove(RenderObject*) { }
-inline void AXObjectCache::remove(Node*) { }
+inline void AXObjectCache::remove(Node&) { }
 inline void AXObjectCache::remove(Widget*) { }
 inline void AXObjectCache::selectedChildrenChanged(RenderObject*) { }

trunk/Source/WebCore/dom/Document.cpp

@@ -2340 +2340 @@
 
 #if HAVE(ACCESSIBILITY)
-    // Sub-frames need to cleanup Nodes in the text marker cache when the Document disappears.
     if (this != &topDocument()) {
-        if (AXObjectCache* cache = existingAXObjectCache())
-            cache->clearTextMarkerNodesInUse(this);
+        // Let the ax cache know that this subframe goes out of scope.
+        if (auto* cache = existingAXObjectCache())
+            cache->prepareForDocumentDestruction(*this);
     }
 #endif

trunk/Source/WebCore/dom/Node.cpp

@@ -327 +327 @@
 #endif
 
-    if (AXObjectCache* cache = document.existingAXObjectCache())
-        cache->remove(this);
+    if (auto* cache = document.existingAXObjectCache())
+        cache->remove(*this);
 }
 
@@ -2010 +2010 @@
     if (AXObjectCache::accessibilityEnabled()) {
         if (auto* cache = oldDocument.existingAXObjectCache())
-            cache->remove(this);
+            cache->remove(*this);
     }