- Revision
- 225613
- Author
- za...@apple.com
- Date
- 2017-12-06 16:55:01 -0800 (Wed, 06 Dec 2017)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (225612 => 225613)
--- trunk/Source/WebCore/ChangeLog 2017-12-07 00:49:11 UTC (rev 225612)
+++ trunk/Source/WebCore/ChangeLog 2017-12-07 00:55:01 UTC (rev 225613)
@@ -1,3 +1,38 @@
+2017-12-06 Zalan Bujtas <za...@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 <beid...@apple.com>
Start writing ServiceWorker registrations to disk.
Modified: trunk/Source/WebCore/accessibility/AXObjectCache.cpp (225612 => 225613)
--- trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2017-12-07 00:49:11 UTC (rev 225612)
+++ trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2017-12-07 00:55:01 UTC (rev 225613)
@@ -716,25 +716,22 @@
remove(m_renderObjectMapping.take(renderer));
}
-void AXObjectCache::remove(Node* node)
+void AXObjectCache::remove(Node& node)
{
- if (!node)
- return;
-
- if (is<Element>(*node)) {
- m_deferredRecomputeIsIgnoredList.remove(downcast<Element>(node));
- m_deferredSelectedChildredChangedList.remove(downcast<Element>(node));
+ if (is<Element>(node)) {
+ m_deferredRecomputeIsIgnoredList.remove(downcast<Element>(&node));
+ m_deferredSelectedChildredChangedList.remove(downcast<Element>(&node));
}
- m_deferredTextChangedList.remove(node);
+ m_deferredTextChangedList.remove(&node);
removeNodeForUse(node);
- remove(m_nodeObjectMapping.take(node));
+ remove(m_nodeObjectMapping.take(&node));
- if (m_currentModalNode == node)
+ if (m_currentModalNode == &node)
m_currentModalNode = nullptr;
- m_modalNodesSet.remove(node);
+ m_modalNodesSet.remove(&node);
- remove(node->renderer());
+ remove(node.renderer());
}
void AXObjectCache::remove(Widget* view)
@@ -2728,21 +2725,28 @@
return result;
}
-void AXObjectCache::clearTextMarkerNodesInUse(Document* document)
+template<typename T>
+static void filterForRemoval(const ListHashSet<T>& list, const Document& document, HashSet<Node*>& nodesToRemove)
{
- 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 (auto* node : list) {
+ if (node->isConnected() && &node->document() != &document)
+ continue;
+ nodesToRemove.add(node);
}
-
- for (const auto& node : nodesToDelete)
- m_textMarkerNodes.remove(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);
+}
bool AXObjectCache::nodeIsTextControl(const Node* node)
{
Modified: trunk/Source/WebCore/accessibility/AXObjectCache.h (225612 => 225613)
--- trunk/Source/WebCore/accessibility/AXObjectCache.h 2017-12-07 00:49:11 UTC (rev 225612)
+++ trunk/Source/WebCore/accessibility/AXObjectCache.h 2017-12-07 00:55:01 UTC (rev 225613)
@@ -158,7 +158,7 @@
AccessibilityObject* get(Node*);
void remove(RenderObject*);
- void remove(Node*);
+ void remove(Node&);
void remove(Widget*);
void remove(AXID);
@@ -320,7 +320,7 @@
void frameLoadingEventNotification(Frame*, AXLoadingEvent);
- void clearTextMarkerNodesInUse(Document*);
+ void prepareForDocumentDestruction(const Document&);
void startCachingComputedObjectAttributesUntilTreeMutates();
void stopCachingComputedObjectAttributes();
@@ -361,7 +361,7 @@
// 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); }
// CharacterOffset functions.
@@ -419,7 +419,7 @@
HashMap<RenderObject*, AXID> m_renderObjectMapping;
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;
WEBCORE_EXPORT static bool gAccessibilityEnhancedUserInterfaceEnabled;
@@ -528,7 +528,7 @@
inline RefPtr<Range> AXObjectCache::rangeForNodeContents(Node*) { return nullptr; }
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*) { }
inline void AXObjectCache::selectedChildrenChanged(Node*) { }
Modified: trunk/Source/WebCore/dom/Document.cpp (225612 => 225613)
--- trunk/Source/WebCore/dom/Document.cpp 2017-12-07 00:49:11 UTC (rev 225612)
+++ trunk/Source/WebCore/dom/Document.cpp 2017-12-07 00:55:01 UTC (rev 225613)
@@ -2339,10 +2339,10 @@
#endif
#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
Modified: trunk/Source/WebCore/dom/Node.cpp (225612 => 225613)
--- trunk/Source/WebCore/dom/Node.cpp 2017-12-07 00:49:11 UTC (rev 225612)
+++ trunk/Source/WebCore/dom/Node.cpp 2017-12-07 00:55:01 UTC (rev 225613)
@@ -326,8 +326,8 @@
document.removeTouchEventHandler(*this, EventHandlerRemoval::All);
#endif
- if (AXObjectCache* cache = document.existingAXObjectCache())
- cache->remove(this);
+ if (auto* cache = document.existingAXObjectCache())
+ cache->remove(*this);
}
void Node::materializeRareData()
@@ -2009,7 +2009,7 @@
if (AXObjectCache::accessibilityEnabled()) {
if (auto* cache = oldDocument.existingAXObjectCache())
- cache->remove(this);
+ cache->remove(*this);
}
if (auto* eventTargetData = this->eventTargetData()) {
