AX: Crash: com.apple.WebKit.WebContent at com.apple.WebCore: WebCore::AXObjectCache...
authorcfleizach@apple.com <cfleizach@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Jan 2015 16:55:10 +0000 (16:55 +0000)
committercfleizach@apple.com <cfleizach@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Jan 2015 16:55:10 +0000 (16:55 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@178038 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash-expected.txt [new file with mode: 0644]
LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash.html [new file with mode: 0644]
LayoutTests/accessibility/resources/frameset.html [new file with mode: 0644]
LayoutTests/accessibility/resources/inform-parent-of-load.html [new file with mode: 0644]
LayoutTests/accessibility/resources/text.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/page/Frame.cpp
Source/WebCore/page/FrameView.cpp

index 7d7aaf4..d0bd9b4 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         [GTK] Unreviewed GTK gardening after r177637.
diff --git a/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash-expected.txt b/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash-expected.txt
new file mode 100644 (file)
index 0000000..61b0860
--- /dev/null
@@ -0,0 +1,9 @@
+This tests that when we access a text marker in a frame that is subsequently replaced by a different frame, we don't leave a hanging node in the cache that leads to a crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS string is 'hello'
+PASS string is 'test text'
+TEST PASSED: NO CRASH
+
diff --git a/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash.html b/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash.html
new file mode 100644 (file)
index 0000000..79f9d1f
--- /dev/null
@@ -0,0 +1,87 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../resources/js-test-pre.js"></script>
+</head>
+<body id="body">
+
+<div id="content" role="group">
+<iframe id="frame" src="resources/frameset.html" onload="frameLoad();" width=500 height=500></iframe>
+</div>
+
+<p id="description"></p>
+<div id="console"></div>
+
+<script>
+
+    description("This tests that when we access a text marker in a frame that is subsequently replaced by a different frame, we don't leave a hanging node in the cache that leads to a crash.");
+
+    var loadCount = 0;
+    var markerRange = 0;
+    var string = 0;
+
+    function subFrameLoaded() {
+
+        // Step 2: When the sub frame of the iframe loads again this method is called (from that sub-frame)
+
+        // Access the old marker range that we kept hanging around and try to access after the frame changes. This should not crash.
+        var frame = accessibilityController.accessibleElementById("content");
+        var webArea = frame.childAtIndex(0).childAtIndex(0);
+        string = webArea.stringForTextMarkerRange(markerRange);
+
+        // Now try to access a node in the sub-frame, and then we'll replace the parent frame.
+        var text = frame.childAtIndex(0).childAtIndex(0).childAtIndex(0).childAtIndex(0).childAtIndex(0);
+        // Access a marker range so that we start tracking a node in our cache.
+        markerRange = text.textMarkerRangeForElement(text);
+        string = text.stringForTextMarkerRange(markerRange);
+        shouldBe("string", "'test text'");
+
+        // Step 3: Replace the top level iframe src while holding onto a marker range and verify there's no crash
+        document.getElementById("frame").onload = function() {
+            document.getElementById("content").removeChild(document.getElementById("frame"));
+            string = accessibilityController.accessibleElementById("content").stringForTextMarkerRange(markerRange);
+
+            debug("TEST PASSED: NO CRASH");
+
+            if (window.testRunner) {
+                testRunner.notifyDone();
+            }
+            gc();
+        };
+
+        document.getElementById("frame").src = "resources/text.html";
+
+        gc();
+    }
+
+    function frameLoad() {
+
+        // Step 1: When the iframe loads get a marker range that is reference is the sub-frame of the iframe.
+        var frame = accessibilityController.accessibleElementById("content");
+
+        var text = frame.childAtIndex(0).childAtIndex(0).childAtIndex(0).childAtIndex(0).childAtIndex(0).childAtIndex(0);
+      
+        // Access a marker range so that we start tracking a node in our cache.
+        markerRange = text.textMarkerRangeForElement(text);
+        string = text.stringForTextMarkerRange(markerRange);
+
+        shouldBe("string", "'hello'"); 
+        gc();
+
+        // Load a new frame in this location which should now invalidate the marker range cache (and not lead to a crash).
+        document.getElementById("frame").contentWindow.frames[0].location = "resources/inform-parent-of-load.html";
+
+        gc();
+    }
+
+    if (window.accessibilityController) {
+        window.jsTestIsAsync = true;
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+    }
+
+</script>
+
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/accessibility/resources/frameset.html b/LayoutTests/accessibility/resources/frameset.html
new file mode 100644 (file)
index 0000000..32c131e
--- /dev/null
@@ -0,0 +1 @@
+<frameset rows='30%,70%'><frame src='text.html'><frame src='text.html'></frameset>
diff --git a/LayoutTests/accessibility/resources/inform-parent-of-load.html b/LayoutTests/accessibility/resources/inform-parent-of-load.html
new file mode 100644 (file)
index 0000000..9feee16
--- /dev/null
@@ -0,0 +1,12 @@
+<html>
+<script>
+    function informParent() {
+        parent.parent.subFrameLoaded();
+    }
+</script>
+<body onload="informParent();">
+
+test text
+
+</body>
+</html>
diff --git a/LayoutTests/accessibility/resources/text.html b/LayoutTests/accessibility/resources/text.html
new file mode 100644 (file)
index 0000000..056c4b5
--- /dev/null
@@ -0,0 +1,5 @@
+<html>
+<body>
+<div role="group"><h1>hello</h1></div>
+</body>
+</html>
index d6f7054..b8e5a73 100644 (file)
@@ -1,3 +1,28 @@
+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>
 
         Unreviewed fix for the CoordinatedGraphics builds after r178034.
index 649d100..6b45e9b 100644 (file)
@@ -2079,6 +2079,14 @@ void Document::prepareForDestruction()
     clearTouchEventListeners();
 #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)
         m_domWindow->willDetachDocumentFromFrame();
index 45fae0d..cfe8be5 100644 (file)
@@ -803,18 +803,6 @@ void Frame::willDetachPage()
 void Frame::disconnectOwnerElement()
 {
     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)
             m_page->decrementSubframeCount();
index 4d32073..9259f4f 100644 (file)
@@ -295,8 +295,11 @@ void FrameView::reset()
 
 void FrameView::removeFromAXObjectCache()
 {
-    if (AXObjectCache* cache = axObjectCache())
+    if (AXObjectCache* cache = axObjectCache()) {
+        if (HTMLFrameOwnerElement* owner = frame().ownerElement())
+            cache->childrenChanged(owner->renderer());
         cache->remove(this);
+    }
 }
 
 void FrameView::resetScrollbars()