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
<rdar://problem/12344806>
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
Created attachment 209995 [details] patch
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.
Created attachment 210021 [details] patch
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
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.
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.
(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.
Created attachment 210024 [details] patch Updated layout test to check two different nodes in an iframe being removed
Comment on attachment 210024 [details] patch Attachment 210024 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1618705
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
Created attachment 210026 [details] patch
Comment on attachment 210026 [details] patch Thanks Darin!
Comment on attachment 210026 [details] patch Clearing flags on attachment: 210026 Committed r154855: <http://trac.webkit.org/changeset/154855>
All reviewed patches have been landed. Closing bug.