Skip to content
Permalink
Browse files
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.



Canonical link: https://commits.webkit.org/158152@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@178038 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
fleizach committed Jan 7, 2015
1 parent 2604675 commit 689215d450ce8c20991271eec952ba2a1f96ca06
@@ -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.
@@ -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

@@ -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>
@@ -0,0 +1 @@
<frameset rows='30%,70%'><frame src='text.html'><frame src='text.html'></frameset>
@@ -0,0 +1,12 @@
<html>
<script>
function informParent() {
parent.parent.subFrameLoaded();
}
</script>
<body onload="informParent();">

test text

</body>
</html>
@@ -0,0 +1,5 @@
<html>
<body>
<div role="group"><h1>hello</h1></div>
</body>
</html>
@@ -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.
@@ -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();
@@ -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();
@@ -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()

0 comments on commit 689215d

Please sign in to comment.