<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[178038] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/178038">178038</a></dd>
<dt>Author</dt> <dd>cfleizach@apple.com</dd>
<dt>Date</dt> <dd>2015-01-07 08:55:10 -0800 (Wed, 07 Jan 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoredomDocumentcpp">trunk/Source/WebCore/dom/Document.cpp</a></li>
<li><a href="#trunkSourceWebCorepageFramecpp">trunk/Source/WebCore/page/Frame.cpp</a></li>
<li><a href="#trunkSourceWebCorepageFrameViewcpp">trunk/Source/WebCore/page/FrameView.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsaccessibilityframedisconnecttextmarkercachecrashexpectedtxt">trunk/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash-expected.txt</a></li>
<li><a href="#trunkLayoutTestsaccessibilityframedisconnecttextmarkercachecrashhtml">trunk/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash.html</a></li>
<li><a href="#trunkLayoutTestsaccessibilityresourcesframesethtml">trunk/LayoutTests/accessibility/resources/frameset.html</a></li>
<li><a href="#trunkLayoutTestsaccessibilityresourcesinformparentofloadhtml">trunk/LayoutTests/accessibility/resources/inform-parent-of-load.html</a></li>
<li><a href="#trunkLayoutTestsaccessibilityresourcestexthtml">trunk/LayoutTests/accessibility/resources/text.html</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (178037 => 178038)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog 2015-01-07 16:44:36 UTC (rev 178037)
+++ trunk/LayoutTests/ChangeLog 2015-01-07 16:55:10 UTC (rev 178038)
</span><span class="lines">@@ -1,3 +1,16 @@
</span><ins>+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.
+
</ins><span class="cx"> 2015-01-07 Carlos Alberto Lopez Perez <clopez@igalia.com>
</span><span class="cx">
</span><span class="cx"> [GTK] Unreviewed GTK gardening after r177637.
</span></span></pre></div>
<a id="trunkLayoutTestsaccessibilityframedisconnecttextmarkercachecrashexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash-expected.txt (0 => 178038)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash-expected.txt 2015-01-07 16:55:10 UTC (rev 178038)
</span><span class="lines">@@ -0,0 +1,9 @@
</span><ins>+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
+
</ins></span></pre></div>
<a id="trunkLayoutTestsaccessibilityframedisconnecttextmarkercachecrashhtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash.html (0 => 178038)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash.html (rev 0)
+++ trunk/LayoutTests/accessibility/frame-disconnect-textmarker-cache-crash.html 2015-01-07 16:55:10 UTC (rev 178038)
</span><span class="lines">@@ -0,0 +1,87 @@
</span><ins>+<!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>
</ins></span></pre></div>
<a id="trunkLayoutTestsaccessibilityresourcesframesethtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/accessibility/resources/frameset.html (0 => 178038)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/accessibility/resources/frameset.html (rev 0)
+++ trunk/LayoutTests/accessibility/resources/frameset.html 2015-01-07 16:55:10 UTC (rev 178038)
</span><span class="lines">@@ -0,0 +1 @@
</span><ins>+<frameset rows='30%,70%'><frame src='text.html'><frame src='text.html'></frameset>
</ins></span></pre></div>
<a id="trunkLayoutTestsaccessibilityresourcesinformparentofloadhtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/accessibility/resources/inform-parent-of-load.html (0 => 178038)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/accessibility/resources/inform-parent-of-load.html (rev 0)
+++ trunk/LayoutTests/accessibility/resources/inform-parent-of-load.html 2015-01-07 16:55:10 UTC (rev 178038)
</span><span class="lines">@@ -0,0 +1,12 @@
</span><ins>+<html>
+<script>
+ function informParent() {
+ parent.parent.subFrameLoaded();
+ }
+</script>
+<body onload="informParent();">
+
+test text
+
+</body>
+</html>
</ins></span></pre></div>
<a id="trunkLayoutTestsaccessibilityresourcestexthtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/accessibility/resources/text.html (0 => 178038)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/accessibility/resources/text.html (rev 0)
+++ trunk/LayoutTests/accessibility/resources/text.html 2015-01-07 16:55:10 UTC (rev 178038)
</span><span class="lines">@@ -0,0 +1,5 @@
</span><ins>+<html>
+<body>
+<div role="group"><h1>hello</h1></div>
+</body>
+</html>
</ins></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (178037 => 178038)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog 2015-01-07 16:44:36 UTC (rev 178037)
+++ trunk/Source/WebCore/ChangeLog 2015-01-07 16:55:10 UTC (rev 178038)
</span><span class="lines">@@ -1,3 +1,28 @@
</span><ins>+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):
+
</ins><span class="cx"> 2015-01-07 Zan Dobersek <zdobersek@igalia.com>
</span><span class="cx">
</span><span class="cx"> Unreviewed fix for the CoordinatedGraphics builds after r178034.
</span></span></pre></div>
<a id="trunkSourceWebCoredomDocumentcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/dom/Document.cpp (178037 => 178038)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/dom/Document.cpp 2015-01-07 16:44:36 UTC (rev 178037)
+++ trunk/Source/WebCore/dom/Document.cpp 2015-01-07 16:55:10 UTC (rev 178038)
</span><span class="lines">@@ -2079,6 +2079,14 @@
</span><span class="cx"> clearTouchEventListeners();
</span><span class="cx"> #endif
</span><span class="cx">
</span><ins>+#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
+
</ins><span class="cx"> disconnectDescendantFrames();
</span><span class="cx"> if (m_domWindow && m_frame)
</span><span class="cx"> m_domWindow->willDetachDocumentFromFrame();
</span></span></pre></div>
<a id="trunkSourceWebCorepageFramecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/Frame.cpp (178037 => 178038)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/Frame.cpp 2015-01-07 16:44:36 UTC (rev 178037)
+++ trunk/Source/WebCore/page/Frame.cpp 2015-01-07 16:55:10 UTC (rev 178038)
</span><span class="lines">@@ -803,18 +803,6 @@
</span><span class="cx"> void Frame::disconnectOwnerElement()
</span><span class="cx"> {
</span><span class="cx"> if (m_ownerElement) {
</span><del>- // 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
-
</del><span class="cx"> m_ownerElement->clearContentFrame();
</span><span class="cx"> if (m_page)
</span><span class="cx"> m_page->decrementSubframeCount();
</span></span></pre></div>
<a id="trunkSourceWebCorepageFrameViewcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/FrameView.cpp (178037 => 178038)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/FrameView.cpp 2015-01-07 16:44:36 UTC (rev 178037)
+++ trunk/Source/WebCore/page/FrameView.cpp 2015-01-07 16:55:10 UTC (rev 178038)
</span><span class="lines">@@ -295,8 +295,11 @@
</span><span class="cx">
</span><span class="cx"> void FrameView::removeFromAXObjectCache()
</span><span class="cx"> {
</span><del>- if (AXObjectCache* cache = axObjectCache())
</del><ins>+ if (AXObjectCache* cache = axObjectCache()) {
+ if (HTMLFrameOwnerElement* owner = frame().ownerElement())
+ cache->childrenChanged(owner->renderer());
</ins><span class="cx"> cache->remove(this);
</span><ins>+ }
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void FrameView::resetScrollbars()
</span></span></pre>
</div>
</div>
</body>
</html>