Bug 236795 - Fix for assert crash in AccessibilityRenderObject::visiblePositionForIndex.
Summary: Fix for assert crash in AccessibilityRenderObject::visiblePositionForIndex.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-17 12:07 PST by Andres Gonzalez
Modified: 2022-02-23 09:52 PST (History)
10 users (show)

See Also:


Attachments
Patch (10.73 KB, patch)
2022-02-17 12:19 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (14.30 KB, patch)
2022-02-18 07:03 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (10.91 KB, patch)
2022-02-21 11:59 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (13.01 KB, patch)
2022-02-21 13:32 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (13.15 KB, patch)
2022-02-21 14:35 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (13.24 KB, patch)
2022-02-22 06:42 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2022-02-17 12:07:04 PST
Fix for assert crash in AccessibilityRenderObject::visiblePositionForIndex.
Comment 1 Andres Gonzalez 2022-02-17 12:19:57 PST
Created attachment 452408 [details]
Patch
Comment 2 Andres Gonzalez 2022-02-17 12:23:50 PST
rdar://89025180
Comment 3 Andres Gonzalez 2022-02-17 12:27:12 PST
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
...
Comment 4 Tyler Wilcock 2022-02-17 12:41:15 PST
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.
Comment 5 chris fleizach 2022-02-17 14:15:43 PST
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
Comment 6 Andres Gonzalez 2022-02-18 07:03:30 PST
Created attachment 452516 [details]
Patch
Comment 7 Andres Gonzalez 2022-02-18 07:13:27 PST
(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.
Comment 8 Andres Gonzalez 2022-02-21 11:59:14 PST
Created attachment 452752 [details]
Patch
Comment 9 Andres Gonzalez 2022-02-21 13:32:11 PST
Created attachment 452766 [details]
Patch
Comment 10 Andres Gonzalez 2022-02-21 14:35:51 PST
Created attachment 452775 [details]
Patch
Comment 11 Darin Adler 2022-02-21 17:01:51 PST
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.
Comment 12 Andres Gonzalez 2022-02-22 06:42:48 PST
Created attachment 452867 [details]
Patch
Comment 13 Andres Gonzalez 2022-02-22 06:54:56 PST
(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.
Comment 14 Darin Adler 2022-02-22 08:50:50 PST
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.
Comment 15 EWS 2022-02-23 09:52:12 PST
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].