Bug 255655 – AX: Fix for hang in AXObjectCache::textMarkerDataForNextCharacterOffset.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

Call graph:
    7345 Thread_56736   DispatchQueue_1: com.apple.main-thread  (serial)
    + 7345 start  (in dyld) + 2200  [0x187afa040]
    +   7345 WebKit::XPCServiceMain(int, char const**)  (in WebKit) + 236  [0x1ab29d3b0]
    +     7345 xpc_main  (in libxpc.dylib) + 64  [0x187ba56d8]
    +       7345 _xpc_main  (in libxpc.dylib) + 324  [0x187bb47d0]
    +         7345 _xpc_objc_main  (in libxpc.dylib) + 684  [0x187ba5b2c]
    +           7345 -[NSRunLoop(NSRunLoop) run]  (in Foundation) + 64  [0x189033398]
    +             7345 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]  (in Foundation) + 212  [0x188fbb9f8]
    +               7345 CFRunLoopRunSpecific  (in CoreFoundation) + 600  [0x187f51c30]
    +                 7345 __CFRunLoopRun  (in CoreFoundation) + 828  [0x187f52620]
    +                   7345 __CFRunLoopDoSources0  (in CoreFoundation) + 244  [0x187f53a18]
    +                     7345 __CFRunLoopDoSource0  (in CoreFoundation) + 176  [0x187f53ca8]
    +                       7345 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__  (in CoreFoundation) + 28  [0x187f53d14]
    +                         7345 WTF::RunLoop::performWork(void*)  (in JavaScriptCore) + 36  [0x1a426f4e0]
    +                           7345 WTF::RunLoop::performWork()  (in JavaScriptCore) + 204  [0x1a426e610]
    +                             7345 WTF::Detail::CallableWrapper<void WTF::callOnMainAndWait<(WTF::MainStyle)0>(WTF::Function<void ()>&&)::'lambda'(), void>::call()  (in JavaScriptCore) + 60  [0x1a425c1b8]
    +                               7345 WTF::Detail::CallableWrapper<objc_object* WebCore::Accessibility::retrieveAutoreleasedValueFromMainThread<objc_object*, -[WebAccessibilityObjectWrapper accessibilityAttributeValue:forParameter:]::$_72>(-[WebAccessibilityObjectWrapper accessibilityAttributeValue:forParameter:]::$_72&&)::'lambda'(), void>::call()  (in WebCore) + 232  [0x1aa69a874]
    +                                 3076 WebCore::AXObjectCache::textMarkerDataForNextCharacterOffset(WebCore::CharacterOffset const&)  (in WebCore) + 452  [0x1a90e621c]
    +                                 ! 1411 WebCore::AXObjectCache::rangeForUnorderedCharacterOffsets(WebCore::CharacterOffset const&, WebCore::CharacterOffset const&)  (in WebCore) + 40,528,...  [0x1a90e4cd8,0x1a90e4ec0,...]
    +                                 ! 972 WebCore::boundaryPoint(WebCore::CharacterOffset const&)  (in WebCore) + 276,268,...  [0x1a90e533c,0x1a90e5334,...]
    +                                 ! 359 WebCore::AXObjectCache::rangeForUnorderedCharacterOffsets(WebCore::CharacterOffset const&, WebCore::CharacterOffset const&)  (in WebCore) + 540  [0x1a90e4ecc]
    +                                 ! : 359 WebCore::boundaryPoint(WebCore::CharacterOffset const&)  (in WebCore) + 32,252,...  [0x1a90e5248,0x1a90e5324,...]
    +                                 ! 334 WebCore::AXObjectCache::rangeForUnorderedCharacterOffsets(WebCore::CharacterOffset const&, WebCore::CharacterOffset const&)  (in WebCore) + 528  [0x1a90e4ec0]
    +                                 !   334 WebCore::boundaryPoint(WebCore::CharacterOffset const&)  (in WebCore) + 32,264,...  [0x1a90e5248,0x1a90e5330,...]
    +                                 2232 WebCore::AXObjectCache::textMarkerDataForNextCharacterOffset(WebCore::CharacterOffset const&)  (in WebCore) + 432,1188,...  [0x1a90e6208,0x1a90e64fc,...]
    +                                 691 WebCore::AXObjectCache::nextCharacterOffset(WebCore::CharacterOffset const&, bool)  (in WebCore) + 404  [0x1a90e67e4]
    +                                 682 WebCore::AXObjectCache::textMarkerDataForNextCharacterOffset(WebCore::CharacterOffset const&)  (in WebCore) + 124  [0x1a90e60d4]
    +                                 ! 682 WebCore::AXObjectCache::nextCharacterOffset(WebCore::CharacterOffset const&, bool)  (in WebCore) + 36,392,...  [0x1a90e6674,0x1a90e67d8,...]
    +                                 664 WebCore::AXObjectCache::rangeForUnorderedCharacterOffsets(WebCore::CharacterOffset const&, WebCore::CharacterOffset const&)  (in WebCore) + 700  [0x1a90e4f6c]
    7345 Thread_56882: com.apple.accessibility.secondary
    + 7345 thread_start  (in libsystem_pthread.dylib) + 8  [0x187e70e3c]
    +   7345 _pthread_start  (in libsystem_pthread.dylib) + 136  [0x187e76034]
    +     7345 axThreadEntry  (in HIServices) + 124  [0x18df82f9c]
    +       7345 CFRunLoopRunSpecific  (in CoreFoundation) + 600  [0x187f51c30]
    +         7345 __CFRunLoopRun  (in CoreFoundation) + 2244  [0x187f52ba8]
    +           7345 __CFRunLoopDoSource1  (in CoreFoundation) + 520  [0x187f541c4]
    +             7345 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__  (in CoreFoundation) + 60  [0x187f542a4]
    +               7345 mshMIGPerform  (in HIServices) + 204  [0x18df5e404]
    +                 7345 _XCopyParameterizedAttributeValue  (in HIServices) + 512  [0x18dfa6a24]
    +                   7345 _AXXMIGCopyParameterizedAttributeValue  (in HIServices) + 464  [0x18df7f934]
    +                     7345 CopyParameterizedAttributeValue  (in AppKit) + 504  [0x18ba4fd20]
    +                       7345 NSAccessibilityEntryPointValueForAttributeWithParameter  (in AppKit) + 232  [0x18bcb2a24]
    +                         7345 NSAccessibilityPerformEntryPointObject  (in AppKit) + 44  [0x18bcb0dd0]
    +                           7345 ___NSAccessibilityEntryPointValueForAttributeWithParameter_block_invoke.779  (in AppKit) + 444  [0x18bcb5a90]
    +                             7345 -[WebAccessibilityObjectWrapper accessibilityAttributeValue:forParameter:]  (in WebCore) + 7804  [0x1aa6904b8]
    +                               7345 WTF::callOnMainThreadAndWait(WTF::Function<void ()>&&)  (in JavaScriptCore) + 296  [0x1a425be84]
    +                                 7345 WTF::Condition::waitUntilUnchecked<WTF::Lock>(WTF::Lock&, WTF::TimeWithDynamicClockType const&)  (in JavaScriptCore) + 292  [0x1a4230aa4]
    +                                   7345 WTF::ParkingLot::parkConditionallyImpl(void const*, WTF::ScopedLambda<bool ()> const&, WTF::ScopedLambda<void ()> const&, WTF::TimeWithDynamicClockType const&)  (in JavaScriptCore) + 2156  [0x1a4268644]
    +                                     7345 _pthread_cond_wait  (in libsystem_pthread.dylib) + 1228  [0x187e765fc]
    +                                       7345 __psynch_cvwait  (in libsystem_kernel.dylib) + 8  [0x187e3930c]

[Radar WebKit Bug Importer]

<rdar://problem/108261895>

[Andres Gonzalez]

Created attachment 465980 [details]
Patch

[chris fleizach]

Can we do that isEqual
Check before we make the text marker data?

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Can we do that isEqual
> Check before we make the text marker data?

So you are saying that if next is equal to the original, then we should return a null CharacterOffset. Before we were returning the original. I like more returning a null, will run the tests to see if that has an effect.

[Andres Gonzalez]

Created attachment 465981 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Can we do that isEqual
> Check before we make the text marker data?

Done.

[Andres Gonzalez]

Created attachment 465995 [details]
Patch

[Darin Adler]

Comment on attachment 465995 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:2663
> +    if (characterOffset.isNull())

This check seems to be unnecessary; the other check alone would fix the bug.

> Source/WebCore/accessibility/AXObjectCache.cpp:2700
> +    if (characterOffset.isNull())

Same here.

[Darin Adler]

Comment on attachment 465995 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:2663
>> +    if (characterOffset.isNull())
> 
> This check seems to be unnecessary; the other check alone would fix the bug.

This check seems to be unnecessary; the other check alone would fix the bug.

>> Source/WebCore/accessibility/AXObjectCache.cpp:2700
>> +    if (characterOffset.isNull())
> 
> Same here.

Same here.

[Andres Gonzalez]

(In reply to Darin Adler from comment #9)
> Comment on attachment 465995 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465995&action=review
> 
> >> Source/WebCore/accessibility/AXObjectCache.cpp:2663
> >> +    if (characterOffset.isNull())
> > 
> > This check seems to be unnecessary; the other check alone would fix the bug.
> 
> This check seems to be unnecessary; the other check alone would fix the bug.
> 
> >> Source/WebCore/accessibility/AXObjectCache.cpp:2700
> >> +    if (characterOffset.isNull())
> > 
> > Same here.
> 
> Same here.

CharacterOffset::isNull() is just a Node* null check, so I think it is worthy having an early return here in case this method is called with a null Node*. That is more likely now than before since the CharacterOffsets off the main thread will have a null Node*. Eventually, my goal is to get rid of CharacterOffsets altogether and use the new AXTextMarkers, but this is the transition stage.

[EWS]

Committed 263172@main (705145c30a47): <https://commits.webkit.org/263172@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465995 [details].