Bug 236795 – Fix for assert crash in AccessibilityRenderObject::visiblePositionForIndex.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andres Gonzalez]

Fix for assert crash in AccessibilityRenderObject::visiblePositionForIndex.

[Andres Gonzalez]

Created attachment 452408 [details]
Patch

[Andres Gonzalez]

rdar://89025180

[Andres Gonzalez]

Steps:
1. Turn on VoiceOver.
2. Browse to https://appleid.apple.com/.
3. Click Sign In.
4. Click Use a different Apple ID btn.
5. In the Apple ID field enter a bogus ID such as “asdf” and click Continue btn.

WebProcess debug build will assert crash.

Stack trace:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
  * frame #0: 0x000000027fec7f9e JavaScriptCore`::WTFCrash() at Assertions.cpp:322:35
    frame #1: 0x000000029382d47b WebCore`WTFCrashWithInfo((null)=373, (null)="./html/HTMLTextFormControlElement.cpp", (null)="WebCore::VisiblePosition WebCore::HTMLTextFormControlElement::visiblePositionForIndex(int) const", (null)=1366) at Assertions.h:732:5
    frame #2: 0x0000000296f819cf WebCore`WebCore::HTMLTextFormControlElement::visiblePositionForIndex(this=0x000000025f0e3410, index=8) const at HTMLTextFormControlElement.cpp:373:5
    frame #3: 0x000000029616dfb3 WebCore`WebCore::AccessibilityRenderObject::visiblePositionForIndex(this=0x000000028d6a4400, index=8) const at AccessibilityRenderObject.cpp:2107:82
    frame #4: 0x00000002960f36dc WebCore`WebCore::AXObjectCache::characterOffsetForIndex(this=0x000000025d4ef7a0, index=8, obj=0x000000028d6a4400) at AXObjectCache.cpp:3117:31
    frame #5: 0x0000000296156dc5 WebCore`WebCore::AccessibilityObject::rangeForPlainTextRange(this=0x000000028d6a4400, range=0x00007ff7b92c94e8) const at AccessibilityObject.cpp:1308:38
    frame #6: 0x0000000298ade6f5 WebCore`-[WebAccessibilityObjectWrapper doAXAttributedStringForRange:]::$_52::operator(this=0x000070000140bf20)() const at WebAccessibilityObjectWrapperMac.mm:3231:40
    frame #7: 0x0000000298ade5f5 WebCore`NSAttributedString* WebCore::Accessibility::retrieveAutoreleasedValueFromMainThread<NSAttributedString*, -[WebAccessibilityObjectWrapper doAXAttributedStringForRange:]::$_52>(this=0x00000002b355b968)::'lambda'()::operator()() const at AccessibilityObjectInterface.h:1685:17
    frame #8: 0x0000000298ade589 WebCore`WTF::Detail::CallableWrapper<NSAttributedString* WebCore::Accessibility::retrieveAutoreleasedValueFromMainThread<NSAttributedString*, -[WebAccessibilityObjectWrapper doAXAttributedStringForRange:]::$_52>(-[WebAccessibilityObjectWrapper doAXAttributedStringForRange:]::$_52&&)::'lambda'(), void>::call(this=0x00000002b355b960) at Function.h:53:39
    frame #9: 0x000000027fef1d92 JavaScriptCore`WTF::Function<void ()>::operator(this=0x00000002b355b990)() const at Function.h:82:35
    frame #10: 0x000000027ff415ed JavaScriptCore`void WTF::callOnMainAndWait<(WTF::MainStyle)0>(this=0x00000002b355b988)>&&)::'lambda'()::operator()() const at MainThread.cpp:123:9
    frame #11: 0x000000027ff41549 JavaScriptCore`WTF::Detail::CallableWrapper<void WTF::callOnMainAndWait<(WTF::MainStyle)0>(WTF::Function<void ()>&&)::'lambda'(), void>::call(this=0x00000002b355b980) at Function.h:53:39
    frame #12: 0x000000027fef1d92 JavaScriptCore`WTF::Function<void ()>::operator(this=0x00007ff7b92c9640)() const at Function.h:82:35
    frame #13: 0x000000027ff78b7e JavaScriptCore`WTF::RunLoop::performWork(this=0x000000025d00c180) at RunLoop.cpp:133:9
    frame #14: 0x000000027ff7c44e JavaScriptCore`WTF::RunLoop::performWork(context=0x000000025d00c180) at RunLoopCF.cpp:46:37
...

[Tyler Wilcock]

Comment on attachment 452408 [details]
Patch

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

> LayoutTests/accessibility/native-text-control-attributed-string.html:37
> +        debug(`Attributed string for range (-1, 1): ${passwordField.attributedStringForRange(-1, 1)}`);

The test will run faster if we buffer test output In a variable and debug it once at the end. But if you feel that's not necessary here that's fine with me.

[chris fleizach]

Comment on attachment 452408 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2112
> +        else if (static_cast<size_t>(index) > textLength)

do we want to check >= here
I don't think we want to set the position to textLength

[Andres Gonzalez]

Created attachment 452516 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #5)
> Comment on attachment 452408 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452408&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2112
> > +        else if (static_cast<size_t>(index) > textLength)
> 
> do we want to check >= here
> I don't think we want to set the position to textLength

textLength position is fine, it means the position after the last char. If we do if index >= textLength then index = texgtLength - 1, we loose the last char of every range.

[Andres Gonzalez]

Created attachment 452752 [details]
Patch

[Andres Gonzalez]

Created attachment 452766 [details]
Patch

[Andres Gonzalez]

Created attachment 452775 [details]
Patch

[Darin Adler]

Comment on attachment 452775 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2113
> +        size_t textLength = textControl.value().length();
> +        if (index < 0)
> +            index = 0;
> +        else if (static_cast<size_t>(index) > textLength)
> +            index = textLength;

<algorithm> has the std::clamp function and <wtf/MathExtras.h> has the clampTo function for this kind of operation. It seems like we can probably use one of those instead of writing it out like this.

[Andres Gonzalez]

Created attachment 452867 [details]
Patch

[Andres Gonzalez]

(In reply to Darin Adler from comment #11)
> Comment on attachment 452775 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452775&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2113
> > +        size_t textLength = textControl.value().length();
> > +        if (index < 0)
> > +            index = 0;
> > +        else if (static_cast<size_t>(index) > textLength)
> > +            index = textLength;
> 
> <algorithm> has the std::clamp function and <wtf/MathExtras.h> has the
> clampTo function for this kind of operation. It seems like we can probably
> use one of those instead of writing it out like this.

Done, thanks for pointing it out.

[Darin Adler]

Comment on attachment 452867 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2109
> +        auto& textControl = downcast<RenderTextControl>(*m_renderer).textFormControlElement();
> +        return textControl.visiblePositionForIndex(std::clamp(index, 0, static_cast<int>(textControl.value().length())));

I suggest naming this just "element" or "control". One-word local names are the best, unless you need multiple words for clarity.

[EWS]

Committed r290377 (247692@main): <https://commits.webkit.org/247692@main>

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