Changeset 178038 in webkit

Timestamp:

Jan 7, 2015 8:55:10 AM (9 years ago)

Author:

Chris Fleizach

Message:

AX: Crash: com.apple.WebKit.WebContent at com.apple.WebCore: WebCore::AXObjectCache::clearTextMarkerNodesInUse + 149
​https://bugs.webkit.org/show_bug.cgi?id=139929

Reviewed by Darin Adler.

Source/WebCore:

When a frame is replaced, there were instances when it was not clearing its associated nodes in the accessibility text marker -> Node cache.
This caused dead Nodes to be left in the cache which would eventually be accessed when the cache was cleaned out at a later time.

To fix this we should be clearing out the cache in Document::prepareForDestruction, instead of Frame::disconnectOwnerElement.

While working on this, it also exposed a problem where when a frame goes away, it doesn't inform its parent to update its children,
which causes an ASSERT to be hit with this test as well.

Tests: accessibility/frame-disconnect-textmarker-cache-crash.html

• dom/Document.cpp:

(WebCore::Document::prepareForDestruction):

• page/Frame.cpp:

(WebCore::Frame::disconnectOwnerElement):

Remove cache management from here since it is superceded by code in Document::prepareForDestruction

• page/FrameView.cpp:

(WebCore::FrameView::removeFromAXObjectCache):

LayoutTests:

• accessibility/frame-disconnect-textmarker-cache-crash-expected.txt: Added.
• accessibility/frame-disconnect-textmarker-cache-crash.html: Added.
• accessibility/resources/frameset.html: Added.
• accessibility/resources/inform-parent-of-load.html: Added.
• accessibility/resources/text.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash-expected.txt (added)
• LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash.html (added)
• LayoutTests/accessibility/resources/frameset.html (added)
• LayoutTests/accessibility/resources/inform-parent-of-load.html (added)
• LayoutTests/accessibility/resources/text.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (1 diff)
• Source/WebCore/page/Frame.cpp (modified) (1 diff)
• Source/WebCore/page/FrameView.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2015-01-07  Chris Fleizach  <cfleizach@apple.com>
+
+        AX: Crash: com.apple.WebKit.WebContent at com.apple.WebCore: WebCore::AXObjectCache::clearTextMarkerNodesInUse + 149
+        https://bugs.webkit.org/show_bug.cgi?id=139929
+
+        Reviewed by Darin Adler.
+
+        * accessibility/frame-disconnect-textmarker-cache-crash-expected.txt: Added.
+        * accessibility/frame-disconnect-textmarker-cache-crash.html: Added.
+        * accessibility/resources/frameset.html: Added.
+        * accessibility/resources/inform-parent-of-load.html: Added.
+        * accessibility/resources/text.html: Added.
+
 2015-01-07  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-01-07  Chris Fleizach  <cfleizach@apple.com>
+
+        AX: Crash: com.apple.WebKit.WebContent at com.apple.WebCore: WebCore::AXObjectCache::clearTextMarkerNodesInUse + 149
+        https://bugs.webkit.org/show_bug.cgi?id=139929
+
+        Reviewed by Darin Adler.
+
+        When a frame is replaced, there were instances when it was not clearing its associated nodes in the accessibility text marker -> Node cache.
+        This caused dead Nodes to be left in the cache which would eventually be accessed when the cache was cleaned out at a later time.
+
+        To fix this we should be clearing out the cache in Document::prepareForDestruction, instead of Frame::disconnectOwnerElement.
+
+        While working on this, it also exposed a problem where when a frame goes away, it doesn't inform its parent to update its children,
+        which causes an ASSERT to be hit with this test as well.
+
+        Tests: accessibility/frame-disconnect-textmarker-cache-crash.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::prepareForDestruction):
+        * page/Frame.cpp:
+        (WebCore::Frame::disconnectOwnerElement):
+            Remove cache management from here since it is superceded by code in Document::prepareForDestruction
+        * page/FrameView.cpp:
+        (WebCore::FrameView::removeFromAXObjectCache):
+
 2015-01-07  Zan Dobersek  <zdobersek@igalia.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -2080 +2080 @@
 #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);
+    }
+#endif
+    
     disconnectDescendantFrames();
     if (m_domWindow && m_frame)

trunk/Source/WebCore/page/Frame.cpp

@@ -804 +804 @@
 {
     if (m_ownerElement) {
-        // We use the ownerElement's document to retrieve the cache, because the contentDocument for this
-        // frame is already detached (and can't access the top level AX cache).
-        // However, we pass in the current document to clearTextMarkerNodesInUse so we can identify the
-        // nodes inside this document that need to be removed from the cache.
-        
-        // We don't clear the AXObjectCache here because we don't want to clear the top level cache
-        // when a sub-frame is removed.
-#if HAVE(ACCESSIBILITY)
-        if (AXObjectCache* cache = m_ownerElement->document().existingAXObjectCache())
-            cache->clearTextMarkerNodesInUse(document());
-#endif
-        
         m_ownerElement->clearContentFrame();
         if (m_page)

trunk/Source/WebCore/page/FrameView.cpp

@@ -296 +296 @@
 void FrameView::removeFromAXObjectCache()
 {
-    if (AXObjectCache* cache = axObjectCache())
+    if (AXObjectCache* cache = axObjectCache()) {
+        if (HTMLFrameOwnerElement* owner = frame().ownerElement())
+            cache->childrenChanged(owner->renderer());
         cache->remove(this);
+    }
 }