Bug 120318 – AX: Crash when VoiceOver reference a TextMarker from an iframe that has been removed

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Summary: Safari sometimes crashes when navigating content contained within an iframe.

Steps to Reproduce: 1. Visit http://fyve.de.
2. Click the "My Fave" link.
3. The login form appears in an iframe. Navigate left and right through this iframe a couple of times.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fff86c284f8 WebCore::Position::Position(WTF::PassRefPtr<WebCore::Node>, WebCore::Position::LegacyEditingOffset) + 40
1   com.apple.WebCore             	0x00007fff866e16c6 WebCore::AXObjectCache::visiblePositionForTextMarkerData(WebCore::TextMarkerData&) + 134
2   com.apple.WebCore             	0x00007fff86dedd2e -[WebAccessibilityObjectWrapper visiblePositionForTextMarker:] + 126
3   com.apple.WebCore             	0x00007fff86dfb08d -[WebAccessibilityObjectWrapper accessibilityAttributeValue:forParameter:] + 9293
4   com.apple.AppKit              	0x00007fff8864490e CopyParameterizedAttributeValue + 328
5   com.apple.HIServices          	0x00007fff891a2d2d _AXXMIGCopyParameterizedAttributeValue + 265
6   com.apple.HIServices          	0x00007fff891aa510 _XCopyParameterizedAttributeValue + 576
7   com.apple.HIServices          	0x00007fff89187f4e mshMIGPerform + 443
8   com.apple.CoreFoundation      	0x00007fff8c52a2d9 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ + 41
9   com.apple.CoreFoundation      	0x00007fff8c52a019 __CFRunLoopDoSource1 + 153
10  com.apple.CoreFoundation      	0x00007fff8c55d19f __CFRunLoopRun + 1775
11  com.apple.CoreFoundation      	0x00007fff8c55c6b2 CFRunLoopRunSpecific + 290
12  com.apple.HIToolbox           	0x00007fff91beb0a4 RunCurrentEventLoopInMode + 209
13  com.apple.HIToolbox           	0x00007fff91beae42 ReceiveNextEventCommon + 356
14  com.apple.HIToolbox           	0x00007fff91beacd3 BlockUntilNextEventMatchingListInMode + 62
15  com.apple.AppKit              	0x00007fff88410613 _DPSNextEvent + 685
16  com.apple.AppKit              	0x00007fff8840fed2 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
17  com.apple.AppKit              	0x00007fff88407283 -[NSApplication run] + 517
18  com.apple.WebCore             	0x00007fff86cc3e0f WebCore::RunLoop::run() + 63
19  com.apple.WebKit2             	0x00007fff8f3d6c8a WebKit::WebProcessMain(WebKit::CommandLine const&) + 2586
20  com.apple.WebKit2             	0x00007fff8f39e5bd WebKitMain + 285
21  com.apple.WebProcess          	0x00000001024ace7b 0x1024ac000 + 3707
22  libdyld.dylib                 	0x00007fff90cea7e1 start + 1

[chris fleizach]

<rdar://problem/12344806>

[chris fleizach]

It looks like what happens is that AXTextMarker references a node in the iframe. the iframe goes away, but the TextMarkerNode cache is not updated correctly (because the document is disconnected by the time it tries). Then we reference a node that has been deallocated and crash

[chris fleizach]

Created attachment 209995 [details]
patch

[Darin Adler]

Comment on attachment 209995 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209995&action=review

> Source/WebCore/ChangeLog:3
> +        AX: Random crashes when navigating in iFrames with VoiceOver

I am not fond of the phrase “random crashes” nor do I have a clear idea what it means. Maybe remove the word “random” from this bug title?

> Source/WebCore/page/Frame.cpp:637
> +        // We don't clear the AXObjectCache here because we don't want to empty the topLevel cache

top level seems better than topLevel in this comment.

> Source/WebCore/page/Frame.cpp:639
> +        if (Document* doc = m_ownerElement->document()) {

Can we use the full word document here instead of the abbreviation, doc?

Why are we checking for null? Elements always have a non-null document.

> Source/WebCore/page/Frame.cpp:641
> +                cache->clearTextMarkerNodesInUse(document());

I don’t understand why document() is correct here if it’s incorrect above.

[chris fleizach]

Created attachment 210021 [details]
patch

[chris fleizach]

Updated patch to address Darin's comments

(In reply to comment #4)
> (From update of attachment 209995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209995&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        AX: Random crashes when navigating in iFrames with VoiceOver
> 
> I am not fond of the phrase “random crashes” nor do I have a clear idea what it means. Maybe remove the word “random” from this bug title?
> 
> > Source/WebCore/page/Frame.cpp:637
> > +        // We don't clear the AXObjectCache here because we don't want to empty the topLevel cache
> 
> top level seems better than topLevel in this comment.
> 
> > Source/WebCore/page/Frame.cpp:639
> > +        if (Document* doc = m_ownerElement->document()) {
> 
> Can we use the full word document here instead of the abbreviation, doc?
> 
> Why are we checking for null? Elements always have a non-null document.
> 
> > Source/WebCore/page/Frame.cpp:641
> > +                cache->clearTextMarkerNodesInUse(document());
> 
> I don’t understand why document() is correct here if it’s incorrect above.

I added some more comments in code why we're using document() in this case that will hopefully be clear.

What we're trying to do is remove a few Nodes from cache of Nodes.
These nodes came from a sub-document [document() in this code snippet]
However, the cache of Nodes lives in the top level document.

So we want to access the cache from the top level document (m_owner->document()) and cleanup the nodes from document() in that cache

[Darin Adler]

Comment on attachment 210021 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210021&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:947
> +void AXObjectCache::clearTextMarkerNodesInUse(Document* doc)

Can we call this “document” instead of “doc” please?

> Source/WebCore/accessibility/AXObjectCache.cpp:958
> +            break;

It looks like this will stop after finding one node.

[Darin Adler]

Comment on attachment 210021 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210021&action=review

>> Source/WebCore/accessibility/AXObjectCache.cpp:958
>> +            break;
> 
> It looks like this will stop after finding one node.

To notice this mistake, the test would have to involve two nodes instead of just one.

[chris fleizach]

(In reply to comment #8)
> (From update of attachment 210021 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210021&action=review
> 
> >> Source/WebCore/accessibility/AXObjectCache.cpp:958
> >> +            break;
> > 
> > It looks like this will stop after finding one node.
> 
> To notice this mistake, the test would have to involve two nodes instead of just one.

Ah good catch. Left over from the first patch that I had to cleanup.

[chris fleizach]

Created attachment 210024 [details]
patch

Updated layout test to check two different nodes in an iframe being removed

[Early Warning System Bot]

Comment on attachment 210024 [details]
patch

Attachment 210024 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1618705

[Early Warning System Bot]

Comment on attachment 210024 [details]
patch

Attachment 210024 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1644059

[chris fleizach]

Created attachment 210026 [details]
patch

[chris fleizach]

Comment on attachment 210026 [details]
patch

Thanks Darin!

[WebKit Commit Bot]

Comment on attachment 210026 [details]
patch

Clearing flags on attachment: 210026

Committed r154855: <http://trac.webkit.org/changeset/154855>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.